| Message ID | 20181113004019.GD170017@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Avoid confusing messages from new index extensions (Re: [PATCH v8 0/7] speed up index load through parallelization) | expand |
Jonathan Nieder <jrnieder@gmail.com> writes: > We almost obey that convention, but there is a problem: when > encountering an unrecognized optional extension, we write > > ignoring FNCY extension > > to stderr, which alarms users. Then the same comment as 2/3 applies to this step.
On 11/12/2018 7:40 PM, Jonathan Nieder wrote: > Documentation/technical/index-format explains: > > 4-byte extension signature. If the first byte is 'A'..'Z' the > extension is optional and can be ignored. > > This allows gracefully introducing a new index extension without > having to rely on all readers having support for it. Mandatory > extensions start with a lowercase letter and optional ones start with > a capital. Thus the versions of Git acting on a shared local > repository do not have to upgrade in lockstep. > > We almost obey that convention, but there is a problem: when > encountering an unrecognized optional extension, we write > > ignoring FNCY extension > > to stderr, which alarms users. This means that in practice we have > had to introduce index extensions in two steps: first add read > support, and then a while later, start writing by default. This > delays when users can benefit from improvements to the index format. > > We cannot change the past, but for index extensions of the future, > there is a straightforward improvement: silence that message except > when tracing. This way, the message is still available when > debugging, but in everyday use it does not show up so (once most Git > users have this patch) we can turn on new optional extensions right > away without alarming people. > The best patch of the bunch. Glad to see it. I'm fine with doing this via advise.unknownIndexExtension as well. Who knows, someone may actually want to see this and not have tracing turned on. I don't know who but it is possible :-) > Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> > --- > Thanks for reading. Thoughts? > > Sincerely, > Jonathan > > read-cache.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/read-cache.c b/read-cache.c > index 290bd54708..65530a68c2 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -1720,7 +1720,7 @@ static int read_index_extension(struct index_state *istate, > if (*ext < 'A' || 'Z' < *ext) > return error("index uses %.4s extension, which we do not understand", > ext); > - fprintf(stderr, "ignoring %.4s extension\n", ext); > + trace_printf("ignoring %.4s extension\n", ext); > break; > } > return 0; >
Jonathan Nieder <jrnieder@gmail.com> writes: > We cannot change the past, but for index extensions of the future, > there is a straightforward improvement: silence that message except > when tracing. This way, the message is still available when > debugging, but in everyday use it does not show up so (once most Git > users have this patch) we can turn on new optional extensions right > away without alarming people. That argument ignores the "let the users know they are using a stale version when they did use (either by accident or deliberately) a more recent one" value, though. Even if we consider that this is only for debugging, I am not sure if tracing is the right place to add. As long as the "optional extensions can be ignored without affecting the correctness" rule holds, there is nothing gained by letting these messages shown for debugging purposes, and if there is such a bug (e.g. we introduced an optional extension but the code that wrote an index with an optional extension wrote the non-optional part in such a way that it cannot be correctly handled without the extension that is supposed to be optional) we'd probably want to let users notice without having to explicitly go into a debugging session. If Googling for "ignoring FNCY ext" gives "discard your index with 'reset HEAD', because an index file with FNCY ext cannot be read without understanding it", that may prevent damages from happening in the first place. On the other hand, hiding it behind tracing would mean the user first need to exprience an unknown breakage first and then has to enable tracing among other 47 different things to diagnose and drill down to the root cause.
On 11/13/2018 10:24 PM, Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: > >> We cannot change the past, but for index extensions of the future, >> there is a straightforward improvement: silence that message except >> when tracing. This way, the message is still available when >> debugging, but in everyday use it does not show up so (once most Git >> users have this patch) we can turn on new optional extensions right >> away without alarming people. > > That argument ignores the "let the users know they are using a stale > version when they did use (either by accident or deliberately) a > more recent one" value, though. > > Even if we consider that this is only for debugging, I am not sure > if tracing is the right place to add. As long as the "optional > extensions can be ignored without affecting the correctness" rule > holds, there is nothing gained by letting these messages shown for > debugging purposes Having recently written a couple of patches that utilize an optional extension - I actually found the warning to be a helpful debugging tool and would like to see them enabled via tracing. It would also be helpful to see the opposite - I'm looking for an optional extension but it is missing. The most common scenario was when I'd be testing my changes in different repos and forget that I needed to force an updated index to be written that contained the extension I was trying to test. The "silently ignore the optional extension" behavior is good for end users but as a developer, I'd like to be able to have it yell at me via tracing. :-) IMHO - if an end user has to turn on tracing, I view that as a failure on our part. No end user should have to understand the inner workings of git to be able to use it effectively. and if there is such a bug (e.g. we introduced > an optional extension but the code that wrote an index with an > optional extension wrote the non-optional part in such a way that it > cannot be correctly handled without the extension that is supposed > to be optional) we'd probably want to let users notice without > having to explicitly go into a debugging session. If Googling for > "ignoring FNCY ext" gives "discard your index with 'reset HEAD', > because an index file with FNCY ext cannot be read without > understanding it", that may prevent damages from happening in the > first place. On the other hand, hiding it behind tracing would mean > the user first need to exprience an unknown breakage first and then > has to enable tracing among other 47 different things to diagnose > and drill down to the root cause. > >
diff --git a/read-cache.c b/read-cache.c index 290bd54708..65530a68c2 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1720,7 +1720,7 @@ static int read_index_extension(struct index_state *istate, if (*ext < 'A' || 'Z' < *ext) return error("index uses %.4s extension, which we do not understand", ext); - fprintf(stderr, "ignoring %.4s extension\n", ext); + trace_printf("ignoring %.4s extension\n", ext); break; } return 0;
Documentation/technical/index-format explains: 4-byte extension signature. If the first byte is 'A'..'Z' the extension is optional and can be ignored. This allows gracefully introducing a new index extension without having to rely on all readers having support for it. Mandatory extensions start with a lowercase letter and optional ones start with a capital. Thus the versions of Git acting on a shared local repository do not have to upgrade in lockstep. We almost obey that convention, but there is a problem: when encountering an unrecognized optional extension, we write ignoring FNCY extension to stderr, which alarms users. This means that in practice we have had to introduce index extensions in two steps: first add read support, and then a while later, start writing by default. This delays when users can benefit from improvements to the index format. We cannot change the past, but for index extensions of the future, there is a straightforward improvement: silence that message except when tracing. This way, the message is still available when debugging, but in everyday use it does not show up so (once most Git users have this patch) we can turn on new optional extensions right away without alarming people. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Thanks for reading. Thoughts? Sincerely, Jonathan read-cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)