# [PATCH v3] git: make "git -C '' " not to barf 5 messages from 2015-03-06 to 2015-03-06. Participants: Karthik Nayak, Eric Sunshine, karthik nayak, Andreas Schwab, Junio C Hamano. Thread: https://gitlist.dev/t/38721 ## Karthik Nayak, 2015-03-06 07:05 Subject: [PATCH v3] git: make "git -C '' " not to barf Message-ID: <1425625534-11869-1-git-send-email-karthik.188@gmail.com> URL: https://gitlist.dev/e/1425625534-11869-1-git-send-email-karthik.188%40gmail.com ``` It now acts like "cd ''" and does not barf and treats it as a no-op. This is useful if a caller function does not want to change directory and hence gives no path value, which would have generally caused git to output an undesired error message. Included a simple test to check the same, as suggested by Junio. Signed-off-by: Karthik Nayak --- git.c | 12 ++++++++---- t/t0056-git-C.sh | 8 ++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/git.c b/git.c index 8c7ee9c..d734afa 100644 --- a/git.c +++ b/git.c @@ -204,10 +204,14 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) fprintf(stderr, "No directory given for -C.\n" ); usage(git_usage_string); } - if (chdir((*argv)[1])) - die_errno("Cannot change to '%s'", (*argv)[1]); - if (envchanged) - *envchanged = 1; + if (*((*argv)[1]) == 0) + ; /* DO not change directory if no directory is given*/ + else { + if (chdir((*argv)[1])) + die_errno("Cannot change to '%s'", (*argv)[1]); + if (envchanged) + *envchanged = 1; + } (*argv)++; (*argc)--; } else { diff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh index 99c0377..a6b52f1 100755 --- a/t/t0056-git-C.sh +++ b/t/t0056-git-C.sh @@ -14,6 +14,14 @@ test_expect_success '"git -C " runs git from the directory ' ' test_cmp expected actual ' +test_expect_success '"git -C " with an empty is a no-op' ' + mkdir -p dir1/subdir && + cd dir1/subdir && + git -C "" rev-parse --show-prefix >actual && + echo subdir/ >expect + test_cmp expect actual +' + test_expect_success 'Multiple -C options: "-C dir1 -C dir2" is equivalent to "-C dir1/dir2"' ' test_create_repo dir1/dir2 && echo 1 >dir1/dir2/b.txt && -- 2.3.1.167.g7f4ba4b.dirty ``` ## Eric Sunshine, 2015-03-06 08:53 Subject: Re: [PATCH v3] git: make "git -C '' " not to barf Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSeTA%3DSfQDouP-WCaSjsMBkUg%3D9Qm8xxfp0jcq%3D%2BGSksg%40mail.gmail.com In-Reply-To: <1425625534-11869-1-git-send-email-karthik.188@gmail.com> ``` On Fri, Mar 6, 2015 at 2:05 AM, Karthik Nayak wrote: > It now acts like "cd ''" and does not barf and treats > it as a no-op. What does "barf" mean in this context? Does the program crash? Spit out nonsensical messages? Misbehave in some fashion? A good commit message should explain the problem with sufficient detail so readers don't need to guess what the "bad" behavior is. > This is useful if a caller function > does not want to change directory and hence gives no > path value, which would have generally caused git to > output an undesired error message. This is an odd justification. A caller not wanting to change the directory wouldn't pass -C in the first place. A better justification might be that die()ing is unnecessarily harsh behavior for what otherwise could be considered a no-op, citing "cd ''" as an example. Also, write in imperative mood, as if you're instructing the code to change itself. Taking the above observations into consideration, you might say: git: treat `-C ' as a no-op when is empty `git -C ""' unhelpfully dies with error "Cannot change to ''", whereas the shell treats `cd ""' as a no-op. Taking the shell's behavior as a precedent, teach git to treat `-C ""' as a no-op, as well. > Included a simple test to check the same, as suggested > by Junio. It is a bit weak to say that Junio "suggested" the test, considering that he actually wrote it[1]. > Signed-off-by: Karthik Nayak > --- > diff --git a/git.c b/git.c > index 8c7ee9c..d734afa 100644 > --- a/git.c > +++ b/git.c > @@ -204,10 +204,14 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) > fprintf(stderr, "No directory given for -C.\n" ); > usage(git_usage_string); > } > - if (chdir((*argv)[1])) > - die_errno("Cannot change to '%s'", (*argv)[1]); > - if (envchanged) > - *envchanged = 1; > + if (*((*argv)[1]) == 0) Saying '\0' rather than 0 would make the intent clearer. > + ; /* DO not change directory if no directory is given*/ > + else { > + if (chdir((*argv)[1])) > + die_errno("Cannot change to '%s'", (*argv)[1]); > + if (envchanged) > + *envchanged = 1; > + } The 'if/else' statement you've composed (with an empty 'if' branch) is unnecessarily complicated when a simple 'if' suffices: if (*(*argv)[1]) { if (chdir((*argv)[1])) die_errno("Cannot change to '%s'", (*argv)[1]); if (envchanged) *envchanged = 1; } > (*argv)++; > (*argc)--; > } else { > diff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh > index 99c0377..a6b52f1 100755 > --- a/t/t0056-git-C.sh > +++ b/t/t0056-git-C.sh > @@ -14,6 +14,14 @@ test_expect_success '"git -C " runs git from the directory ' ' > test_cmp expected actual > ' > > +test_expect_success '"git -C " with an empty is a no-op' ' > + mkdir -p dir1/subdir && > + cd dir1/subdir && When Junio composed this test[1], he intentionally wrapped it in a subshell via '(' and ')'. The problem with dropping the subshell, as you did here, is that the 'cd' in this test will still be in effect when tests following this one are run, which typically will break them. Wrapping the test in a subshell side-steps the problem because the parent shell is not affected by 'cd' within the subshell. To summarize: Don't remove the subshell from Junio's example. (You lucked out in this case, by accident, since the following tests are not impacted by such ill-behavior.) > + git -C "" rev-parse --show-prefix >actual && > + echo subdir/ >expect Broken &&-chain. > + test_cmp expect actual > +' > + > test_expect_success 'Multiple -C options: "-C dir1 -C dir2" is equivalent to "-C dir1/dir2"' ' > test_create_repo dir1/dir2 && > echo 1 >dir1/dir2/b.txt && > -- > 2.3.1.167.g7f4ba4b.dirty [1]: http://article.gmane.org/gmane.comp.version-control.git/264871 ``` ## karthik nayak, 2015-03-06 10:53 Subject: Re: [PATCH v3] git: make "git -C '' " not to barf Message-ID: <54F98711.6010401@gmail.com> URL: https://gitlist.dev/e/54F98711.6010401%40gmail.com In-Reply-To: ``` On 03/06/2015 02:23 PM, Eric Sunshine wrote: > On Fri, Mar 6, 2015 at 2:05 AM, Karthik Nayak wrote: > > It now acts like "cd ''" and does not barf and treats > > it as a no-op. > > What does "barf" mean in this context? Does the program crash? Spit > out nonsensical messages? Misbehave in some fashion? A good commit > message should explain the problem with sufficient detail so readers > don't need to guess what the "bad" behavior is. > > > This is useful if a caller function > > does not want to change directory and hence gives no > > path value, which would have generally caused git to > > output an undesired error message. > > This is an odd justification. A caller not wanting to change the > directory wouldn't pass -C in the first place. A better justification > might be that die()ing is unnecessarily harsh behavior for what > otherwise could be considered a no-op, citing "cd ''" as an example. > > Also, write in imperative mood, as if you're instructing the code to > change itself. > > Taking the above observations into consideration, you might say: > > git: treat `-C ' as a no-op when is empty > > `git -C ""' unhelpfully dies with error "Cannot change to ''", > whereas the shell treats `cd ""' as a no-op. Taking the shell's > behavior as a precedent, teach git to treat `-C ""' as a no-op, as > well. > Hey Eric, I see what you mean. > > Included a simple test to check the same, as suggested > > by Junio. > > It is a bit weak to say that Junio "suggested" the test, considering > that he actually wrote it[1]. > > Signed-off-by: Karthik Nayak > > --- > > diff --git a/git.c b/git.c > > index 8c7ee9c..d734afa 100644 > > --- a/git.c > > +++ b/git.c > > @@ -204,10 +204,14 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) > > fprintf(stderr, "No directory given for -C.\n" ); > > usage(git_usage_string); > > } > > - if (chdir((*argv)[1])) > > - die_errno("Cannot change to '%s'", (*argv)[1]); > > - if (envchanged) > > - *envchanged = 1; > > + if (*((*argv)[1]) == 0) > > Saying '\0' rather than 0 would make the intent clearer. > > > + ; /* DO not change directory if no directory is given*/ > > + else { > > + if (chdir((*argv)[1])) > > + die_errno("Cannot change to '%s'", (*argv)[1]); > > + if (envchanged) > > + *envchanged = 1; > > + } > > The 'if/else' statement you've composed (with an empty 'if' branch) is > unnecessarily complicated when a simple 'if' suffices: Yes, will change this. > > if (*(*argv)[1]) { > if (chdir((*argv)[1])) > die_errno("Cannot change to '%s'", (*argv)[1]); > if (envchanged) > *envchanged = 1; > } > > > (*argv)++; > > (*argc)--; > > } else { > > diff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh > > index 99c0377..a6b52f1 100755 > > --- a/t/t0056-git-C.sh > > +++ b/t/t0056-git-C.sh > > @@ -14,6 +14,14 @@ test_expect_success '"git -C " runs git from the directory ' ' > > test_cmp expected actual > > ' > > > > +test_expect_success '"git -C " with an empty is a no-op' ' > > + mkdir -p dir1/subdir && > > + cd dir1/subdir && > > When Junio composed this test[1], he intentionally wrapped it in a > subshell via '(' and ')'. The problem with dropping the subshell, as > you did here, is that the 'cd' in this test will still be in effect > when tests following this one are run, which typically will break > them. Wrapping the test in a subshell side-steps the problem because > the parent shell is not affected by 'cd' within the subshell. To > summarize: Don't remove the subshell from Junio's example. > > (You lucked out in this case, by accident, since the following tests > are not impacted by such ill-behavior.) I did not know that. I got lucky, definitely. > > > + git -C "" rev-parse --show-prefix >actual && > > + echo subdir/ >expect > > Broken &&-chain. > > > + test_cmp expect actual > > +' > > + > > test_expect_success 'Multiple -C options: "-C dir1 -C dir2" is equivalent to "-C dir1/dir2"' ' > > test_create_repo dir1/dir2 && > > echo 1 >dir1/dir2/b.txt && > > -- > > 2.3.1.167.g7f4ba4b.dirty > > [1]: http://article.gmane.org/gmane.comp.version-control.git/264871 > Thanks for the code review, will be back with the next patch :D ``` ## Andreas Schwab, 2015-03-06 15:57 Subject: Re: [PATCH v3] git: make "git -C '' " not to barf Message-ID: <87d24mnmon.fsf@igel.home> URL: https://gitlist.dev/e/87d24mnmon.fsf%40igel.home In-Reply-To: <1425625534-11869-1-git-send-email-karthik.188@gmail.com> ``` Karthik Nayak writes: > + if (*((*argv)[1]) == 0) IMHO (*argv)[1][0] is easier to understand. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ``` ## Junio C Hamano, 2015-03-06 18:31 Subject: Re: [PATCH v3] git: make "git -C '' " not to barf Message-ID: URL: https://gitlist.dev/e/xmqqpp8myo3q.fsf%40gitster.dls.corp.google.com In-Reply-To: <87d24mnmon.fsf@igel.home> ``` Andreas Schwab writes: > Karthik Nayak writes: > >> + if (*((*argv)[1]) == 0) > > IMHO (*argv)[1][0] is easier to understand. Thanks for saying that. I had to scratch my head every time I had to see this change from various people ;-) ```