Quoting Junio C Hamano <[email protected]>:
@@ -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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
