sources/path: if PathSource::list_files fails while using a git repo, retry without it

[codyps]

Sometimes discover_git_repo() can succeed even though the git repo
isn't complete. This resulted in us failing to build in some cases (I
noted the case where HEAD is valid but other objects are missing due to
alternates not being accessible, and we trigger use of list_files() by
having a build script). Resolve this by always retrying without a git
repo if we fail to list files while using one.

Fixes #10311.

Note: this may cause retries in list_files that are unrelated to git issues. I think these should be minor (if they ever occur). It is possible to structure things as a loop rather than a separate function and only trigger retries on git issues, but the separate function seemed cleaner. (It's also possible one of the other alternates solutions in #10311 might be a better choice because of some downside I'm not aware of. Let me know if that's the case).

Also, right now this code will never indicate if there was an issue examining the git repo (and silently fall back to the non-git-repo file listing). This makes sense given the issue I reported in #10311, but seems like it might allow us to unknowingly break handling of the git files list (given it will silently fallback). Thoughts here would be useful.

[rust-highfive]

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

[weihanglo]

Thanks for the PR! I am wary of the retry mechanism. PathSource::list_files are used in three places at this moment:

1. Determine fingerprint of rustdoc or package with a build script.
2. cargo package
3. cargo vendor

Since the file-listing logic in list_files_git and list_files_walk are totally different, I am afraid that some unexpected/sensitive files might be included without notice.

[codyps]

It's important to note that in many cases, an invalid git repo already causes us to use the list_files_walk mechanism: for example, if one is using git worktree for their git directories (and makes the source inaccessible) or if one uses git clone --reference but without a valid HEAD (ie: no extra commit as in the reproducer in #10311) we already fall back to using list_files_walk instead of list_files_git.

It's my belief that we're using list_files_git for better accuracy if include is not specified, but it's not a requirement, just an optimization to better match user expectations wrt what files are included. In other words: it's not great if we have a less accurate mechanism (like list_files_git is less accurate than using include in metadata), but it doesn't seem like this would block us from using a fallback.

[joshtriplett]

I definitely am not comfortable with the silent fallback; at a minimum it should emit a detailed warning (e.g. about a missing object).

However, even better, I'd love to see Cargo just emit that as an error and stop, and suggest that the user could pass something like --no-git to bypass the broken git repo.

[codyps]

@joshtriplett what are your thoughts on the boundary between "fallback" and "detecting a valid git directory" here? In some ways, we're already falling back to not using git in many cases.

More generally, I think it's useful to consider the mental model here: It's (from my perspective as a user) somewhat surprising that cargo itself is trying to treat the location where my crate is located as a git repository. It's nice that it does (when it's not broken, like this bug), but it's somewhat questionable when it causes build failures.

One option we have is to adopt the mental model that if treating the directory as a git repository fails, then clearly the directory was not actually a git repository. This is essentially the model of this PR.

Another option though is for us to reduce our reliance on a fully formed git repository. The cargo documentation for include/exclude (which also documents its use of git to some extent), only indicates that the .gitignore files are being used if the crate lives in a git repo (https://doc.rust-lang.org/cargo/reference/manifest.html#the-exclude-and-include-fields). This particular bug in cargo occurs because that documentation isn't entirely accurate: while the gitignore system is used, cargo is also using the git index combined with the gitignore system to determine appropriate files to list. If cargo instead just did what was claimed by the documentation, this error would not occur (because git here is failing while trying to examine the index).

A related option to the last one is to continue examining the index (with a documentation update), but make the index not being usable result in us just using the gitignore system.

[joshtriplett]

@jmesmon My thinking here is primarily about consequences: I'm concerned about falling back from git to not-git causing cargo publish or cargo package to include a file that the user doesn't want included (e.g. because we don't pay attention to .gitignore and similar in the non-git case). That's an especially terrible failure, which may result in harm to a user through publishing information they didn't want. (It could also result in less awful but still annoying failure modes, as well as issues of publishing too little and ending up with a non-functional crate.)

One way to reduce potential harms would be to warn but continue in less consequential situations, such as cargo build, but hard-error in more consequential situations, such as cargo publish or cargo package. That seems much safer.

Another orthogonal improvement would be trying to be more consistent about what information we process, such as always processing .gitignore files even if we're not in a git repository. (We could do that with the ignore crate, for instance.) We'd have to take care about backwards compatibility, but doing what we can to improve consistency between these cases seems like an improvement.

I'd prioritize reducing potential harms in the publish/package/etc case, though.

[codyps]

Sure, it sounds like you're proposing that rather than this fix, we do something more complicated. To do the more complicated thing (if we do decide to go that way), we do need to more definitively define what "inside a git repo" means, as that's part of the reason that this bug occurred in the first place. Don't see any one proposing/clarifying what that might mean (other than myself). It would be very useful to have direct discussion of this point.

Some breakdown of conditions as they currently exist vs how this PR would change things:

condition	"in git repo" in current cargo	"in git repo" in this PR
HEAD exists and all it's file/tree objects exist, but parent commits may not	✔️	✔️
HEAD exists, but file/tree objects it refers to may not	✔️ (broken build)	✖️
.git (and subdirs) exist, but HEAD object does not	✖️	✖️
.git exists, but has no content	✖️	✖️

It will be difficult to make changes to this code without us establishing what this boundary is supposed to be. Is it simply "does git_discover_repo work" (as it is currently defined in cargo) or is some other definition better for us here? Given the concern folks have with having list_files return too many files (because we don't handle ignores if not "in a git repo"), it seems like it might make sense for it to decide we're in a git repo more often than git_discover_repo (even though this PR makes us decide we're in a git repo less often to fix the current build failure).

[codyps]

Oh, also: we're already using the ignore crate for gitignores here. We just inseperably (at the moment) mix it in with using the libgit2 index as well (hence the bug).

[joshtriplett]

@jmesmon At the moment, I think I'd be satisfied if we just turn the warning into a hard error if we're in cargo publish or cargo package. That would address all my concerns about including too many files by mistake.

[alexcrichton]

r? @joshtriplett

[weihanglo]

Ping @jmesmon. Just checking in to see if you are still interested in working on this. The comment here telling one of the expected solution. Let us know if you can continue in that way, or have any question. Thank you!

[bors]

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

[weihanglo]

I'm going to close due to inactivity. We are still interested in some solutions here, specifically #10313 (comment). Feel free to reopen if you have a chance to work on it again. Thank you.