On 4/14/2022 5:14 PM, Junio C Hamano wrote: > Josh Steadmon <steadmon@xxxxxxxxxx> writes: > >>> 'git show' is relatively simple to get working in a way that doesn't fail >>> when it would previously succeed, but there are some sutbleties when the >>> user passes a directory path. If that path happens to be a sparse directory >>> entry, we suddenly start succeeding and printing the tree information! >>> >>> Since this behavior can change depending on the sparse checkout definition >>> and the state of index entries within that directory, this new behavior >>> would be more likely to confuse users than help them. >> >> The two paragraphs above did not really convey to me on first >> read-through what the problem was. IIUC the main points are: >> >> * git-show does not currently work with the sparse index. >> * A simple change fixes the above, but causes us to sometimes print >> unexpected information about trees. >> * We can use the simple change in our implementation, but must do >> additional work to make sure we only print the expected information. >> >> I think this could be clarified by: >> * Including examples of the unhelpful output from the simple >> implementation. >> * Describing in more detail the situations that trigger this. >> * Describing why those situations don't currently happen without support >> for sparse indexes. > > I think the issues patches 2-4 addresses are not limited to any > specific command, but how ":<path-in-the-index>" syntax is resolved > to an object name (including the way how error messages are issued > when the given path is not found in the index). Yes, this is the critical bit. It's the only part of "git show" that cares about the index. > But the title of the series and presentation place undue stress on > "git show", which I suspect may be confusing to some readers. > > For example, with these patches, wouldn't "git rev-parse" start > working better, even though the proposed log message for no commit > in the series talks about "rev-parse" and no tests are done with the > "rev-parse" command? Probably, assuming we unset the command_requires_full_index protection on 'git rev-parse'. This might be a good case for Shaoxuan to try. > I am not suggesting that commands other than "show" should be also > discussed in detail or tested. It would help readers if we said > that we are using 'show' merely as an example to demonstrate how > ":<path-in-the-index>" syntax interacts with the sparse index > entries (1) before the series, and (2) how well the improved > object-name parsing logic works after the series. I can see that. Some background: as we shipped our sparse index integrations in the microsoft/git fork and measured performance of real users, we noticed that 'git show' was a frequently-used command that went from ~0.5 seconds to ~4 seconds in monorepo situations. This was unexpected, because we didn't know about the ":<path>" syntax. Further, it seemed that some third-party tools were triggering this behavior as a frequent way to check on staged content. That motivated quickly shipping this integration. Performance improved to ~0.1 seconds because of the reduced index size. Thanks, -Stolee
