Re: [PATCH v2 3/9] ref_transaction_update(): die on disallowed flags

[Thomas Gummerer <t.gummerer@gmail.com>]

On 11/07, Martin Ågren wrote:
> On 5 November 2017 at 09:42, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> > Callers shouldn't be passing disallowed flags into
> > `ref_transaction_update()`. So instead of masking them off, treat it
> > as a bug if any are set.
> >
> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> > ---
> >  refs.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/refs.c b/refs.c
> > index 62a7621025..7c1e206e08 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -940,7 +940,8 @@ int ref_transaction_update(struct ref_transaction *transaction,
> >                 return -1;
> >         }
> >
> > -       flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;
> > +       if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
> > +               BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);
> >
> >         flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
> 
> The masking out is for sanity, but also partly to squelch a
> compiler-warning. Thomas reported [1] that dieing does not make the
> warning go away, but that masking out does. Of course, avoiding warnings
> is not the ultimate goal, and -Wnonnull is not part of DEVELOPER_CFLAGS.
> Thomas reluctantly suggested that one could do your check and then do
> the masking...

Thanks for doing the digging and cc'ing me here.

Interestingly enough  my compiler no longer complains about this.
Looking at what changed, gcc got upgraded to 7.2.0 (from 7.1.1) in
between then and now, but I can still reproduce this if I base this
patch on top of the old commit.

So bisecting leads me to 89f3bbdd3b ("refs: update ref transactions to
use struct object_id", 2017-10-15).  It's too late for me to dig in to
what in this made the compiler warning go away, but it seems
reasonable enough.

With that said I think the patch we have here is definitely an
improvement, I would have prefered to do it this way in the first
place, but the compiler was standing in my way :)

Thanks Michael for cleaning this up!

> Maybe it would be worth a note in the commit message. But blaming these
> lines quickly leads to c788c54cd (refs: strip out not allowed flags from
> ref_transaction_update, 2017-09-12), which describes this already. OTOH,
> since the warning does not hit these lines, but a bit below, maybe it's
> even worth a comment in the code.
> 
> I'm not saying we should sprinkle comments for each warning we hit...
> Anyway, those were the thoughts than ran through my mind.
> 
> [1] https://public-inbox.org/git/20170924204541.GA2853@hank/