Re: [PATCH] describe --contains: default to HEAD when no commit-ish is given
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 21, 2015, 16:30 UTC
- Message-ID
- <xmqqoai07gen.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1440168632-15412-1-git-send-email-szeder@ira.uka.de>
SZEDER Gábor <szeder@ira.uka.de> writes:
Show 6 quoted lines
> 'git describe --contains' doesn't default to HEAD when no commit is > given, and it doesn't produce any output, not even an error: > > ~/src/git ((v2.5.0))$ ./git describe --contains > ~/src/git ((v2.5.0))$ ./git describe --contains HEAD > v2.5.0^0
Good spotting. I think defaulting to HEAD is a good move.
Show 13 quoted lines
> diff --git a/builtin/describe.c b/builtin/describe.c
> index ce36032b1f..0e31ac5cb9 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -443,10 +443,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
> if (pattern)
> argv_array_pushf(&args, "--refs=refs/tags/%s", pattern);
> }
> - while (*argv) {
> - argv_array_push(&args, *argv);
> - argv++;
> - }
> + if (argc)"What would this code do to 'describe --all --contains'?" was my knee-jerk reaction, but the options are all parsed by this code and nothing is delegated to name-rev, so 'if (!argc)' here is truly the lack of any revisions to be described, which is good.
Show 6 quoted lines
> + while (*argv) {
> + argv_array_push(&args, *argv);
> + argv++;
> + }
> + else
> + argv_array_push(&args, "HEAD");By the way, I usually prefer a fatter 'else' clause when everything else is equal, i.e.
if (!argc)
argv_array_push(&args, "HEAD"); /* default to HEAD */
else {
while (*argv) {
...
}
}because it is easy to miss tiny else-clause while reading code, but it is harder to miss tiny then-clause. In this case, however, the while loop can be replaced with argv_array_pushv() these days, so perhaps
if (!argc)
argv_array_push(&args, "HEAD"); /* default to HEAD ... */
else
argv_array_pushv(&args, argv); /* or relay what we got */or something?
> return cmd_name_rev(args.argc, args.argv, prefix); > }
Show 19 quoted lines
> > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh > index 57d50156bb..bf52a0a1cc 100755 > --- a/t/t6120-describe.sh > +++ b/t/t6120-describe.sh > @@ -115,6 +115,14 @@ check_describe e-3-* --first-parent --tags > > check_describe $(git rev-parse --short HEAD) --exact-match --always HEAD > > +test_expect_success 'describe --contains defaults to HEAD without commit-ish' ' > + echo "A^0" >expect && > + git checkout A && > + test_when_finished "git checkout -" && > + git describe --contains >actual && > + test_cmp expect actual > +' > : >err.expect > check_describe A --all A^0 > test_expect_success 'no warning was displayed for A' '