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