Re: [PATCH 1/2] format-patch: make format.noprefix a boolean

[Junio C Hamano <gitster@pobox.com>]

kristofferhaugsbakk@fastmail.com writes:

> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> The config `format.noprefix` was added in 8d5213de (format-patch: add
> format.noprefix option, 2023-03-09) to support no-prefix on paths.
> That was immediately after making git-format-patch(1) not respect
> `diff.noprefix`.[1]
>
> The intent was to mirror `diff.noprefix`. But this config was
> unintentionally[2] implemented by enabling no-prefix if any kind of
> value is set.
>
> † 1: c169af8f (format-patch: do not respect diff.noprefix, 2023-03-09)
> † 2: https://lore.kernel.org/all/20260211073553.GA1867915@coredump.intra.peff.net/
>
> Let’s indeed mirror `diff.noprefix` by treating it as a boolean.
>
> This is a breaking change. And as far as breaking changes go it is
> pretty benign:
>
> • The documentation claims that this config is equivalent to
>   `diff.noprefix`; this is just a bug fix if the documentation is
>   what defines the application interface
> • Only users with non-boolean values will run into problems when we
>   try to parse it as a boolean. But what would (1) make them suspect
>   they could do that in the first place, and (2) have motivated them to
>   do it?
> • Users who have set this to `false` and expect that to mean *enable
>   format.noprefix* (current behavior) will now have the opposite
>   experience. Which is not a reasonable setup.
>
> Let’s only offer a breaking change fig leaf by hinting about the
> previous behavior before dying.

One case that is often problematic is what happens to those who use
the same set of configuration variables with different versions of
Git, before and after such behaviour change.  But I do not think
this is such a bad thing.  The only reason why they had this
variable set (to any value, or to a value-less true) with existing
versions of Git is because they wanted to omit the prefixes, so when
a new version of Git dies with "Heh, 'nothanks' is not a valid
boolean value", they can edit the configuration variable to "1".

And from that point of view, I think the hint given together with
the "bad boolean" error can and should be phrased a bit more
strongly, i.e.,

> +		format_no_prefix = git_parse_maybe_bool(value);
> +		if (format_no_prefix < 0) {
> +			int status = die_message(
> +				_("bad boolean config value '%s' for '%s'"),
> +				value, var);
> +			fprintf(stderr,
> +				_("hint: '%s' used to accept any value but "
> +				  "now only\n"
> +				  "hint: accepts boolean values, like '%s'\n"),
> +				var, "diff.noprefix");

The target audience of this (hint) is those who have set this
variable to a non-boolean strring from the existing version of Git,
and the only thing they meant to express was "I do not want any
prefix", so "we used to accept any value as true, but now accepts
only valid boolean values", perhaps?  That would nudge those who
wrote "[format] noprefix = NoThanks" to rewrite it correctly to
"true" or "1", and not "no".

This is a related tangent, but shouldn't this use advise() without
configuration?  There is no need to allocate an advice_type and use
advise_if_enabled(), because correcting a malformed configuration is
an action enough to squelch the message.

> +			exit(status);
> +		}
>  		return 0;
>  	}