Jeff King [off-list ref] writes:
quoted hunk
@@ -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?
On Thu, Feb 02, 2012 at 04:47:31PM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
@@ -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
quoted
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