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

[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().

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:

	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?

Thanks.