Re: [PATCH v2 5/5] ls-remote: add support for showing symrefs

[Thomas Gummerer <t.gummerer@gmail.com>]

On 01/18, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > +--symrefs::
> > +	Show the symrefs in addition to the other refs.
> > +
>
> Micronit.  The above sounds as if the output would not talk about
> HEAD at all unless this option is given, which is not what you meant
> here.  "In addition to the object pointed by it, show the underlying
> ref pointed by it when showing a symbolic ref" or something like that,
> perhaps?

Thanks, that sounds better, will change.

> > @@ -58,6 +60,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
> >  			 N_("take url.<base>.insteadOf into account")),
> >  		OPT_SET_INT(0, "exit-code", &status,
> >  			    N_("exit with exit code 2 if no matching refs are found"), 2),
> > +		OPT_BOOL(0, "symrefs", &symrefs, N_("show symrefs")),
>
> Likewise.
>
> By the way, unlike "--heads" and "--tags", which is to choose a
> group of refs to be shown, this controls one specific aspect,
> i.e. target of symbolic ref, of each thing that is being shown,
> without affecting the set of refs that the command talks about.
>
> Shouldn't this option be "--symref" (and variable named as
> show_symref_target or something more explicit)?

The change of the variable name definitely makes sense.  My thoughts
were that potentially multiple symrefs are shown, so the plural would
make sense, but after reading your explanation I think I agree with
using --symref as the name.  Thanks.

> Other than these nits, the code itself looks good.
>
> Thanks.
>

--
Thomas