On Wed, Mar 4, 2020 at 3:16 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > "Heba Waly via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
> > + /*
> > + 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
