Hi Stolee, Thanks for the series! All my comments are from the perspective of someone without much knowledge of the index, much less sparse-index, or sparse-checkout. I am not sure whether the project should cater to reviewers in my position, but I did have trouble understanding most of this series. I didn't have any concerns with the implementation, but the cover letter, commit messages, and tests were fairly confusing and I think can be explained better with a few changes: On 2022.04.07 16:37, Derrick Stolee via GitGitGadget wrote: > This continues our sequence of integrating builtins with the sparse index. > > '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. > Here is an outline of this series: > > * Patch 1: Add more tests around 'git show :' in t1092. > > * Patch 2: Make 'git show' stop expanding the index by default. Make note > of this behavior change in the tests. > > * Patches 3-4: Make the subtle changes to object-name.c that help us reject > sparse directories (patch 3) and print the correct error message (patch > 4). > > Patches 2-4 could realistically be squashed into a single commit, but I > thought it might be instructive to show these individual steps, especially > as an example for our GSoC project. > > This series is based on the current 'master'. I know that Victoria intends > to submit her 'git stash' integration soon, and this provides a way to test > if our split of test changes in t1092 are easy to merge without conflict. If > that is successful, then I will likely submit my integration with the > 'sparse-checkout' builtin after this series is complete. > > Thanks, -Stolee Thanks, Josh
