{"thread":{"id":"42658","subject":"[PATCH] pathspec: prevent empty strings as pathspecs","startedAt":"2016-06-19T00:59:06Z","lastAt":"2016-06-19T20:33:31Z","messageCount":7,"participants":["Emily Xie","David Turner","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"289594","messageId":"20160619005704.1771-1-emilyxxie@gmail.com","threadId":"42658","inReplyTo":null,"subject":"[PATCH] pathspec: prevent empty strings as pathspecs","fromName":"Emily Xie","fromEmail":"emilyxxie@gmail.com","sentAt":"2016-06-19T00:57:04Z","receivedAt":"2016-06-19T00:59:06Z","isPatch":true,"sender":{"key":"emilyxxie@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5360525?v=4"},"body":"For any command that takes a pathspec, passing an empty string will\nexecute the command on all files in the current directory. This\nresults in unexpected behavior. For example, git add \"\" adds all\nfiles to staging, while git rm -rf \"\" recursively removes all files\nfrom the working tree and index. This patch prevents such cases by\nthrowing an error message whenever an empty string is detected as a\npathspec.\n\nSigned-off-by: Emily Xie \nReported-by: David Turner \nMentored-by: Michail Denchev \nThanks-to: Sarah Sharp and James Sharp \n---\n pathspec.c | 6 +++++-\n t/t3600-rm.sh | 4 ++++\n t/t3700-add.sh | 4 ++++\n 3 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex c9e9b6c..11901a2 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -402,8 +402,12 @@ void parse_pathspec(struct pathspec *pathspec,\n \t}\n \n \tn = 0;\n-\twhile (argv[n])\n+\twhile (argv[n]) {\n+\t\tif (*argv[n] == '\\0') {\n+\t\t\tdie(\"Empty string is not a valid pathspec.\");\n+\t\t}\n \t\tn++;\n+\t}\n \n \tpathspec->nr = n;\n \tALLOC_ARRAY(pathspec->items, n);\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex d046d98..1d7dc79 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -791,6 +791,10 @@ test_expect_success 'setup for testing rm messages' '\n \tgit add bar.txt foo.txt\n '\n \n+test_expect_success 'rm files with empty string pathspec' '\n+ test_must_fail git rm \"\"\n+'\n+\n test_expect_success 'rm files with different staged content' '\n \tcat >expect <<-\\EOF &&\n \terror: the following files have staged content different from both the\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex f14a665..49cb080 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -207,6 +207,10 @@ test_expect_success POSIXPERM,SANITY 'git add should fail atomically upon an unr\n \t! ( git ls-files foo1 | grep foo1 )\n '\n \n+test_expect_success 'git add empty string as pathspec must fail' '\n+ test_must_fail git add \"\"\n+'\n+\n rm -f foo2\n \n test_expect_success POSIXPERM,SANITY 'git add --ignore-errors' '\n-- \n2.8.4\n\n"},{"id":"289597","messageId":"1466300967.28660.9.camel@frank","threadId":"42658","inReplyTo":"20160619005704.1771-1-emilyxxie@gmail.com","subject":"Re: [PATCH] pathspec: prevent empty strings as pathspecs","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-06-19T01:49:27Z","receivedAt":"2016-06-19T01:49:38Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Sat, 2016-06-18 at 20:57 -0400, Emily Xie wrote:\n> For any command that takes a pathspec, passing an empty string will\n> execute the command on all files in the current directory. This\n> results in unexpected behavior. For example, git add \"\" adds all\n> files to staging, while git rm -rf \"\" recursively removes all files\n> from the working tree and index. This patch prevents such cases by\n> throwing an error message whenever an empty string is detected as a\n> pathspec.\n> \n> Signed-off-by: Emily Xie \n> Reported-by: David Turner \n> Mentored-by: Michail Denchev \n> Thanks-to: Sarah Sharp and James Sharp \n> ---\n\nOverall, lgtm.\n\nThe reason for this behavior is that, generally, traversals start at\nsome tree, and if there are no elements in the pathspec, the recursion\nends with that tree. Because this is a UX issue, fixing it at the\npathspec level seems correct to me.\n\n\n> pathspec.c | 6 +++++-\n> t/t3600-rm.sh | 4 ++++\n> t/t3700-add.sh | 4 ++++\n> 3 files changed, 13 insertions(+), 1 deletion(-)\n> \n> diff --git a/pathspec.c b/pathspec.c\n> index c9e9b6c..11901a2 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -402,8 +402,12 @@ void parse_pathspec(struct pathspec *pathspec,\n> \t}\n> \n> \tn = 0;\n> -\twhile (argv[n])\n> +\twhile (argv[n]) {\n> +\t\tif (*argv[n] == '\\0') {\n> +\t\t\tdie(\"Empty string is not a valid pathspec.\");\n> +\t\t}\n\nnit: git style doesn't use {} on one-statement ifs. \n\n> \t\tn++;\n> +\t}\n> \n> \tpathspec->nr = n;\n> \tALLOC_ARRAY(pathspec->items, n);\n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> index d046d98..1d7dc79 100755\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n> @@ -791,6 +791,10 @@ test_expect_success 'setup for testing rm messages' '\n> \tgit add bar.txt foo.txt\n> '\n> \n> +test_expect_success 'rm files with empty string pathspec' '\n> + test_must_fail git rm \"\"\n> +'\n> +\n\nMaybe check the error message here?\n\n"},{"id":"289598","messageId":"CAPig+cR5V7ZX8J9caYKTv4ECG_GP=N6geYQ+eReigicUC27cnA@mail.gmail.com","threadId":"42658","inReplyTo":"1466300967.28660.9.camel@frank","subject":"Re: [PATCH] pathspec: prevent empty strings as pathspecs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-19T02:23:30Z","receivedAt":"2016-06-19T02:23:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jun 18, 2016 at 9:49 PM, David Turner wrote:\n> On Sat, 2016-06-18 at 20:57 -0400, Emily Xie wrote:\n>> + while (argv[n]) {\n>> + if (*argv[n] == '\\0') {\n>> + die(\"Empty string is not a valid pathspec.\");\n>> + }\n>\n> nit: git style doesn't use {} on one-statement ifs.\n\nAlso, drop the full-stop (period) from the error message and, to be\nconsistent with modern practice, don't capitalize the first word.\nFinally, the string should be localizable, so wrap it in _(...):\n\n die(_(\"empty string not valid pathspec\"));\n"},{"id":"289600","messageId":"xmqqmvmhyiut.fsf@gitster.mtv.corp.google.com","threadId":"42658","inReplyTo":"20160619005704.1771-1-emilyxxie@gmail.com","subject":"Re: [PATCH] pathspec: prevent empty strings as pathspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-19T03:10:02Z","receivedAt":"2016-06-19T03:10:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Xie writes:\n\n> For any command that takes a pathspec, passing an empty string will\n> execute the command on all files in the current directory. This\n> results in unexpected behavior. For example, git add \"\" adds all\n> files to staging, while git rm -rf \"\" recursively removes all files\n> from the working tree and index. This patch prevents such cases by\n> throwing an error message whenever an empty string is detected as a\n> pathspec.\n\nThis change is likely to break people's existing scripts.\n\nFor example, a script may want to inspect some state of the whole\nworking tree and then after the inspection was satisfactory do\nsomething that affects paths that are inside the directory you were\noriginally in. You would write something like this:\n\n\t#!/bin/sh\n\toriginal=$(git rev-parse --show-prefix)\n\n cd \"$(git rev-parse --show-toplevel)\"\n ... do the whole-tree pre-inspection\n ... using various git commands here\n\n\t# Then finally do the \"action\"\n git add \"$original\"\n\nThis works even if you happen to be at the top-level of the working\ntree, but with your patch it suddenly breaks. It may be trivial to\nupdate the script to use \".\" when $original is empty, but that is\nnot an excuse. The important thing is that they need to be told\nabout having to change their script before this change actually\nhappens. Suddenly breaking other people without telling is simply\nrude and unacceptable.\n\nAt least you would need two-step process to introduce a change like\nthis to warn the people whose tools and workflows you are breaking.\nThat is, (1) in one release, you add code to only detect the case\nyou will be changing the behaviour in a later version and give\nwarning messages, and (2) in another release that is several release\ncycles later, stop warning and actually change the behaviour.\n\nThat is, if we assume that passing an empty string by mistake is a\nproblem in the real world that we want to save users from. After\nall, even though there is _no_ good reason why we _need_ to take\n'git add \"\"' as the same as 'git add .', there is no need not to,\neither. When a user says 'git add sub/sub/' we would add everything\nin that directory, 'git add sub/' would add even more, and it is\nlogically consistent for 'git add \"\"' to add everything. This is a\nborder-line \"doctor, if I pass an empty string, it is taken the same\nway as a dot. -- don't do that then.\"\n\nA kind of change that satisifies the following criteria:\n\n (1) I can tell that the author of the change really means well.\n\n (2) The problem the change is fixing does not seem to be a huge\n issue in real life.\n\n (3) The code with the patch would behave is not wrong per-se; it\n probably is how we would have designed the feature if we knew\n better.\n\n (4) However, we are not designing things from scratch without\n existing users, and the change potentially breaks uncountable\n number of existing users.\n\nis something I really hate having to evaluate. This is one of them.\n\nSo, I am of two minds.\n\nI do not mind a two-step breaking of compatibility to address this\nissue; I would also understand if the author thinks it is not worth\nthe hassle to do so. The sudden behaviour change with this patch\nalone is however not acceptable, I would think.\n"},{"id":"289605","messageId":"576623F7.3020700@novalis.org","threadId":"42658","inReplyTo":"xmqqmvmhyiut.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: prevent empty strings as pathspecs","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-06-19T04:47:51Z","receivedAt":"2016-06-19T04:47:56Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On 06/18/2016 11:10 PM, Junio C Hamano wrote:\n> Emily Xie writes:\n>\n>> For any command that takes a pathspec, passing an empty string will\n>> execute the command on all files in the current directory. This\n>> results in unexpected behavior. For example, git add \"\" adds all\n>> files to staging, while git rm -rf \"\" recursively removes all files\n>> from the working tree and index. This patch prevents such cases by\n>> throwing an error message whenever an empty string is detected as a\n>> pathspec.\n>\n> This change is likely to break people's existing scripts.\n>\n> For example, a script may want to inspect some state of the whole\n> working tree and then after the inspection was satisfactory do\n> something that affects paths that are inside the directory you were\n> originally in. You would write something like this:\n>\n> \t#!/bin/sh\n> \toriginal=$(git rev-parse --show-prefix)\n>\n> cd \"$(git rev-parse --show-toplevel)\"\n> ... do the whole-tree pre-inspection\n> ... using various git commands here\n>\n> \t# Then finally do the \"action\"\n> git add \"$original\"\n>\n> This works even if you happen to be at the top-level of the working\n> tree, but with your patch it suddenly breaks. It may be trivial to\n> update the script to use \".\" when $original is empty, but that is\n> not an excuse. The important thing is that they need to be told\n> about having to change their script before this change actually\n> happens. Suddenly breaking other people without telling is simply\n> rude and unacceptable.\n\nThe help for rev-parse --show-prefix says:\n\"When the command is invoked from a subdirectory, show the path of\nthe current directory relative to the top-level directory.\"\n\nThis doesn't say what it does when invoked at the top-level. Maybe we \nshould just change it to output \"./\", which would make such scripts \nwork? It would not necessarily fix all possible scripts, but it would \nfix these. It seem odd to insist on preserving this undocumented \nbehavior of git add/rm.\n\n> At least you would need two-step process to introduce a change like\n> this to warn the people whose tools and workflows you are breaking.\n> That is, (1) in one release, you add code to only detect the case\n> you will be changing the behaviour in a later version and give\n> warning messages, and (2) in another release that is several release\n> cycles later, stop warning and actually change the behaviour.\n>\n> That is, if we assume that passing an empty string by mistake is a\n> problem in the real world that we want to save users from.\n\nI discovered this when one of our shell-scripts had a variable was \naccidentally empty. The user I was supporting was very confused. So I \nthink it is a problem in the real world.\n\n\n"},{"id":"289607","messageId":"xmqq60t5ya55.fsf@gitster.mtv.corp.google.com","threadId":"42658","inReplyTo":"576623F7.3020700@novalis.org","subject":"Re: [PATCH] pathspec: prevent empty strings as pathspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-19T06:18:14Z","receivedAt":"2016-06-19T06:18:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> The help for rev-parse --show-prefix says:\n> \"When the command is invoked from a subdirectory, show the path of\n> the current directory relative to the top-level directory.\"\n>\n> This doesn't say what it does when invoked at the top-level. Maybe we\n> should just change it to output \"./\", which would make such scripts\n> work? It would not necessarily fix all possible scripts, but it would\n> fix these. It seem odd to insist on preserving this undocumented\n> behavior of git add/rm.\n\nFor two obvious reasons, you do not want to go there.\n\nAs you seem to understand in the above, \"--show-prefix\" was given\nmerely as an example, so updating its behaviour would not change the\nfundamental problem with the patch under discussion at all. It will\nbreak scripts that knew, documented or not, that pathspec match is a\nprefix substring match that honors directory boundary and an empty\nstring is defined long time ago to match any path and it does not\nmatter if that definition was made by mistake.\n\nAlso, if you change \"--show-prefix\" output, documented or not, that\nwill break other scripts that knew that checking it against an empty\nstring is a valid way to check if you are already at the top level\nof the working tree. And I highly suspect that such a change is\nmuch harder to advertise and warn existing scripts, compared to the\nchange proposed by the patch under discussion. The output routine\nin show-prefix knows what it is giving to the caller, but does not\nknow how the caller uses it and what for. Compared to that, ...\n\n>> At least you would need two-step process to introduce a change like\n>> this to warn the people whose tools and workflows you are breaking.\n>> That is, (1) in one release, you add code to only detect the case\n>> you will be changing the behaviour in a later version and give\n>> warning messages, and (2) in another release that is several release\n>> cycles later, stop warning and actually change the behaviour.\n\n... is a lot easier transition (again, assuming that this change is\nworth doing in the first place).\n"},{"id":"289641","messageId":"CAAin2AQjEUdE4Di1FJPHOFmwWEJX8-zCq_fRHiF=9RZ5S28XsQ@mail.gmail.com","threadId":"42658","inReplyTo":"xmqq60t5ya55.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: prevent empty strings as pathspecs","fromName":"Emily Xie","fromEmail":"emilyxxie@gmail.com","sentAt":"2016-06-19T20:32:18Z","receivedAt":"2016-06-19T20:33:31Z","isPatch":true,"sender":{"key":"emilyxxie@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5360525?v=4"},"body":"Thanks for the quick responses on this patch!\n\nIn response to Junio:\n\n> \"At least you would need two-step process to introduce a change like\n> this to warn the people whose tools and workflows you are breaking.\n> That is, (1) in one release, you add code to only detect the case\n> you will be changing the behaviour in a later version and give\n> warning messages, and (2) in another release that is several release\n> cycles later, stop warning and actually change the behaviour....\n\n> I do not mind a two-step breaking of compatibility to address this\n> issue; I would also understand if the author thinks it is not worth\n> the hassle to do so. The sudden behaviour change with this patch\n> alone is however not acceptable, I would think.\"\n\nI understand your hesitance with the original method. If the agreed\nsolution is a two-step implementation, I think it would definitely be\nworth my time and hassle----in part because I'm particularly excited\nabout the prospect of contributing to Git, but mostly because I do\nbelieve that it would be a good improvement.\n\nGiven this, I'll edit the patch and re-submit to only emit warning\nmessages for now.\n\n- Emily\n"}]}