On Mon, Oct 30, 2023 at 02:37:14PM -0400, Taylor Blau wrote:
> On Thu, Oct 26, 2023 at 11:56:37AM +0200, Patrick Steinhardt wrote:
> > It's not immediately obvious options which options are applicable to
> > what subcommand in git-show-ref(1) because all options exist as global
> > state. This can easily cause confusion for the reader.
> >
> > Refactor options for the `--exclude-existing` subcommand to be contained
> > in a separate structure. This structure is stored on the stack and
> > passed down as required. Consequently, it clearly delimits the scope of
> > those options and requires the reader to worry less about global state.
> >
> > Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
>
> All makes sense, but...
>
> > @@ -19,8 +19,7 @@ static const char * const show_ref_usage[] = {
> > };
> >
> > static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
> > - quiet, hash_only, abbrev, exclude_arg;
> > -static const char *exclude_existing_arg;
> > + quiet, hash_only, abbrev;
> >
> > static void show_one(const char *refname, const struct object_id *oid)
> > {
> > @@ -95,6 +94,11 @@ static int add_existing(const char *refname,
> > return 0;
> > }
> >
> > +struct exclude_existing_options {
> > + int enabled;
>
> ...do we need an .enabled here? I think checking whether or not .pattern
> is NULL is sufficient, but perhaps there is another use of .enabled
> later on in the series...
This is the second time that this question comes up, which is likely not
all that surprising. Quoting my first reply:
On Wed, Oct 25, 2023 at 01:50:44PM +0200, Patrick Steinhardt wrote:
> Yeah, we do. It's perfectly valid to pass `--exclude-existing` without
> the optional pattern argument. We still want to use this mode in that
> case, but don't populate the pattern.
>
> An alternative would be to assign something like a sentinel value in
> here. But I'd think that it's clearer to instead have an explicit
> separate field for this.
Anyway, the fact that this question comes up again indicates that I need
to comment this better.
Patrick
Attachment:
signature.asc
Description: PGP signature
