From 9b08091cb72bdd1d8b37446bcf0a6d73d319bf10 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 2 Sep 2022 03:53:28 +0000 Subject: [PATCH 1/3] diff: have submodule_format logic avoid additional diff headers 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 "phoney" filepairs (i.e. filepairs with both modes being 0). Reported-by: Philippe Blain Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- diff.c | 29 +++++++++++++++++++++++------ t/t4069-remerge-diff.sh | 8 ++++++++ 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/diff.c b/diff.c index 974626a621..7f8637f300 100644 --- a/diff.c +++ b/diff.c @@ -3398,6 +3398,21 @@ static void add_formatted_headers(struct strbuf *msg, line_prefix, meta, reset); } +static int diff_filepair_is_phoney(struct diff_filespec *one, + struct diff_filespec *two) +{ + /* + * This function specifically looks for pairs injected by + * create_filepairs_for_header_only_notifications(). Such + * pairs are "phoney" in that they do not represent any + * content or even mode difference, but were inserted because + * diff_queued_diff previously had no pair associated with + * that path but we needed some pair to avoid losing the + * "remerge CONFLICT" header associated with the path. + */ + return !DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two); +} + static void builtin_diff(const char *name_a, const char *name_b, struct diff_filespec *one, @@ -3429,14 +3444,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)) && + (!diff_filepair_is_phoney(one, two))) { 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)) && + (!diff_filepair_is_phoney(one, two))) { show_submodule_inline_diff(o, one->path ? one->path : two->path, &one->oid, &two->oid, two->dirty_submodule); @@ -3456,12 +3473,12 @@ static void builtin_diff(const char *name_a, b_two = quote_two(b_prefix, name_b + (*name_b == '/')); lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null"; lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null"; - if (!DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two)) { + if (diff_filepair_is_phoney(one, two)) { /* - * We should only reach this point for pairs from + * We should only reach this point for pairs generated 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 + * these, we want to avoid the "/dev/null" special casing + * above, because we do not want such pairs shown as either * "new file" or "deleted file" below. */ lbl[0] = a_one; diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh index 9e7cac68b1..e3e6fbd97b 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 && From 71a146dc701931363d78aa21ad215da648252617 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 2 Sep 2022 03:53:29 +0000 Subject: [PATCH 2/3] diff: fix filtering of additional headers under --remerge-diff Commit 95433eeed9 ("diff: add ability to insert additional headers for paths", 2022-02-02) introduced the possibility of additional headers. Because there could be conflicts with no content differences (e.g. a modify/delete conflict resolved in favor of taking the modified file as-is), that commit also modified the diff_queue_is_empty() and diff_flush_patch() logic to ensure these headers were included even if there was no associated content diff. However, when the pickaxe is active, we really only want the remerge conflict headers to be shown when there is an associated content diff. Adjust the logic in these two functions accordingly. This also removes the TEST_PASSES_SANITIZE_LEAK=true declaration from t4069, as there is apparently some kind of memory leak with the pickaxe code. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- diff.c | 2 ++ t/t4069-remerge-diff.sh | 17 ++++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/diff.c b/diff.c index 7f8637f300..2439310ae1 100644 --- a/diff.c +++ b/diff.c @@ -5869,6 +5869,7 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o) { int include_conflict_headers = (additional_headers(o, p->one->path) && + !o->pickaxe_opts && (!o->filter || filter_bit_tst(DIFF_STATUS_UNMERGED, o))); /* @@ -5924,6 +5925,7 @@ int diff_queue_is_empty(struct diff_options *o) int i; int include_conflict_headers = (o->additional_path_headers && + !o->pickaxe_opts && (!o->filter || filter_bit_tst(DIFF_STATUS_UNMERGED, o))); if (include_conflict_headers) diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh index e3e6fbd97b..95a16d19ae 100755 --- a/t/t4069-remerge-diff.sh +++ b/t/t4069-remerge-diff.sh @@ -2,7 +2,6 @@ test_description='remerge-diff handling' -TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # This test is ort-specific @@ -90,6 +89,22 @@ test_expect_success 'remerge-diff with both a resolved conflict and an unrelated test_cmp expect actual ' +test_expect_success 'pickaxe still includes additional headers for relevant changes' ' + # reuses "expect" from the previous testcase + + git log --oneline --remerge-diff -Sacht ab_resolution >tmp && + sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual && + test_cmp expect actual +' + +test_expect_success 'can filter out additional headers with pickaxe' ' + git show --remerge-diff --submodule=log --find-object=HEAD ab_resolution >actual && + test_must_be_empty actual && + + git show --remerge-diff -S"not present" --all >actual && + test_must_be_empty actual +' + test_expect_success 'setup non-content conflicts' ' git switch --orphan base && From 67360b75c605e6c1458304378bda8255f72d96b9 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 2 Sep 2022 03:53:30 +0000 Subject: [PATCH 3/3] diff: fix filtering of merge commits under --remerge-diff Commit 95433eeed9 ("diff: add ability to insert additional headers for paths", 2022-02-02) introduced the possibility of additional headers. Because there could be conflicts with no content differences (e.g. a modify/delete conflict resolved in favor of taking the modified file as-is), that commit also modified the diff_queue_is_empty() and diff_flush_patch() logic to ensure these headers were included even if there was no associated content diff. However, the added logic was a bit inconsistent between these two functions. diff_queue_is_empty() overlooked the fact that the additional headers strmap could be non-NULL and empty, which would cause it to display commits that should have been filtered out. Fix the diff_queue_is_empty() logic to also account for additional_path_headers being empty. Reported-by: Philippe Blain Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- diff.c | 1 + t/t4069-remerge-diff.sh | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/diff.c b/diff.c index 2439310ae1..e1f9cef2f3 100644 --- a/diff.c +++ b/diff.c @@ -5925,6 +5925,7 @@ int diff_queue_is_empty(struct diff_options *o) int i; int include_conflict_headers = (o->additional_path_headers && + strmap_get_size(o->additional_path_headers) && !o->pickaxe_opts && (!o->filter || filter_bit_tst(DIFF_STATUS_UNMERGED, o))); diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh index 95a16d19ae..07323ebafe 100755 --- a/t/t4069-remerge-diff.sh +++ b/t/t4069-remerge-diff.sh @@ -56,6 +56,11 @@ test_expect_success 'remerge-diff on a clean merge' ' test_cmp expect actual ' +test_expect_success 'remerge-diff on a clean merge with a filter' ' + git show --oneline --remerge-diff --diff-filter=U bc_resolution >actual && + test_must_be_empty actual +' + test_expect_success 'remerge-diff with both a resolved conflict and an unrelated change' ' git log -1 --oneline ab_resolution >tmp && cat <<-EOF >>tmp &&