Improve check_not_dirty git repo detection and status shell output of results.

[dekellum]

Fixes #5823

Also fix associated tests. This is an attempt to clarify windows+git package behavior as uncovered in #5786. This is a Work In Progress not yet ready for merge.

This conflicts with #5786, but either could be merged first, requiring a rebase/merge in the other.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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. Due to 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.

[dekellum]

Checks pass and this fixes the windows disparity of #5823. This conflicts with #5786, but its probably best if this is merged to master first, then I'll merge and resolve conflicts in #5786, avoiding the need for a cfg(windows) specific test variant.

@alexcrichton: r? Merge?

[dekellum]

Ping @alexcrichton, this bug fix should be easy to review and merge, no?

[ehuss]

This looks to be the same as https://github.com/alexcrichton/git2-rs/pull/335. I did a quick check, and converting the \ to / in status_file() fixes it. I would say the correct fix is to have git2-rs convert all paths, or (wishful thinking) have libgit2 handle windows-style paths (that's not correct).

[alexcrichton]

Thanks for the PR for this! I'm not sure I quite understand what's going on here. I'd definitely believe that libgit2's handling of \ vs / isn't always what we expect, but the tests aren't currently failing and it doesn't look like many major changes happened here to the tests?

[dekellum]

I want a reasonable workaround without (probably, let me know otherwise) changing the minimum release of git2-rs as a cargo dependency, and I also don't want to wait for that release. Could I just add that (\) to (/) conversion here on L171? The reason I suggest adding it, is that the verbose console logging, and the fall-back-to-workspace is I think value add, even if you are correct on the cause, @ehuss. Thoughts?

@alexcrichton minimal changes to tests in this PR is correct, but your point is alluding me. As to what is going on: Most of the tests use the verbose console output, or files included in the package. While on master check_not_dirty doesn't include verbose console output and doesn't influence the files included (sans #5786), so given that, check_not_dirty and its git-repo detection, hasn't really been thoroughly tested for the no-repo-found vs clean-repo cases (which look identical for the mentioned outputs). I hope that makes sense? I'm not sure its what you are asking however.

[ehuss]

@alexcrichton the repro is:

1. cargo new foo
2. cd foo
3. cargo new bar
4. cd bar
5. cargo package

Step 5 should fail, but it passes on Windows because it uses the path bar\\Cargo.toml to git_status_file(), and it returns None instead of WT_NEW.

[alexcrichton]

Hm so do we have a failing test on Windows today? How come these tests aren't failing on CI?

[ehuss]

Hm so do we have a failing test on Windows today? How come these tests aren't failing on CI?

No, there are no current tests that exercise this code path with a slash in it. The closest is package_verbose, but it specifies --no-verify which turns it off. @dekellum used the code in check_not_dirty to implement the new feature, and in the process uncovered the problem because it failed to get the git info in the package_verbose test on windows.

[dekellum]

@ehuss isn't answering my questions, but perhaps there are multiple possible fixes for #5823 (@ehuss's experiment, and this PR)? I'm not sure at present what is best: one, the other, or both?

[ehuss]

@ehuss isn't answering my questions

Oh, sorry I missed that. My vote is to update git2-rs to use the correct path type. I also might be willing to help audit the api to see if there are any other places that need updating, but I probably won't get to that soon. What would be the benefit of running discover twice, once with an incorrect path? Edit: Oh I misunderstood, you said with a path translation. I'm not sure that's possible (to translate it here).

[dekellum]

Thanks @ehuss, as I said, yes, fix git2-rs for the root cause; but if we can workaround it here first, that is helpful. I went from detected-broken-on-windows (adc5074) to fixed-on-all-platforms (ad0b252) in this PR. Would you suggest any other changes for this fix? Why couldn't I just convert the slashes on that line, on windows only?

[ehuss]

If git-rs is fixed, these changes wouldn't be necessary, correct? Personally I prefer to fix root issues if possible rather than work around them. Also, I'm not sure I understand why discover is being called twice.

[alexcrichton]

I agree with @ehuss that if the issue is in git2 we should fix it there as it ideally would be fixed in libgit2 but git2 is our best bet for now.

[dekellum]

So do you just want to close this? The verbose console logging changes included here make the issue on windows at least testable, which seems healthy, no? Or perhaps you are less concerned about that?

[alexcrichton]

Sure yeah, closing this is ok for now. Would you like to create a dedicated PR for the UI changes? Would you be interested in making the change to git2-rs?

[dekellum]

Based objectively on this experience of two rejected PRs, with little to no reasonable cause understood, no I do not wish to make the changes in git2-rs, no thanks

Edit: My success rate has been pretty low here, so I want to make sure we are talking about the same thing and clearly. By this:

Would you like to create a dedicated PR for the UI changes?

Do you mean, a new PR that adds the verbose console logging for check_not_dirty and fixes the associated tests, but without any attempt to actually workaround the #5823 issue on windows?

(Despite the fact that this PR demonstrates a workaround, at least for the test failure found?)

[dekellum]

Oh, and note: the tests for this potential new PR will necessarily depend on a real fix in git2-rs.

[alexcrichton]

@dekellum oh you mentioned that the console logging changes seemed like a good idea regardless so we can always have PRs for things like that, and having a workaround is fine for now as well if you'd prefer to not make a fix to git2-rs