[PATCH] pathspec: die on empty strings as pathspec

[PATCH] pathspec: die on empty strings as pathspec

[Emily Xie]

An empty string as a pathspec element matches all paths.  A buggy
script, however, could accidentally assign an empty string to a
variable that then gets passed to a Git command invocation, e.g.:

  path=... compute a path to be removed in $path ...
        git rm -r "$path"

which would unintentionally remove all paths in the current
directory.

The fix for this issue comprises of two steps. Step 1, which warns
that empty strings as pathspecs will become invalid, has already
been implemented in commit d426430 ("pathspec: warn on empty strings
as pathspec", 2016-06-22).

This patch is step 2. It removes the warning and throws an error
instead.

Signed-off-by: Emily Xie <emilyxxie@gmail.com>
Reported-by: David Turner <novalis@novalis.org>
---
 pathspec.c     | 10 ++++------
 t/t3600-rm.sh  |  5 ++---
 t/t3700-add.sh |  5 ++---
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 50f76ff..65e18b1 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -605,7 +605,7 @@ void parse_pathspec(struct pathspec *pathspec,
 {
 	struct pathspec_item *item;
 	const char *entry = argv ? *argv : NULL;
-	int i, n, prefixlen, warn_empty_string, nr_exclude = 0;
+	int i, n, prefixlen, nr_exclude = 0;
 
 	memset(pathspec, 0, sizeof(*pathspec));
 
@@ -638,12 +638,10 @@ void parse_pathspec(struct pathspec *pathspec,
 	}
 
 	n = 0;
-	warn_empty_string = 1;
 	while (argv[n]) {
-		if (*argv[n] == '\0' && warn_empty_string) {
-			warning(_("empty strings as pathspecs will be made invalid in upcoming releases. "
-				  "please use . instead if you meant to match all paths"));
-			warn_empty_string = 0;
+		if (*argv[n] == '\0') {
+			die("empty string is not a valid pathspec. "
+				  "please use . instead if you meant to match all paths");
 		}
 		n++;
 	}
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5f9913b..c787eac 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -858,9 +858,8 @@ test_expect_success 'rm files with two different errors' '
 	test_i18ncmp expect actual
 '
 
-test_expect_success 'rm empty string should invoke warning' '
-	git rm -rf "" 2>output &&
-	test_i18ngrep "warning: empty strings" output
+test_expect_success 'rm empty string should fail' '
+	test_must_fail git rm -rf ""
 '
 
 test_done
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index f3a4b4a..40a0d2b 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -331,9 +331,8 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out
 	test_i18ncmp expect.err actual.err
 '
 
-test_expect_success 'git add empty string should invoke warning' '
-	git add "" 2>output &&
-	test_i18ngrep "warning: empty strings" output
+test_expect_success 'git add empty string should fail' '
+	test_must_fail git add ""
 '
 
 test_expect_success 'git add --chmod=[+-]x stages correctly' '
-- 
2.8.4

Re: [PATCH] pathspec: die on empty strings as pathspec

[Junio C Hamano]

Emily Xie <emilyxxie@gmail.com> writes:

> An empty string as a pathspec element matches all paths.  A buggy
> script, however, could accidentally assign an empty string to a
> variable that then gets passed to a Git command invocation, e.g.:
>
>   path=... compute a path to be removed in $path ...
>         git rm -r "$path"
>
> which would unintentionally remove all paths in the current
> directory.
>
> The fix for this issue comprises of two steps. Step 1, which warns
> that empty strings as pathspecs will become invalid, has already
> been implemented in commit d426430 ("pathspec: warn on empty strings
> as pathspec", 2016-06-22).
>
> This patch is step 2. It removes the warning and throws an error
> instead.
>
> Signed-off-by: Emily Xie <emilyxxie@gmail.com>
> Reported-by: David Turner <novalis@novalis.org>
> ---

We started this at v2.11.0 at the end of November 2016, and this
cycle is expected to complete at around the end of July 2017, so
this patch makes it a 8-month deprecation cycle.  I think that it
should be long enough.

Thanks.

> diff --git a/pathspec.c b/pathspec.c
> index 50f76ff..65e18b1 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -638,12 +638,10 @@ void parse_pathspec(struct pathspec *pathspec,
>  	}
>  
>  	n = 0;
> -	warn_empty_string = 1;
>  	while (argv[n]) {
> -		if (*argv[n] == '\0' && warn_empty_string) {
> -			warning(_("empty strings as pathspecs will be made invalid in upcoming releases. "
> -				  "please use . instead if you meant to match all paths"));
> -			warn_empty_string = 0;
> +		if (*argv[n] == '\0') {
> +			die("empty string is not a valid pathspec. "
> +				  "please use . instead if you meant to match all paths");
>  		}
>  		n++;
>  	}

The {} around a single statement becomes unnecessary, so I'll remove
them while queuing.

Re: [PATCH] pathspec: die on empty strings as pathspec

[Jeff King]

On Sat, Jun 10, 2017 at 01:28:54AM +0900, Junio C Hamano wrote:

> Emily Xie <emilyxxie@gmail.com> writes:
> 
> > An empty string as a pathspec element matches all paths.  A buggy
> > script, however, could accidentally assign an empty string to a
> > variable that then gets passed to a Git command invocation, e.g.:
> >
> >   path=... compute a path to be removed in $path ...
> >         git rm -r "$path"
> >
> > which would unintentionally remove all paths in the current
> > directory.
> >
> > The fix for this issue comprises of two steps. Step 1, which warns
> > that empty strings as pathspecs will become invalid, has already
> > been implemented in commit d426430 ("pathspec: warn on empty strings
> > as pathspec", 2016-06-22).
> >
> > This patch is step 2. It removes the warning and throws an error
> > instead.
> >
> > Signed-off-by: Emily Xie <emilyxxie@gmail.com>
> > Reported-by: David Turner <novalis@novalis.org>
> > ---
> 
> We started this at v2.11.0 at the end of November 2016, and this
> cycle is expected to complete at around the end of July 2017, so
> this patch makes it a 8-month deprecation cycle.  I think that it
> should be long enough.

That's long enough for people who actually ran the intermediate
versions.  But what about people on distros who jump from v2.10 or lower
straight to v2.14?

I think to catch them we'd literally need years on our deprecation
schedules. Maybe it's not worth caring about. That's an awful long time
for people who _are_ upgrading each version to see the warning. Of
course the end game in this case is that it becomes an error, so they'll
be notified either way. :)

I'm OK with this case being a relatively quick deprecation, but your
"long enough" made me wonder how we would decide that.

-Peff

[1] Debian stable is set to release in a week or so. People doing that
    stable upgrade will go from v2.1.4 to v2.11.0. So they won't
    actually skip the deprecation notice, but they'll probably live with
    the warning for 3 years or so.

Re: [PATCH] pathspec: die on empty strings as pathspec

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That's long enough for people who actually ran the intermediate
> versions.  But what about people on distros who jump from v2.10 or lower
> straight to v2.14?
>
> I think to catch them we'd literally need years on our deprecation
> schedules. Maybe it's not worth caring about. That's an awful long time
> for people who _are_ upgrading each version to see the warning. Of
> course the end game in this case is that it becomes an error, so they'll
> be notified either way. :)
>
> I'm OK with this case being a relatively quick deprecation, but your
> "long enough" made me wonder how we would decide that.
>
> -Peff
>
> [1] Debian stable is set to release in a week or so. People doing that
>     stable upgrade will go from v2.1.4 to v2.11.0. So they won't
>     actually skip the deprecation notice, but they'll probably live with
>     the warning for 3 years or so.

Yuck X-<.

Re: [PATCH] pathspec: die on empty strings as pathspec

[Brandon Williams]

On 06/06, Emily Xie wrote:
> An empty string as a pathspec element matches all paths.  A buggy
> script, however, could accidentally assign an empty string to a
> variable that then gets passed to a Git command invocation, e.g.:
> 
>   path=... compute a path to be removed in $path ...
>         git rm -r "$path"
> 
> which would unintentionally remove all paths in the current
> directory.
> 
> The fix for this issue comprises of two steps. Step 1, which warns
> that empty strings as pathspecs will become invalid, has already
> been implemented in commit d426430 ("pathspec: warn on empty strings
> as pathspec", 2016-06-22).
> 
> This patch is step 2. It removes the warning and throws an error
> instead.
> 
> Signed-off-by: Emily Xie <emilyxxie@gmail.com>
> Reported-by: David Turner <novalis@novalis.org>
> ---
>  pathspec.c     | 10 ++++------
>  t/t3600-rm.sh  |  5 ++---
>  t/t3700-add.sh |  5 ++---
>  3 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/pathspec.c b/pathspec.c
> index 50f76ff..65e18b1 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -605,7 +605,7 @@ void parse_pathspec(struct pathspec *pathspec,
>  {
>  	struct pathspec_item *item;
>  	const char *entry = argv ? *argv : NULL;
> -	int i, n, prefixlen, warn_empty_string, nr_exclude = 0;
> +	int i, n, prefixlen, nr_exclude = 0;
>  
>  	memset(pathspec, 0, sizeof(*pathspec));
>  
> @@ -638,12 +638,10 @@ void parse_pathspec(struct pathspec *pathspec,
>  	}
>  
>  	n = 0;
> -	warn_empty_string = 1;
>  	while (argv[n]) {
> -		if (*argv[n] == '\0' && warn_empty_string) {
> -			warning(_("empty strings as pathspecs will be made invalid in upcoming releases. "
> -				  "please use . instead if you meant to match all paths"));
> -			warn_empty_string = 0;
> +		if (*argv[n] == '\0') {
> +			die("empty string is not a valid pathspec. "
> +				  "please use . instead if you meant to match all paths");

The spacing (indentation) of the second line here is really weird.

>  		}
>  		n++;
>  	}
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index 5f9913b..c787eac 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -858,9 +858,8 @@ test_expect_success 'rm files with two different errors' '
>  	test_i18ncmp expect actual
>  '
>  
> -test_expect_success 'rm empty string should invoke warning' '
> -	git rm -rf "" 2>output &&
> -	test_i18ngrep "warning: empty strings" output
> +test_expect_success 'rm empty string should fail' '
> +	test_must_fail git rm -rf ""
>  '
>  
>  test_done
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index f3a4b4a..40a0d2b 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -331,9 +331,8 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out
>  	test_i18ncmp expect.err actual.err
>  '
>  
> -test_expect_success 'git add empty string should invoke warning' '
> -	git add "" 2>output &&
> -	test_i18ngrep "warning: empty strings" output
> +test_expect_success 'git add empty string should fail' '
> +	test_must_fail git add ""
>  '
>  
>  test_expect_success 'git add --chmod=[+-]x stages correctly' '
> -- 
> 2.8.4
> 

-- 
Brandon Williams

Re: [PATCH] pathspec: die on empty strings as pathspec

[Junio C Hamano]

Emily Xie <emilyxxie@gmail.com> writes:

> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index f3a4b4a..40a0d2b 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -331,9 +331,8 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out
>  	test_i18ncmp expect.err actual.err
>  '
>  
> -test_expect_success 'git add empty string should invoke warning' '
> -	git add "" 2>output &&
> -	test_i18ngrep "warning: empty strings" output
> +test_expect_success 'git add empty string should fail' '
> +	test_must_fail git add ""
>  '

Doesn't this affect the tests that follow this step?  We used to
warn but went ahead and added all of them anyway, but now we fail
without adding anything, which means that the state of the index
before and after this patch would be different.

>  test_expect_success 'git add --chmod=[+-]x stages correctly' '

Re: [PATCH] pathspec: die on empty strings as pathspec

[Emily Xie]

I ran the tests and none of them failed. Technically, the state of the
index would indeed be different with these new changes, but this
shouldn't be an issue. In the current version, there's one only item
added to the index that ends up getting used in subsequent tests. That
is, foo1, which is tested in: 'git add --chmod=[+-]x stages
correctly'.

With the current code, foo1 gets added to the index with a mode of
100644. When the 'git add --chmod=[+-]x stages correctly' test
executes, it removes foo1 from the working directory, creates a new
foo1, and runs 'git add --chmod=+x'. It then checks if foo1 is in the
index with a file mode of 100755.

Given how things are set up, the tests pass either way. But I think
it'd actually be nicer with the new changes in this patch, as foo1
would not already be in the index by the time  'git add --chmod=[+-]x
stages correctly' executes.

On Sat, Jun 10, 2017 at 1:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Emily Xie <emilyxxie@gmail.com> writes:
>
> > diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> > index f3a4b4a..40a0d2b 100755
> > --- a/t/t3700-add.sh
> > +++ b/t/t3700-add.sh
> > @@ -331,9 +331,8 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out
> >       test_i18ncmp expect.err actual.err
> >  '
> >
> > -test_expect_success 'git add empty string should invoke warning' '
> > -     git add "" 2>output &&
> > -     test_i18ngrep "warning: empty strings" output
> > +test_expect_success 'git add empty string should fail' '
> > +     test_must_fail git add ""
> >  '
>
> Doesn't this affect the tests that follow this step?  We used to
> warn but went ahead and added all of them anyway, but now we fail
> without adding anything, which means that the state of the index
> before and after this patch would be different.
>
> >  test_expect_success 'git add --chmod=[+-]x stages correctly' '

Re: [PATCH] pathspec: die on empty strings as pathspec

[Junio C Hamano]

Emily Xie <emilyxxie@gmail.com> writes:

> I ran the tests and none of them failed. 

This is not about a test you touched, but applied to or merged to
any of the recent integration branches (like 'master' or 'maint')

    $ make
    $ cd t
    $ GIT_TEST_LONG=YesPlease sh ./t0027-*.sh

fails at the very beginning.  I do not know if 0027 is the only one
that triggers this failure, though.

THanks.

Re: [PATCH] pathspec: die on empty strings as pathspec

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Emily Xie <emilyxxie@gmail.com> writes:
>
>> I ran the tests and none of them failed. 
>
> This is not about a test you touched, but applied to or merged to
> any of the recent integration branches (like 'master' or 'maint')
>
>     $ make
>     $ cd t
>     $ GIT_TEST_LONG=YesPlease sh ./t0027-*.sh
>
> fails at the very beginning.  I do not know if 0027 is the only one
> that triggers this failure, though.

t0027 should pass with this, I think.

I am not sure if we even want the dot there, but at least that is
what the original author of the test intended to do when s/he
decided to pass an empty string as the pathspec.

 t/t0027-auto-crlf.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 93725895a4..e41c9b3bb2 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -322,7 +322,7 @@ test_expect_success 'setup master' '
 	echo >.gitattributes &&
 	git checkout -b master &&
 	git add .gitattributes &&
-	git commit -m "add .gitattributes" "" &&
+	git commit -m "add .gitattributes" . &&
 	printf "\$Id: 0000000000000000000000000000000000000000 \$\nLINEONE\nLINETWO\nLINETHREE"     >LF &&
 	printf "\$Id: 0000000000000000000000000000000000000000 \$\r\nLINEONE\r\nLINETWO\r\nLINETHREE" >CRLF &&
 	printf "\$Id: 0000000000000000000000000000000000000000 \$\nLINEONE\r\nLINETWO\nLINETHREE"   >CRLF_mix_LF &&

Re: [PATCH] pathspec: die on empty strings as pathspec

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I am not sure if we even want the dot there, but at least that is
> what the original author of the test intended to do when s/he
> decided to pass an empty string as the pathspec.
>
>  t/t0027-auto-crlf.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I'll queue the following _before_ your "final step", so that the
whole thing can be advanced to 'next' and hopefully to 'master' in
some future releases.  

Given that we ourselves did not even notice until now that one of
our scripts get broken by this "final step", even though we kept
issuing the warning message, we may want to re-think our overall
strategy for deprecation.  We've assumed that "keep behaving as
before but warn for a few years and then finally give a hard
failure" would be sufficient, but depending on how the script that
employ our programs hide the warnings from the end users, that may
not be a good enough transition strategy.

At the same time we re-think the deprecation strategy, we also need
to see if we can update our test framework to help us better.
Ideally, we could have caught this existing breakage of passing ""
as pathspec in the test soon after we went into "keep behaving as
before but warn" stage.  We didn't and found it out only when we
switched to "finally give a hard failure" stage.  That is very
unsatisfactory.

Needless to say, neither of the above two comes from _your_ change.
It's just something we need to improve in a larger scope of the
whole project I realized.

Thanks.

-- >8 --
Subject: [PATCH] t0027: do not use an empty string as a pathspec element

In an upcoming update, we will finally make an empty string illegal
as an element in a pathspec; it traditionally meant the same as ".",
i.e. include everything, so update this test that passes "" to pass
a dot instead.

At this point in the test sequence, there is no modified path that
need to be further added before committing; the working tree is
empty except for .gitattributes which was just added to the index.
So we could instead pass no pathspec, but this is a conversion more
faithful to the original.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0027-auto-crlf.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 93725895a4..e41c9b3bb2 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -322,7 +322,7 @@ test_expect_success 'setup master' '
 	echo >.gitattributes &&
 	git checkout -b master &&
 	git add .gitattributes &&
-	git commit -m "add .gitattributes" "" &&
+	git commit -m "add .gitattributes" . &&
 	printf "\$Id: 0000000000000000000000000000000000000000 \$\nLINEONE\nLINETWO\nLINETHREE"     >LF &&
 	printf "\$Id: 0000000000000000000000000000000000000000 \$\r\nLINEONE\r\nLINETWO\r\nLINETHREE" >CRLF &&
 	printf "\$Id: 0000000000000000000000000000000000000000 \$\nLINEONE\r\nLINETWO\nLINETHREE"   >CRLF_mix_LF &&
-- 
2.13.1-678-g93553a431c

Re: [PATCH] pathspec: die on empty strings as pathspec

[Torsten Bögershausen]

On 23/06/17 20:04, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:

Sorry for my mess, see below

> 
>> I am not sure if we even want the dot there, but at least that is
>> what the original author of the test intended to do when s/he
>> decided to pass an empty string as the pathspec.
>>
>>   t/t0027-auto-crlf.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I'll queue the following _before_ your "final step", so that the
> whole thing can be advanced to 'next' and hopefully to 'master' in
> some future releases.
> 
> Given that we ourselves did not even notice until now that one of
> our scripts get broken by this "final step", even though we kept
> issuing the warning message, we may want to re-think our overall
> strategy for deprecation.  We've assumed that "keep behaving as
> before but warn for a few years and then finally give a hard
> failure" would be sufficient, but depending on how the script that
> employ our programs hide the warnings from the end users, that may
> not be a good enough transition strategy.
> 
> At the same time we re-think the deprecation strategy, we also need
> to see if we can update our test framework to help us better.
> Ideally, we could have caught this existing breakage of passing ""
> as pathspec in the test soon after we went into "keep behaving as
> before but warn" stage.  We didn't and found it out only when we
> switched to "finally give a hard failure" stage.  That is very
> unsatisfactory.
> 
> Needless to say, neither of the above two comes from _your_ change.
> It's just something we need to improve in a larger scope of the
> whole project I realized.
> 
> Thanks.
> 
> -- >8 --
> Subject: [PATCH] t0027: do not use an empty string as a pathspec element
> 
> In an upcoming update, we will finally make an empty string illegal
> as an element in a pathspec; it traditionally meant the same as ".",
> i.e. include everything, so update this test that passes "" to pass
> a dot instead.
> 
> At this point in the test sequence, there is no modified path that
> need to be further added before committing; the working tree is
> empty except for .gitattributes which was just added to the index.
> So we could instead pass no pathspec, but this is a conversion more
> faithful to the original.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   t/t0027-auto-crlf.sh | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index 93725895a4..e41c9b3bb2 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -322,7 +322,7 @@ test_expect_success 'setup master' '
>   	echo >.gitattributes &&
>   	git checkout -b master &&
>   	git add .gitattributes &&
> -	git commit -m "add .gitattributes" "" &&
> +	git commit -m "add .gitattributes" . &&

Reading the context here, there shouldn't be a "pathspec" at all,
as .gitattributes had been added just one line above.

The line should have been from the very beginning:
	git commit -m "add .gitattributes"  &&

# What do you think ?


>   	printf "\$Id: 0000000000000000000000000000000000000000 \$\nLINEONE\nLINETWO\nLINETHREE"     >LF &&
>   	printf "\$Id: 0000000000000000000000000000000000000000 \$\r\nLINEONE\r\nLINETWO\r\nLINETHREE" >CRLF &&
>   	printf "\$Id: 0000000000000000000000000000000000000000 \$\nLINEONE\r\nLINETWO\nLINETHREE"   >CRLF_mix_LF &&
> 

Re: [PATCH] pathspec: die on empty strings as pathspec

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On 23/06/17 20:04, Junio C Hamano wrote:
>...
>> At this point in the test sequence, there is no modified path that
>> need to be further added before committing; the working tree is
>> empty except for .gitattributes which was just added to the index.
>> So we could instead pass no pathspec, but this is a conversion more
>> faithful to the original.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>   t/t0027-auto-crlf.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
>> index 93725895a4..e41c9b3bb2 100755
>> --- a/t/t0027-auto-crlf.sh
>> +++ b/t/t0027-auto-crlf.sh
>> @@ -322,7 +322,7 @@ test_expect_success 'setup master' '
>>   	echo >.gitattributes &&
>>   	git checkout -b master &&
>>   	git add .gitattributes &&
>> -	git commit -m "add .gitattributes" "" &&
>> +	git commit -m "add .gitattributes" . &&
>
> Reading the context here, there shouldn't be a "pathspec" at all,
> as .gitattributes had been added just one line above.
>
> The line should have been from the very beginning:
> 	git commit -m "add .gitattributes"  &&
>
> # What do you think ?

See above ;-)

And to be more explicit, it is not a fault of your earlier change,
either, that we didn't notice this problem since we started "warning".

We need to see if we can update our test framework to help us
better; I really think the test framework could and should have
helped us to notice this soon after we went into "keep behaving as
before but warn" stage.