Elijah Newren wrote: > On Thu, Feb 24, 2022 at 2:34 PM Victoria Dye via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: >> >> From: Victoria Dye <vdye@xxxxxxxxxx> >> >> Add the 'recursive' flag to 'wt_status_collect_changes_index(...)'. Without > > Perhaps "Set the 'recursive' diff option flag in > 'wt_status_collect_changes_index(...)'" ? There's no function > argument named 'recursive' in wt_status_collect_changes_index() before > or after your changes, which is what the wording led me to think of. > Re-reading that now, I agree. Will fix! >> the 'recursive' flag, 'git status' could report index changes incorrectly >> when the following conditions were met: >> >> * sparse index is enabled >> * there is a difference between the index and HEAD in a file inside a >> *subdirectory* of a sparse directory >> * the sparse directory index entry is *not* expanded in-core >> >> In this scenario, 'git status' would not recurse into the sparse directory's >> subdirectories to identify which file contained the difference between the >> index and HEAD. Instead, it would report the immediate subdirectory itself >> as "modified". >> >> Example: >> >> $ git init >> $ mkdir -p sparse/sub >> $ echo test >sparse/sub/foo >> $ git add . >> $ git commit -m "commit 1" >> $ echo somethingelse >sparse/sub/foo >> $ git add . >> $ git commit -a -m "commit 2" >> $ git sparse-checkout set --cone --sparse-index 'sparse' >> $ git reset --soft HEAD~1 >> $ git status >> On branch master >> You are in a sparse checkout. >> >> Changes to be committed: >> (use "git restore --staged <file>..." to unstage) >> modified: sparse/sub >> >> The 'recursive' diff option in 'wt_status_collect_changes_index' corrects >> this by indicating that 'git status' should recurse into sparse directories >> to find modified files. Given the same repository setup as the example >> above, the corrected result of `git status` is: >> >> $ git status >> On branch master >> You are in a sparse checkout. >> >> Changes to be committed: >> (use "git restore --staged <file>..." to unstage) >> modified: sparse/sub/foo >> >> Signed-off-by: Victoria Dye <vdye@xxxxxxxxxx> >> --- >> t/t1092-sparse-checkout-compatibility.sh | 7 +++++++ >> wt-status.c | 9 +++++++++ >> 2 files changed, 16 insertions(+) >> >> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh >> index 9ef7cd80885..b1dcaa0e642 100755 >> --- a/t/t1092-sparse-checkout-compatibility.sh >> +++ b/t/t1092-sparse-checkout-compatibility.sh >> @@ -278,6 +278,13 @@ test_expect_success 'status with options' ' >> test_all_match git status --porcelain=v2 -uno >> ' >> >> +test_expect_success 'status with diff in unexpanded sparse directory' ' >> + init_repos && >> + test_all_match git checkout rename-base && >> + test_all_match git reset --soft rename-out-to-out && >> + test_all_match git status --porcelain=v2 >> +' >> + >> test_expect_success 'status reports sparse-checkout' ' >> init_repos && >> git -C sparse-checkout status >full && >> diff --git a/wt-status.c b/wt-status.c >> index 335e723a71e..4a5b9beeca1 100644 >> --- a/wt-status.c >> +++ b/wt-status.c >> @@ -651,6 +651,15 @@ static void wt_status_collect_changes_index(struct wt_status *s) >> rev.diffopt.detect_rename = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename; >> rev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit; >> rev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score; >> + >> + /* >> + * The `recursive` option must be enabled to show differences in files >> + * *more than* one level deep in a sparse directory index entry (e.g., given >> + * sparse directory 'sparse-dir/', reporting a difference in the file >> + * 'sparse-dir/another-dir/my-file'). >> + */ >> + rev.diffopt.flags.recursive = 1; > > Kind of clever, and makes sense. > > I'm wondering if there's an alternate wording that might be helpful > here or in the commit message, that instead of just saying the > 'recursive' option is necessary, perhaps says a little bit about why > it helps. In particular, the diff machinery, by default, is not > recursive and stops at comparing the first level of trees. (See e.g. > the -r option in diff-tree, it's just that it's turned on by default > in 'git diff' and by the -p option in 'git log'.) I'm guessing the > recursive option never needed to be turned on previously within > wt-status, due to something about the nature of the index only holding > files previously. Now, however, the sparse index changes that. (And > it also suggests that perhaps we should look to see if other commands > run the diff machinery without the recursive flag, and see if they > need it now due to sparse indices.) > > Granted, I'm not totally sure how to work these facts in (in part > because I don't know how comparison to the index normally avoids the > need for the recursive flag), and maybe what you have is fine. Just > thought I'd point it out since I wasn't aware of the non-recursive > nature of the diff machinery until I started doing things with > diff-tree. > All good points - I'll reword the commit message and code comment to explain that 1) diff is not recursive by default, and 2) why the diff of a sparse directory requires `recursive = 1`. > >> + >> copy_pathspec(&rev.prune_data, &s->pathspec); >> run_diff_index(&rev, 1); >> object_array_clear(&rev.pending); >> -- >> gitgitgadget
