Re: Alternates corruption issue

[Jeff King <peff@peff.net>]

On Thu, Feb 02, 2012 at 04:47:31PM -0800, Junio C Hamano wrote:

> 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