Re: [PATCH v3 4/6] revision: add new parameter to exclude hidden refs

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 8, 2022, 09:22 UTC
Message-ID: <Y2ofxE5qY3gnZLpe@ncase>
In-Reply-To: <221107.86pmdyx4me.gmgdl@evledraar.gmail.com>

On Mon, Nov 07, 2022 at 02:34:45PM +0100, Ævar Arnfjörð Bjarmason wrote:

> On Mon, Nov 07 2022, Patrick Steinhardt wrote:

[snip]

Show 10 quoted lines

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