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

["SZEDER Gábor" <szeder@ira.uka.de>]

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

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

Exactly.  parse-opts removes all --options from argv as it processes
them, barfs at --unknown-options, so all what remains must be treated
as a commit-ish.  And if nothing is left, well, then there was none
given.

>> +			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?

Indeed, I didn't notice argv_array_pushv() being added, log tells me
it happened quite recently.  I suppose with both branches becoming a
one-liner the order of them can remain what it was in the patch,
this sparing the negation from 'if (!argc)'.

v2 comes in a minute.


Gábor