Thread (1 message) 1 message, 1 author, 2016-06-15

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:06:17

SZEDER Gábor [off-list ref] writes:
quoted
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)'.
Another reason to favor the way the code is illustrated for
educational purposes above is it is easier to see exceptional case
first, i.e. "if we have nothing then we do this special thing, but
otherwise we do the normal thing".

But that is a much weaker preference than the preference to "fatter
else"; I could go either way.
v2 comes in a minute.
Thanks.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help