[PATCH v4] git: treat "-C <treat>" as a no-op when <path> is empty

[PATCH v4] git: treat "-C <treat>" as a no-op when <path> is empty

[Karthik Nayak]

'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.

Test to check the no-op behaviour of "-C <path>" when <path> is
empty, written by Junio C Hamano.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunchine <sunshine@sunshineco.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 git.c            | 10 ++++++----
 t/t0056-git-C.sh | 10 ++++++++++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index 8c7ee9c..b062e0e 100644
--- a/git.c
+++ b/git.c
@@ -204,10 +204,12 @@ 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]) {
+				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..2630e75 100755
--- a/t/t0056-git-C.sh
+++ b/t/t0056-git-C.sh
@@ -14,6 +14,16 @@ 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 v4] git: treat "-C <treat>" as a no-op when <path> is empty

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> '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.
>
> Test to check the no-op behaviour of "-C <path>" when <path> is
> empty, written by Junio C Hamano.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunchine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---

This iteration looks sensible, except that the Subject reads
strange.  Will queue with minor tweaks to the log message,
and perhaps with a fix to unreadable *(*argv)[1] that was
mentioned elsewhere.

Thanks.

>  git.c            | 10 ++++++----
>  t/t0056-git-C.sh | 10 ++++++++++
>  2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/git.c b/git.c
> index 8c7ee9c..b062e0e 100644
> --- a/git.c
> +++ b/git.c
> @@ -204,10 +204,12 @@ 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]) {
> +				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..2630e75 100755
> --- a/t/t0056-git-C.sh
> +++ b/t/t0056-git-C.sh
> @@ -14,6 +14,16 @@ 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 &&

Re: [PATCH v4] git: treat "-C <treat>" as a no-op when <path> is empty

[Eric Sunshine]

On Fri, Mar 6, 2015 at 6:18 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> '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.
>
> Test to check the no-op behaviour of "-C <path>" when <path> is
> empty, written by Junio C Hamano.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunchine <sunshine@sunshineco.com>

s/Sunchine/Sunshine/

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>

Re: [PATCH v4] git: treat "-C <treat>" as a no-op when <path> is empty

[karthik nayak]

 > This iteration looks sensible, except that the Subject reads
 > strange.  Will queue with minor tweaks to the log message,
 > and perhaps with a fix to unreadable *(*argv)[1] that was
 > mentioned elsewhere.
 >
 > Thanks.

Hey could you tell me what seems strange, so I can improve on
it the next time.
Also "*(*argv)[1]" seems more readable to me, maybe more of a perspective?

Thanks


On 03/07/2015 07:49 AM, Eric Sunshine wrote:
> On Fri, Mar 6, 2015 at 6:18 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> '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.
>>
>> Test to check the no-op behaviour of "-C <path>" when <path> is
>> empty, written by Junio C Hamano.
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Helped-by: Eric Sunchine <sunshine@sunshineco.com>
>
> s/Sunchine/Sunshine/
>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>

Sorry Eric for the genuine mistake.

Re: [PATCH v4] git: treat "-C <treat>" as a no-op when <path> is empty

[Eric Sunshine]

On Sat, Mar 7, 2015 at 5:49 AM, karthik nayak <karthik.188@gmail.com> wrote:
>> This iteration looks sensible, except that the Subject reads
>> strange.  Will queue with minor tweaks to the log message,
>> and perhaps with a fix to unreadable *(*argv)[1] that was
>> mentioned elsewhere.
>
> Hey could you tell me what seems strange, so I can improve on
> it the next time.

Junio means that you somehow botched the Subject: line when you copied
the commit message I suggested into your new version of the patch.
Instead of <path>, you wrote <treat>.

> Also "*(*argv)[1]" seems more readable to me, maybe more of a perspective?

I also had considered suggesting (*argv)[1][0] as more readable, but
it is primarily personal taste, and I didn't want to bike-shed the
issue.

Re: [PATCH v4] git: treat "-C <treat>" as a no-op when <path> is empty

[karthik nayak]

On 03/08/2015 10:08 AM, Eric Sunshine wrote:
> On Sat, Mar 7, 2015 at 5:49 AM, karthik nayak <karthik.188@gmail.com> wrote:
> >> This iteration looks sensible, except that the Subject reads
> >> strange.  Will queue with minor tweaks to the log message,
> >> and perhaps with a fix to unreadable *(*argv)[1] that was
> >> mentioned elsewhere.
> >
> > Hey could you tell me what seems strange, so I can improve on
> > it the next time.
>
> Junio means that you somehow botched the Subject: line when you copied
> the commit message I suggested into your new version of the patch.
> Instead of <path>, you wrote <treat>.
>
Oops! I'm too anxious i guess.
> > Also "*(*argv)[1]" seems more readable to me, maybe more of a perspective?
>
> I also had considered suggesting (*argv)[1][0] as more readable, but
> it is primarily personal taste, and I didn't want to bike-shed the
> issue.
>
Thanks.

Re: [PATCH v4] git: treat "-C <treat>" as a no-op when <path> is empty

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sat, Mar 7, 2015 at 5:49 AM, karthik nayak <karthik.188@gmail.com> wrote:
>
>> Also "*(*argv)[1]" seems more readable to me, maybe more of a perspective?
>
> I also had considered suggesting (*argv)[1][0] as more readable, but
> it is primarily personal taste, and I didn't want to bike-shed the
> issue.

I didn't mention that in earlier review rounds for the same reason,
but I saw Andreas Schwab also independently made the same comment,
so that makes three of us.

That does not change the fact that this is merely a matter of
preference; I wouldn't even call this one a "taste" (use of which
implies that there are judgement involved, as in "good taste" and
"bad taste").

But because it is "preference", the only time we can discuss is when
a new code is submitted and is under review.  Once it is in the
tree, it is not really worth extra patch noise to go and change.

As everybody knows, POINTER[0] and *POINTER are equivalent.  We have
quite a few places where we say "let's treat passing an empty string
the same as not passing an argument at all" with

	if (!POINTER || !*POINTER)
        	; /* no-op */
	else {
        	/* do something with POINTER */
                fn(POINTER);
	}

and we could say !POINTER[0] instead of !*POINTER, interchangeably.

We tend to prefer (again, I do not think this is particularly a
"taste" thing) *POINTER over POINTER[0] when POINTER is just a
single variable in the above pattern we often see in our code.

But when POINTER is an expression like (*argv)[1], where you unwrap
the operators according to their precedences, it often is easier to
read if you do not have to flip your eyes left and right too often.

You first look at "argv", then notice the prefix "*" (you have to
move your eyes to the left here) and know argv points at a location
that holds a pointer.  Then you notice the suffix [1] (now to the
right) and know that pointer points at an array and the expression
is talking about in its second element.

Now, you want to say that second element is actually a pointer to a
string and want to talk about the beginning of that string.  If you
express it as "*(*argv)[1]", it forces the reader to go back to the
left end once more.  If you write it as "(*argv)[1][0]", the reader
can keep going to the right, starting from the last thing the reader
read and understood (which is the "[1]" at the right end).

At least, that is how I analyze _my_ preference---the latter feels
easier on my eyes.

But as I said this is a mere preference thing.

Re: [PATCH v4] git: treat "-C <treat>" as a no-op when <path> is empty

[karthik nayak]

On 03/08/2015 12:44 PM, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Sat, Mar 7, 2015 at 5:49 AM, karthik nayak <karthik.188@gmail.com> wrote:
>>
>>> Also "*(*argv)[1]" seems more readable to me, maybe more of a perspective?
>>
>> I also had considered suggesting (*argv)[1][0] as more readable, but
>> it is primarily personal taste, and I didn't want to bike-shed the
>> issue.
>
> I didn't mention that in earlier review rounds for the same reason,
> but I saw Andreas Schwab also independently made the same comment,
> so that makes three of us.
>
> That does not change the fact that this is merely a matter of
> preference; I wouldn't even call this one a "taste" (use of which
> implies that there are judgement involved, as in "good taste" and
> "bad taste").
>
> But because it is "preference", the only time we can discuss is when
> a new code is submitted and is under review.  Once it is in the
> tree, it is not really worth extra patch noise to go and change.
>
> As everybody knows, POINTER[0] and *POINTER are equivalent.  We have
> quite a few places where we say "let's treat passing an empty string
> the same as not passing an argument at all" with
>
> 	if (!POINTER || !*POINTER)
>          	; /* no-op */
> 	else {
>          	/* do something with POINTER */
>                  fn(POINTER);
> 	}
>
> and we could say !POINTER[0] instead of !*POINTER, interchangeably.
>
> We tend to prefer (again, I do not think this is particularly a
> "taste" thing) *POINTER over POINTER[0] when POINTER is just a
> single variable in the above pattern we often see in our code.
>
> But when POINTER is an expression like (*argv)[1], where you unwrap
> the operators according to their precedences, it often is easier to
> read if you do not have to flip your eyes left and right too often.
>
> You first look at "argv", then notice the prefix "*" (you have to
> move your eyes to the left here) and know argv points at a location
> that holds a pointer.  Then you notice the suffix [1] (now to the
> right) and know that pointer points at an array and the expression
> is talking about in its second element.
>
> Now, you want to say that second element is actually a pointer to a
> string and want to talk about the beginning of that string.  If you
> express it as "*(*argv)[1]", it forces the reader to go back to the
> left end once more.  If you write it as "(*argv)[1][0]", the reader
> can keep going to the right, starting from the last thing the reader
> read and understood (which is the "[1]" at the right end).
>
> At least, that is how I analyze _my_ preference---the latter feels
> easier on my eyes.
>
> But as I said this is a mere preference thing.
>

The way you put it, it makes a lot of sense that most would prefer 
"(*argv)[1][0]" rather than "*(*argv)[1]".

Thanks for clearing that out.
Regards
-Karthik