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

On 01/17, Jeff King wrote:

On Sun, Jan 17, 2016 at 12:04:01PM +0100, Thomas Gummerer wrote:

quoted

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.

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+		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