Message ID | e4414cf630da284bdb11f5613ca0ce6413c6a443.1661926908.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | Output fixes for --remerge-diff | expand |
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > Commit 95433eeed9 ("diff: add ability to insert additional headers for > paths", 2022-02-02) introduced the possibility of additional headers, > created in create_filepairs_for_header_only_notifications(). These are > represented by inserting additional pairs in diff_queued_diff which > always have a mode of 0 and a null_oid. When these were added, one > code path was noted to assume that at least one of the diff_filespecs > in the pair were valid, and that codepath was corrected. > > The submodule_format handling is another codepath with the same issue; > it would operate on these additional headers and attempt to display them > as submodule changes. Prevent that by explicitly checking for both > modes being 0. It may make sense to give a concrete name for the condition these new codepaths check, which presumably exists in the part that was touched in 95433eeed9 when "that codepath was corrected". I think you want to treat a diffpair with at least one side with non-zero mode as a "real" thing (as opposed to the phony "additional headers" hack), so perhaps int diff_filepair_is_phoney(struct diff_filespec *one, struct diff_filespec *two) { return !DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two); } or something like that. The use of the FILE_VALID macro here is very much deliberate, and tries to match the more recent hack after this hunk that says: if (!DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two)) { /* * We should only reach this point for pairs from * create_filepairs_for_header_only_notifications(). For * these, we should avoid the "/dev/null" special casing * above, meaning we avoid showing such pairs as either * "new file" or "deleted file" below. */ lbl[0] = a_one; lbl[1] = b_two; } We shouldn't expect readers to understand (one->mode || two->mode) is about the same hack as the other one.
On Wed, Aug 31, 2022 at 3:20 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > Commit 95433eeed9 ("diff: add ability to insert additional headers for > > paths", 2022-02-02) introduced the possibility of additional headers, > > created in create_filepairs_for_header_only_notifications(). These are > > represented by inserting additional pairs in diff_queued_diff which > > always have a mode of 0 and a null_oid. When these were added, one > > code path was noted to assume that at least one of the diff_filespecs > > in the pair were valid, and that codepath was corrected. > > > > The submodule_format handling is another codepath with the same issue; > > it would operate on these additional headers and attempt to display them > > as submodule changes. Prevent that by explicitly checking for both > > modes being 0. > > It may make sense to give a concrete name for the condition these > new codepaths check, which presumably exists in the part that was > touched in 95433eeed9 when "that codepath was corrected". I think > you want to treat a diffpair with at least one side with non-zero > mode as a "real" thing (as opposed to the phony "additional headers" > hack), so perhaps > > int diff_filepair_is_phoney(struct diff_filespec *one, > struct diff_filespec *two) > { > return !DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two); > } > > or something like that. The use of the FILE_VALID macro here is > very much deliberate, and tries to match the more recent hack after > this hunk that says: > > if (!DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two)) { > /* > * We should only reach this point for pairs from > * create_filepairs_for_header_only_notifications(). For > * these, we should avoid the "/dev/null" special casing > * above, meaning we avoid showing such pairs as either > * "new file" or "deleted file" below. > */ > lbl[0] = a_one; > lbl[1] = b_two; > } > > We shouldn't expect readers to understand (one->mode || two->mode) > is about the same hack as the other one. I like that; I'll make that change.
diff --git a/diff.c b/diff.c index 974626a6211..be23f660570 100644 --- a/diff.c +++ b/diff.c @@ -3429,14 +3429,16 @@ static void builtin_diff(const char *name_a, if (o->submodule_format == DIFF_SUBMODULE_LOG && (!one->mode || S_ISGITLINK(one->mode)) && - (!two->mode || S_ISGITLINK(two->mode))) { + (!two->mode || S_ISGITLINK(two->mode)) && + (one->mode || two->mode)) { show_submodule_diff_summary(o, one->path ? one->path : two->path, &one->oid, &two->oid, two->dirty_submodule); return; } else if (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF && (!one->mode || S_ISGITLINK(one->mode)) && - (!two->mode || S_ISGITLINK(two->mode))) { + (!two->mode || S_ISGITLINK(two->mode)) && + (one->mode || two->mode)) { show_submodule_inline_diff(o, one->path ? one->path : two->path, &one->oid, &two->oid, two->dirty_submodule); diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh index 9e7cac68b1c..e3e6fbd97b2 100755 --- a/t/t4069-remerge-diff.sh +++ b/t/t4069-remerge-diff.sh @@ -185,6 +185,14 @@ test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no dif test_cmp expect actual ' +test_expect_success 'submodule formatting ignores additional headers' ' + # Reuses "expect" from last testcase + + git show --oneline --remerge-diff --diff-filter=U --submodule=log >tmp && + sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual && + test_cmp expect actual +' + test_expect_success 'remerge-diff w/ diff-filter=R: relevant file + conflict header' ' git log -1 --oneline resolution >tmp && cat <<-EOF >>tmp &&