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

["Martin Ågren" <martin.agren@gmail.com>]

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...

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/