On Mon, Nov 07, 2022 at 02:34:45PM +0100, Ævar Arnfjörð Bjarmason wrote:
> On Mon, Nov 07 2022, Patrick Steinhardt wrote:
[snip]
> > +	if (strcmp(section, "transfer") && strcmp(section, "receive") &&
> > +	    strcmp(section, "uploadpack"))
> > +		die(_("unsupported section for hidden refs: %s"), section);
> > +
> > +	if (exclusions->hidden_refs.nr)
> > +		die(_("--exclude-hidden= passed more than once"));
> 
> We usually just ignore the first of --foo=bar --foo=baz and take "baz"
> in our CLI use. Is it better to die here than just clear the previous
> one & continue?

It's something I was torn on. I ultimately chose to die though because
of the difference between `--exclude` and `--exclude-hidden`: the former
one will happily add additional patterns, all of which will ultimately
be ignored. So as a user you might rightfully expect that the latter
will work the same: if both `--exclude-hidden=uploadpack` and
`--exclude-hidden=receive` are specified, you might want to have both be
ignored.

To me it wasn't quite clear how to support multiple instances of
`transfer.hideRefs` though as there is also the concept of un-hiding
already-hidden refs. So I wanted to avoid going into this discussion to
make the patch series a little bit smaller.

By dying instead of silently overriding the previous argument we retain
the ability to iterate on this at a later point though to implement
above behaviour, if the usecase ever arises.

Patrick