On 11/20/2018 4:26 AM, Ævar Arnfjörð Bjarmason wrote:
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".
I agree that we should have different messages for mandatory and optional extensions. I don't think we should try and educate the end user on the implementation detail that git makes lower cases mandatory and upper case optional (ie drop the 'All-lowercase..." part). They will never see the lower vs upper case difference and can't do anything about it anyway.
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.
Agree - not all are guaranteed to be perf related.
+ "\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.
I like this. I personally never knew you could turn of the "spent xxx seconds finding untracked files" advice until I worked on this patch series. This would help make that feature more discoverable.
