Re: [PATCH] describe --contains: default to HEAD when no commit-ish is given

[Junio C Hamano <gitster@pobox.com>]

SZEDER Gábor <szeder@ira.uka.de> writes:

> '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.

> 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.

> +			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);
>  	}

>  
> 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' '