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

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Jan 17, 2016, 17:27 UTC
Message-ID: <20160117172706.GD7100@hank>
In-Reply-To: <20160117144409.GA15519@sigill.intra.peff.net>

On 01/17, Jeff King wrote:

Show 26 quoted lines

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

I was looking for something like this, but totally overlooked it when going through the docs. Thanks, will change.

Show 5 quoted lines

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

Indeed, I wasn't really happy about it, but couldn't come up with anything better. Your version sounds much better, will fix.

Show 7 quoted lines

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

Makes sense, I'll declare the exec option as hidden, as that's the one that's not documented anywhere else either.

Show 6 quoted lines

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

Makes sense, will change in the re-roll.

Show 9 quoted lines

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

Thanks for the review!

>
> -Peff

-- Thomas