Output fixes for --remerge-diff

Philippe Blain found and reported a couple issues with the output of --remerge-diff[1]. After digging in, I think one of them actually counts as two separate issues, so here's a series with three patches to fix these issues. Each includes testcases to keep us from regressing.

Note: The issue fixed by the third commit for --remerge-diff is also an issue exhibited by 'git log --cc $FILTER_RULES $COMMIT' (or by -c instead of --cc). However, as far as I can tell the causes are different and come from separate codepaths; this series focuses on --remerge-diff and hence makes no attempt to fix independent (even if similar) --cc or -c issues.

[1]
https://lore.kernel.org/git/[email protected]/

Elijah Newren (3):
diff: have submodule_format logic avoid additional diff headers
diff: fix filtering of additional headers under --remerge-diff
diff: fix filtering of merge commits under --remerge-diff

 diff.c                  |  9 +++++++--
 t/t4069-remerge-diff.sh | 30 +++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 3 deletions(-)

base-commit: 6c8e4ee870332d11e4bba84901654b355a9ff016
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1342%2Fnewren%2Fremerge-diff-output-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1342/newren/remerge-diff-output-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1342

gitgitgadget

From: Elijah Newren <[email protected]>

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.

---
 diff.c                  | 6 ++++--
 t/t4069-remerge-diff.sh | 8 ++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

index 974626a6211..be23f660570 100644
@@ -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);

index 9e7cac68b1c..e3e6fbd97b2 100755
@@ -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 &&

gitgitgadget

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.

From: Elijah Newren <[email protected]>

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.

---
 diff.c                  |  2 ++
 t/t4069-remerge-diff.sh | 17 ++++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

index be23f660570..9535e755c73 100644
@@ -5854,6 +5854,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)));
 
 	/*

@@ -5909,6 +5910,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)

index e3e6fbd97b2..95a16d19aec 100755
@@ -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 &&
 

gitgitgadget

In the longer term, I think we may have to redo the way additional headers are inserted to the diff_queue. All the diff code I am familiar with (read: written before this hack was introduced) trusts that diff_queue.nr is the number of paths that are returned by the diff frontend, and unless there is diffcore_break involved, there will be at most one diff_filepair that is about a path.

Why do these need to be separate entries in the queue, not a new attribute added to an existing filepair? Are we inserting pieces of information that are not about any paths that are involved in the diff?

They usually are added to an existing filepair, but as your last question alludes to, there isn't always one that exists.

One example where this can happen is with a modify/delete conflict. If someone got one of those and decided to keep the modified file as-is when resolving the conflict, then there'd be no content diff to show for that file when investigating the resulting merge commit with --remerge-diff. But since the point of --remerge-diff is to show whatever work the user did to resolve conflicts, and that file was conflicted, we want to show _something_ for that path. Thus the reason for creating a new "phoney" filepair just for attaching the "remerge CONFLICT (modify/delete) ..." notice.

From: Elijah Newren <[email protected]>

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.

---
 diff.c                  | 1 +
 t/t4069-remerge-diff.sh | 5 +++++
 2 files changed, 6 insertions(+)

index 9535e755c73..f5c0e0599c9 100644
@@ -5910,6 +5910,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)));
 

index 95a16d19aec..07323ebafe0 100755
@@ -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 &&

gitgitgadget

Including this to 'seen' seems to break the leaks-check CI job X-<.

https://github.com/git/git/runs/8124648321?check_suite_focus=true

That's...surprising. Any chance of a mis-merge?

I ask for two reasons:
* This series, built on main, passed the leaks-check job.
* The link you provide points to t4069 as the test failing, but the
second patch of this series removes the TEST_PASSES_SANITIZE_LEAK=true line from t4069, which should make that test a no-op for the leaks-check job.

Actually, looks not like a mis-merge, but some kind of faulty git am application. The merge in question isn't available for me to fetch, but clicking through the UI from the link you provide eventually leads me to:

    https://github.com/git/git/commit/81f120208d02afee71543d4f588b471950f156f2

which does NOT match what I submitted:

    https://lore.kernel.org/git/feac97494600e522125b7bb202f4dc5ca020ca99.1661926908.git.gitgitgadget@gmail.com/

It's close, but despite still including this part of my commit message:

"""
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.
"""

it's missing this part of the diff:

index e3e6fbd97b2..95a16d19aec 100755
@@ -2,7 +2,6 @@
 
 test_description='remerge-diff handling'
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # This test is ort-specific

That part of the diff is important. I did not add any leaks to the code (I did run the leaks-checking job and looked through the output to verify that none of them involved any codepath I added or modified), but I did add some test code which exercises pre-existing memory leaks, and testing those codepaths is critical to verify I got the appropriate fixes. Any idea what happened here?

Either way, I'm going to resubmit the series due to your other suggestion. So long as the unfortunate munging doesn't occur again, things should be fine if you just take the new series.

Ah, that.

Yes, I did rebase it to 'maint' to be nice to our users as this is not a new feature development but a bugfix or two.

This is why I hate the leak-check CI job (yes, I do help maintain all parts of the tree, but it does not mean I have to love every bit of the codebase, and this is one of the things I love to hate).

Instead of saying "subcommand X with feature Y? It ought to be clean so complain if leak checker find something. subcommand Z? It is known to be unclean, so do not bother", it says "In this test in entirety, we currently happen to use only the ones that are clean" and penalizes developers who wants to use an unclean tool merely for checking. The approach is fundamentally flawed and does not play well with multiple integration branches, just like we saw here.

We've discussed doing it that way before. I wouldn't be fundamentally opposed, but I do think we're far enough along the way to being leak-free that we'd want to mark more than just a "top-level" command as leak-free.

It's just also not the case that we even could do that in all but the most trivial cases. Most commands still leak somewhere in some obscure cases, but we have entire tests now where the code they run in those common cases doesn't leak.

However, in this case this seems to just be a case that Elijah tested his code on base X, and you applied it on base Y.

I don't really see how the approach you're suggesting would be any more likely to be resilient in the face of that. Then we'd presumably use some command that's leak-free on "master", but that command wouldn't be leak-free on "maint".

Two things. Seeing the leak-check breakage quite often, I doubt how much trust I can place in your "far enough along the way" statement. Also, I do not think we thought the alternative was to mark only the top-level. The test could inspect the crash after the fact, and say "ah, allocation made by xcalloc() called from this and that functions are still known to be leaky, so do not stop and mark the CI job a failure for this one", for example?

Philippe Blain found and reported a couple issues with the output of --remerge-diff[1]. After digging in, I think one of them actually counts as two separate issues, so here's a series with three patches to fix these issues. Each includes testcases to keep us from regressing.

Changes since v1:

* Added a new diff_filepair_is_phoney() function to make the code more self-documenting, as suggested by Junio.
* Note: Patch 2, as called out in its commit message, disables TEST_PASSES_SANITIZE_LEAK for t4069. This is not because I introduce any memory leaks, but because I add new testcases invoking additional parts of the code (pickaxe stuff) which already had pre-existing leaks. This is not a change since v1, but this somehow accidentally got munged out of Junio's application of my v1.

Note: The issue fixed by the third commit for --remerge-diff is also an issue exhibited by 'git log --cc $FILTER_RULES $COMMIT' (or by -c instead of --cc). However, as far as I can tell the causes are different and come from separate codepaths; this series focuses on --remerge-diff and hence makes no attempt to fix independent (even if similar) --cc or -c issues.

[1]
https://lore.kernel.org/git/[email protected]/

Elijah Newren (3):
diff: have submodule_format logic avoid additional diff headers
diff: fix filtering of additional headers under --remerge-diff
diff: fix filtering of merge commits under --remerge-diff

 diff.c                  | 17 ++++++++++++++---
 t/t4069-remerge-diff.sh | 30 +++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 4 deletions(-)

base-commit: 6c8e4ee870332d11e4bba84901654b355a9ff016
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1342%2Fnewren%2Fremerge-diff-output-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1342/newren/remerge-diff-output-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1342

Range-diff vs v1:

1: e4414cf630d ! 1: aea0bbc1c6a diff: have submodule_format logic avoid additional diff headers

     @@ Commit message
          Signed-off-by: Elijah Newren <[email protected]>
      
       ## diff.c ##
     +@@ diff.c: 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)
     ++{
     ++	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,
      @@ diff.c: 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)) {
     ++	    (!diff_filepair_is_phoney(one, two))) {
       		show_submodule_diff_summary(o, one->path ? one->path : two->path,
       				&one->oid, &two->oid,
       				two->dirty_submodule);
     @@ diff.c: static void builtin_diff(const char *name_a,
       		   (!one->mode || S_ISGITLINK(one->mode)) &&
      -		   (!two->mode || S_ISGITLINK(two->mode))) {
      +		   (!two->mode || S_ISGITLINK(two->mode)) &&
     -+		   (one->mode || 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);
     +@@ diff.c: 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
     + 		 * create_filepairs_for_header_only_notifications().  For
      
       ## t/t4069-remerge-diff.sh ##
      @@ t/t4069-remerge-diff.sh: test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no dif
 2:  feac9749460 = 2:  3bd622d5b45 diff: fix filtering of additional headers under --remerge-diff
 3:  46ea0d7dd65 = 3:  7903f8380dc diff: fix filtering of merge commits under --remerge-diff

gitgitgadget

From: Elijah Newren <[email protected]>

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.

---
 diff.c                  | 14 +++++++++++---
 t/t4069-remerge-diff.sh |  8 ++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

index 974626a6211..d672efed7ab 100644
@@ -3398,6 +3398,12 @@ 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)
+{
+	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 +3435,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,7 +3464,7 @@ 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
 		 * create_filepairs_for_header_only_notifications().  For

index 9e7cac68b1c..e3e6fbd97b2 100755
@@ -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 &&

gitgitgadget

"Elijah Newren via GitGitGadget" <[email protected]> writes:

As there is no guarantee that we'd never add another hack to use a different kind of "phoney" filepair to diff_queue for another purpose, either the name of the helper function, or a comment in front of it, should say something that is sufficient to help readers understand ...

From: Elijah Newren <[email protected]>

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.

---
 diff.c                  |  2 ++
 t/t4069-remerge-diff.sh | 17 ++++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

index d672efed7ab..85905261ce4 100644
@@ -5860,6 +5860,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)));
 
 	/*

@@ -5915,6 +5916,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)

index e3e6fbd97b2..95a16d19aec 100755
@@ -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 &&
 

gitgitgadget

From: Elijah Newren <[email protected]>

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.

---
 diff.c                  | 1 +
 t/t4069-remerge-diff.sh | 5 +++++
 2 files changed, 6 insertions(+)

index 85905261ce4..5ea9071ff07 100644
@@ -5916,6 +5916,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)));
 

index 95a16d19aec..07323ebafe0 100755
@@ -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 &&

gitgitgadget

Philippe Blain found and reported a couple issues with the output of --remerge-diff[1]. After digging in, I think one of them actually counts as two separate issues, so here's a series with three patches to fix these issues. Each includes testcases to keep us from regressing.

Changes since v2:

* Added a comment describing the rationale for the new diff_filepair_is_phoney() function.

Changes since v1:

* Added a new diff_filepair_is_phoney() function to make the code more self-documenting, as suggested by Junio.
* Note: Patch 2, as called out in its commit message, disables TEST_PASSES_SANITIZE_LEAK for t4069. This is not because I introduce any memory leaks, but because I add new testcases invoking additional parts of the code (pickaxe stuff) which already had pre-existing leaks. This is not a change since v1, but this somehow accidentally got munged out of Junio's application of my v1.

Note: The issue fixed by the third commit for --remerge-diff is also an issue exhibited by 'git log --cc $FILTER_RULES $COMMIT' (or by -c instead of --cc). However, as far as I can tell the causes are different and come from separate codepaths; this series focuses on --remerge-diff and hence makes no attempt to fix independent (even if similar) --cc or -c issues.

[1]
https://lore.kernel.org/git/[email protected]/

Elijah Newren (3):
diff: have submodule_format logic avoid additional diff headers
diff: fix filtering of additional headers under --remerge-diff
diff: fix filtering of merge commits under --remerge-diff

 diff.c                  | 32 ++++++++++++++++++++++++++------
 t/t4069-remerge-diff.sh | 30 +++++++++++++++++++++++++++++-
 2 files changed, 55 insertions(+), 7 deletions(-)

base-commit: 6c8e4ee870332d11e4bba84901654b355a9ff016
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1342%2Fnewren%2Fremerge-diff-output-fixes-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1342/newren/remerge-diff-output-fixes-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1342

Range-diff vs v2:

1: aea0bbc1c6a ! 1: ed28acaed97 diff: have submodule_format logic avoid additional diff headers
@@ Commit message

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.
+ as submodule changes. Prevent that by explicitly checking for "phoney"
+ filepairs (i.e. filepairs with both modes being 0).

@@ diff.c: static void add_formatted_headers(struct strbuf *msg,
+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);
+}
+
@@ diff.c: static void builtin_diff(const char *name_a,
- 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 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;

## t/t4069-remerge-diff.sh ##
@@ t/t4069-remerge-diff.sh: test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no dif

 2:  3bd622d5b45 = 2:  f91bea2bbc3 diff: fix filtering of additional headers under --remerge-diff
 3:  7903f8380dc = 3:  084a037756d diff: fix filtering of merge commits under --remerge-diff

gitgitgadget

From: Elijah Newren <[email protected]>

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).

---
 diff.c                  | 29 +++++++++++++++++++++++------
 t/t4069-remerge-diff.sh |  8 ++++++++
 2 files changed, 31 insertions(+), 6 deletions(-)

index 974626a6211..7f8637f3006 100644
@@ -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;

index 9e7cac68b1c..e3e6fbd97b2 100755
@@ -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 &&

gitgitgadget

From: Elijah Newren <[email protected]>

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.

---
 diff.c                  |  2 ++
 t/t4069-remerge-diff.sh | 17 ++++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

index 7f8637f3006..2439310ae1f 100644
@@ -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)

index e3e6fbd97b2..95a16d19aec 100755
@@ -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 &&
 

gitgitgadget

From: Elijah Newren <[email protected]>

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.

---
 diff.c                  | 1 +
 t/t4069-remerge-diff.sh | 5 +++++
 2 files changed, 6 insertions(+)

index 2439310ae1f..e1f9cef2f38 100644
@@ -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)));
 

index 95a16d19aec..07323ebafe0 100755
@@ -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 &&

gitgitgadget