Re: [PATCH 4/4] format-rev: learn --abbrev, --color, and --date

On Sat, Aug 15, 2026, at 04:17, Junio C Hamano wrote:
> [email protected] writes:
>
>> +static int date_cb(const struct option *option,
>> +		   const char *arg,
>> +		   int unset)
>> +{
>> +	struct rev_info *data = option->value;
>> +	parse_date_format(arg, &data->date_mode);
>> +	data->date_mode_explicit = 1;
>> +	BUG_ON_OPT_NEG(unset);
>> +	return 0;
>> +}
>
> This BUG_ON_OPT_NEG(unset) is a bit curious and confusing to me.  If
> the caller could pass unset==1 (e.g., "--no-date"), option->value
> would be NULL, and we would already have dereferenced data->date_mode
> when preparing to call parse_date_format().

Well spotted.

>
> On the other hand, ...
>
>> +		OPT_CALLBACK_F(0, "date", &data.rev, N_("date"),
>> +			       N_("date format"),
>> +			       PARSE_OPT_NONEG, date_cb),
>
> ... because we mark the option entry with PARSE_OPT_NONEG,
> "--no-date" would not cause date_cb() to be called with unset==1.
>
> I guess, from existing uses of BUG_ON_OPT_NEG() elsewhere (like
> apply.c), that the intention is to notice when this callback
> function is broken by future changes, i.e., somebody careless makes
> the calling parse_options(), or an additional side caller that calls
> this callback directly, pass unset==1 and option->value==NULL
> combinations.  But then the assertion should come before the first
> potentially problematic use, i.e., in this order:

This is totally a monkey see and moneky do situation. Mirroring
parse-options flags as `BUG` statements. Down to the outright wrong
assertion/BUG placement. So this needs to be changed

I have these same statements on the existing callback, for `--null`:


    static int format_nul_cb(const struct option *option,
                             const char *arg,
                             int unset)
    {
            struct format_rev_data *data = option->value;
            data->nul_input = 1;
            data->nul_output = 1;
            BUG_ON_OPT_NEG(unset);
            BUG_ON_OPT_ARG(arg);
            return 0;
    }

But this does not have the `NULL` deref. problem since we just
unconditionally set two boolean values. Still, for readability it’s
better for these two statements to go at the start. Since they are
preconditions. I will add this as a patch/commit to the series.

>
> 	struct rev_info *data = option->value;
>
> 	BUG_ON_OPT_NEG(unset);
> 	parse_date_format(arg, &data->date_mode);
> 	data->date_mode_explicit = 1;
> 	return 0;
>
> or the assertion will not trigger before the code segfaults, no?