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.