Re: [PATCH 3/4] ls-remote: use parse-options api

From: Jeff King <peff@peff.net>
Date: Jan 17, 2016, 14:44 UTC
Message-ID: <20160117144409.GA15519@sigill.intra.peff.net>
In-Reply-To: <1453028643-13978-4-git-send-email-t.gummerer@gmail.com>

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.

Show 7 quoted lines

> +	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