Re: [PATCH v7 3/4] advice: revamp advise API

From: Heba Waly <heba.waly@gmail.com>
Date: Mar 4, 2020, 03:22 UTC
Message-ID: <CACg5j26fiJQJZ1CZy2AHWEiPmzJ+XsiA+Zfsp4Mc6MgP9i=Crg@mail.gmail.com>
In-Reply-To: <xmqqlfoha5ia.fsf@gitster-ct.c.googlers.com>

On Wed, Mar 4, 2020 at 3:16 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 27 quoted lines

>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >> +static struct {
> >> +    const char *key;
> >> +    int enabled;
> >> +} advice_setting[] = {
> >> +    [ADVICE_ADD_EMBEDDED_REPO]                      = { "addEmbeddedRepo", 1 },
> >
> > It would be nicer to future developers to flip the polarity, as we
> > do not have to write 1 all over the place, especially if we plan to
> > extend the structure over time and to use designated initializers
> > for only certain fields...
>
> Just to avoid needless churn, I think this does not matter in the
> longer term, so .enabled is OK as-is.  The reason I say so is
> because, even though renaming to .disabled to allow initializers to
> default it to 0 is nicer for those who write the initializers
> manually, and it especially is true when we have more fields in the
> struct (we may add descriptive text so that we can issue an on-line
> help, for example), but I expect that would happen much later than
> we start generating these parts of the source code in two places
> (the initializer for advice_setting[] and the advice_type enum) from
> a single source by mechanical process.  And the auto-generation will
> eliminate the burden of writing 1 manually.

Agree.

> Turning this into "break;" would make it similar to the loop before
> this one, and will allow other people to add more code after this
> loop later.

The loop with break is redundant and will be removed in the next patch, but the rest of the loops in this function end with return which I think makes more sense assuming any new code/loop that will need to be added in the future is expected to handle a different configuration group.

Show 9 quoted lines

> > +     /*
> > +       Any advice type can be used for testing, but NESTED_TAG was selected
> > +       here and in t0018 where this command is being executed.
> > +      */
>
> Style (will fix up locally).
>
> Thanks.  I think this is reasonable with or without the suggested
> fixes.

Great, thanks.

Heba