{"thread":{"id":"38721","subject":"[PATCH v3] git: make \"git -C '' \" not to barf","startedAt":"2015-03-06T07:05:34Z","lastAt":"2015-03-06T18:31:21Z","messageCount":5,"participants":["Karthik Nayak","Eric Sunshine","karthik nayak","Andreas Schwab","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"257130","messageId":"1425625534-11869-1-git-send-email-karthik.188@gmail.com","threadId":"38721","inReplyTo":null,"subject":"[PATCH v3] git: make \"git -C '' \" not to barf","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-03-06T07:05:34Z","receivedAt":"2015-03-06T07:05:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"It now acts like \"cd ''\" and does not barf and treats\nit as a no-op. This is useful if a caller function\ndoes not want to change directory and hence gives no\npath value, which would have generally caused git to\noutput an undesired error message.\n\nIncluded a simple test to check the same, as suggested\nby Junio.\n\nSigned-off-by: Karthik Nayak \n---\n git.c | 12 ++++++++----\n t/t0056-git-C.sh | 8 ++++++++\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 8c7ee9c..d734afa 100644\n--- a/git.c\n+++ b/git.c\n@@ -204,10 +204,14 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\tfprintf(stderr, \"No directory given for -C.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tif (chdir((*argv)[1]))\n-\t\t\t\tdie_errno(\"Cannot change to '%s'\", (*argv)[1]);\n-\t\t\tif (envchanged)\n-\t\t\t\t*envchanged = 1;\n+\t\t\tif (*((*argv)[1]) == 0)\n+\t\t\t\t; /* DO not change directory if no directory is given*/\n+\t\t\telse {\n+\t\t\t\tif (chdir((*argv)[1]))\n+\t\t\t\t\tdie_errno(\"Cannot change to '%s'\", (*argv)[1]);\n+\t\t\t\tif (envchanged)\n+\t\t\t\t\t*envchanged = 1;\n+\t\t\t}\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t} else {\ndiff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh\nindex 99c0377..a6b52f1 100755\n--- a/t/t0056-git-C.sh\n+++ b/t/t0056-git-C.sh\n@@ -14,6 +14,14 @@ test_expect_success '\"git -C \" runs git from the directory ' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success '\"git -C \" with an empty is a no-op' '\n+\tmkdir -p dir1/subdir &&\n+\tcd dir1/subdir &&\n+\tgit -C \"\" rev-parse --show-prefix >actual &&\n+\techo subdir/ >expect\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'Multiple -C options: \"-C dir1 -C dir2\" is equivalent to \"-C dir1/dir2\"' '\n \ttest_create_repo dir1/dir2 &&\n \techo 1 >dir1/dir2/b.txt &&\n-- \n2.3.1.167.g7f4ba4b.dirty\n"},{"id":"257135","messageId":"CAPig+cSeTA=SfQDouP-WCaSjsMBkUg=9Qm8xxfp0jcq=+GSksg@mail.gmail.com","threadId":"38721","inReplyTo":"1425625534-11869-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v3] git: make \"git -C '' \" not to barf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-06T08:53:39Z","receivedAt":"2015-03-06T08:53:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 6, 2015 at 2:05 AM, Karthik Nayak wrote:\n> It now acts like \"cd ''\" and does not barf and treats\n> it as a no-op.\n\nWhat does \"barf\" mean in this context? Does the program crash? Spit\nout nonsensical messages? Misbehave in some fashion? A good commit\nmessage should explain the problem with sufficient detail so readers\ndon't need to guess what the \"bad\" behavior is.\n\n> This is useful if a caller function\n> does not want to change directory and hence gives no\n> path value, which would have generally caused git to\n> output an undesired error message.\n\nThis is an odd justification. A caller not wanting to change the\ndirectory wouldn't pass -C in the first place. A better justification\nmight be that die()ing is unnecessarily harsh behavior for what\notherwise could be considered a no-op, citing \"cd ''\" as an example.\n\nAlso, write in imperative mood, as if you're instructing the code to\nchange itself.\n\nTaking the above observations into consideration, you might say:\n\n git: treat `-C ' as a no-op when is empty\n\n `git -C \"\"' unhelpfully dies with error \"Cannot change to ''\",\n whereas the shell treats `cd \"\"' as a no-op. Taking the shell's\n behavior as a precedent, teach git to treat `-C \"\"' as a no-op, as\n well.\n\n> Included a simple test to check the same, as suggested\n> by Junio.\n\nIt is a bit weak to say that Junio \"suggested\" the test, considering\nthat he actually wrote it[1].\n\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/git.c b/git.c\n> index 8c7ee9c..d734afa 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -204,10 +204,14 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> fprintf(stderr, \"No directory given for -C.\\n\" );\n> usage(git_usage_string);\n> }\n> - if (chdir((*argv)[1]))\n> - die_errno(\"Cannot change to '%s'\", (*argv)[1]);\n> - if (envchanged)\n> - *envchanged = 1;\n> + if (*((*argv)[1]) == 0)\n\nSaying '\\0' rather than 0 would make the intent clearer.\n\n> + ; /* DO not change directory if no directory is given*/\n> + else {\n> + if (chdir((*argv)[1]))\n> + die_errno(\"Cannot change to '%s'\", (*argv)[1]);\n> + if (envchanged)\n> + *envchanged = 1;\n> + }\n\nThe 'if/else' statement you've composed (with an empty 'if' branch) is\nunnecessarily complicated when a simple 'if' suffices:\n\n if (*(*argv)[1]) {\n if (chdir((*argv)[1]))\n die_errno(\"Cannot change to '%s'\", (*argv)[1]);\n if (envchanged)\n *envchanged = 1;\n }\n\n> (*argv)++;\n> (*argc)--;\n> } else {\n> diff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh\n> index 99c0377..a6b52f1 100755\n> --- a/t/t0056-git-C.sh\n> +++ b/t/t0056-git-C.sh\n> @@ -14,6 +14,14 @@ test_expect_success '\"git -C \" runs git from the directory ' '\n> test_cmp expected actual\n> '\n>\n> +test_expect_success '\"git -C \" with an empty is a no-op' '\n> + mkdir -p dir1/subdir &&\n> + cd dir1/subdir &&\n\nWhen Junio composed this test[1], he intentionally wrapped it in a\nsubshell via '(' and ')'. The problem with dropping the subshell, as\nyou did here, is that the 'cd' in this test will still be in effect\nwhen tests following this one are run, which typically will break\nthem. Wrapping the test in a subshell side-steps the problem because\nthe parent shell is not affected by 'cd' within the subshell. To\nsummarize: Don't remove the subshell from Junio's example.\n\n(You lucked out in this case, by accident, since the following tests\nare not impacted by such ill-behavior.)\n\n> + git -C \"\" rev-parse --show-prefix >actual &&\n> + echo subdir/ >expect\n\nBroken &&-chain.\n\n> + test_cmp expect actual\n> +'\n> +\n> test_expect_success 'Multiple -C options: \"-C dir1 -C dir2\" is equivalent to \"-C dir1/dir2\"' '\n> test_create_repo dir1/dir2 &&\n> echo 1 >dir1/dir2/b.txt &&\n> --\n> 2.3.1.167.g7f4ba4b.dirty\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/264871\n"},{"id":"257147","messageId":"54F98711.6010401@gmail.com","threadId":"38721","inReplyTo":"CAPig+cSeTA=SfQDouP-WCaSjsMBkUg=9Qm8xxfp0jcq=+GSksg@mail.gmail.com","subject":"Re: [PATCH v3] git: make \"git -C '' \" not to barf","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-03-06T10:53:05Z","receivedAt":"2015-03-06T10:53:05Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\nOn 03/06/2015 02:23 PM, Eric Sunshine wrote:\n> On Fri, Mar 6, 2015 at 2:05 AM, Karthik Nayak wrote:\n> > It now acts like \"cd ''\" and does not barf and treats\n> > it as a no-op.\n>\n> What does \"barf\" mean in this context? Does the program crash? Spit\n> out nonsensical messages? Misbehave in some fashion? A good commit\n> message should explain the problem with sufficient detail so readers\n> don't need to guess what the \"bad\" behavior is.\n>\n> > This is useful if a caller function\n> > does not want to change directory and hence gives no\n> > path value, which would have generally caused git to\n> > output an undesired error message.\n>\n> This is an odd justification. A caller not wanting to change the\n> directory wouldn't pass -C in the first place. A better justification\n> might be that die()ing is unnecessarily harsh behavior for what\n> otherwise could be considered a no-op, citing \"cd ''\" as an example.\n>\n> Also, write in imperative mood, as if you're instructing the code to\n> change itself.\n>\n> Taking the above observations into consideration, you might say:\n>\n> git: treat `-C ' as a no-op when is empty\n>\n> `git -C \"\"' unhelpfully dies with error \"Cannot change to ''\",\n> whereas the shell treats `cd \"\"' as a no-op. Taking the shell's\n> behavior as a precedent, teach git to treat `-C \"\"' as a no-op, as\n> well.\n>\nHey Eric,\nI see what you mean.\n> > Included a simple test to check the same, as suggested\n> > by Junio.\n>\n> It is a bit weak to say that Junio \"suggested\" the test, considering\n> that he actually wrote it[1].\n> > Signed-off-by: Karthik Nayak \n> > ---\n> > diff --git a/git.c b/git.c\n> > index 8c7ee9c..d734afa 100644\n> > --- a/git.c\n> > +++ b/git.c\n> > @@ -204,10 +204,14 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> > fprintf(stderr, \"No directory given for -C.\\n\" );\n> > usage(git_usage_string);\n> > }\n> > - if (chdir((*argv)[1]))\n> > - die_errno(\"Cannot change to '%s'\", (*argv)[1]);\n> > - if (envchanged)\n> > - *envchanged = 1;\n> > + if (*((*argv)[1]) == 0)\n>\n> Saying '\\0' rather than 0 would make the intent clearer.\n>\n> > + ; /* DO not change directory if no directory is given*/\n> > + else {\n> > + if (chdir((*argv)[1]))\n> > + die_errno(\"Cannot change to '%s'\", (*argv)[1]);\n> > + if (envchanged)\n> > + *envchanged = 1;\n> > + }\n>\n> The 'if/else' statement you've composed (with an empty 'if' branch) is\n> unnecessarily complicated when a simple 'if' suffices:\nYes, will change this.\n>\n> if (*(*argv)[1]) {\n> if (chdir((*argv)[1]))\n> die_errno(\"Cannot change to '%s'\", (*argv)[1]);\n> if (envchanged)\n> *envchanged = 1;\n> }\n>\n> > (*argv)++;\n> > (*argc)--;\n> > } else {\n> > diff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh\n> > index 99c0377..a6b52f1 100755\n> > --- a/t/t0056-git-C.sh\n> > +++ b/t/t0056-git-C.sh\n> > @@ -14,6 +14,14 @@ test_expect_success '\"git -C \" runs git from the directory ' '\n> > test_cmp expected actual\n> > '\n> >\n> > +test_expect_success '\"git -C \" with an empty is a no-op' '\n> > + mkdir -p dir1/subdir &&\n> > + cd dir1/subdir &&\n>\n> When Junio composed this test[1], he intentionally wrapped it in a\n> subshell via '(' and ')'. The problem with dropping the subshell, as\n> you did here, is that the 'cd' in this test will still be in effect\n> when tests following this one are run, which typically will break\n> them. Wrapping the test in a subshell side-steps the problem because\n> the parent shell is not affected by 'cd' within the subshell. To\n> summarize: Don't remove the subshell from Junio's example.\n>\n> (You lucked out in this case, by accident, since the following tests\n> are not impacted by such ill-behavior.)\nI did not know that. I got lucky, definitely.\n>\n> > + git -C \"\" rev-parse --show-prefix >actual &&\n> > + echo subdir/ >expect\n>\n> Broken &&-chain.\n>\n> > + test_cmp expect actual\n> > +'\n> > +\n> > test_expect_success 'Multiple -C options: \"-C dir1 -C dir2\" is equivalent to \"-C dir1/dir2\"' '\n> > test_create_repo dir1/dir2 &&\n> > echo 1 >dir1/dir2/b.txt &&\n> > --\n> > 2.3.1.167.g7f4ba4b.dirty\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/264871\n>\nThanks for the code review, will be back with the next patch :D\n"},{"id":"257167","messageId":"87d24mnmon.fsf@igel.home","threadId":"38721","inReplyTo":"1425625534-11869-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v3] git: make \"git -C '' \" not to barf","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2015-03-06T15:57:28Z","receivedAt":"2015-03-06T15:57:28Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Karthik Nayak writes:\n\n> +\t\t\tif (*((*argv)[1]) == 0)\n\nIMHO (*argv)[1][0] is easier to understand.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"257184","messageId":"xmqqpp8myo3q.fsf@gitster.dls.corp.google.com","threadId":"38721","inReplyTo":"87d24mnmon.fsf@igel.home","subject":"Re: [PATCH v3] git: make \"git -C '' \" not to barf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-06T18:31:21Z","receivedAt":"2015-03-06T18:31:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Schwab writes:\n\n> Karthik Nayak writes:\n>\n>> +\t\t\tif (*((*argv)[1]) == 0)\n>\n> IMHO (*argv)[1][0] is easier to understand.\n\nThanks for saying that. I had to scratch my head every time I had\nto see this change from various people ;-)\n"}]}