'git add' regression in git-1.7?

'git add' regression in git-1.7?

[SungHyun Nam]

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

Re: 'git add' regression in git-1.7?

[Avery Pennarun]

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

Re: 'git add' regression in git-1.7?

[SungHyun Nam]

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

Re: 'git add' regression in git-1.7?

[Avery Pennarun]

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

Re: 'git add' regression in git-1.7?

[Jeff King]

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

Re: 'git add' regression in git-1.7?

[Jeff King]

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

Re: 'git add' regression in git-1.7?

[Jeff King]

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

Re: 'git add' regression in git-1.7?

[Junio C Hamano]

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.

[PATCH] add: fail "git add ignored-dir/file" without -f

[Junio C Hamano]

"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

[PATCH 1/3] t0050: mark non-working test as such

[Junio C Hamano]

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

Re: 'git add' regression in git-1.7?

[Jeff King]

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

Re: 'git add' regression in git-1.7?

[Jeff King]

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

Re: 'git add' regression in git-1.7?

[Junio C Hamano]

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.

Re: 'git add' regression in git-1.7?

[Jeff King]

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

Re: 'git add' regression in git-1.7?

[Junio C Hamano]

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.

Re: 'git add' regression in git-1.7?

[Jeff King]

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

Re: 'git add' regression in git-1.7?

[Junio C Hamano]

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.