Re: [PATCH v2 3/9] ref_transaction_update(): die on disallowed flags
- Date: Tue, 7 Nov 2017 21:53:26 +0100
- From: Martin Ågren <martin.agren@xxxxxxxxx>
- Subject: Re: [PATCH v2 3/9] ref_transaction_update(): die on disallowed flags
On 5 November 2017 at 09:42, Michael Haggerty <mhagger@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> 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  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
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.