On 01/17, Jeff King wrote:
> 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.
> > + 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.
> > + 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.
> > + 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.
>
> > + 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
--
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