On Sun, Jan 17, 2016 at 12:04:01PM +0100, Thomas Gummerer wrote:
> Currently ls-remote uses a hand rolled parser for the its command line
> arguments. Use the parse-options api instead of the hand rolled parser
> to simplify the code and make it easier to add new arguments. In
> addition this improves the help message.
Sounds like a good idea.
> + int tags = 0, heads = 0, refs = 0;
> [...]
> + OPT_SET_INT('t', "tags", &tags, N_("limit to tags"), REF_TAGS),
> + OPT_SET_INT('h', "heads", &heads, N_("limit to heads"), REF_HEADS),
> + OPT_SET_INT(0, "refs", &refs, N_("no magic fake tag refs"), REF_NORMAL),
> [...]
> + flags = tags | heads | refs;
Is there any reason these can't be:
OPT_BIT('t', "tags", &flags, N_("limit to tags"), REF_TAGS),
OPT_BIT('h', "heads", &flags, N_("limit to heads"), REF_HEADS),
OPT_BIT(0, "refs", &flags, N_("no magic fake tag refs"), REF_NORMAL),
to make their interaction more obvious? I wondered if there was
anything tricky going on (like some of the bits for each option
overlapping), but I didn't see anything.
> + OPT_SET_INT(0, "refs", &refs, N_("no magic fake tag refs"), REF_NORMAL),
I imagine you took the help string from the comment in check_ref. We can
probably come up with something more descriptive for the user-facing
string. :) How about "do not show peeled tags"?
> + OPT_STRING(0, "upload-pack", &uploadpack, N_("exec"),
> + N_("path of git-upload-pack on the remote host")),
> + OPT_STRING(0, "exec", &uploadpack, N_("exec"),
> + N_("path of git-upload-pack on the remote host")),
Since these are redundant with each other, should we declare one
"hidden" to not appear in "-h" output?
> + OPT_SET_INT(0, "get-url", &get_url,
> + N_("take url.<base>.insteadOf into account"), 1),
Should this one be OPT_BOOL? I think "--no-get-url" works either way (it
resets the variable to 0), but OPT_BOOL communicates the intent more
clearly, I think.
> + OPT_SET_INT(0, "exit-code", &status,
> + N_("exit with exit code 2 if no matching refs are found"), 2),
This one can't be OPT_BOOL, obviously. What happens with
"--no-exit-code"? We'll set it back to "0", which I think is the right
thing to do. Good.
The rest of the patch looked good to me.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html