threads / patch / 46129

patchpathspec: die on empty strings as pathspec

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

## tl;dr

12 messages between Jun 7, 2017 and Jun 23, 2017. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

Emily Xie· Jun 7, 2017, 03:33 UTC · lore

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(-)
Show changes to 3 files +8 −12

pathspec.c, t/t3600-rm.sh, t/t3700-add.sh

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
Junio C Hamano· Jun 9, 2017, 16:28 UTC · re: Emily Xie · lore

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

Emily Xie <emilyxxie@gmail.com> writes:
Show 21 quoted lines
> 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.
Show 20 quoted lines
> 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.

Jeff King· Jun 10, 2017, 06:21 UTC · re: Junio C Hamano · lore

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

On Sat, Jun 10, 2017 at 01:28:54AM +0900, Junio C Hamano wrote:
Show 28 quoted lines
> 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.
Junio C Hamano· Jun 10, 2017, 11:03 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> 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-<.
Brandon Williams· Jun 9, 2017, 16:50 UTC · re: Emily Xie · lore

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

On 06/06, Emily Xie wrote:
Show 52 quoted lines
> 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.
Show 38 quoted lines
>  		}
>  		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
Junio C Hamano· Jun 10, 2017, 05:54 UTC · re: Emily Xie · lore

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

Emily Xie <emilyxxie@gmail.com> writes:
Show 14 quoted lines
> 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' '
Emily Xie· Jun 23, 2017, 02:34 UTC · re: Junio C Hamano · lore

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

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:
Show 24 quoted lines
>
> 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' '
Junio C Hamano· Jun 23, 2017, 04:09 UTC · re: Emily Xie · lore

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

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.
Junio C Hamano· Jun 23, 2017, 04:46 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> 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(-)
Show changes to t/t0027-auto-crlf.sh +1 −1
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 &&
Junio C Hamano· Jun 23, 2017, 18:04 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> 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(-)
Show changes to t/t0027-auto-crlf.sh +1 −1
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
Torsten Bögershausen· Jun 23, 2017, 20:52 UTC · re: Junio C Hamano · lore

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

On 23/06/17 20:04, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
Sorry for my mess, see below
Show 64 quoted lines
> 
>> 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 &&
>
Junio C Hamano· Jun 23, 2017, 20:59 UTC · re: Torsten Bögershausen · lore

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

Torsten Bögershausen <tboegi@web.de> writes:
Show 31 quoted lines
> 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.

← back to recent threads