On Fri, Aug 16, 2019 at 12:52 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Elijah Newren <newren@xxxxxxxxx> writes:
>
> > There are lots of options that callers can set, yet most have a limited
> > range of valid values, some options are meant for output (e.g.
> > opt->obuf, which is expected to start empty), and callers are expected
> > to not set opt->priv. Add several sanity checks to ensure callers
> > provide sane values.
> > ...
>
> The change to the struct does not seem to have much with the above
> rationale.
If the only possible values are 0 and 1, I can either add assertions
to check that at run time, or make the compiler check it for us by
confining its value to a single bit. A compile-time check seems more
robust...
> > diff --git a/merge-recursive.h b/merge-recursive.h
> > index 978847e672..d201ee80fb 100644
> > --- a/merge-recursive.h
> > +++ b/merge-recursive.h
> > @@ -27,7 +27,7 @@ struct merge_options {
> > } detect_directory_renames;
> > int rename_limit;
> > int rename_score;
> > - int show_rename_progress;
> > + int show_rename_progress : 1;
>
> And a one-bit wide bitfield that is signed is always an iffy idea.
...assuming of course I don't mess it up like this, not noticing that
it's an int that needs to be changed to an unsigned. I'll fix this
up.
But the fact that you flagged the struct change -- would you prefer
some commit message explanation of how it's related, or was it more
the case that you felt it was a different kind of change and wanted it
split out into a separate patch? I'm suspecting the former but am not
quite sure.
