Git tags when packaging #841

[tinco]

Fixes #841

This PR is not ready to merge yet, filing it to ask for code review and guidance. This is my first contribution to a rust-lang project. Is this PR going in the right direction? Is there consensus that cargo publish should git tag?

Also, I used git-rs here, but I feel this code would be much more succinct and less fragile if it would invoke git through shell commands. A downside to that would be that cargo would depend on git being installed, I guess we want to avoid that?

Todo: I need git2 0.3 for git push --tags to work so at the moment I don't do that. Can I upgrade git-rs in this PR or should I file a separate one?

All critiques and opinions welcome, thanks!

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[steveklabnik]

I guess we want to avoid that?

Yes, introducing a new dependency wouldn't be great. Why would it be less fragile?

[alexcrichton]

Whoa awesome, happy to see this implemented now! @steveklabnik is right in that we should avoid executing commands at all costs as we're not guaranteed that git is installed everywhere. I'm also curious how you found git2-rs fragile? I've found the support in libgit2 to be quite robust and have been very impressed by it in general.

I need git2 0.3 for git push

We avoided git2 0.3 in the past because it didn't build on the bots we build Cargo on (one of the OSX builders was too old), but this was fixed in upstream libgit2 and I just pushed new versions of git2-rs to pick up the updates, so we should be good to go in updating to 0.3 now. To do the update you can probably just revert this PR.

All that being said, though, this probably doesn't want to do any network traffic, just make the tag itself. Overall you may want to take a look at what bundler does in this respect, for example, as we probably want to just mirror what happens over there. For example:

• The network activity probably is left up to the user because it may not go to the origin remote but perhaps some other.
• The tag and/or publish may want to be avoided if there are uncommitted changes in the repository.
• The tag may want to be avoided if there's already an existing tag of that name.
• etc

[alexcrichton]

Ah and as a final note, can you be sure to add some tests as part of this PR?

[steveklabnik]

In the original issue, I show what Bundler does, but @alexcrichton is right that there's som eextra error handling.

[steveklabnik]

(and it always does the push, it just does git push && git push --tags, so it's not naming origin.

[tinco]

Thanks guys. The reason I said it's fragile is that it's pretty low level. For example in a git command pushing to the configured matching remote and branch it git push and it will magically figure everything out and fail if something non standard is going on.

In libgit you have to find out what the current branch is by getting the symbolic reference of HEAD, if it is a symbolic reference, if it's not then I think you're in a detached state and probably just should throw an error. Then you fill in the current branch in the refspecs, but for the complete refspec you'll have to find the matching remote, which I haven't found out yet how to do so I hardcoded origin. So it's not fragile per se, just a whole lot of low level git commands.

Not complaining, I actually like figuring it all out, I get to flaunt my git skills ;) but the fact is that it's going to be over 20 lines instead of 3 with if statements and weak types :)

Should I revert that git-rs 3.0 PR in this PR?

[steveklabnik]

That makes total sense, thanks :)

[alexcrichton]

Yeah feel free to throw in the upgrade to git2 0.3 here!

[tinco]

Hi guys I'm having trouble with getting the test to work and I feel like I'm really missing something in how the test framework works. In the last commit you see I add a call to repo(test_project_path) to make it init a git project there. I added tracing println!'s everywhere and all the code seems to run perfect but a git dir is just never created, or it is but is deleted right after? Also there's a setup function in the test file that also seems to initiate a git dir, but it's not called anywhere. 😕

[tinco]

@alexcrichton sorry I got stuck a little, should I spend more time figuring this out or do you know what's going on?

[bors]

☔ The latest upstream changes (presumably #1972) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

Each invocation of cargo_process actually deletes the entire source tree and rebuilds it, so maybe that's the problem you're seeing?

[tinco]

Made some progress. What should I do when there's no git repo, warn or fail silently?

[alexcrichton]

If the source tree isn't a git repo it should probably just abort this path entirely because it could be something like mercurial or SVN (or just not in a VCS)

[tinco]

I'm noobing out a bit again, the compiler says this:

src/cargo/ops/registry.rs:62:5: 103:6 note: expansion site
src/cargo/ops/registry.rs:86:33: 86:61 error: the type of this value must be known in this context
src/cargo/ops/registry.rs:86         let branch_name: &str = try!(branch.name()).unwrap();
                                                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

I can't get it to go away and I'm not sure why it wouldn't detect the type. Is it because Branch is supposed to have a lifetime?

[alexcrichton]

Looks like you found the issue? (or @m4rw3r perhaps?)

[m4rw3r]

It was the line before the error above which was the source, current stable did not actually error on that line despite try! expanding to a match-expression which requires a Result. I tried to compile the code on nightly which produced a better error message showing parts of the expanded code and it complained that git2::Branch::wrap() does not return a Result. So the solution was to remove the try around git2::Branch::wrap().

[alexcrichton]

Closing due to inactivity, but feel free to resubmit with a rebase and comments addressed!