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

From: SZEDER Gábor <szeder@ira.uka.de>
Date: Aug 24, 2015, 16:14 UTC
Message-ID: <20150824181417.Horde.2DAlveA2ZaXbdApUl8wLOQ8@webmail.informatik.kit.edu>
In-Reply-To: <xmqqoai07gen.fsf@gitster.dls.corp.google.com>

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

Show 15 quoted lines

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

Show 29 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?

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