| Message ID | 20181120061544.GF144753@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Avoid confusing messages from new index extensions | expand |
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.
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.
Ben Peart <peartben@gmail.com> writes: >> 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. I agree that the "warn and continue" message should say "optional" (meaning: safe to ignore but you would want to take note) while "cannot continue" message should say something different. I do not mind a more verbose error message when we saw unknown but required extension, but unlike the "warn and continue" case, the program will stop and die with such an error right there, so I am not sure if it is worth allowing to tone it down by putting some part of the verbosity behind the advise() mechanism. >>> trace_printf("ignoring %.4s extension\n", ext); >>> + if (advice_unknown_index_extension) { >>> + warning(_("ignoring optional %.4s index extension"), ext); So from that point of view, the distinction between this message and this one >>> return error("index uses %.4s extension, which we do not understand", >>> ext); is halfway there. The message needs to anticipate and answer an end-user reaction: "we do not understand" so what? I am still puzzled by the insistence of 3/5 and this step that wants to kill the coalmine canary. But I am even more puzzled by the first two steps that want to disable the two optional extensions. What's so different this time with the new optional extensions? The other early optional extensions like cache-tree or resolve-undo were added unconditionally and by definition appeared much earlier in git-core than any other Git reimplementations. verbody who recorded the fact that s/he resolved merge conflicts got REUC, and we would have given warning when an older Git did not understand these extensions [*1*]. We knudged users to more modern Git by preparing the old Gits to warn when there are unknown extensions, either by upgrading their Git themselves, or by bugging their toolsmiths. Nobody complained to propose to rip the messages like this round. This series has a strong smell of pushing back by the toolsmiths who refuse to promptly upgrade to help their users, and that is why I do not feel entirely happy with this series. [Footnote] *1* A Git that did not understand TREE would have been silent, as it was the first extension and that was the first time we became aware of the need to warn unknown extensions.
Hi, Junio C Hamano wrote: > I am still puzzled by the insistence of 3/5 and this step that wants > to kill the coalmine canary. But I am even more puzzled by the > first two steps that want to disable the two optional extensions. > > What's so different this time with the new optional extensions? > > The other early optional extensions like cache-tree or resolve-undo > were added unconditionally and by definition appeared much earlier > in git-core than any other Git reimplementations. Everbody who > recorded the fact that s/he resolved merge conflicts got REUC, and > we would have given warning when an older Git did not understand > these extensions [*1*]. We knudged users to more modern Git by > preparing the old Gits to warn when there are unknown extensions, > either by upgrading their Git themselves, or by bugging their > toolsmiths. Nobody complained to propose to rip the messages like > this round. This series has a strong smell of pushing back by the > toolsmiths who refuse to promptly upgrade to help their users, and > that is why I do not feel entirely happy with this series. I acknowledge your puzzlement. I'm not sure what to do about it. There are a few significant differences from the REUC case: 1. This happens whenever the index is refreshed. REUC, as you mentioned, only affected resolutions of conflicted merges. So users ran into it less often. 2. I never ran into the REUC case. If I had, I would have sent the same patch then. 3. Time has passed and people's standards may have gone up. I wish I had been around when the message was added in the first place, so that I could have provided the same feedback about the message then. But I do not think that that should be held against me. I'm describing a real user problem. Are the commit messages unclear? Is there some missing use case that this version of the patch misses? I don't *think* you intend to say "sure, you got user reports, but (those users are wrong | those users are not real | you are not interpreting those users correctly)", but that is what I am hearing. On the other hand, I don't want to discourage useful review feedback, and I think adding the advise() call was a real improvement. I'm just getting confused about why I am still not being heard. Jonathan
onathan Nieder wrote: > Junio C Hamano wrote: >> I am still puzzled by the insistence of 3/5 and this step that wants >> to kill the coalmine canary. But I am even more puzzled by the >> first two steps that want to disable the two optional extensions. [...] > I acknowledge your puzzlement. I'm not sure what to do about it. > > There are a few significant differences from the REUC case: > > 1. This happens whenever the index is refreshed. REUC, as you > mentioned, only affected resolutions of conflicted merges. So > users ran into it less often. > > 2. I never ran into the REUC case. If I had, I would have sent the > same patch then. > > 3. Time has passed and people's standards may have gone up. > > I wish I had been around when the message was added in the first > place, so that I could have provided the same feedback about the > message then. But I do not think that that should be held against me. > I'm describing a real user problem. And to be clear, it is the first two patches that address the immediate user problem. Whatever improvements we make to the warning message today, we cannot retroactively change the other versions of Git that users are using that want to access the same repository. Jonathan
Junio C Hamano wrote: > This series has a strong smell of pushing back by the > toolsmiths who refuse to promptly upgrade to help their users, and > that is why I do not feel entirely happy with this series. Last reply, I promise. :) This sentence might have the key to the misunderstanding. Let me say a little more about where this showed up in the internal deployment here, to clarify things a little. At Google we deploy snapshots of the "next" branch approximately weekly so that we can find problems early before they affect a published release. We rely on the ability to roll back quickly when a problem is discovered, and we might care more about compatibility than some others because of that. A popular tool within Google has a bundled copy of Git (also a snapshot of the "next" branch, but from a few weeks prior) and when we deployed Git with the EOIE and IEOT extensions, users of that tool very quickly reported the mysterious message. That said, the maintainers of that tool did not complain at all, so hopefully I can allay your worries about toolsmiths pushing back. Once the problem reached my attention (a few days later than I would have liked it to), the Git team at Google knew that we could not roll back and were certainly alarmed about what that means about our ability to cope with other problems should we need to. But we were able to quickly update that popular tool --- no issue. Instead, we ran into a number of other users running into the same problem, when sharing repositories between machines using sshfs, etc. That, plus the aforementioned inability to roll back Git if we need to, meant that this was a serious issue so we quickly addressed it in the internal installation. In general, we haven't had much trouble getting people to use Git 2.19.1 or newer. So the problem here does not have to do with users being slow to upgrade. Instead, it's simply that upgrading Git should not cause the older, widely deployed version of Git to complain about the repositories it acts on. That's a recipe for difficult debugging situations, it can lead to people upgrading less quickly and reporting bugs later, and all in all it's a bad situation to be in. I've used tools like Subversion that would upgrade repositories so they are unusable by the previous version and experienced all of these problems. So I consider it important *to Git upstream* to handle this well in the Git 2.20 release. We can flip the default soon after, even as soon as 2.21. Moreover, I am not the only one who ran into this --- e.g. from [1], 2018-10-19: 17:10 <peff> jrnieder: Yes, I noticed that annoyance myself. ;) 17:11 <newren> Yeah, I saw that message a few times and was slightly annoyed as well. Now, a meta point. Throughout this discussion, I have been hoping for some acknowledgement of the problem --- e.g. an "I am sympathetic to what you are trying to do, but <X>". I wasn't able to find that, and that is part of what contributed to the feeling of not being heard. Thanks for your patient explanations, and hope that helps, Jonathan [1] https://colabti.org/irclogger/irclogger_log/git-devel?date=2018-10-19#l114
Jonathan Nieder <jrnieder@gmail.com> writes: > Now, a meta point. Throughout this discussion, I have been hoping for > some acknowledgement of the problem --- e.g. an "I am sympathetic to > what you are trying to do, but <X>". I wasn't able to find that, and > that is part of what contributed to the feeling of not being heard. I had little sympathy for what you were trying to do, i.e. killing the coalmine canary that warns users about using older version of Git when there clearly is a sign that a newer one is available to them and they have already used it; there was no problem to be acknowledged. Not before "why is it different this time?" question gets answered anyway. And seeing the same "let's not enable the new extension" patch again without much improved justification contributed greatly to the feeling of not being heard. The feeling is mutual.
Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: >> Now, a meta point. Throughout this discussion, I have been hoping for >> some acknowledgement of the problem --- e.g. an "I am sympathetic to >> what you are trying to do, but <X>". I wasn't able to find that, and >> that is part of what contributed to the feeling of not being heard. [...] > And seeing the same "let's not enable the new extension" patch again > without much improved justification contributed greatly to the > feeling of not being heard. Thanks for the pointer. I had not understood before that you were unhappy with those commit messages. The commit message describes symptoms and the motivation for the change. I was confused at the original replies to patch 1 and 2 that seemed to be more about patch 3; patch 1 and 2 are meaningful without patch 3, so it would be odd to include a justification for patch 3 in their commit message. That said, it sounds like their commit messages are not adequate. I'd appreciate help from someone else to improve them. > The feeling is mutual. I was trying to diagnose what was going wrong with the conversation so as to move things forward on a better footing. It seems I only escalated things more. :( Sorry about that, and I hope there's some way to move forward. What is the best way to handle this? I am feeling somewhat burnt by this review process. If Ben and I, working together, are able to come up with a series that we both like, will you consider it for 2.20? Is there some other trusted contributor, such as Peff or Duy, that you would trust to represent your wishes so I can pursue their Reviewed-by without risking getting burnt in the same way again? Jonathan
Jonathan Nieder <jrnieder@gmail.com> writes: > I don't *think* you intend to say "sure, you got user reports, but > (those users are wrong | those users are not real | you are not > interpreting those users correctly)", but that is what I am hearing. What I have been saying is "we are sending a wrong message to those users by not clearly saying 'optional' (i.e. it is OK for your Git not to understand this optional bits of information---you do not have to get alarmed immediately) and also not hinting where that optional thing comes from (i.e. if users realized they come from the future, the coalmine canary message will serve its purpose of reminding them that a newer Git is not just available but has been used already in their repository and help them to rectify the situation sooner)". As the deployed versions of Git will keep sending the wrong message, I do not mind applying 1/5 and 2/5, given especially that Ben seems to be OK with the plan. I however do not think 3 thru 5 is ready yet with this round---there were some discussions on phrasing in this thread.
Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: >> I don't *think* you intend to say "sure, you got user reports, but >> (those users are wrong | those users are not real | you are not >> interpreting those users correctly)", but that is what I am hearing. > > What I have been saying is "we are sending a wrong message to those > users by not clearly saying 'optional' (i.e. it is OK for your Git > not to understand this optional bits of information---you do not > have to get alarmed immediately) and also not hinting where that > optional thing comes from (i.e. if users realized they come from the > future, the coalmine canary message will serve its purpose of > reminding them that a newer Git is not just available but has been > used already in their repository and help them to rectify the > situation sooner)". > > As the deployed versions of Git will keep sending the wrong message, > I do not mind applying 1/5 and 2/5, given especially that Ben seems > to be OK with the plan. I however do not think 3 thru 5 is ready > yet with this round---there were some discussions on phrasing in > this thread. Thanks much --- that helps a lot. Would you mind taking patch 4/5 as well? (It's a tweak to the configuration introduced in patches 1 and 2 that addresses a concern Ben Peart had.) As for patches 3 and 5, I agree. In particular, patch 5 needs an s/performance//, and it seems that the commit messages need some work as well. Sorry for getting the conversation in the wrong direction, and I'm glad to hear we have a good way forward. Sincerely, Jonathan
Junio C Hamano <gitster@pobox.com> writes: > As the deployed versions of Git will keep sending the wrong message, > I do not mind applying 1/5 and 2/5, given especially that Ben seems > to be OK with the plan. I however do not think 3 thru 5 is ready > yet with this round---there were some discussions on phrasing in > this thread. I ran out of time looking at the surrounding code, but I think 1, 2 and 4 form a set that would give us immediate benefit to fast track to the upcoming release. I do not know if it makes sense to have 3 and 5 separate; I suspect a single patch that does "clarify the warning, and allow those who have no choice in which version of Git to choose squelch it" would suffice. The phrasing in 5 received a couple of good concrete suggestions already, so it is not ready in its current form but need a bit of wordsmithing. I also do not think a new "trace_printf" would particularly help. If I stared it a lot longer, I may spot more issues in it. But what that step does primarily would help long after the upcoming release and in that sense can wait a bit longer than 1, 2 & 4 (which I am hoping can be merged in -rc1).
Junio C Hamano <gitster@pobox.com> writes: > I do not know if it makes sense to have 3 and 5 separate; I suspect > a single patch that does "clarify the warning, and allow those who > have no choice in which version of Git to choose squelch it" would > suffice. I actually do not mind two patches for these, but I think the separation presented in the series is wrong (first to kill the canary completely, and then add it as if it were a completely separate advice). It would make more sense, at least to me, if - the earlier step is to clarify the warning on two points (i.e. this is safe to ignore, but you may want to know that you are using a stale Git when we see evidence that a newer one has already been used here) and then - the later step is to optionally make it possible to squelch it for those who do not have control over what version of Git they are allowed to run. But again, a single patch to do all of that is also fine.
On Wed, Nov 21 2018, Jonathan Nieder wrote: > Junio C Hamano wrote: > >> This series has a strong smell of pushing back by the >> toolsmiths who refuse to promptly upgrade to help their users, and >> that is why I do not feel entirely happy with this series. > > Last reply, I promise. :) > > This sentence might have the key to the misunderstanding. Let me say > a little more about where this showed up in the internal deployment > here, to clarify things a little. > > At Google we deploy snapshots of the "next" branch approximately > weekly so that we can find problems early before they affect a > published release. We rely on the ability to roll back quickly when a > problem is discovered, and we might care more about compatibility than > some others because of that. > > A popular tool within Google has a bundled copy of Git (also a > snapshot of the "next" branch, but from a few weeks prior) and when we > deployed Git with the EOIE and IEOT extensions, users of that tool > very quickly reported the mysterious message. > > That said, the maintainers of that tool did not complain at all, so > hopefully I can allay your worries about toolsmiths pushing back. > Once the problem reached my attention (a few days later than I would > have liked it to), the Git team at Google knew that we could not roll > back and were certainly alarmed about what that means about our > ability to cope with other problems should we need to. But we were > able to quickly update that popular tool --- no issue. > > Instead, we ran into a number of other users running into the same > problem, when sharing repositories between machines using sshfs, etc. > That, plus the aforementioned inability to roll back Git if we need > to, meant that this was a serious issue so we quickly addressed it in > the internal installation. > > In general, we haven't had much trouble getting people to use Git > 2.19.1 or newer. So the problem here does not have to do with users > being slow to upgrade. > > Instead, it's simply that upgrading Git should not cause the older, > widely deployed version of Git to complain about the repositories it > acts on. That's a recipe for difficult debugging situations, it can > lead to people upgrading less quickly and reporting bugs later, and > all in all it's a bad situation to be in. I've used tools like > Subversion that would upgrade repositories so they are unusable by the > previous version and experienced all of these problems. > > So I consider it important *to Git upstream* to handle this well in > the Git 2.20 release. We can flip the default soon after, even as > soon as 2.21. > > Moreover, I am not the only one who ran into this --- e.g. from [1], > 2018-10-19: > > 17:10 <peff> jrnieder: Yes, I noticed that annoyance myself. ;) > 17:11 <newren> Yeah, I saw that message a few times and was slightly > annoyed as well. > > Now, a meta point. Throughout this discussion, I have been hoping for > some acknowledgement of the problem --- e.g. an "I am sympathetic to > what you are trying to do, but <X>". I wasn't able to find that, and > that is part of what contributed to the feeling of not being heard. > > Thanks for your patient explanations, and hope that helps, > Jonathan I think it makes total sense to fix this. I had not spotted this myself since I tend to just roll forward and only use one version of git on one system, but fixing this makes sense.
diff --git a/advice.c b/advice.c index 5f35656409..91a55046fd 100644 --- a/advice.c +++ b/advice.c @@ -24,6 +24,7 @@ int advice_add_embedded_repo = 1; int advice_ignored_hook = 1; int advice_waiting_for_editor = 1; int advice_graft_file_deprecated = 1; +int advice_unknown_index_extension = 1; int advice_checkout_ambiguous_remote_branch_name = 1; static int advice_use_color = -1; @@ -78,6 +79,7 @@ static struct { { "ignoredHook", &advice_ignored_hook }, { "waitingForEditor", &advice_waiting_for_editor }, { "graftFileDeprecated", &advice_graft_file_deprecated }, + { "unknownIndexExtension", &advice_unknown_index_extension }, { "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name }, /* make this an alias for backward compatibility */ diff --git a/advice.h b/advice.h index 696bf0e7d2..8da0845cfc 100644 --- a/advice.h +++ b/advice.h @@ -24,6 +24,7 @@ extern int advice_add_embedded_repo; extern int advice_ignored_hook; extern int advice_waiting_for_editor; extern int advice_graft_file_deprecated; +extern int advice_unknown_index_extension; extern int advice_checkout_ambiguous_remote_branch_name; int git_default_advice_config(const char *var, const char *value); diff --git a/read-cache.c b/read-cache.c index 002ed2c1e4..d1d903e5a1 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1727,6 +1727,17 @@ static int read_index_extension(struct index_state *istate, return error("index uses %.4s extension, which we do not understand", ext); trace_printf("ignoring %.4s extension\n", ext); + if (advice_unknown_index_extension) { + warning(_("ignoring optional %.4s index extension"), ext); + 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" + "\n" + "Run \"%s\"\n" + "to suppress this message."), + "git config advice.unknownIndexExtension false"); + } break; } return 0;
It is not unusual for multiple distinct versions of Git to act on a single repository. For example, some IDEs bundle a particular version of Git, which can be a different version from the system copy of Git, or on a Mac, /usr/bin/git quickly goes out of sync with the Homebrew git in /usr/local/bin/git. When a newer version of Git writes an index file that an older version of Git does not know how to make full use of, this is a teaching opportunity. The user may not be aware of what version of Git they are using. Print an advice message to help the user to use the most full featured version of Git (e.g. by futzing with their PATH). warning: ignoring optional IEOT index extension hint: This is likely due to the file having been written by a newer hint: version of Git than is reading it. You can upgrade Git to hint: take advantage of performance improvements from the updated hint: file format. hint: hint: You can run "git config advice.unknownIndexExtension false" hint: to suppress this message. This replaces the message ignoring IEOT extension that existed previously and did not provide enough detail for a user to act on it or suppress it. Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- New, based on Junio's hints about the message removed in patch 3/5. That's the end of the series. Thanks for reading, and thanks again for your help so far. advice.c | 2 ++ advice.h | 1 + read-cache.c | 11 +++++++++++ 3 files changed, 14 insertions(+)