[RFD/PATCH] add: ignore only ignored files

[RFD/PATCH] add: ignore only ignored files

[Michael J Gruber]

"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(-)

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

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

[Junio C Hamano]

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).

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

[Jeff King]

On Wed, Nov 19, 2014 at 03:52:33PM +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.
> 
> 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

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

[Junio C Hamano]

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.

> 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.

;-)

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

[Michael J Gruber]

Junio C Hamano schrieb am 19.11.2014 um 22:43:
> 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.

>> 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

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

[Jeff King]

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? :)

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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'.

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

[Jeff King]

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. :)

-Peff

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

[Michael J Gruber]

Jeff King schrieb am 20.11.2014 um 19:20:
> 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

[PATCHv2] add: ignore only ignored files

[Michael J Gruber]

"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(-)

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

Re: [PATCHv2] add: ignore only ignored files

[Jeff King]

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.

> 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

Re: [PATCHv2] add: ignore only ignored files

[Torsten Bögershausen]

>> +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!

Re: [PATCHv2] add: ignore only ignored files

[Jeff King]

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.

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.

> 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

Re: [PATCHv2] add: ignore only ignored files

[Torsten Bögershausen]

On 2014-11-22 20.19, Jeff King wrote:
> 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. 
> 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

Re: [PATCHv2] add: ignore only ignored files

[Jeff King]

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.

> 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

Re: [PATCHv2] add: ignore only ignored files

[Junio C Hamano]

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.

Re: [PATCHv2] add: ignore only ignored files

[Jeff King]

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).

-Peff

Re: [PATCHv2] add: ignore only ignored files

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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(-)

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

Re: [PATCHv2] add: ignore only ignored files

[Torsten Bögershausen]

On 2014-11-24 18.41, Junio C Hamano wrote:
...
>>> 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 :-)
....

Re: [PATCHv2] add: ignore only ignored files

[Jeff King]

On Mon, Nov 24, 2014 at 09:41:00AM -0800, Junio C Hamano wrote:

> 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

Re: [PATCHv2] add: ignore only ignored files

[Michael J Gruber]

Torsten Bögershausen schrieb am 22.11.2014 um 15:59:
>>> +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.

> 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

Re: [PATCHv2] add: ignore only ignored files

[Michael J Gruber]

Jeff King schrieb am 21.11.2014 um 19:01:
> 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 ;)

>> 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