# [PATCH] rev-parse --show-prefix: add in trailing newline 9 messages from 2012-04-09 to 2012-04-10. Participants: Ross Lagerwall, Junio C Hamano, Jonathan Nieder, Andreas Schwab, Nguyen Thai Ngoc Duy. Thread: https://gitlist.dev/t/30188 ## Ross Lagerwall, 2012-04-09 13:27 Subject: [PATCH] rev-parse --show-prefix: add in trailing newline Message-ID: <1333978076-29968-1-git-send-email-rosslagerwall@gmail.com> URL: https://gitlist.dev/e/1333978076-29968-1-git-send-email-rosslagerwall%40gmail.com ``` 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 --- 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 ``` ## Junio C Hamano, 2012-04-09 19:07 Subject: Re: [PATCH] rev-parse --show-prefix: add in trailing newline Message-ID: <7v1unwwwov.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1unwwwov.fsf%40alter.siamese.dyndns.org In-Reply-To: <1333978076-29968-1-git-send-email-rosslagerwall@gmail.com> ``` Ross Lagerwall 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" ``` ## Ross Lagerwall, 2012-04-10 05:30 Subject: Re: [PATCH] rev-parse --show-prefix: add in trailing newline Message-ID: <4F83C589.20609@gmail.com> URL: https://gitlist.dev/e/4F83C589.20609%40gmail.com In-Reply-To: <7v1unwwwov.fsf@alter.siamese.dyndns.org> ``` 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 ``` ## Jonathan Nieder, 2012-04-10 14:04 Subject: Re: [PATCH] rev-parse --show-prefix: add in trailing newline Message-ID: <20120410140421.GC2444@burratino> URL: https://gitlist.dev/e/20120410140421.GC2444%40burratino In-Reply-To: <7v1unwwwov.fsf@alter.siamese.dyndns.org> ``` 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. ``` ## Andreas Schwab, 2012-04-10 14:41 Subject: Re: [PATCH] rev-parse --show-prefix: add in trailing newline Message-ID: URL: https://gitlist.dev/e/m2vcl7ir86.fsf%40igel.home In-Reply-To: <7v1unwwwov.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano 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." ``` ## Junio C Hamano, 2012-04-10 16:14 Subject: Re: [PATCH] rev-parse --show-prefix: add in trailing newline Message-ID: <7v8vi3sgx8.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8vi3sgx8.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Andreas Schwab writes: > This is already broken because of the extra slash. Yeah, you are right. ``` ## Jonathan Nieder, 2012-04-10 17:36 Subject: Re: [PATCH] rev-parse --show-prefix: add in trailing newline Message-ID: <20120410173616.GA4300@burratino> URL: https://gitlist.dev/e/20120410173616.GA4300%40burratino In-Reply-To: <1333978076-29968-1-git-send-email-rosslagerwall@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? Thanks, Jonathan ``` ## Junio C Hamano, 2012-04-10 17:56 Subject: Re: [PATCH] rev-parse --show-prefix: add in trailing newline Message-ID: <7vty0rpj2i.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vty0rpj2i.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120410173616.GA4300@burratino> ``` Jonathan Nieder 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 ;-) ``` ## Nguyen Thai Ngoc Duy, 2012-04-10 22:51 Subject: Re: [PATCH] rev-parse --show-prefix: add in trailing newline Message-ID: URL: https://gitlist.dev/e/CACsJy8Dc9xi%3Dz7x4OfeAWHSeuUOFFVFf93GFOfauVB7c%3D_cZ1Q%40mail.gmail.com In-Reply-To: <20120410173616.GA4300@burratino> ``` 2012/4/11 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? 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 ```