threads / patch / 38008

patchadd: ignore only ignored files

Subject: [RFD/PATCH] add: ignore only ignored files

## tl;dr

22 messages between Nov 19, 2014 and Nov 25, 2014. Diffs are folded; open one to read it.

replies: 21people: 4as markdown or json

Michael J Gruber· Nov 19, 2014, 14:52 UTC · lore

"git add foo bar" adds neither foo nor bar when bar is ignored, but dies to let the user recheck their command invocation. This becomes less helpful when "git add foo.*" is subject to shell expansion and some of the expanded files are ignored.

"git add --ignore-errors" is supposed to ignore errors when indexing some files and adds the others. It does ignore errors from actual indexing attempts, but does not ignore the error "file is ignored" as outlined above.

Change "git add --ignore-errors foo bar" to add foo when bar is ignored, i.e. to ignore the ignore error.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Thus buggs me whenever I add a new TeX project when there are *.aux and
*.log around also[*]. Aside from that personal itch: For a user reading the
documentation, the difference between "thinking about indexing a file"
and "technically indexing a file" is probably irrelevant, and most would
expect "git add --ignore-errors" to ignore the "file is ignored" error
as well and continue adding the remaining files.

This is a behaviour change, though (albeit only for the option/config case), so it's RFD. If the direction is OK a test would be in order.

We could add yet another ignore option, of course, which just screams to be called --ignore-ignored, along with a config add.ignoreIgnored. I dunno...

[*] And who wants to quote their *, really? ;)
 builtin/add.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to builtin/add.c +3 −1
diff --git a/builtin/add.c b/builtin/add.c
index ae6d3e2..8cadb71 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -284,7 +284,9 @@ static int add_files(struct dir_struct *dir, int flags)
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
 		fprintf(stderr, _("Use -f if you really want to add them.\n"));
-		die(_("no files added"));
+		if (!ignore_add_errors)
+			die(_("no files added"));
+		exit_status = 1;
 	}
 
 	for (i = 0; i < dir->nr; i++)

-- 
2.2.0.rc2.293.gc05a35d
Junio C Hamano· Nov 19, 2014, 18:51 UTC · re: Michael J Gruber · lore

Re: [RFD/PATCH] add: ignore only ignored files

Michael J Gruber <git@drmicha.warpmail.net> writes:
> ... and most would
> expect "git add --ignore-errors" to ignore the "file is ignored" error
> as well and continue adding the remaining files.

Yeah, I think that makes sense (but please don't take it as the final decision yet---I'd like to hear from others).

Jeff King· Nov 19, 2014, 19:15 UTC · re: Michael J Gruber · lore

Re: [RFD/PATCH] add: ignore only ignored files

On Wed, Nov 19, 2014 at 03:52:33PM +0100, Michael J Gruber wrote:
Show 14 quoted lines
> "git add foo bar" adds neither foo nor bar when bar is ignored, but dies
> to let the user recheck their command invocation. This becomes less
> helpful when "git add foo.*" is subject to shell expansion and some of
> the expanded files are ignored.
> 
> "git add --ignore-errors" is supposed to ignore errors when indexing
> some files and adds the others. It does ignore errors from actual
> indexing attempts, but does not ignore the error "file is ignored" as
> outlined above.
> 
> Change "git add --ignore-errors foo bar" to add foo when bar is ignored,
> i.e. to ignore the ignore error.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
FWIW, your complaint and this new behavior makes sense to me.

Typically I keep a very neat .gitignore file and just use "git add .", which _does_ ignore those files. The real problem here is that git cannot tell the difference between "the user explicitly asked for foo.aux, so we should complain" and "oops, foo.aux got caught in a shell expansion".

I almost wonder if skipping ignored files should _always_ be a warning, not a hard error. I guess that has unpleasant side effects for scripts which call "git add XXX" and check the exit code, who may be unpleasantly surprised that they missed out on some content.

Perhaps we could do a hybrid: add the files that were not ignored, but then still exit non-zero. Careful scripts need to check the exit status of "git add" anyway, and sloppy humans with over-broad wildcards typically do not care about the exit status.

-Peff
Junio C Hamano· Nov 19, 2014, 21:43 UTC · re: Jeff King · lore

Re: [RFD/PATCH] add: ignore only ignored files

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> Typically I keep a very neat .gitignore file and just use "git add .",
> which _does_ ignore those files. The real problem here is that git
> cannot tell the difference between "the user explicitly asked for
> foo.aux, so we should complain" and "oops, foo.aux got caught in a shell
> expansion".

Yup. I also find myself doing "git cmd -- \*.ext" to let Git, not my shell, handle the patterns.

Show 9 quoted lines
> I almost wonder if skipping ignored files should _always_ be a warning,
> not a hard error. I guess that has unpleasant side effects for scripts
> which call "git add XXX" and check the exit code, who may be
> unpleasantly surprised that they missed out on some content.
>
> Perhaps we could do a hybrid: add the files that were not ignored, but
> then still exit non-zero. Careful scripts need to check the exit status
> of "git add" anyway, and sloppy humans with over-broad wildcards
> typically do not care about the exit status.
;-)
Michael J Gruber· Nov 20, 2014, 09:42 UTC · re: Junio C Hamano · lore

Re: [RFD/PATCH] add: ignore only ignored files

Junio C Hamano schrieb am 19.11.2014 um 22:43:
Show 10 quoted lines
> Jeff King <peff@peff.net> writes:
> 
>> Typically I keep a very neat .gitignore file and just use "git add .",
>> which _does_ ignore those files. The real problem here is that git
>> cannot tell the difference between "the user explicitly asked for
>> foo.aux, so we should complain" and "oops, foo.aux got caught in a shell
>> expansion".
> 
> Yup.  I also find myself doing "git cmd -- \*.ext" to let Git, not
> my shell, handle the patterns.
That is the correct way, of course.
Show 12 quoted lines
>> I almost wonder if skipping ignored files should _always_ be a warning,
>> not a hard error. I guess that has unpleasant side effects for scripts
>> which call "git add XXX" and check the exit code, who may be
>> unpleasantly surprised that they missed out on some content.
>>
>> Perhaps we could do a hybrid: add the files that were not ignored, but
>> then still exit non-zero. Careful scripts need to check the exit status
>> of "git add" anyway, and sloppy humans with over-broad wildcards
>> typically do not care about the exit status.
> 
> ;-)
> 
You can simply say "Michael" in your last subclause above :)

I'm wondering whether that behaviour change (without --ignore-errors) is OK - I don't mind, but hey, I usually don't.

I think it all comes down to the fact whether specifying an ignored file on the command line is considered an error or only "possibly a user error" we should dwim around. "git add" being plumbing, I'm somehow hesitant to change behaviour unless git is asked to ignore error. And if I'm hesitant already... oh wait. git add is declared porcelain? I would not have guessed without looking it up.

Michael
Jeff King· Nov 20, 2014, 15:56 UTC · re: Michael J Gruber · lore

Re: [RFD/PATCH] add: ignore only ignored files

On Thu, Nov 20, 2014 at 10:42:16AM +0100, Michael J Gruber wrote:
Show 12 quoted lines
> >> Perhaps we could do a hybrid: add the files that were not ignored, but
> >> then still exit non-zero. Careful scripts need to check the exit status
> >> of "git add" anyway, and sloppy humans with over-broad wildcards
> >> typically do not care about the exit status.
> > 
> > ;-)
> > 
> 
> You can simply say "Michael" in your last subclause above :)
> 
> I'm wondering whether that behaviour change (without --ignore-errors) is
> OK - I don't mind, but hey, I usually don't.

I can't think of a case that it really hurts, but then I have not thought too hard on it. If you want to play with it, I think the patch is as simple as:

Show changes to builtin/add.c +1 −2
diff --git a/builtin/add.c b/builtin/add.c
index ae6d3e2..1074e32 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -284,7 +284,7 @@ static int add_files(struct dir_struct *dir, int flags)
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
 		fprintf(stderr, _("Use -f if you really want to add them.\n"));
-		die(_("no files added"));
+		exit_status = 1;
 	}
 
 	for (i = 0; i < dir->nr; i++)

It needs a tweak to t3700.35, which expects the "fatal:" line on stderr.
But other than that, it passes all tests. So it must be good, right? :)

-Peff
Junio C Hamano· Nov 20, 2014, 17:23 UTC · re: Jeff King · lore

Re: [RFD/PATCH] add: ignore only ignored files

Jeff King <peff@peff.net> writes:
Show 35 quoted lines
> On Thu, Nov 20, 2014 at 10:42:16AM +0100, Michael J Gruber wrote:
>
>> >> Perhaps we could do a hybrid: add the files that were not ignored, but
>> >> then still exit non-zero. Careful scripts need to check the exit status
>> >> of "git add" anyway, and sloppy humans with over-broad wildcards
>> >> typically do not care about the exit status.
>> > 
>> > ;-)
>> > 
>> 
>> You can simply say "Michael" in your last subclause above :)
>> 
>> I'm wondering whether that behaviour change (without --ignore-errors) is
>> OK - I don't mind, but hey, I usually don't.
>
> I can't think of a case that it really hurts, but then I have not
> thought too hard on it. If you want to play with it, I think the patch
> is as simple as:
>
> diff --git a/builtin/add.c b/builtin/add.c
> index ae6d3e2..1074e32 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -284,7 +284,7 @@ static int add_files(struct dir_struct *dir, int flags)
>  		for (i = 0; i < dir->ignored_nr; i++)
>  			fprintf(stderr, "%s\n", dir->ignored[i]->name);
>  		fprintf(stderr, _("Use -f if you really want to add them.\n"));
> -		die(_("no files added"));
> +		exit_status = 1;
>  	}
>  
>  	for (i = 0; i < dir->nr; i++)
>
> It needs a tweak to t3700.35, which expects the "fatal:" line on stderr.
> But other than that, it passes all tests. So it must be good, right? :)
;-)

It indeed is a behaviour change, but I do not expect it would be too heavy a change to require us a transition period or major version bump. But because that is just my expectation, which is not what real world users would expect, so I'd prefer to cook such a change for at least a cycle and a half in 'next'.

Jeff King· Nov 20, 2014, 18:20 UTC · re: Junio C Hamano · lore

Re: [RFD/PATCH] add: ignore only ignored files

On Thu, Nov 20, 2014 at 09:23:21AM -0800, Junio C Hamano wrote:
Show 24 quoted lines
> > diff --git a/builtin/add.c b/builtin/add.c
> > index ae6d3e2..1074e32 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -284,7 +284,7 @@ static int add_files(struct dir_struct *dir, int flags)
> >  		for (i = 0; i < dir->ignored_nr; i++)
> >  			fprintf(stderr, "%s\n", dir->ignored[i]->name);
> >  		fprintf(stderr, _("Use -f if you really want to add them.\n"));
> > -		die(_("no files added"));
> > +		exit_status = 1;
> >  	}
> >  
> >  	for (i = 0; i < dir->nr; i++)
> >
> > It needs a tweak to t3700.35, which expects the "fatal:" line on stderr.
> > But other than that, it passes all tests. So it must be good, right? :)
> 
> ;-)
> 
> It indeed is a behaviour change, but I do not expect it would be too
> heavy a change to require us a transition period or major version
> bump.  But because that is just my expectation, which is not what
> real world users would expect, so I'd prefer to cook such a change
> for at least a cycle and a half in 'next'.

Oh, definitely. Showing the patch at all was my not-so-subtle attempt to convince Michael to take over the topic so I did not have to worry about such things. :)

-Peff
Michael J Gruber· Nov 21, 2014, 15:39 UTC · re: Jeff King · lore

Re: [RFD/PATCH] add: ignore only ignored files

Jeff King schrieb am 20.11.2014 um 19:20:
Show 30 quoted lines
> On Thu, Nov 20, 2014 at 09:23:21AM -0800, Junio C Hamano wrote:
> 
>>> diff --git a/builtin/add.c b/builtin/add.c
>>> index ae6d3e2..1074e32 100644
>>> --- a/builtin/add.c
>>> +++ b/builtin/add.c
>>> @@ -284,7 +284,7 @@ static int add_files(struct dir_struct *dir, int flags)
>>>  		for (i = 0; i < dir->ignored_nr; i++)
>>>  			fprintf(stderr, "%s\n", dir->ignored[i]->name);
>>>  		fprintf(stderr, _("Use -f if you really want to add them.\n"));
>>> -		die(_("no files added"));
>>> +		exit_status = 1;
>>>  	}
>>>  
>>>  	for (i = 0; i < dir->nr; i++)
>>>
>>> It needs a tweak to t3700.35, which expects the "fatal:" line on stderr.
>>> But other than that, it passes all tests. So it must be good, right? :)
>>
>> ;-)
>>
>> It indeed is a behaviour change, but I do not expect it would be too
>> heavy a change to require us a transition period or major version
>> bump.  But because that is just my expectation, which is not what
>> real world users would expect, so I'd prefer to cook such a change
>> for at least a cycle and a half in 'next'.
> 
> Oh, definitely. Showing the patch at all was my not-so-subtle attempt to
> convince Michael to take over the topic so I did not have to worry about
> such things. :)

Well, given that I figured out how to do it with the option, it may come at no additional surprise that I figured out how to do it without the option, as well.

The real extra surprise is - not only that test fix is in my tree already - it even comes with a test that actually tests the new intended behavior. Whoaaa!

I'll resend it soon so that Jeff can take over and adjust the commit message ;)

Michael, just kidding
Michael J Gruber· Nov 21, 2014, 16:08 UTC · re: Michael J Gruber · lore

[PATCHv2] add: ignore only ignored files

"git add foo bar" adds neither foo nor bar when bar is ignored, but dies to let the user recheck their command invocation. This becomes less helpful when "git add foo.*" is subject to shell expansion and some of the expanded files are ignored.

"git add --ignore-errors" is supposed to ignore errors when indexing some files and adds the others. It does ignore errors from actual indexing attempts, but does not ignore the error "file is ignored" as outlined above. This is unexpected.

Change "git add foo bar" to add foo when bar is ignored, but issue a warning and return a failure code as before the change.

That is, in the case of trying to add ignored files we now act the same way (with or without "--ignore-errors") in which we act for more severe indexing errors when "--ignore-errors" is specified.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
As discussed, we change behavior even without the option.
I.a.w.: We declare "add ignored file" as a minor error compared
to an actual indexing error.

My sincere thanks go out to Jeff without whom I could not possibly have come up with a patch like this :)

 builtin/add.c  | 2 +-
 t/t3700-add.sh | 8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)
Show changes to 2 files +8 −2

builtin/add.c, t/t3700-add.sh

diff --git a/builtin/add.c b/builtin/add.c
index ae6d3e2..1074e32 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -284,7 +284,7 @@ static int add_files(struct dir_struct *dir, int flags)
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
 		fprintf(stderr, _("Use -f if you really want to add them.\n"));
-		die(_("no files added"));
+		exit_status = 1;
 	}
 
 	for (i = 0; i < dir->nr; i++)
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index fe274e2..f7ff1f5 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -91,6 +91,13 @@ test_expect_success 'error out when attempting to add ignored ones without -f' '
 	! (git ls-files | grep "\\.ig")
 '
 
+test_expect_success 'error out when attempting to add ignored ones but add others' '
+	touch a.if &&
+	test_must_fail git add a.?? &&
+	! (git ls-files | grep "\\.ig") &&
+	(git ls-files | grep a.if)
+'
+
 test_expect_success 'add ignored ones with -f' '
 	git add -f a.?? &&
 	git ls-files --error-unmatch a.ig
@@ -311,7 +318,6 @@ cat >expect.err <<\EOF
 The following paths are ignored by one of your .gitignore files:
 ignored-file
 Use -f if you really want to add them.
-fatal: no files added
 EOF
 cat >expect.out <<\EOF
 add 'track-this'

-- 
2.2.0.rc2.293.gc05a35d
Jeff King· Nov 21, 2014, 18:01 UTC · re: Michael J Gruber · lore

Re: [PATCHv2] add: ignore only ignored files

On Fri, Nov 21, 2014 at 05:08:19PM +0100, Michael J Gruber wrote:
Show 16 quoted lines
> "git add foo bar" adds neither foo nor bar when bar is ignored, but dies
> to let the user recheck their command invocation. This becomes less
> helpful when "git add foo.*" is subject to shell expansion and some of
> the expanded files are ignored.
> 
> "git add --ignore-errors" is supposed to ignore errors when indexing
> some files and adds the others. It does ignore errors from actual
> indexing attempts, but does not ignore the error "file is ignored" as
> outlined above. This is unexpected.
> 
> Change "git add foo bar" to add foo when bar is ignored, but issue
> a warning and return a failure code as before the change.
> 
> That is, in the case of trying to add ignored files we now act the same
> way (with or without "--ignore-errors") in which we act for more
> severe indexing errors when "--ignore-errors" is specified.

Thanks, this looks pretty good to me. I agree with Junio's sense that we should cook it extra long to give people time to react.

> My sincere thanks go out to Jeff without whom I could not possibly
> have come up with a patch like this :)

:) Sorry if I was being obnoxious before. Sometimes contributors need a gentle push to keep going, but I should know by now that you are not such a person.

Show 14 quoted lines
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index fe274e2..f7ff1f5 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -91,6 +91,13 @@ test_expect_success 'error out when attempting to add ignored ones without -f' '
>  	! (git ls-files | grep "\\.ig")
>  '
>  
> +test_expect_success 'error out when attempting to add ignored ones but add others' '
> +	touch a.if &&
> +	test_must_fail git add a.?? &&
> +	! (git ls-files | grep "\\.ig") &&
> +	(git ls-files | grep a.if)
> +'

I am somewhat allergic to pipes in our test suite, because they can mask errors (especially with a negated grep, because we do not know if they correctly produced any output at all). But I guess this is matching the surrounding code, and it is quite unlikely for `ls-files` to fail in any meaningful way here. So I think it's fine.

-Peff
Torsten Bögershausen· Nov 22, 2014, 14:59 UTC · re: Jeff King · lore

Re: [PATCHv2] add: ignore only ignored files

Show 14 quoted lines
>> +test_expect_success 'error out when attempting to add ignored ones but add others' '
>> +	touch a.if &&
>> +	test_must_fail git add a.?? &&
>> +	! (git ls-files | grep "\\.ig") &&
>> +	(git ls-files | grep a.if)
>> +'
> 
> I am somewhat allergic to pipes in our test suite, because they can mask
> errors (especially with a negated grep, because we do not know if they
> correctly produced any output at all). But I guess this is matching the
> surrounding code, and it is quite unlikely for `ls-files` to fail in any
> meaningful way here. So I think it's fine.
> 
> -Peff

2 small comments: Why the escaped "\\.ig" and the unescaped "a.if" ?

The other question, this is a more general one, strikes me every time I see ! grep

Should we avoid it by writing "test_must_fail" instead of "!" ? (The current code base has a mixture of both)

The following came into my mind when working on another grepy thing, and it may be unnecessary clumsy:

test_expect_success 'error out when attempting to add ignored ones but add others' '
	touch a.if &&
	test_must_fail git add a.?? &&
	git ls-files >files.txt &&
	test_must_fail grep a.ig files.txt >/dev/null &&
	grep a.if files.txt >/dev/null &&
	rm files.txt
'
(It feels as if there should be a "grepnot" ;-)

The 3rd comment: Thanks for taking this up!

Jeff King· Nov 22, 2014, 19:19 UTC · re: Torsten Bögershausen · lore

Re: [PATCHv2] add: ignore only ignored files

On Sat, Nov 22, 2014 at 03:59:12PM +0100, Torsten Bögershausen wrote:
Show 10 quoted lines
> >> +test_expect_success 'error out when attempting to add ignored ones but add others' '
> >> +	touch a.if &&
> >> +	test_must_fail git add a.?? &&
> >> +	! (git ls-files | grep "\\.ig") &&
> >> +	(git ls-files | grep a.if)
> >> +'
> [...]
> 
> 2 small comments:
> Why the escaped "\\.ig" and the unescaped "a.if"  ?
I agree that is inconsistent, and I don't see any reason for it.
> The other question, this is a more general one, strikes me every time I see
> ! grep
> 
> Should we avoid it by writing "test_must_fail" instead of "!" ?

No. The point of test_must_fail over "!" is to check that not only does the command fail, but it fails with a clean exit rather than a signal death. The general philosophy is that this is useful for git (which we are testing), and not for third-party tools that we are using to check our outputs. In other words, we do not expect grep to segfault, and do not need to bother checking it.

I do not think there is a real _downside_ to using test_must_fail for grep, except that it is a bit more verbose.

And that describes the goal, of course; actual implementation has been less consistent. Possibly because I do not know that those instructions are written down anywhere. We usually catch such things in review these days, but there are many inconsistent spots in the existing suite.

Show 10 quoted lines
> The following came into my mind when working on another grepy thing,
> and it may be unnecessary clumsy:
> 
> test_expect_success 'error out when attempting to add ignored ones but add others' '
> 	touch a.if &&
> 	test_must_fail git add a.?? &&
> 	git ls-files >files.txt &&
> 	test_must_fail grep a.ig files.txt >/dev/null &&
> 	grep a.if files.txt >/dev/null &&
> 	rm files.txt

Right, my "allergic to pipes" was basically advocating using a tempfile. But as noted above, it should remain "! grep" here. And there is no need to redirect the output of grep, as the test suite does it already (in fact, it is preferable not to, because somebody debugging the test with "-v" will get more output).

-Peff
Torsten Bögershausen· Nov 22, 2014, 21:20 UTC · re: Jeff King · lore

Re: [PATCHv2] add: ignore only ignored files

On 2014-11-22 20.19, Jeff King wrote:
Show 29 quoted lines
> On Sat, Nov 22, 2014 at 03:59:12PM +0100, Torsten Bögershausen wrote:
> 
>>>> +test_expect_success 'error out when attempting to add ignored ones but add others' '
>>>> +	touch a.if &&
>>>> +	test_must_fail git add a.?? &&
>>>> +	! (git ls-files | grep "\\.ig") &&
>>>> +	(git ls-files | grep a.if)
>>>> +'
>> [...]
>>
>> 2 small comments:
>> Why the escaped "\\.ig" and the unescaped "a.if"  ?
> 
> I agree that is inconsistent, and I don't see any reason for it.
> 
>> The other question, this is a more general one, strikes me every time I see
>> ! grep
>>
>> Should we avoid it by writing "test_must_fail" instead of "!" ?
> 
> No. The point of test_must_fail over "!" is to check that not only does
> the command fail, but it fails with a clean exit rather than a signal
> death.  The general philosophy is that this is useful for git (which we
> are testing), and not for third-party tools that we are using to check
> our outputs. In other words, we do not expect grep to segfault, and do
> not need to bother checking it.
> 
> I do not think there is a real _downside_ to using test_must_fail for
> grep, except that it is a bit more verbose.
We may burn CPU cycles 
> 
> And that describes the goal, of course; actual implementation has been
> less consistent. Possibly because I do not know that those instructions
> are written down anywhere.

There is a hint in test-lib-functions.sh, but a short notice in CodingGuidelines could be useful, once there is an agreement about grep, which I think we have.

Show 21 quoted lines
> We usually catch such things in review these
> days, but there are many inconsistent spots in the existing suite.
> 
>> The following came into my mind when working on another grepy thing,
>> and it may be unnecessary clumsy:
>>
>> test_expect_success 'error out when attempting to add ignored ones but add others' '
>> 	touch a.if &&
>> 	test_must_fail git add a.?? &&
>> 	git ls-files >files.txt &&
>> 	test_must_fail grep a.ig files.txt >/dev/null &&
>> 	grep a.if files.txt >/dev/null &&
>> 	rm files.txt
> 
> Right, my "allergic to pipes" was basically advocating using a tempfile.
> But as noted above, it should remain "! grep" here. And there is no need
> to redirect the output of grep, as the test suite does it already (in
> fact, it is preferable not to, because somebody debugging the test with
> "-v" will get more output).
> 
> -Peff
I counted 19 "test_must_fail grep" under t/*sh, and 201 "! grep".

As a general rule for further review of shell scripts can we say ? ! git # incorrect, we don't capture e.g. segfaults of signal test_must_fail grep # correct, but not preferred for new code ! grep # preferred for new code test_must_fail git # correct

Jeff King· Nov 23, 2014, 19:50 UTC · re: Torsten Bögershausen · lore

Re: [PATCHv2] add: ignore only ignored files

On Sat, Nov 22, 2014 at 10:20:10PM +0100, Torsten Bögershausen wrote:
> > I do not think there is a real _downside_ to using test_must_fail for
> > grep, except that it is a bit more verbose.
> We may burn CPU cycles

It adds a single if/else chain. If your shell does not implement that as a fast builtin, you have bigger performance problems. :)

> I counted 19 "test_must_fail grep" under t/*sh, and 201 "! grep".

I do not mind a patch to fix them, but with the usual caveat of avoiding stepping on the toes of any topics in flight. It is also fine to leave them until the area is touched.

Show 5 quoted lines
> As a general rule for further review of shell scripts can we say ?
> ! git                # incorrect, we don't capture e.g. segfaults of signal 
> test_must_fail grep  # correct, but not preferred for new code
> ! grep               # preferred for new code
> test_must_fail git   # correct

I think that's true. The snippet from t/README Junio quoted lays it out pretty clearly, I think. If you didn't know there was documentation in t/README that was worth reading before writing tests, then that is the thing I think should go in CodingGuidelines.

-Peff
Junio C Hamano· Nov 23, 2014, 18:10 UTC · re: Jeff King · lore

Re: [PATCHv2] add: ignore only ignored files

Jeff King <peff@peff.net> writes:
> ... Possibly because I do not know that those instructions
> are written down anywhere. We usually catch such things in review these
> days, but there are many inconsistent spots in the existing suite.
t/README has this
    Don't:
     - use '! git cmd' when you want to make sure the git command exits
       with failure in a controlled way by calling "die()".  Instead,
       use 'test_must_fail git cmd'.  This will signal a failure if git
       dies in an unexpected way (e.g. segfault).
       On the other hand, don't use test_must_fail for running regular
       platform commands; just use '! cmd'.

Though it can be improved by justifying "just use '! cmd'" further with a bit of rationale (e.g. "We are not in the business of verifying that world given to us sanely works."), I think the current text is sufficient.

Do we refer to t/README from CodingGuidelines where we tell the developers to always write tests to prevent other people from breaking tomorrow what you did today? If not, perhaps that is what needs to be added.

Jeff King· Nov 23, 2014, 19:46 UTC · re: Junio C Hamano · lore

Re: [PATCHv2] add: ignore only ignored files

On Sun, Nov 23, 2014 at 10:10:47AM -0800, Junio C Hamano wrote:
Show 17 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > ... Possibly because I do not know that those instructions
> > are written down anywhere. We usually catch such things in review these
> > days, but there are many inconsistent spots in the existing suite.
> 
> t/README has this
> 
>     Don't:
> 
>      - use '! git cmd' when you want to make sure the git command exits
>        with failure in a controlled way by calling "die()".  Instead,
>        use 'test_must_fail git cmd'.  This will signal a failure if git
>        dies in an unexpected way (e.g. segfault).
> 
>        On the other hand, don't use test_must_fail for running regular
>        platform commands; just use '! cmd'.

Thanks, I did not actually look and relied on my memory, which was obviously wrong. I agree that the instructions there are sufficient.

> Do we refer to t/README from CodingGuidelines where we tell the
> developers to always write tests to prevent other people from
> breaking tomorrow what you did today?  If not, perhaps that is what
> needs to be added.

That might make sense. It might also be that Torsten simply overlooked it when asking his question (i.e., there is nothing to fix, documentation is not always read completely, and we can move on).

-Peff
Junio C Hamano· Nov 24, 2014, 17:41 UTC · re: Jeff King · lore

Re: [PATCHv2] add: ignore only ignored files

Jeff King <peff@peff.net> writes:
Show 31 quoted lines
> On Sun, Nov 23, 2014 at 10:10:47AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > ... Possibly because I do not know that those instructions
>> > are written down anywhere. We usually catch such things in review these
>> > days, but there are many inconsistent spots in the existing suite.
>> 
>> t/README has this
>> 
>>     Don't:
>> 
>>      - use '! git cmd' when you want to make sure the git command exits
>>        with failure in a controlled way by calling "die()".  Instead,
>>        use 'test_must_fail git cmd'.  This will signal a failure if git
>>        dies in an unexpected way (e.g. segfault).
>> 
>>        On the other hand, don't use test_must_fail for running regular
>>        platform commands; just use '! cmd'.
>
> Thanks, I did not actually look and relied on my memory, which was
> obviously wrong. I agree that the instructions there are sufficient.
>
>> Do we refer to t/README from CodingGuidelines where we tell the
>> developers to always write tests to prevent other people from
>> breaking tomorrow what you did today?  If not, perhaps that is what
>> needs to be added.
>
> That might make sense. It might also be that Torsten simply overlooked
> it when asking his question (i.e., there is nothing to fix,
> documentation is not always read completely, and we can move on).

We actually do not have a reference to it anywhere. For now, this should suffice.

 Documentation/SubmittingPatches | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to Documentation/SubmittingPatches +2 −1
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index fa71b5f..a3861a6 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -57,7 +57,8 @@ change, the approach taken by the change, and if relevant how this
 differs substantially from the prior version, are all good things
 to have.
 
-Make sure that you have tests for the bug you are fixing.
+Make sure that you have tests for the bug you are fixing.  See
+t/README for guidance of writing tests.
 
 When adding a new feature, make sure that you have new tests to show
 the feature triggers the new behaviour when it should, and to show the
Torsten Bögershausen· Nov 24, 2014, 20:22 UTC · re: Junio C Hamano · lore

Re: [PATCHv2] add: ignore only ignored files

On 2014-11-24 18.41, Junio C Hamano wrote: ...

Show 8 quoted lines
>>> Do we refer to t/README from CodingGuidelines where we tell the
>>> developers to always write tests to prevent other people from
>>> breaking tomorrow what you did today?  If not, perhaps that is what
>>> needs to be added.
>>
>> That might make sense. It might also be that Torsten simply overlooked
>> it when asking his question (i.e., there is nothing to fix,
>> documentation is not always read completely, and we can move on).

Thanks, until yesterday I was unaware of t/README, but now I am :-) ....

Jeff King· Nov 25, 2014, 03:57 UTC · re: Junio C Hamano · lore

Re: [PATCHv2] add: ignore only ignored files

On Mon, Nov 24, 2014 at 09:41:00AM -0800, Junio C Hamano wrote:
Show 17 quoted lines
> We actually do not have a reference to it anywhere.  For now, this
> should suffice.
> 
>  Documentation/SubmittingPatches | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index fa71b5f..a3861a6 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -57,7 +57,8 @@ change, the approach taken by the change, and if relevant how this
>  differs substantially from the prior version, are all good things
>  to have.
>  
> -Make sure that you have tests for the bug you are fixing.
> +Make sure that you have tests for the bug you are fixing.  See
> +t/README for guidance of writing tests.
That looks a good improvement to me.
-Peff
Michael J Gruber· Nov 24, 2014, 10:29 UTC · re: Torsten Bögershausen · lore

Re: [PATCHv2] add: ignore only ignored files

Torsten Bögershausen schrieb am 22.11.2014 um 15:59:
Show 17 quoted lines
>>> +test_expect_success 'error out when attempting to add ignored ones but add others' '
>>> +	touch a.if &&
>>> +	test_must_fail git add a.?? &&
>>> +	! (git ls-files | grep "\\.ig") &&
>>> +	(git ls-files | grep a.if)
>>> +'
>>
>> I am somewhat allergic to pipes in our test suite, because they can mask
>> errors (especially with a negated grep, because we do not know if they
>> correctly produced any output at all). But I guess this is matching the
>> surrounding code, and it is quite unlikely for `ls-files` to fail in any
>> meaningful way here. So I think it's fine.
>>
>> -Peff
> 
> 2 small comments:
> Why the escaped "\\.ig" and the unescaped "a.if"  ?

Well, the first one is copied straight from another test and the second one is by me. ;)

I want to test that no files ending in .ig are added, but that one file a.if is added. Knowing how the test is structured, it is higly unlikely that other people will add a file where the dot in a.if matches something other than a dot, but in the case of .ig I wouldn't be so sure. We could take the extra safety measure and quote "a\\.if" also, but to me that seems to make the test less readable.

Show 20 quoted lines
> The other question, this is a more general one, strikes me every time I see
> ! grep
> 
> Should we avoid it by writing "test_must_fail" instead of "!" ?
> (The current code base has a mixture of both)
> 
> The following came into my mind when working on another grepy thing,
> and it may be unnecessary clumsy:
> 
> test_expect_success 'error out when attempting to add ignored ones but add others' '
> 	touch a.if &&
> 	test_must_fail git add a.?? &&
> 	git ls-files >files.txt &&
> 	test_must_fail grep a.ig files.txt >/dev/null &&
> 	grep a.if files.txt >/dev/null &&
> 	rm files.txt
> '
> 
> (It feels as if there should be a "grepnot" ;-)
> 
I guess that was clarified in the ongoing discussion.
> The 3rd comment:
> Thanks for taking this up!
Just scratching my own itches mostly these days :)
Michael
Michael J Gruber· Nov 24, 2014, 10:23 UTC · re: Jeff King · lore

Re: [PATCHv2] add: ignore only ignored files

Jeff King schrieb am 21.11.2014 um 19:01:
Show 28 quoted lines
> On Fri, Nov 21, 2014 at 05:08:19PM +0100, Michael J Gruber wrote:
> 
>> "git add foo bar" adds neither foo nor bar when bar is ignored, but dies
>> to let the user recheck their command invocation. This becomes less
>> helpful when "git add foo.*" is subject to shell expansion and some of
>> the expanded files are ignored.
>>
>> "git add --ignore-errors" is supposed to ignore errors when indexing
>> some files and adds the others. It does ignore errors from actual
>> indexing attempts, but does not ignore the error "file is ignored" as
>> outlined above. This is unexpected.
>>
>> Change "git add foo bar" to add foo when bar is ignored, but issue
>> a warning and return a failure code as before the change.
>>
>> That is, in the case of trying to add ignored files we now act the same
>> way (with or without "--ignore-errors") in which we act for more
>> severe indexing errors when "--ignore-errors" is specified.
> 
> Thanks, this looks pretty good to me. I agree with Junio's sense that we
> should cook it extra long to give people time to react.
> 
>> My sincere thanks go out to Jeff without whom I could not possibly
>> have come up with a patch like this :)
> 
> :) Sorry if I was being obnoxious before. Sometimes contributors need a
> gentle push to keep going, but I should know by now that you are not
> such a person.
We were just having fun with each other ;)
Show 23 quoted lines
>> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
>> index fe274e2..f7ff1f5 100755
>> --- a/t/t3700-add.sh
>> +++ b/t/t3700-add.sh
>> @@ -91,6 +91,13 @@ test_expect_success 'error out when attempting to add ignored ones without -f' '
>>  	! (git ls-files | grep "\\.ig")
>>  '
>>  
>> +test_expect_success 'error out when attempting to add ignored ones but add others' '
>> +	touch a.if &&
>> +	test_must_fail git add a.?? &&
>> +	! (git ls-files | grep "\\.ig") &&
>> +	(git ls-files | grep a.if)
>> +'
> 
> I am somewhat allergic to pipes in our test suite, because they can mask
> errors (especially with a negated grep, because we do not know if they
> correctly produced any output at all). But I guess this is matching the
> surrounding code, and it is quite unlikely for `ls-files` to fail in any
> meaningful way here. So I think it's fine.
> 
> -Peff
>

I do prefer test_cmp myself, also because it tells you much more in case of a broken test - a failed boolean chain doesn't even tell you where it broke.

In this specific case, many more tests would need to be rewriten, though, so I preferred to keep the style of the surrounding code.

Michael

← back to recent threads