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

On Sat, Aug 15, 2026, at 04:17, Junio C Hamano wrote:

kristofferhaugsbakk@fastmail.com writes:

quoted

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

quoted

+		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?