[PATCH] rev-parse --show-prefix: add in trailing newline

[PATCH] rev-parse --show-prefix: add in trailing newline

[Ross Lagerwall]

Print out a trailing newline when --show-prefix is run with cwd
at the top level of the tree which results in an empty prefix.
Behavior is now like --show-cdup.

Fixes an expected failure in t1501.

Signed-off-by: Ross Lagerwall <rosslagerwall@gmail.com>
---
 builtin/rev-parse.c |    2 ++
 t/t1501-worktree.sh |    2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 98d1cbe..733f626 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -634,6 +634,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			if (!strcmp(arg, "--show-prefix")) {
 				if (prefix)
 					puts(prefix);
+				else
+					putchar('\n');
 				continue;
 			}
 			if (!strcmp(arg, "--show-cdup")) {
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index e661147..8f36aa9 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -68,7 +68,7 @@ test_expect_success 'inside work tree' '
 	)
 '
 
-test_expect_failure 'empty prefix is actually written out' '
+test_expect_success 'empty prefix is actually written out' '
 	echo >expected &&
 	(
 		cd work &&
-- 
1.7.10

Re: [PATCH] rev-parse --show-prefix: add in trailing newline

[Junio C Hamano]

Ross Lagerwall <rosslagerwall@gmail.com> writes:

> diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
> index e661147..8f36aa9 100755
> --- a/t/t1501-worktree.sh
> +++ b/t/t1501-worktree.sh
> @@ -68,7 +68,7 @@ test_expect_success 'inside work tree' '
>  	)
>  '
>  
> -test_expect_failure 'empty prefix is actually written out' '
> +test_expect_success 'empty prefix is actually written out' '
>  	echo >expected &&
>  	(
>  		cd work &&

fc68875 (t1501 (rev-parse): clarify, 2010-07-24) says it is a "possible
bug", but I do not think the expectation of the new test added by that
commit is entirely correct.

Because the assignment will strip the trailing LF away adding an extra
newline at the end would not matter in practice in many scripts, e.g.

	a=$(git rev-parse --show-prefix)
        if test -z "$a"
        then
		echo we are at toplevel
	fi

or

	pfx=$(git rev-parse --show-prefix)
	cd_to_toplevel
	for arg
        do
		case "$arg" in
                /*)
                       	user refers to "$arg" that is a full path ;;
		*)
			user refers to "$pfx$arg" ;;
		esac
	done

But it will break existing scripts if they expect the following to work:

	file="$(git rev-parse --show-prefix)/$1"
        cd_to_toplevel
        test -f "$file"

Re: [PATCH] rev-parse --show-prefix: add in trailing newline

[Ross Lagerwall]

On 2012/04/09 21:07, Junio C Hamano wrote:
> fc68875 (t1501 (rev-parse): clarify, 2010-07-24) says it is a "possible
> bug", but I do not think the expectation of the new test added by that
> commit is entirely correct.
>
> Because the assignment will strip the trailing LF away adding an extra
> newline at the end would not matter in practice in many scripts, e.g.
>

True, and the current behavior does exactly what the man page says:
"When the command is invoked from a subdirectory, show the path of the 
current directory relative to the top-level directory."

So then perhaps the test should be removed?

Also, --show-cdup has a similar description for the man page but does 
output a newline when invoked from the root dir. Should this behavior be 
made consistent?

-- 
Ross Lagerwall

Re: [PATCH] rev-parse --show-prefix: add in trailing newline

[Jonathan Nieder]

Junio C Hamano wrote:

> But it will break existing scripts if they expect the following to work:
>
> 	file="$(git rev-parse --show-prefix)/$1"
>         cd_to_toplevel
>         test -f "$file"

Huh?  I tried this:

	$ foo="$(printf '\n')/bar"
	$ printf '%s\n' "$foo"
	/bar

If that didn't work, it would mean that "git rev-parse --show-prefix"
would never work when the prefix is nonempty, because it is followed
by a newline.

Re: [PATCH] rev-parse --show-prefix: add in trailing newline

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> writes:

> Because the assignment will strip the trailing LF away

No, it doesn't.  Command substitution does.

> But it will break existing scripts if they expect the following to work:
>
> 	file="$(git rev-parse --show-prefix)/$1"
>         cd_to_toplevel
>         test -f "$file"

This is already broken because of the extra slash.

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] rev-parse --show-prefix: add in trailing newline

[Junio C Hamano]

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

> This is already broken because of the extra slash.

Yeah, you are right.

Re: [PATCH] rev-parse --show-prefix: add in trailing newline

[Jonathan Nieder]

(cc-ing Duy)
Hi,

Ross Lagerwall wrote:

> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -634,6 +634,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  			if (!strcmp(arg, "--show-prefix")) {
>  				if (prefix)
>  					puts(prefix);
> +				else
> +					putchar('\n');
>  				continue;

This makes the output more consistent but it is a little puzzling how
it does that.  Why is prefix NULL instead of "" when we are at the
toplevel of the worktree?

Thanks,
Jonathan

Re: [PATCH] rev-parse --show-prefix: add in trailing newline

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> (cc-ing Duy)
> Hi,
>
> Ross Lagerwall wrote:
>
>> --- a/builtin/rev-parse.c
>> +++ b/builtin/rev-parse.c
>> @@ -634,6 +634,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>>  			if (!strcmp(arg, "--show-prefix")) {
>>  				if (prefix)
>>  					puts(prefix);
>> +				else
>> +					putchar('\n');
>>  				continue;
>
> This makes the output more consistent but it is a little puzzling how
> it does that.  Why is prefix NULL instead of "" when we are at the
> toplevel of the worktree?

Interesting point ;-)

Re: [PATCH] rev-parse --show-prefix: add in trailing newline

[Nguyen Thai Ngoc Duy]

2012/4/11 Jonathan Nieder <jrnieder@gmail.com>:
> (cc-ing Duy)
> Hi,
>
> Ross Lagerwall wrote:
>
>> --- a/builtin/rev-parse.c
>> +++ b/builtin/rev-parse.c
>> @@ -634,6 +634,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>>                       if (!strcmp(arg, "--show-prefix")) {
>>                               if (prefix)
>>                                       puts(prefix);
>> +                             else
>> +                                     putchar('\n');
>>                               continue;
>
> This makes the output more consistent but it is a little puzzling how
> it does that.  Why is prefix NULL instead of "" when we are at the
> toplevel of the worktree?

It dates back to the first version of setup_git_directory() in d288a70
([PATCH] Make "git diff" work inside relative subdirectories -
2005-08-16). If we get "", setup_git_.. converts it to NULL. Although
I think Linus made a mistake in the first version of "rev-parse
--git-dir" doing "puts(prefix);" unconditionally in the same commit.
Junio fixed --git-dir in 4866ccf (Rationalize output selection in
rev-parse. - 2005-08-24) with the "if (prefix)" clause and should have
added the "else putchar('\n');" there too, I think.
-- 
Duy