threads / patch / 30188

patchrev-parse --show-prefix: add in trailing newline

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

## tl;dr

9 messages between Apr 9, 2012 and Apr 10, 2012. Diffs are folded; open one to read it.

replies: 8people: 5as markdown or json

Ross Lagerwall· Apr 9, 2012, 13:27 UTC · lore

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(-)
Show changes to 2 files +3 −1

builtin/rev-parse.c, t/t1501-worktree.sh

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
Junio C Hamano· Apr 9, 2012, 19:07 UTC · re: Ross Lagerwall · lore

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

Ross Lagerwall <rosslagerwall@gmail.com> writes:
Show 13 quoted lines
> 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"
Ross Lagerwall· Apr 10, 2012, 05:30 UTC · re: Junio C Hamano · lore

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

On 2012/04/09 21:07, Junio C Hamano wrote:
Show 7 quoted lines
> 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
Jonathan Nieder· Apr 10, 2012, 14:04 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 5 quoted lines
> 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.

Andreas Schwab· Apr 10, 2012, 14:41 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
> Because the assignment will strip the trailing LF away
No, it doesn't.  Command substitution does.
Show 5 quoted lines
> 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."
Jonathan Nieder· Apr 10, 2012, 17:36 UTC · re: Ross Lagerwall · lore

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

(cc-ing Duy) Hi,

Ross Lagerwall wrote:
Show 9 quoted lines
> --- 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

Junio C Hamano· Apr 10, 2012, 17:56 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 18 quoted lines
> (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 ;-)
Nguyen Thai Ngoc Duy· Apr 10, 2012, 22:51 UTC · re: Jonathan Nieder · lore

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

2012/4/11 Jonathan Nieder <jrnieder@gmail.com>:
Show 18 quoted lines
> (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

← back to recent threads