* 'git add' regression in git-1.7?
@ 2010-02-19 4:30 SungHyun Nam
2010-02-19 4:42 ` Avery Pennarun
0 siblings, 1 reply; 17+ messages in thread
From: SungHyun Nam @ 2010-02-19 4:30 UTC (permalink / raw)
To: git
Hello,
'git add' does NOT add files in ignored path.
When the .gitignore file contains:
tmp/
If I do:
git add tmp/test.txt
Nothing happens.
If I removed 'tmp/' from the .gitignore, git add works
fine.
Because I have backup copies of GIT, I tested serveral versions.
The version below works as expected.
git version 1.6.6.243.gff6d2
And below does NOT work.
git version 1.7.0.rc1.7.gc0da5
git version 1.7.0.31.g1df487
Thanks,
namsh
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: 'git add' regression in git-1.7? 2010-02-19 4:30 'git add' regression in git-1.7? SungHyun Nam @ 2010-02-19 4:42 ` Avery Pennarun 2010-02-19 5:04 ` SungHyun Nam 0 siblings, 1 reply; 17+ messages in thread From: Avery Pennarun @ 2010-02-19 4:42 UTC (permalink / raw) To: SungHyun Nam; +Cc: git On Thu, Feb 18, 2010 at 11:30 PM, SungHyun Nam <goweol@gmail.com> wrote: > 'git add' does NOT add files in ignored path. > > When the .gitignore file contains: > tmp/ > If I do: > git add tmp/test.txt > Nothing happens. Try using: git add -f tmp/test.txt Avery ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-02-19 4:42 ` Avery Pennarun @ 2010-02-19 5:04 ` SungHyun Nam 2010-02-19 5:15 ` Avery Pennarun 0 siblings, 1 reply; 17+ messages in thread From: SungHyun Nam @ 2010-02-19 5:04 UTC (permalink / raw) To: git Avery Pennarun wrote: > On Thu, Feb 18, 2010 at 11:30 PM, SungHyun Nam<goweol@gmail.com> wrote: >> 'git add' does NOT add files in ignored path. >> >> When the .gitignore file contains: >> tmp/ >> If I do: >> git add tmp/test.txt >> Nothing happens. > > Try using: > git add -f tmp/test.txt Thanks, it works. Well, before sending the previous email, I checked the RelNotes-1.7.*.txt, and could not find such a change by searching 'git add'. So, I thought it's a regression. Regards, namsh ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-02-19 5:04 ` SungHyun Nam @ 2010-02-19 5:15 ` Avery Pennarun 2010-02-19 5:34 ` Jeff King 0 siblings, 1 reply; 17+ messages in thread From: Avery Pennarun @ 2010-02-19 5:15 UTC (permalink / raw) To: SungHyun Nam; +Cc: git On Fri, Feb 19, 2010 at 12:04 AM, SungHyun Nam <goweol@gmail.com> wrote: > Well, before sending the previous email, I checked the > RelNotes-1.7.*.txt, and could not find such a change by searching > 'git add'. So, I thought it's a regression. As far as I know, git add has refused to add ignored files for as long as I can remember. Maybe there was briefly a bug in this behaviour that was later fixed... If you use 'git bisect' on the git repo, you could probably discover what happened, in case you're interested. Avery ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-02-19 5:15 ` Avery Pennarun @ 2010-02-19 5:34 ` Jeff King 2010-02-19 6:02 ` Jeff King 0 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2010-02-19 5:34 UTC (permalink / raw) To: Avery Pennarun; +Cc: SungHyun Nam, git On Fri, Feb 19, 2010 at 12:15:02AM -0500, Avery Pennarun wrote: > On Fri, Feb 19, 2010 at 12:04 AM, SungHyun Nam <goweol@gmail.com> wrote: > > Well, before sending the previous email, I checked the > > RelNotes-1.7.*.txt, and could not find such a change by searching > > 'git add'. So, I thought it's a regression. > > As far as I know, git add has refused to add ignored files for as long > as I can remember. Maybe there was briefly a bug in this behaviour > that was later fixed... > > If you use 'git bisect' on the git repo, you could probably discover > what happened, in case you're interested. It was intentional. Try 48ffef9 (ls-files: fix overeager pathspec optimization, 2010-01-08). -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-02-19 5:34 ` Jeff King @ 2010-02-19 6:02 ` Jeff King 2010-02-19 8:24 ` Jeff King 0 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2010-02-19 6:02 UTC (permalink / raw) To: Avery Pennarun; +Cc: SungHyun Nam, git On Fri, Feb 19, 2010 at 12:34:31AM -0500, Jeff King wrote: > > On Fri, Feb 19, 2010 at 12:04 AM, SungHyun Nam <goweol@gmail.com> wrote: > > > Well, before sending the previous email, I checked the > > > RelNotes-1.7.*.txt, and could not find such a change by searching > > > 'git add'. So, I thought it's a regression. > > > > As far as I know, git add has refused to add ignored files for as long > > as I can remember. Maybe there was briefly a bug in this behaviour > > that was later fixed... > > > > If you use 'git bisect' on the git repo, you could probably discover > > what happened, in case you're interested. > > It was intentional. Try 48ffef9 (ls-files: fix overeager pathspec > optimization, 2010-01-08). But this is a little disturbing still: $ git init $ mkdir dir $ touch dir/sub $ touch root $ echo dir >.gitignore $ echo root >>.gitignore $ git add root The following paths are ignored by one of your .gitignore files: root Use -f if you really want to add them. fatal: no files added $ echo $? 128 $ git add dir The following paths are ignored by one of your .gitignore files: dir Use -f if you really want to add them. fatal: no files added $ echo $? 128 $ git add dir/sub $ echo $? 0 but we didn't actually add the file. -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-02-19 6:02 ` Jeff King @ 2010-02-19 8:24 ` Jeff King 2010-03-01 2:00 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2010-02-19 8:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: Avery Pennarun, SungHyun Nam, git On Fri, Feb 19, 2010 at 01:02:49AM -0500, Jeff King wrote: > But this is a little disturbing still: > > $ git init > $ mkdir dir > $ touch dir/sub > $ touch root > $ echo dir >.gitignore > $ echo root >>.gitignore > > $ git add root > The following paths are ignored by one of your .gitignore files: > root > Use -f if you really want to add them. > fatal: no files added > $ echo $? > 128 > > $ git add dir > The following paths are ignored by one of your .gitignore files: > dir > Use -f if you really want to add them. > fatal: no files added > $ echo $? > 128 > > $ git add dir/sub > $ echo $? > 0 > > but we didn't actually add the file. Junio, This seems to be caused by dir.c:treat_one_path. In the first few lines: int exclude = excluded(dir, path, &dtype); if (exclude && (dir->flags & DIR_COLLECT_IGNORED) && in_pathspec(path, *len, simplify)) dir_add_ignored(dir, path, *len); we see that the prefix "dir" is excluded, but it is not in our pathspec ("dir/sub"), so we do not add it to the ignored list. This is related to your recent 48ffef9 (ls-files: fix overeager pathspec optimization, 2010-01-08), as before then we actually didn't consider "dir/sub" to be ignored at all. The in_pathspec check did not originate there; it's from my e96980e (builtin-add: simplify (and increase accuracy of) exclude handling, 2007-06-12). But it is definitely still necessary. I'm not sure of the right way to fix this. We can drop further down into the directory hierarchy when doing COLLECT_IGNORED and look for actual files, but that may have a negative performance impact. Perhaps we can go further only if we are a prefix of a pathspec. Or maybe there is some way to be more clever. I dunno. I'm out of ideas for the evening, and since you looked at this not too long ago, I thought you might have some insight. -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-02-19 8:24 ` Jeff King @ 2010-03-01 2:00 ` Junio C Hamano 2010-03-01 3:25 ` [PATCH] add: fail "git add ignored-dir/file" without -f Junio C Hamano 2010-03-09 22:37 ` 'git add' regression in git-1.7? Jeff King 0 siblings, 2 replies; 17+ messages in thread From: Junio C Hamano @ 2010-03-01 2:00 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Avery Pennarun, SungHyun Nam, git Jeff King <peff@peff.net> writes: > I'm not sure of the right way to fix this. We can drop further down into > the directory hierarchy when doing COLLECT_IGNORED and look for actual > files, but that may have a negative performance impact. Wouldn't that have negative correctness impact? I don't see an obvious way out, other than perhaps checking the set of pathspecs twice. One thing that might help is to carry the seen[] array a bit longer so that we do not have to lose sight of what paths we were given but didn't match. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] add: fail "git add ignored-dir/file" without -f 2010-03-01 2:00 ` Junio C Hamano @ 2010-03-01 3:25 ` Junio C Hamano 2010-03-01 8:26 ` [PATCH 1/3] t0050: mark non-working test as such Junio C Hamano 2010-03-09 22:37 ` 'git add' regression in git-1.7? Jeff King 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2010-03-01 3:25 UTC (permalink / raw) To: Jeff King; +Cc: Avery Pennarun, SungHyun Nam, git "git add <pathspec>" usually fails the request and gives an advice message to use the -f option when <pathspec> exactly names an existing path in the work tree. However, we didn't issue the warning nor fail the request when the <pathspec> matches an existing path but it is ignored because a higher level directory is ignored as a whole. In such a case, we do not even descend into it (and we shouldn't). This catches such a case and issues some warning. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * This is still provisional and I am not fully happy with it; it seems to pass the tests. The error message is based on the full directory name we are culling, not on the actual pathspec the user gave us, as we do not have access to it. dir.c | 15 +++++----- t/t2204-add-ignored.sh | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 7 deletions(-) create mode 100755 t/t2204-add-ignored.sh diff --git a/dir.c b/dir.c index 00d698d..af4ba92 100644 --- a/dir.c +++ b/dir.c @@ -413,13 +413,10 @@ static struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathna return dir->entries[dir->nr++] = dir_entry_new(pathname, len); } -static struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname, int len) +static void dir_add_ignored(struct dir_struct *dir, const char *pathname, int len) { - if (!cache_name_is_other(pathname, len)) - return NULL; - ALLOC_GROW(dir->ignored, dir->ignored_nr+1, dir->ignored_alloc); - return dir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len); + dir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len); } enum exist_status { @@ -638,7 +635,8 @@ static enum path_treatment treat_one_path(struct dir_struct *dir, { int exclude = excluded(dir, path, &dtype); if (exclude && (dir->flags & DIR_COLLECT_IGNORED) - && in_pathspec(path, *len, simplify)) + && in_pathspec(path, *len, simplify) + && cache_name_is_other(path, *len)) dir_add_ignored(dir, path, *len); /* @@ -841,8 +839,11 @@ static int treat_leading_path(struct dir_struct *dir, return 0; blen = baselen; if (treat_one_path(dir, pathbuf, &blen, simplify, - DT_DIR, NULL) == path_ignored) + DT_DIR, NULL) == path_ignored) { + if (dir->flags & DIR_COLLECT_IGNORED) + dir_add_ignored(dir, pathbuf, baselen); return 0; /* do not recurse into it */ + } if (len <= baselen) return 1; /* finished checking */ } diff --git a/t/t2204-add-ignored.sh b/t/t2204-add-ignored.sh new file mode 100755 index 0000000..c1ce12b --- /dev/null +++ b/t/t2204-add-ignored.sh @@ -0,0 +1,70 @@ +#!/bin/sh + +test_description='giving ignored paths to git add' + +. ./test-lib.sh + +test_expect_success setup ' + mkdir sub dir dir/sub && + echo sub >.gitignore && + echo ign >>.gitignore && + for p in . sub dir dir/sub + do + >"$p/ign" && + >"$p/file" || exit 1 + done +' + +for i in file dir/file dir 'd*' +do + test_expect_success "no complaints for unignored $i" ' + rm -f .git/index && + git add "$i" && + git ls-files "$i" >out && + test -s out + ' +done + +for i in ign dir/ign dir/sub dir/sub/*ign sub/file sub sub/* +do + test_expect_success "complaints for ignored $i" ' + rm -f .git/index && + test_must_fail git add "$i" 2>err && + git ls-files "$i" >out && + ! test -s out && + grep -e "Use -f if" err && + cat err + ' +done + +for i in sub sub/* +do + test_expect_success "complaints for ignored $i in dir" ' + rm -f .git/index && + ( + cd dir && + test_must_fail git add "$i" 2>err && + git ls-files "$i" >out && + ! test -s out && + grep -e "Use -f if" err && + cat err + ) + ' +done + +for i in ign file +do + test_expect_success "complaints for ignored $i in sub" ' + rm -f .git/index && + ( + cd sub && + test_must_fail git add "$i" 2>err && + git ls-files "$i" >out && + ! test -s out && + grep -e "Use -f if" err && + cat err + ) + ' +done + +test_done -- 1.7.0.1.241.g6604f ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 1/3] t0050: mark non-working test as such 2010-03-01 3:25 ` [PATCH] add: fail "git add ignored-dir/file" without -f Junio C Hamano @ 2010-03-01 8:26 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2010-03-01 8:26 UTC (permalink / raw) To: Steffen Prohaska; +Cc: Jeff King, Avery Pennarun, SungHyun Nam, git The test is to prepare an empty file "camelcase" in the index, remove and replace it with another file "CamelCase" with "1" as its contents in the working tree, and add it to the index, in a repository configured to be case insensitive. However, the test actually checked ls-files knows about a pathname that matches "camelcase" case insensitively. It didn't check if the added contents actually was the updated one. Mark the test as non-working. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * This comes from 8a19aaa (t0050: Add test for case insensitive add, 2008-05-11); back then the code did add both camelcase and CamelCase, and somewhere after that we once fixed it, but 1e5f764 (builtin-add.c: optimize -A option and "git add .", 2008-07-22) broke it in a different way (namely, it stopped adding the updated contents); I am not going to have time to debug it further anytime soon (hint hint)... t/t0050-filesystem.sh | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-) diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh index 89282cc..41df6bc 100755 --- a/t/t0050-filesystem.sh +++ b/t/t0050-filesystem.sh @@ -108,13 +108,17 @@ $test_case 'merge (case change)' ' ' -$test_case 'add (with different case)' ' + + +test_expect_failure 'add (with different case)' ' git reset --hard initial && rm camelcase && echo 1 >CamelCase && git add CamelCase && - test $(git ls-files | grep -i camelcase | wc -l) = 1 + camel=$(git ls-files | grep -i camelcase) && + test $(echo "$camel" | wc -l) = 1 && + test "z$(git cat-file blob :$camel)" = z1 ' -- 1.7.0.1.241.g6604f ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-03-01 2:00 ` Junio C Hamano 2010-03-01 3:25 ` [PATCH] add: fail "git add ignored-dir/file" without -f Junio C Hamano @ 2010-03-09 22:37 ` Jeff King 2010-03-09 23:09 ` Jeff King 1 sibling, 1 reply; 17+ messages in thread From: Jeff King @ 2010-03-09 22:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: Avery Pennarun, SungHyun Nam, git On Sun, Feb 28, 2010 at 06:00:17PM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > I'm not sure of the right way to fix this. We can drop further down into > > the directory hierarchy when doing COLLECT_IGNORED and look for actual > > files, but that may have a negative performance impact. > > Wouldn't that have negative correctness impact? I don't see an obvious > way out, other than perhaps checking the set of pathspecs twice. One > thing that might help is to carry the seen[] array a bit longer so that we > do not have to lose sight of what paths we were given but didn't match. Sorry for the very late reply. Day-job has kept me busy. No, we would still be correct if we recurse into the ignored directory _only_ to collect the ignored bits (so we don't even bother if COLLECT_IGNORED isn't set). But what I don't like is that you take a performance hit, because in most cases you won't ever care what's inside those directories. You need to recurse only when: - you actually care about all files. git-add does. git-status does not (unless you explicitly told it to show directories). So that would probably need a flag passed to fill_directory. - you have a pathspec that means the contents of the directory might be interesting. Right now we check in_pathspec in treat_one_path. But I think we would need to recognize that "subdir/file" is means "subdir" is in our pathspec (and that "sub*" means the same thing). Your solution does something similar after the fact, but I am not 100% satisfied with it. I'll respond separately to that patch. -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-03-09 22:37 ` 'git add' regression in git-1.7? Jeff King @ 2010-03-09 23:09 ` Jeff King 2010-03-10 7:06 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2010-03-09 23:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Avery Pennarun, SungHyun Nam, git On Tue, Mar 09, 2010 at 05:37:30PM -0500, Jeff King wrote: > No, we would still be correct if we recurse into the ignored directory > _only_ to collect the ignored bits (so we don't even bother if > COLLECT_IGNORED isn't set). But what I don't like is that you take a > performance hit, because in most cases you won't ever care what's inside > those directories. You need to recurse only when: > > - you actually care about all files. git-add does. git-status does not > (unless you explicitly told it to show directories). So that would > probably need a flag passed to fill_directory. > > - you have a pathspec that means the contents of the directory might > be interesting. Right now we check in_pathspec in treat_one_path. > But I think we would need to recognize that "subdir/file" is > means "subdir" is in our pathspec (and that "sub*" means the same > thing). Actually, if we accept that the message simply mentions the excluded path, i.e.: $ git add subdir/file The following paths are ignored by one of your .gitignore files: subdir Use -f if you really want to add them. then we don't really need to recurse. We just need to fix in_pathspec to flag files that are _relevant_ to a pathspec. And something like this seems to fix the OP's problem: diff --git a/dir.c b/dir.c index 00d698d..5091bfd 100644 --- a/dir.c +++ b/dir.c @@ -554,13 +554,17 @@ static int simplify_away(const char *path, int pathlen, const struct path_simpli return 0; } -static int in_pathspec(const char *path, int len, const struct path_simplify *simplify) +static int relevant_pathspec(const char *path, int len, const struct path_simplify *simplify) { if (simplify) { for (; simplify->path; simplify++) { if (len == simplify->len && !memcmp(path, simplify->path, len)) return 1; + if (len < simplify->len + && simplify->path[len] == '/' + && !memcmp(path, simplify->path, len)) + return 1; } } return 0; @@ -638,7 +642,7 @@ static enum path_treatment treat_one_path(struct dir_struct *dir, { int exclude = excluded(dir, path, &dtype); if (exclude && (dir->flags & DIR_COLLECT_IGNORED) - && in_pathspec(path, *len, simplify)) + && relevant_pathspec(path, *len, simplify)) dir_add_ignored(dir, path, *len); /* Which is similar to your fix, but hoisted into the ignore-collection phase. Like the original code and your patch, it suffers from using a straight memcmp. I think it should actually be checking the pathspec expansion to catch things like 'sub*/file' being relevant to 'subdir'. -Peff ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-03-09 23:09 ` Jeff King @ 2010-03-10 7:06 ` Junio C Hamano 2010-03-11 7:15 ` Jeff King 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2010-03-10 7:06 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Avery Pennarun, SungHyun Nam, git Jeff King <peff@peff.net> writes: > Actually, if we accept that the message simply mentions the excluded > path, i.e.: > > $ git add subdir/file > The following paths are ignored by one of your .gitignore files: > subdir > Use -f if you really want to add them. > > then we don't really need to recurse. We just need to fix in_pathspec to > flag files that are _relevant_ to a pathspec. Clever and to the point. > And something like this seems to fix the OP's problem: > ... > Which is similar to your fix, but hoisted into the ignore-collection > phase. Like the original code and your patch, it suffers from using a > straight memcmp. I think it should actually be checking the pathspec > expansion to catch things like 'sub*/file' being relevant to 'subdir'. Yeah. Care to roll a patch to replace 13bb0ce (builtin-add: fix exclude handling, 2010-02-28)? We probably should build the glob matching on top of your version instead. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-03-10 7:06 ` Junio C Hamano @ 2010-03-11 7:15 ` Jeff King 2010-03-14 6:34 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2010-03-11 7:15 UTC (permalink / raw) To: Junio C Hamano; +Cc: Avery Pennarun, SungHyun Nam, git On Tue, Mar 09, 2010 at 11:06:15PM -0800, Junio C Hamano wrote: > > And something like this seems to fix the OP's problem: > > ... > > Which is similar to your fix, but hoisted into the ignore-collection > > phase. Like the original code and your patch, it suffers from using a > > straight memcmp. I think it should actually be checking the pathspec > > expansion to catch things like 'sub*/file' being relevant to 'subdir'. > > Yeah. Care to roll a patch to replace 13bb0ce (builtin-add: fix exclude > handling, 2010-02-28)? We probably should build the glob matching on top > of your version instead. Patch is below. It's based on your 13bb0ce^, and assumes 13bb0c3 itself will be reverted. However, doesn't your patch 2/3 that adds t2204 break bisectability? The fix doesn't come until 3/3. It should test_expect_failure, or it should come after. I thought about globbing for a minute. I don't think the change in dir.c would be too hard, but it will expose another corner case for add. If we have _anything_ in dir->ignored, add currently complains. But if I do something like: $ touch bar baz $ echo baz >.gitignore $ git add 'b*' right now it adds 'bar' and silently ignores 'baz', because COLLECT_IGNORED fails to realize that 'baz' is interesting to us. If we fix the COLLECT_IGNORED bug with globbing, it will start to complain that 'baz' was ignored. I think we could get around it by switching git-add to complain about ignored files _only_ if there is a pathspec that is not "seen". If everything was seen, then we know that even if there are ignored paths, they were all part of pathspecs that at least partially matched. However, it is still a bit unsatisfying; in the case of failure, we don't know which of the ignored files came from which pathspec. So we will print the whole list of ignored paths, even though some of them may not have been responsible for the error. Another option is to declare the current behavior wrong. Letting the shell glob produces different results for obvious reasons: $ git add b* ;# will fail, because we see individual pathspecs but perhaps that is the "feature" of letting add glob itself. Personally I have never asked "git add" to glob on my behalf, so I don't know why people would do it. Anyway, here's the patch. -- >8 -- Subject: [PATCH] dir: fix COLLECT_IGNORED on excluded prefixes As we walk the directory tree, if we see an ignored path, we want to add it to the ignored list only if it matches any pathspec that we were given. We used to check for the pathspec to appear explicitly. E.g., if we see "subdir/file" and it is excluded, we check to see if we have "subdir/file" in our pathspec. However, this interacts badly with the optimization to avoid recursing into ignored subdirectories. If "subdir" as a whole is ignored, then we never recurse, and consider only whether "subdir" itself is in our pathspec. It would not match a pathspec of "subdir/file" explicitly, even though it is the reason that subdir/file would be excluded. This manifests itself to the user as "git add subdir/file" failing to correctly note that the pathspec was ignored. This patch extends the in_pathspec logic to include prefix directory case. Signed-off-by: Jeff King <peff@peff.net> --- dir.c | 20 ++++++++++++++++++-- 1 files changed, 18 insertions(+), 2 deletions(-) diff --git a/dir.c b/dir.c index 00d698d..14ac91a 100644 --- a/dir.c +++ b/dir.c @@ -554,13 +554,29 @@ static int simplify_away(const char *path, int pathlen, const struct path_simpli return 0; } -static int in_pathspec(const char *path, int len, const struct path_simplify *simplify) +/* + * This function tells us whether an excluded path matches a + * list of "interesting" pathspecs. That is, whether a path matched + * by any of the pathspecs could possibly be ignored by excluding + * the specified path. This can happen if: + * + * 1. the path is mentioned explicitly in the pathspec + * + * 2. the path is a directory prefix of some element in the + * pathspec + */ +static int exclude_matches_pathspec(const char *path, int len, + const struct path_simplify *simplify) { if (simplify) { for (; simplify->path; simplify++) { if (len == simplify->len && !memcmp(path, simplify->path, len)) return 1; + if (len < simplify->len + && simplify->path[len] == '/' + && !memcmp(path, simplify->path, len)) + return 1; } } return 0; @@ -638,7 +654,7 @@ static enum path_treatment treat_one_path(struct dir_struct *dir, { int exclude = excluded(dir, path, &dtype); if (exclude && (dir->flags & DIR_COLLECT_IGNORED) - && in_pathspec(path, *len, simplify)) + && exclude_matches_pathspec(path, *len, simplify)) dir_add_ignored(dir, path, *len); /* -- 1.7.0.2.393.g3eb23 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-03-11 7:15 ` Jeff King @ 2010-03-14 6:34 ` Junio C Hamano 2010-03-14 20:44 ` Jeff King 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2010-03-14 6:34 UTC (permalink / raw) To: Jeff King; +Cc: Avery Pennarun, SungHyun Nam, git Jeff King <peff@peff.net> writes: > Another option is to declare the current behavior wrong. Letting the > shell glob produces different results for obvious reasons: > > $ git add b* ;# will fail, because we see individual pathspecs > > but perhaps that is the "feature" of letting add glob itself. Personally > I have never asked "git add" to glob on my behalf, so I don't know why > people would do it. I know of one reason: $ git add '*.[ch]' will add a/b.c and c/d/f.h for you. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-03-14 6:34 ` Junio C Hamano @ 2010-03-14 20:44 ` Jeff King 2010-03-15 2:02 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2010-03-14 20:44 UTC (permalink / raw) To: Junio C Hamano; +Cc: Avery Pennarun, SungHyun Nam, git On Sat, Mar 13, 2010 at 10:34:34PM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Another option is to declare the current behavior wrong. Letting the > > shell glob produces different results for obvious reasons: > > > > $ git add b* ;# will fail, because we see individual pathspecs > > > > but perhaps that is the "feature" of letting add glob itself. Personally > > I have never asked "git add" to glob on my behalf, so I don't know why > > people would do it. > > I know of one reason: > > $ git add '*.[ch]' > > will add a/b.c and c/d/f.h for you. Hrm, that makes handling globs with ignores a bit trickier. If I have: $ touch root.c $ mkdir subdir && touch subdir/file.c $ echo subdir >.gitignore $ git add '*.[ch]' what should happen? I would say it should probably not generate an error, but just add 'root.c'. In which case, I think we perhaps actively _don't_ want to complain about ignored globs at all. If they match nothing except excluded files, we will already complain that the pathspec was useless. And if they do match some other files, we will silently except. The only "downside" versus handling globs in the ignore code is that for the no-matches case we say "pathspec did not match" and not "it _could_ have matched, but these files were ignored, and you need -f to add them". But I don't think that latter message even makes sense for a glob. If you show me just the glob, then it isn't helpful; I don't know which ignored files matched the glob. And if you show me the list of files, it is likely to contain unhelpful cruft like "build/generated.c". So I won't just repeat my command with "-f" anyway; I will find the ignored file I was interested in adding and specify it explicitly. So if that reasoning is sound, I think we want to just leave git-add's behavior as it is currently (with my patch from earlier in this thread applied, of course). You get different error messages for "git add *.c" and "git add '*.c'", but that is only natural. You also get different _behavior_, and that is intentional. Other callers of COLLECT_IGNORED could conceivably want different globbing behavior, but right now git-add is the only caller. So it's certainly not worth caring about at this point. -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 'git add' regression in git-1.7? 2010-03-14 20:44 ` Jeff King @ 2010-03-15 2:02 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2010-03-15 2:02 UTC (permalink / raw) To: Jeff King; +Cc: Avery Pennarun, SungHyun Nam, git Jeff King <peff@peff.net> writes: > In which case, I think we perhaps actively _don't_ want to complain > about ignored globs at all. > ... > So if that reasoning is sound, I think we want to just leave git-add's > behavior as it is currently (with my patch from earlier in this thread > applied, of course). You get different error messages for "git add *.c" > and "git add '*.c'", but that is only natural. You also get different > _behavior_, and that is intentional. Yeah, I think that makes sense. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2010-03-15 2:03 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-02-19 4:30 'git add' regression in git-1.7? SungHyun Nam 2010-02-19 4:42 ` Avery Pennarun 2010-02-19 5:04 ` SungHyun Nam 2010-02-19 5:15 ` Avery Pennarun 2010-02-19 5:34 ` Jeff King 2010-02-19 6:02 ` Jeff King 2010-02-19 8:24 ` Jeff King 2010-03-01 2:00 ` Junio C Hamano 2010-03-01 3:25 ` [PATCH] add: fail "git add ignored-dir/file" without -f Junio C Hamano 2010-03-01 8:26 ` [PATCH 1/3] t0050: mark non-working test as such Junio C Hamano 2010-03-09 22:37 ` 'git add' regression in git-1.7? Jeff King 2010-03-09 23:09 ` Jeff King 2010-03-10 7:06 ` Junio C Hamano 2010-03-11 7:15 ` Jeff King 2010-03-14 6:34 ` Junio C Hamano 2010-03-14 20:44 ` Jeff King 2010-03-15 2:02 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).