Re: Alternates corruption issue
- From
Jeff King <peff@peff.net>
- Date
- Feb 3, 2012, 12:02 UTC
- Message-ID
- <20120203120215.GA31441@sigill.intra.peff.net>
- In-Reply-To
- <7vzkd0u4ik.fsf@alter.siamese.dyndns.org>
On Thu, Feb 02, 2012 at 04:47:31PM -0800, Junio C Hamano wrote:
Show 22 quoted lines
> Jeff King <peff@peff.net> writes:
>
> > @@ -324,8 +324,11 @@ const char *enter_repo(const char *path, int strict)
> > return NULL;
> > len = strlen(used_path);
> > for (i = 0; suffix[i]; i++) {
> > + struct stat st;
> > strcpy(used_path + len, suffix[i]);
> > - if (!access(used_path, F_OK)) {
> > + if (!stat(used_path, &st) &&
> > + (S_ISREG(st.st_mode) ||
> > + (S_ISDIR(st.st_mode) && is_git_directory(used_path)))) {
>
> Hmm, how would this change interact with
>
> > strcat(validated_path, suffix[i]);
> > break;
> > }
>
> gitfile = read_gitfile(used_path);
>
> that appear after the context in the patch?It assumes that any file named ".git" is worth reading and selecting. And then later we actually read_gitfile to find out if it's worth-while. There is no change of behavior from before the patch, as we would similarly notice the file (without checking if it's a real gitfile) and then later read it and possibly fail.
However, with the ordering change, there is a technically a regression in one case: a random file "foo" next to a repo "foo.git". Saying "git ls-remote foo" used to prefer "foo.git", and will now select the file "foo" only to fail.
The code-path in clone's get_repo_path handles this properly (it checks that the path is really a valid gitfile before finishing the loop). The gitfile-reading from later in enter_repo could be hoisted into the loop. If was trying to make a less-invasive change; if we're going to do that much rewriting, it probably makes sense to factor out the logic from get_repo_path and have them share the code.
Thanks for noticing. I saw this issue when I was writing the original version of the patch, and meant to revisit it and at least document it in the commit message, but I ended up forgetting.
-Peff