{"thread":{"id":"46129","subject":"[PATCH] pathspec: die on empty strings as pathspec","startedAt":"2017-06-07T03:33:40Z","lastAt":"2017-06-23T20:59:45Z","messageCount":12,"participants":["Emily Xie","Junio C Hamano","Brandon Williams","Jeff King","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"321661","messageId":"20170607033308.33550-1-emilyxxie@gmail.com","threadId":"46129","inReplyTo":null,"subject":"[PATCH] pathspec: die on empty strings as pathspec","fromName":"Emily Xie","fromEmail":"emilyxxie@gmail.com","sentAt":"2017-06-07T03:33:08Z","receivedAt":"2017-06-07T03:33:40Z","isPatch":true,"sender":{"key":"emilyxxie@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5360525?v=4"},"body":"An empty string as a pathspec element matches all paths. A buggy\nscript, however, could accidentally assign an empty string to a\nvariable that then gets passed to a Git command invocation, e.g.:\n\n path=... compute a path to be removed in $path ...\n git rm -r \"$path\"\n\nwhich would unintentionally remove all paths in the current\ndirectory.\n\nThe fix for this issue comprises of two steps. Step 1, which warns\nthat empty strings as pathspecs will become invalid, has already\nbeen implemented in commit d426430 (\"pathspec: warn on empty strings\nas pathspec\", 2016-06-22).\n\nThis patch is step 2. It removes the warning and throws an error\ninstead.\n\nSigned-off-by: Emily Xie \nReported-by: David Turner \n---\n pathspec.c | 10 ++++------\n t/t3600-rm.sh | 5 ++---\n t/t3700-add.sh | 5 ++---\n 3 files changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 50f76ff..65e18b1 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -605,7 +605,7 @@ void parse_pathspec(struct pathspec *pathspec,\n {\n \tstruct pathspec_item *item;\n \tconst char *entry = argv ? *argv : NULL;\n-\tint i, n, prefixlen, warn_empty_string, nr_exclude = 0;\n+\tint i, n, prefixlen, nr_exclude = 0;\n \n \tmemset(pathspec, 0, sizeof(*pathspec));\n \n@@ -638,12 +638,10 @@ void parse_pathspec(struct pathspec *pathspec,\n \t}\n \n \tn = 0;\n-\twarn_empty_string = 1;\n \twhile (argv[n]) {\n-\t\tif (*argv[n] == '\\0' && warn_empty_string) {\n-\t\t\twarning(_(\"empty strings as pathspecs will be made invalid in upcoming releases. \"\n-\t\t\t\t \"please use . instead if you meant to match all paths\"));\n-\t\t\twarn_empty_string = 0;\n+\t\tif (*argv[n] == '\\0') {\n+\t\t\tdie(\"empty string is not a valid pathspec. \"\n+\t\t\t\t \"please use . instead if you meant to match all paths\");\n \t\t}\n \t\tn++;\n \t}\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 5f9913b..c787eac 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -858,9 +858,8 @@ test_expect_success 'rm files with two different errors' '\n \ttest_i18ncmp expect actual\n '\n \n-test_expect_success 'rm empty string should invoke warning' '\n-\tgit rm -rf \"\" 2>output &&\n-\ttest_i18ngrep \"warning: empty strings\" output\n+test_expect_success 'rm empty string should fail' '\n+\ttest_must_fail git rm -rf \"\"\n '\n \n test_done\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex f3a4b4a..40a0d2b 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -331,9 +331,8 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out\n \ttest_i18ncmp expect.err actual.err\n '\n \n-test_expect_success 'git add empty string should invoke warning' '\n-\tgit add \"\" 2>output &&\n-\ttest_i18ngrep \"warning: empty strings\" output\n+test_expect_success 'git add empty string should fail' '\n+\ttest_must_fail git add \"\"\n '\n \n test_expect_success 'git add --chmod=[+-]x stages correctly' '\n-- \n2.8.4\n\n"},{"id":"321837","messageId":"xmqq60g56s6x.fsf@gitster.mtv.corp.google.com","threadId":"46129","inReplyTo":"20170607033308.33550-1-emilyxxie@gmail.com","subject":"Re: [PATCH] pathspec: die on empty strings as pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-09T16:28:54Z","receivedAt":"2017-06-09T16:29:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Xie writes:\n\n> An empty string as a pathspec element matches all paths. A buggy\n> script, however, could accidentally assign an empty string to a\n> variable that then gets passed to a Git command invocation, e.g.:\n>\n> path=... compute a path to be removed in $path ...\n> git rm -r \"$path\"\n>\n> which would unintentionally remove all paths in the current\n> directory.\n>\n> The fix for this issue comprises of two steps. Step 1, which warns\n> that empty strings as pathspecs will become invalid, has already\n> been implemented in commit d426430 (\"pathspec: warn on empty strings\n> as pathspec\", 2016-06-22).\n>\n> This patch is step 2. It removes the warning and throws an error\n> instead.\n>\n> Signed-off-by: Emily Xie \n> Reported-by: David Turner \n> ---\n\nWe started this at v2.11.0 at the end of November 2016, and this\ncycle is expected to complete at around the end of July 2017, so\nthis patch makes it a 8-month deprecation cycle. I think that it\nshould be long enough.\n\nThanks.\n\n> diff --git a/pathspec.c b/pathspec.c\n> index 50f76ff..65e18b1 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -638,12 +638,10 @@ void parse_pathspec(struct pathspec *pathspec,\n> \t}\n> \n> \tn = 0;\n> -\twarn_empty_string = 1;\n> \twhile (argv[n]) {\n> -\t\tif (*argv[n] == '\\0' && warn_empty_string) {\n> -\t\t\twarning(_(\"empty strings as pathspecs will be made invalid in upcoming releases. \"\n> -\t\t\t\t \"please use . instead if you meant to match all paths\"));\n> -\t\t\twarn_empty_string = 0;\n> +\t\tif (*argv[n] == '\\0') {\n> +\t\t\tdie(\"empty string is not a valid pathspec. \"\n> +\t\t\t\t \"please use . instead if you meant to match all paths\");\n> \t\t}\n> \t\tn++;\n> \t}\n\nThe {} around a single statement becomes unnecessary, so I'll remove\nthem while queuing.\n"},{"id":"321841","messageId":"20170609165048.GC81269@google.com","threadId":"46129","inReplyTo":"20170607033308.33550-1-emilyxxie@gmail.com","subject":"Re: [PATCH] pathspec: die on empty strings as pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-09T16:50:48Z","receivedAt":"2017-06-09T16:50:58Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/06, Emily Xie wrote:\n> An empty string as a pathspec element matches all paths. A buggy\n> script, however, could accidentally assign an empty string to a\n> variable that then gets passed to a Git command invocation, e.g.:\n> \n> path=... compute a path to be removed in $path ...\n> git rm -r \"$path\"\n> \n> which would unintentionally remove all paths in the current\n> directory.\n> \n> The fix for this issue comprises of two steps. Step 1, which warns\n> that empty strings as pathspecs will become invalid, has already\n> been implemented in commit d426430 (\"pathspec: warn on empty strings\n> as pathspec\", 2016-06-22).\n> \n> This patch is step 2. It removes the warning and throws an error\n> instead.\n> \n> Signed-off-by: Emily Xie \n> Reported-by: David Turner \n> ---\n> pathspec.c | 10 ++++------\n> t/t3600-rm.sh | 5 ++---\n> t/t3700-add.sh | 5 ++---\n> 3 files changed, 8 insertions(+), 12 deletions(-)\n> \n> diff --git a/pathspec.c b/pathspec.c\n> index 50f76ff..65e18b1 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -605,7 +605,7 @@ void parse_pathspec(struct pathspec *pathspec,\n> {\n> \tstruct pathspec_item *item;\n> \tconst char *entry = argv ? *argv : NULL;\n> -\tint i, n, prefixlen, warn_empty_string, nr_exclude = 0;\n> +\tint i, n, prefixlen, nr_exclude = 0;\n> \n> \tmemset(pathspec, 0, sizeof(*pathspec));\n> \n> @@ -638,12 +638,10 @@ void parse_pathspec(struct pathspec *pathspec,\n> \t}\n> \n> \tn = 0;\n> -\twarn_empty_string = 1;\n> \twhile (argv[n]) {\n> -\t\tif (*argv[n] == '\\0' && warn_empty_string) {\n> -\t\t\twarning(_(\"empty strings as pathspecs will be made invalid in upcoming releases. \"\n> -\t\t\t\t \"please use . instead if you meant to match all paths\"));\n> -\t\t\twarn_empty_string = 0;\n> +\t\tif (*argv[n] == '\\0') {\n> +\t\t\tdie(\"empty string is not a valid pathspec. \"\n> +\t\t\t\t \"please use . instead if you meant to match all paths\");\n\nThe spacing (indentation) of the second line here is really weird.\n\n> \t\t}\n> \t\tn++;\n> \t}\n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> index 5f9913b..c787eac 100755\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n> @@ -858,9 +858,8 @@ test_expect_success 'rm files with two different errors' '\n> \ttest_i18ncmp expect actual\n> '\n> \n> -test_expect_success 'rm empty string should invoke warning' '\n> -\tgit rm -rf \"\" 2>output &&\n> -\ttest_i18ngrep \"warning: empty strings\" output\n> +test_expect_success 'rm empty string should fail' '\n> +\ttest_must_fail git rm -rf \"\"\n> '\n> \n> test_done\n> diff --git a/t/t3700-add.sh b/t/t3700-add.sh\n> index f3a4b4a..40a0d2b 100755\n> --- a/t/t3700-add.sh\n> +++ b/t/t3700-add.sh\n> @@ -331,9 +331,8 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out\n> \ttest_i18ncmp expect.err actual.err\n> '\n> \n> -test_expect_success 'git add empty string should invoke warning' '\n> -\tgit add \"\" 2>output &&\n> -\ttest_i18ngrep \"warning: empty strings\" output\n> +test_expect_success 'git add empty string should fail' '\n> +\ttest_must_fail git add \"\"\n> '\n> \n> test_expect_success 'git add --chmod=[+-]x stages correctly' '\n> -- \n> 2.8.4\n> \n\n-- \nBrandon Williams\n"},{"id":"321879","messageId":"xmqqfuf82xr5.fsf@gitster.mtv.corp.google.com","threadId":"46129","inReplyTo":"20170607033308.33550-1-emilyxxie@gmail.com","subject":"Re: [PATCH] pathspec: die on empty strings as pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-10T05:54:38Z","receivedAt":"2017-06-10T05:54:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Xie writes:\n\n> diff --git a/t/t3700-add.sh b/t/t3700-add.sh\n> index f3a4b4a..40a0d2b 100755\n> --- a/t/t3700-add.sh\n> +++ b/t/t3700-add.sh\n> @@ -331,9 +331,8 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out\n> \ttest_i18ncmp expect.err actual.err\n> '\n> \n> -test_expect_success 'git add empty string should invoke warning' '\n> -\tgit add \"\" 2>output &&\n> -\ttest_i18ngrep \"warning: empty strings\" output\n> +test_expect_success 'git add empty string should fail' '\n> +\ttest_must_fail git add \"\"\n> '\n\nDoesn't this affect the tests that follow this step? We used to\nwarn but went ahead and added all of them anyway, but now we fail\nwithout adding anything, which means that the state of the index\nbefore and after this patch would be different.\n\n> test_expect_success 'git add --chmod=[+-]x stages correctly' '\n"},{"id":"321884","messageId":"20170610062151.fv7d4audrwav2hy3@sigill.intra.peff.net","threadId":"46129","inReplyTo":"xmqq60g56s6x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: die on empty strings as pathspec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-10T06:21:51Z","receivedAt":"2017-06-10T06:21:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 10, 2017 at 01:28:54AM +0900, Junio C Hamano wrote:\n\n> Emily Xie writes:\n> \n> > An empty string as a pathspec element matches all paths. A buggy\n> > script, however, could accidentally assign an empty string to a\n> > variable that then gets passed to a Git command invocation, e.g.:\n> >\n> > path=... compute a path to be removed in $path ...\n> > git rm -r \"$path\"\n> >\n> > which would unintentionally remove all paths in the current\n> > directory.\n> >\n> > The fix for this issue comprises of two steps. Step 1, which warns\n> > that empty strings as pathspecs will become invalid, has already\n> > been implemented in commit d426430 (\"pathspec: warn on empty strings\n> > as pathspec\", 2016-06-22).\n> >\n> > This patch is step 2. It removes the warning and throws an error\n> > instead.\n> >\n> > Signed-off-by: Emily Xie \n> > Reported-by: David Turner \n> > ---\n> \n> We started this at v2.11.0 at the end of November 2016, and this\n> cycle is expected to complete at around the end of July 2017, so\n> this patch makes it a 8-month deprecation cycle. I think that it\n> should be long enough.\n\nThat's long enough for people who actually ran the intermediate\nversions. But what about people on distros who jump from v2.10 or lower\nstraight to v2.14?\n\nI think to catch them we'd literally need years on our deprecation\nschedules. Maybe it's not worth caring about. That's an awful long time\nfor people who _are_ upgrading each version to see the warning. Of\ncourse the end game in this case is that it becomes an error, so they'll\nbe notified either way. :)\n\nI'm OK with this case being a relatively quick deprecation, but your\n\"long enough\" made me wonder how we would decide that.\n\n-Peff\n\n[1] Debian stable is set to release in a week or so. People doing that\n stable upgrade will go from v2.1.4 to v2.11.0. So they won't\n actually skip the deprecation notice, but they'll probably live with\n the warning for 3 years or so.\n"},{"id":"321911","messageId":"xmqqvao414wb.fsf@gitster.mtv.corp.google.com","threadId":"46129","inReplyTo":"20170610062151.fv7d4audrwav2hy3@sigill.intra.peff.net","subject":"Re: [PATCH] pathspec: die on empty strings as pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-10T11:03:16Z","receivedAt":"2017-06-10T11:03:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That's long enough for people who actually ran the intermediate\n> versions. But what about people on distros who jump from v2.10 or lower\n> straight to v2.14?\n>\n> I think to catch them we'd literally need years on our deprecation\n> schedules. Maybe it's not worth caring about. That's an awful long time\n> for people who _are_ upgrading each version to see the warning. Of\n> course the end game in this case is that it becomes an error, so they'll\n> be notified either way. :)\n>\n> I'm OK with this case being a relatively quick deprecation, but your\n> \"long enough\" made me wonder how we would decide that.\n>\n> -Peff\n>\n> [1] Debian stable is set to release in a week or so. People doing that\n> stable upgrade will go from v2.1.4 to v2.11.0. So they won't\n> actually skip the deprecation notice, but they'll probably live with\n> the warning for 3 years or so.\n\nYuck X-<.\n"},{"id":"323050","messageId":"CAAin2ARfUCYM95L_k3fSQ--reQD3uMG72ah=Xg1xvvgKcU+QkQ@mail.gmail.com","threadId":"46129","inReplyTo":"xmqqfuf82xr5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: die on empty strings as pathspec","fromName":"Emily Xie","fromEmail":"emilyxxie@gmail.com","sentAt":"2017-06-23T02:34:31Z","receivedAt":"2017-06-23T02:35:18Z","isPatch":true,"sender":{"key":"emilyxxie@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5360525?v=4"},"body":"I ran the tests and none of them failed. Technically, the state of the\nindex would indeed be different with these new changes, but this\nshouldn't be an issue. In the current version, there's one only item\nadded to the index that ends up getting used in subsequent tests. That\nis, foo1, which is tested in: 'git add --chmod=[+-]x stages\ncorrectly'.\n\nWith the current code, foo1 gets added to the index with a mode of\n100644. When the 'git add --chmod=[+-]x stages correctly' test\nexecutes, it removes foo1 from the working directory, creates a new\nfoo1, and runs 'git add --chmod=+x'. It then checks if foo1 is in the\nindex with a file mode of 100755.\n\nGiven how things are set up, the tests pass either way. But I think\nit'd actually be nicer with the new changes in this patch, as foo1\nwould not already be in the index by the time 'git add --chmod=[+-]x\nstages correctly' executes.\n\nOn Sat, Jun 10, 2017 at 1:54 AM, Junio C Hamano wrote:\n>\n> Emily Xie writes:\n>\n> > diff --git a/t/t3700-add.sh b/t/t3700-add.sh\n> > index f3a4b4a..40a0d2b 100755\n> > --- a/t/t3700-add.sh\n> > +++ b/t/t3700-add.sh\n> > @@ -331,9 +331,8 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out\n> > test_i18ncmp expect.err actual.err\n> > '\n> >\n> > -test_expect_success 'git add empty string should invoke warning' '\n> > - git add \"\" 2>output &&\n> > - test_i18ngrep \"warning: empty strings\" output\n> > +test_expect_success 'git add empty string should fail' '\n> > + test_must_fail git add \"\"\n> > '\n>\n> Doesn't this affect the tests that follow this step? We used to\n> warn but went ahead and added all of them anyway, but now we fail\n> without adding anything, which means that the state of the index\n> before and after this patch would be different.\n>\n> > test_expect_success 'git add --chmod=[+-]x stages correctly' '\n"},{"id":"323052","messageId":"xmqqefubgxa3.fsf@gitster.mtv.corp.google.com","threadId":"46129","inReplyTo":"CAAin2ARfUCYM95L_k3fSQ--reQD3uMG72ah=Xg1xvvgKcU+QkQ@mail.gmail.com","subject":"Re: [PATCH] pathspec: die on empty strings as pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-23T04:09:40Z","receivedAt":"2017-06-23T04:09:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Xie writes:\n\n> I ran the tests and none of them failed. \n\nThis is not about a test you touched, but applied to or merged to\nany of the recent integration branches (like 'master' or 'maint')\n\n $ make\n $ cd t\n $ GIT_TEST_LONG=YesPlease sh ./t0027-*.sh\n\nfails at the very beginning. I do not know if 0027 is the only one\nthat triggers this failure, though.\n\nTHanks.\n\n"},{"id":"323054","messageId":"xmqq60fngvky.fsf@gitster.mtv.corp.google.com","threadId":"46129","inReplyTo":"xmqqefubgxa3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: die on empty strings as pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-23T04:46:21Z","receivedAt":"2017-06-23T04:46:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Emily Xie writes:\n>\n>> I ran the tests and none of them failed. \n>\n> This is not about a test you touched, but applied to or merged to\n> any of the recent integration branches (like 'master' or 'maint')\n>\n> $ make\n> $ cd t\n> $ GIT_TEST_LONG=YesPlease sh ./t0027-*.sh\n>\n> fails at the very beginning. I do not know if 0027 is the only one\n> that triggers this failure, though.\n\nt0027 should pass with this, I think.\n\nI am not sure if we even want the dot there, but at least that is\nwhat the original author of the test intended to do when s/he\ndecided to pass an empty string as the pathspec.\n\n t/t0027-auto-crlf.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\nindex 93725895a4..e41c9b3bb2 100755\n--- a/t/t0027-auto-crlf.sh\n+++ b/t/t0027-auto-crlf.sh\n@@ -322,7 +322,7 @@ test_expect_success 'setup master' '\n \techo >.gitattributes &&\n \tgit checkout -b master &&\n \tgit add .gitattributes &&\n-\tgit commit -m \"add .gitattributes\" \"\" &&\n+\tgit commit -m \"add .gitattributes\" . &&\n \tprintf \"\\$Id: 0000000000000000000000000000000000000000 \\$\\nLINEONE\\nLINETWO\\nLINETHREE\" >LF &&\n \tprintf \"\\$Id: 0000000000000000000000000000000000000000 \\$\\r\\nLINEONE\\r\\nLINETWO\\r\\nLINETHREE\" >CRLF &&\n \tprintf \"\\$Id: 0000000000000000000000000000000000000000 \\$\\nLINEONE\\r\\nLINETWO\\nLINETHREE\" >CRLF_mix_LF &&\n"},{"id":"323118","messageId":"xmqqzicyeg2p.fsf@gitster.mtv.corp.google.com","threadId":"46129","inReplyTo":"xmqq60fngvky.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: die on empty strings as pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-23T18:04:14Z","receivedAt":"2017-06-23T18:04:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I am not sure if we even want the dot there, but at least that is\n> what the original author of the test intended to do when s/he\n> decided to pass an empty string as the pathspec.\n>\n> t/t0027-auto-crlf.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nI'll queue the following _before_ your \"final step\", so that the\nwhole thing can be advanced to 'next' and hopefully to 'master' in\nsome future releases. \n\nGiven that we ourselves did not even notice until now that one of\nour scripts get broken by this \"final step\", even though we kept\nissuing the warning message, we may want to re-think our overall\nstrategy for deprecation. We've assumed that \"keep behaving as\nbefore but warn for a few years and then finally give a hard\nfailure\" would be sufficient, but depending on how the script that\nemploy our programs hide the warnings from the end users, that may\nnot be a good enough transition strategy.\n\nAt the same time we re-think the deprecation strategy, we also need\nto see if we can update our test framework to help us better.\nIdeally, we could have caught this existing breakage of passing \"\"\nas pathspec in the test soon after we went into \"keep behaving as\nbefore but warn\" stage. We didn't and found it out only when we\nswitched to \"finally give a hard failure\" stage. That is very\nunsatisfactory.\n\nNeedless to say, neither of the above two comes from _your_ change.\nIt's just something we need to improve in a larger scope of the\nwhole project I realized.\n\nThanks.\n\n-- >8 --\nSubject: [PATCH] t0027: do not use an empty string as a pathspec element\n\nIn an upcoming update, we will finally make an empty string illegal\nas an element in a pathspec; it traditionally meant the same as \".\",\ni.e. include everything, so update this test that passes \"\" to pass\na dot instead.\n\nAt this point in the test sequence, there is no modified path that\nneed to be further added before committing; the working tree is\nempty except for .gitattributes which was just added to the index.\nSo we could instead pass no pathspec, but this is a conversion more\nfaithful to the original.\n\nSigned-off-by: Junio C Hamano \n---\n t/t0027-auto-crlf.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\nindex 93725895a4..e41c9b3bb2 100755\n--- a/t/t0027-auto-crlf.sh\n+++ b/t/t0027-auto-crlf.sh\n@@ -322,7 +322,7 @@ test_expect_success 'setup master' '\n \techo >.gitattributes &&\n \tgit checkout -b master &&\n \tgit add .gitattributes &&\n-\tgit commit -m \"add .gitattributes\" \"\" &&\n+\tgit commit -m \"add .gitattributes\" . &&\n \tprintf \"\\$Id: 0000000000000000000000000000000000000000 \\$\\nLINEONE\\nLINETWO\\nLINETHREE\" >LF &&\n \tprintf \"\\$Id: 0000000000000000000000000000000000000000 \\$\\r\\nLINEONE\\r\\nLINETWO\\r\\nLINETHREE\" >CRLF &&\n \tprintf \"\\$Id: 0000000000000000000000000000000000000000 \\$\\nLINEONE\\r\\nLINETWO\\nLINETHREE\" >CRLF_mix_LF &&\n-- \n2.13.1-678-g93553a431c\n\n\n\n\n"},{"id":"323153","messageId":"802ccd37-c09a-8343-63fe-a42c682a8311@web.de","threadId":"46129","inReplyTo":"xmqqzicyeg2p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: die on empty strings as pathspec","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-06-23T20:52:44Z","receivedAt":"2017-06-23T20:52:04Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\nOn 23/06/17 20:04, Junio C Hamano wrote:\n> Junio C Hamano writes:\n\nSorry for my mess, see below\n\n> \n>> I am not sure if we even want the dot there, but at least that is\n>> what the original author of the test intended to do when s/he\n>> decided to pass an empty string as the pathspec.\n>>\n>> t/t0027-auto-crlf.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> I'll queue the following _before_ your \"final step\", so that the\n> whole thing can be advanced to 'next' and hopefully to 'master' in\n> some future releases.\n> \n> Given that we ourselves did not even notice until now that one of\n> our scripts get broken by this \"final step\", even though we kept\n> issuing the warning message, we may want to re-think our overall\n> strategy for deprecation. We've assumed that \"keep behaving as\n> before but warn for a few years and then finally give a hard\n> failure\" would be sufficient, but depending on how the script that\n> employ our programs hide the warnings from the end users, that may\n> not be a good enough transition strategy.\n> \n> At the same time we re-think the deprecation strategy, we also need\n> to see if we can update our test framework to help us better.\n> Ideally, we could have caught this existing breakage of passing \"\"\n> as pathspec in the test soon after we went into \"keep behaving as\n> before but warn\" stage. We didn't and found it out only when we\n> switched to \"finally give a hard failure\" stage. That is very\n> unsatisfactory.\n> \n> Needless to say, neither of the above two comes from _your_ change.\n> It's just something we need to improve in a larger scope of the\n> whole project I realized.\n> \n> Thanks.\n> \n> -- >8 --\n> Subject: [PATCH] t0027: do not use an empty string as a pathspec element\n> \n> In an upcoming update, we will finally make an empty string illegal\n> as an element in a pathspec; it traditionally meant the same as \".\",\n> i.e. include everything, so update this test that passes \"\" to pass\n> a dot instead.\n> \n> At this point in the test sequence, there is no modified path that\n> need to be further added before committing; the working tree is\n> empty except for .gitattributes which was just added to the index.\n> So we could instead pass no pathspec, but this is a conversion more\n> faithful to the original.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> t/t0027-auto-crlf.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\n> index 93725895a4..e41c9b3bb2 100755\n> --- a/t/t0027-auto-crlf.sh\n> +++ b/t/t0027-auto-crlf.sh\n> @@ -322,7 +322,7 @@ test_expect_success 'setup master' '\n> \techo >.gitattributes &&\n> \tgit checkout -b master &&\n> \tgit add .gitattributes &&\n> -\tgit commit -m \"add .gitattributes\" \"\" &&\n> +\tgit commit -m \"add .gitattributes\" . &&\n\nReading the context here, there shouldn't be a \"pathspec\" at all,\nas .gitattributes had been added just one line above.\n\nThe line should have been from the very beginning:\n\tgit commit -m \"add .gitattributes\" &&\n\n# What do you think ?\n\n\n> \tprintf \"\\$Id: 0000000000000000000000000000000000000000 \\$\\nLINEONE\\nLINETWO\\nLINETHREE\" >LF &&\n> \tprintf \"\\$Id: 0000000000000000000000000000000000000000 \\$\\r\\nLINEONE\\r\\nLINETWO\\r\\nLINETHREE\" >CRLF &&\n> \tprintf \"\\$Id: 0000000000000000000000000000000000000000 \\$\\nLINEONE\\r\\nLINETWO\\nLINETHREE\" >CRLF_mix_LF &&\n> \n"},{"id":"323155","messageId":"xmqqr2yactdx.fsf@gitster.mtv.corp.google.com","threadId":"46129","inReplyTo":"802ccd37-c09a-8343-63fe-a42c682a8311@web.de","subject":"Re: [PATCH] pathspec: die on empty strings as pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-23T20:59:38Z","receivedAt":"2017-06-23T20:59:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> On 23/06/17 20:04, Junio C Hamano wrote:\n>...\n>> At this point in the test sequence, there is no modified path that\n>> need to be further added before committing; the working tree is\n>> empty except for .gitattributes which was just added to the index.\n>> So we could instead pass no pathspec, but this is a conversion more\n>> faithful to the original.\n>>\n>> Signed-off-by: Junio C Hamano \n>> ---\n>> t/t0027-auto-crlf.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\n>> index 93725895a4..e41c9b3bb2 100755\n>> --- a/t/t0027-auto-crlf.sh\n>> +++ b/t/t0027-auto-crlf.sh\n>> @@ -322,7 +322,7 @@ test_expect_success 'setup master' '\n>> \techo >.gitattributes &&\n>> \tgit checkout -b master &&\n>> \tgit add .gitattributes &&\n>> -\tgit commit -m \"add .gitattributes\" \"\" &&\n>> +\tgit commit -m \"add .gitattributes\" . &&\n>\n> Reading the context here, there shouldn't be a \"pathspec\" at all,\n> as .gitattributes had been added just one line above.\n>\n> The line should have been from the very beginning:\n> \tgit commit -m \"add .gitattributes\" &&\n>\n> # What do you think ?\n\nSee above ;-)\n\nAnd to be more explicit, it is not a fault of your earlier change,\neither, that we didn't notice this problem since we started \"warning\".\n\nWe need to see if we can update our test framework to help us\nbetter; I really think the test framework could and should have\nhelped us to notice this soon after we went into \"keep behaving as\nbefore but warn\" stage.\n"}]}