On Thu, Feb 02, 2012 at 04:47:31PM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html