Re: [PATCH 5/5] index: offer advice for unknown index extensions

[Ævar Arnfjörð Bjarmason <avarab@gmail.com>]

On Tue, Nov 20 2018, Jonathan Nieder wrote:

Just commenting here on the end-state of this since it's easier than
each patch at a time:

First, do we still need to be doing %.4s instead of just %s? It would be
easier for translators / to understand what's going on if it were just
%s. I.e. "this is the extension name" v.s. "this is the first 4 bytes of
whatever it is...".

>  			return error("index uses %.4s extension, which we do not understand",
>  				     ext);

Missing _(). Not the fault of this series, but something to fix while
we're at it.

Also not the fault of this series, the "is this upper case" test is
unportable, but this is probably the tip of the iceberg for git not
working on EBCDIC systems.

This message should say something like "Index uses the mandatory %s
extension" to clarify and distinguish it from the below. We don't
understand the upper-case one either, but the important distinction is
that one is mandatory, and the other can be dropped. The two messages
should make this clear.

Also, having advice() for that case is even more valuable since we have
a hard error at this point. So something like:

    "This is likely due to the index having been written by a future
    version of Git. All-lowercase index extensions are mandatory, as
    opposed to optional all-uppercase ones which we'll drop with a
    warning if we see them".

>  		trace_printf("ignoring %.4s extension\n", ext);
> +		if (advice_unknown_index_extension) {
> +			warning(_("ignoring optional %.4s index extension"), ext);

Should start with upper-case. Good that it says "optional".

> +			advise(_("This is likely due to the file having been written by a newer\n"
> +				 "version of Git than is reading it. You can upgrade Git to\n"
> +				 "take advantage of performance improvements from the updated\n"
> +				 "file format.\n"

Let's not promise performance improvements with this extension in a
future version. We have no idea what the extension is, yeah right now
it's going to be true for the extension that prompted this patch series,
but may not be in the future. So just something like this for the last
sentence:

    You can try upgrading Git to use this new index format.

> +				 "\n"
> +				 "Run \"%s\"\n"
> +				 "to suppress this message."),
> +			       "git config advice.unknownIndexExtension false");

Somewhat of an aside, but if I grep:

    git grep -C10 'git config advice\..*false' -- '*.[ch]'

There's a few existing examples of this, but the majority of advice()
messages don't say in the message how you can turn these off. Do we
think this a message users would especially like to turn off? I have the
opposite impression, it's a one-off in most cases, although not in the
case where an editor has an embedded git.

I think it would make sense to add this sort of thing to the advice()
API, i.e.:

    advice_with_config_hint(_("<message>"), "unknownIndexExtension");

Which would then know how to consistently print this advice about how to
turn off the warning.