Fix #9932

[mikewl]

This fixes #9932 by first creating a TEMP directory to clone into. Once the clone is complete the contents of TEMP are copied into METADATA.

I am not sure if the comments are excessive. Will prune if necessary.

[tkelman]

Missing an end here, and please use 4-space indent. I like the comments though.

[mikewl]

Actually - how did that compile?! Fixing now.

[tkelman]

it didn't compile, at least not on travis

[tkelman]

Good idea.

[tkelman]

any reason not to do mv here?

[mikewl]

None, other than I forgot it existed!

[mikewl]

Crap. I clearly am not good with git yet >_<
Not sure why my indentation keeps on going off, indent is set to 4 and I keep on running auto indent on it.

Sorry about the nonsense!

[tkelman]

and I keep on running auto indent on it.

Might be a bug with the Julia plugin for whatever editor you're using.

[mikewl]

I fixed it manually and cleaned it all up, really sorry about the crap >_<

[tkelman]

No worries, git can be really frustrating as you first get used to doing more complicated things with it.

I think lines 62 and 63 are still under-indented. Would be nice if we had an auto-formatter.

[mikewl]

As you commented that I fixed it.

Seeing as that is my 2nd biggest issue (not knowing how to rebase properly would be no.1 - time to make sure I can do that right) where can I look how to do that? Can it be done as a github plugin?

[tkelman]

You got 65-67, but not 62-63, from what I can see.

I do wonder how much infrastructure the github web editor has in common with atom, whether you can make atom settings for smart indenting apply within the web editor...

[tkelman]

Something a little like https://github.com/google/yapf or clang-format that we could just run automatically and not have to think about it would be ideal, but I don't know if anyone has come up with anything standalone for Julia that isn't just leveraging existing text editors' plugin infrastructure.

[mikewl]

I may try and do something similar to YAPF! I think I edited and committed those lines now 4 times with atom only actually saving them after I closed atom. I was quite annoyed.

Does Julia have a more comprehensive guideline for Julia code other than the one in CONTRIBUTING.MD? Fixing indentation would be quite easy I think, as well as trailing whitespace.

It should probably be done in Julia, then it could be a package which could also emit an executable once the work there is completed.

Any name that would be preferred? I should have a chance to get to starting this tonight. Will ask for comments once I get to a working state.

[tkelman]

I don't think we do have a coherent code style guideline beyond CONTRIBUTING.md. Take a look at what the different editor plugins do, maybe bug the authors of them and/or solicit julia-users for some thoughts (and name suggestions). https://github.com/jakebolewski/JuliaParser.jl will probably also be very useful to verify that a formatter only changes appearance and not meaning.

[tkelman]

Oh dear. Back to the subject of this PR, it looks like it's failing the pkg test on Travis (and I can reproduce that with a local checkout of this branch). Need to get to the bottom of that... are you working from a source build of Julia?

[mikewl]

I am working on a source build of Julia yes. Internet is being painfully slow right now unfortunately.

Found the issue. Hidden folders are not copied so the .git folder is never copied over.

I think I am going to leave this as is and will have to open up a second PR modifying mv and cp to include a hidden option. I will have to make new tests for that won't I?

[tkelman]

Interesting. Yes, tests for new functionality are always good. @peter1000 was just recently working on recursive cp, but I don't know whether the tests included any hidden subfolders.

[peter1000]

If you want I can look into the cp hidden folder issue? Just asking to not double the effort.

[peter1000]

Seems if I do

julia> cp("Docile.jl", "test")

test contains also the .git folder

julia> versioninfo()
Julia Version 0.4.0-dev+4524
Commit 982c60a (2015-04-27 00:18 UTC)
Platform Info:
  System: Linux (x86_64-unknown-linux-gnu)
  CPU: Intel(R) Core(TM) i3-2310M CPU @ 2.10GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Sandybridge)
  LAPACK: libopenblas
  LIBM: libopenlibm
  LLVM: libLLVM-3.3

[tkelman]

though we are using mv here so maybe the underlying problem is in sendfile

[peter1000]

seems to work too

mv("DocOpt.jl", "test2")

"test2" has the .git folder

not sure what happens if there are wrong permissions or files are currently open by an other application: just a thought

[mikewl]

Hmmm. Think I am going to do a make clean and start from there again.

Sorry for bugging you @peter1000.

I am quite confused as to how the error is being thrown. Built it on windows with everything copying right now. Same error as before though:

fatal: Not a git repository (or any of the parent directories): .git
ERROR: failed process: Process(setenv(`git rev-parse --git-dir`; dir="/home/michaelwl/.julia/v0.4/METADATA"), ProcessExited(128)) [128]
 in readbytes at process.jl:458
 in dir at pkg/git.jl:10
 in git at pkg/git.jl:16
 in set_remote_url at pkg/git.jl:106
 in init at pkg/dir.jl:46
 in init at pkg.jl:17

[peter1000]

hi Mike43110,

no problem. I'm on linux and can't help you with Windows.

What I could do is copy you commit and try it out here - just not sure how to test this if it works. If you shortly instruct me I can do that for you.

[mikewl]

Don't worry, I was sorely mistaken it seems! mv and cp are fine I am just utterly, utterly blind.
New to working on Linux so unfortunately there I am a complete idiot.

Thanks for the offer though! Lesson learned: even if it seems unrelated, run the test suite anyway!
Lesson 2: Read it at least 5 times, I seem to miss the things I am looking for.

I will fix this. My internet has bombed out multiple times just testing this, making this fix quite important for me! (Wonderful South Africa)

[mikewl]

All green at last.

Thanks guys for putting up with me, I checked practically everything I could before stumbling onto the issue that git clone made a folder METADATA inside the temp folder while it cloned directly into metadata_dir. I feel quite stupid now!

But in the end, green ticks at last and now a failed init which happens oh so often won't be as much of a pain anymore!

[tkelman]

I think REQUIRE and META_BRANCH should be moved to joinpath(dir, ...) instead of metadata_dir ?

@StefanKarpinski can you take a look at this?

[mikewl]

Thanks for the comments @tkelman, those files were in the incorrect positions.

I fixed them and added tests to check their positions.

[mikewl]

Looks like I will need to test this on Ubuntu again instead of Windows.

Had a small error in the test. Windows should have failed as well.
@test isfile(joinpath(Pkg.dir(),"METADATA_BRANCH")) should have failed on windows and Linux.

[mikewl]

All is green again. I am a little concerned that windows passed initially as it shouldn't have. Is there a bug with isfile?

[tkelman]

We don't run the pkg tests on appveyor right now.

[tkelman]

Would be good to squash so we don't introduce a commit that is known to fail tests in the history of master, for the sake of future use of git bisect.

I think this could be merged after squashing, but would really like to get @StefanKarpinski's buy-in on anything Pkg-related.

[mikewl]

Aah! That clears up that piece of confusion thanks.

Understand that you want his input first completely. While he is here:
Would it be alright to try and tackle #7005? I noticed after changing my clone URL for Julia to https instead of git I manage to get a much better download speed both on windows and linux. So I am wondering if adding a flag to the Pkg related stuff to tell it to use https instead of git would provide a similar effect for Julia on my wonderfully terrible internet.

[elextr]

Just a thought, why not use base.mktempdir() and then you won't need to do this test.

[vtjnash]

it's also best not to call rm when you can avoid it, since the user would be most unhappy to lose a working directory they happened to call TEMP.

[mikewl]

I will remove that and use Base.mktempdir()
There is a potential for a failed init() to cause the subsequent tests to fail because of an unclean directory - Pkg.status() reported the TEMP folder which would cause the test @test isempty(Pkg.installed()) to fail.
Though if I clone directly to the tmpdir instead of to the package location initially there won't be any issues!

[mikewl]

current commit is just to have a place to fallback to.
Should probably add [ci skip] until its sorted. Internet is a bit spotty at the moment again making testing this a pain!

Ok! Internet working right so I could test and it all passes locally at the latest commit.

Will check if it goes green then will rebase.

[tkelman]

Let me run the pkg test on this locally just to make sure there isn't anything strange with mktempdir on Windows.

Would it be alright to try and tackle #7005? I noticed after changing my clone URL for Julia to https instead of git I manage to get a much better download speed both on windows and linux. So I am wondering if adding a flag to the Pkg related stuff to tell it to use https instead of git would provide a similar effect for Julia on my wonderfully terrible internet.

Not within the same PR, but it is a little unfortunate that nothing has come of that yet despite being bumped a few weeks ago. As a separate PR, moving towards doing more things over https (probably just by default, for security's sake) would be a good direction I think.

[tkelman]

pkg test works okay on windows at mikewl@5709b35

[mikewl]

I wonder if doing it in a minimally invasive way won't be good (for #7005).

splitmeta = split(meta,":")
    if !usehttps
        splitmeta[1] = "git"
    else
        splitmeta[1] = "https"
    end
    newmeta = ""
    for pos = 1:length(splitmeta) - 1
        newmeta = newmeta * splitmeta[pos] * ":"
    end
    newmeta = newmeta * splitmeta[end]

This can be used to change the url to https without needing to alter the METADATA repo.
I was thinking this can be placed as a function in pkg.jl or where appropriate so that depending on the flag usehttps will direct the url to wherever desired.

I tested this on init(). It works and the url in .git/config is https when the flag is set.
It should use the protocol chosen every time as the stored url isn't what is used but the transformed one. So using https initially doesn't mean you can't use the git protocol if desired.

Would this be fine for an initial change? It just allows the desired url to be selected. I need to find a proxy to test it through still.

[tkelman]

I think a regex might be a more efficient way of doing that string operation, but opening an initial proof-of-concept pr in that direction sounds like a good idea to me. Could comment on #7005 after opening to solicit feedback from people who know more about these things than I do.

[mikewl]

I did a regex initially then I realised that urls like "git://.../httpsServer.jl" may get affected. Using the array split ensures only the initial value is altered. Though I have minimal regex experience!

I am busy working on that and the formatter anyway. Worst case the PR gets rejected with a request to go in a different direction!

Thanks for the assistance though, very much appreciated.

[pao]

@Mike43110 The regex character ^, as the first character of an expression, anchors to the beginning of the string.

[tkelman]

Bump @StefanKarpinski can you take a look?

[tkelman]

I'm going to merge this, since it seems like a step in the right direction and getting reviewers for PR's has apparently become extremely difficult lately.

If we find any unforeseen consequences or problems with this we can always revert it.

[StefanKarpinski]

Sorry for the lack of review. This seems like a good approach and I'm sure we'll hear about it if it's broken.

[ScottPJones]

@StefanKarpinski If you could also please review the documentation additions that you suggested (because of my surprise issues with coming from C/C++), I'd appreciate it! ;-) #11081 -> #11099

[kmsquire]

@ScottPJones, it's generally better to bump the relevant issues with @StefanKarpinski (or whomever's attention you want) than to add to a non-related issue. Cheers!

[tkelman]

And general-purpose documentation improvements can usually be reviewed by a much larger number of people than specific details about Pkg, or compiler internals, or linear algebra, what have you, which tend to have different sets of informal "code owners."

[jakebolewski]

why is this not rm(temp_dir, recursive=true)?

[tkelman]

don't think it has to be since there shouldn't be anything left in it at this point, but maybe more defensive that way

[jakebolewski]

Arg sorry for the noise, it is moving the directories not copying them.

[mikewl]

@jakebolewski If the directory is not empty it then will error.
I had it the other way initially but thought that if some other change comes around which isn't accounted for in this then it will error.

I am not too sure whether this is a good thing though. It means if somebody adds an extra item to Pkg.init() it needs to be explicitly handled whereas if I had moved temp_dir to dir instead of each item individually that wouldn't have been a problem.

[StefanKarpinski]

It's better for that not to be a silent error, but it seems like this is kind of an unfortunately brittle arrangement.

[tkelman]

It wouldn't be silent, would it? I'd think make test-pkg would show the error pretty quickly. If dir already happens to exist then I don't think it would work to move temp_dir to dir. I don't think we have a force kwarg for mv.

[StefanKarpinski]

Right, the current behavior is better since it would fail if this directory weren't empty, so I guess we're ok.

[mikewl]

Without using -force which isn't available there is the option of removing the specific version directory completely though that also seems like it could have unintended consequences as well.

[peter1000]

julia could add the kwarg: remove_destination::Bool=false to mv to be in line with cp

cp(src::AbstractString, dst::AbstractString; remove_destination::Bool=false, follow_symlinks::Bool=false)

If that is wanted I could do it.

[StefanKarpinski]

Yes please, @peter1000!

[peter1000]

Ok probably tomorrow with docs and test case.

[mikewl]

Haunts @peter1000

[peter1000]

According to my reading: the current implementation of mv is not supposed to work on directories even the doc say: Move a file from src to dst

see related issue I opend: #11145