Output from "rev-parse --resolve-git-dir" changed: two lines (prints original argument)

Output from "rev-parse --resolve-git-dir" changed: two lines (prints original argument)

[Daniel Hahler]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

I have noticed that the output from "git rev-parse --resolve-git-dir" changed.

While it used to only print the resolved git dir, it now prints the argument passed to it itself:

    % git rev-parse --resolve-git-dir .git
    /path/to/root/.git/modules/vim/bundle/reporoot
    .git

This is with Git version 1.8.5.3, but also happens with master.

You can use "--flags" to avoid this ("git rev-parse --flags --resolve-git-dir .git"), but it is unclear from the documentation if that's the intended beahviour.

It seems like the "--resolve-git-dir" subcommand needs to consume the argument passed to it.


Regards,
Daniel.

-- 
http://daniel.hahler.de/


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 255 bytes --]

[PATCH] rev-parse: fix --resolve-git-dir argument handling

[John Keeping]

There are two problems here:

1) If no argument is provided, then the command segfaults
2) The argument is not consumed, so there will be excess output

Fix both of these in one go by restructuring the handler for this
option.

Reported-by: Daniel Hahler <genml+git-2014@thequod.de>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
 builtin/rev-parse.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index aaeb611..645cc4a 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -738,9 +738,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--resolve-git-dir")) {
-				const char *gitdir = resolve_gitdir(argv[i+1]);
+				const char *gitdir;
+				if (++i >= argc)
+					die("--resolve-git-dir requires an argument");
+				gitdir = resolve_gitdir(argv[i]);
 				if (!gitdir)
-					die("not a gitdir '%s'", argv[i+1]);
+					die("not a gitdir '%s'", argv[i]);
 				puts(gitdir);
 				continue;
 			}
-- 
1.9.0.6.g037df60.dirty

Re: [PATCH] rev-parse: fix --resolve-git-dir argument handling

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> There are two problems here:
>
> 1) If no argument is provided, then the command segfaults
> 2) The argument is not consumed, so there will be excess output
>
> Fix both of these in one go by restructuring the handler for this
> option.
>
> Reported-by: Daniel Hahler <genml+git-2014@thequod.de>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---

Looks sensible; thanks.

>  builtin/rev-parse.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index aaeb611..645cc4a 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -738,9 +738,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  				continue;
>  			}
>  			if (!strcmp(arg, "--resolve-git-dir")) {
> -				const char *gitdir = resolve_gitdir(argv[i+1]);
> +				const char *gitdir;
> +				if (++i >= argc)
> +					die("--resolve-git-dir requires an argument");
> +				gitdir = resolve_gitdir(argv[i]);
>  				if (!gitdir)
> -					die("not a gitdir '%s'", argv[i+1]);
> +					die("not a gitdir '%s'", argv[i]);
>  				puts(gitdir);
>  				continue;
>  			}

Re: [PATCH] rev-parse: fix --resolve-git-dir argument handling

[Junio C Hamano]

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

> John Keeping <john@keeping.me.uk> writes:
>
>> There are two problems here:
>>
>> 1) If no argument is provided, then the command segfaults
>> 2) The argument is not consumed, so there will be excess output
>>
>> Fix both of these in one go by restructuring the handler for this
>> option.
>>
>> Reported-by: Daniel Hahler <genml+git-2014@thequod.de>
>> Signed-off-by: John Keeping <john@keeping.me.uk>
>> ---
>
> Looks sensible; thanks.

Ehh, I spoke too fast. Don't we already have this queued as a43219f2
(rev-parse: check i before using argv[i] against argc, 2014-01-28)?

Re: [PATCH] rev-parse: fix --resolve-git-dir argument handling

[John Keeping]

On Tue, Feb 18, 2014 at 04:25:37PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > John Keeping <john@keeping.me.uk> writes:
> >
> >> There are two problems here:
> >>
> >> 1) If no argument is provided, then the command segfaults
> >> 2) The argument is not consumed, so there will be excess output
> >>
> >> Fix both of these in one go by restructuring the handler for this
> >> option.
> >>
> >> Reported-by: Daniel Hahler <genml+git-2014@thequod.de>
> >> Signed-off-by: John Keeping <john@keeping.me.uk>
> >> ---
> >
> > Looks sensible; thanks.
> 
> Ehh, I spoke too fast. Don't we already have this queued as a43219f2
> (rev-parse: check i before using argv[i] against argc, 2014-01-28)?

Yes, and it catches more cases than mine.  I only checked against master
and had missed that when it went past on the list.