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

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 7, 2017, 20:53 UTC
Message-ID: <CAN0heSpOpfCmf8k8kZz1L-1qb-5mDHMLbQYaLPKE=twOhgyi8A@mail.gmail.com>
In-Reply-To: <10901328af1e6e04e8c7c7b39977ec85f6c58142.1509870243.git.mhagger@alum.mit.edu>

On 5 November 2017 at 09:42, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Show 22 quoted lines

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