[PATCH v3] git: make "git -C '' <cmd>" not to barf

[PATCH v3] git: make "git -C '' <cmd>" not to barf

[Karthik Nayak]

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 <karthik.188@gmail.com>
---
 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 <path>" runs git from the directory <path>' '
 	test_cmp expected actual
 '
 
+test_expect_success '"git -C <path>" with an empty <path> 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

Re: [PATCH v3] git: make "git -C '' <cmd>" not to barf

[Eric Sunshine]

On Fri, Mar 6, 2015 at 2:05 AM, Karthik Nayak <karthik.188@gmail.com> 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 <path>' as a no-op when <path> 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 <karthik.188@gmail.com>
> ---
> 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 <path>" runs git from the directory <path>' '
>         test_cmp expected actual
>  '
>
> +test_expect_success '"git -C <path>" with an empty <path> 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

Re: [PATCH v3] git: make "git -C '' <cmd>" not to barf

[karthik nayak]

On 03/06/2015 02:23 PM, Eric Sunshine wrote:
> On Fri, Mar 6, 2015 at 2:05 AM, Karthik Nayak <karthik.188@gmail.com> 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 <path>' as a no-op when <path> 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 <karthik.188@gmail.com>
> > ---
> > 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 <path>" runs git from the directory <path>' '
> >          test_cmp expected actual
> >   '
> >
> > +test_expect_success '"git -C <path>" with an empty <path> 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

Re: [PATCH v3] git: make "git -C '' <cmd>" not to barf

[Andreas Schwab]

Karthik Nayak <karthik.188@gmail.com> 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."

Re: [PATCH v3] git: make "git -C '' <cmd>" not to barf

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Karthik Nayak <karthik.188@gmail.com> 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 ;-)