[PATCH 0/3] Output fixes for --remerge-diff

[PATCH 0/3] Output fixes for --remerge-diff

[Elijah Newren via 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.

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/43cf2a1d-058a-fd79-befe-7d9bc62581ed@gmail.com/

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

[PATCH 1/3] diff: have submodule_format logic avoid additional diff headers

[Elijah Newren via GitGitGadget]

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.

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 diff.c                  | 6 ++++--
 t/t4069-remerge-diff.sh | 8 ++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

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 &&
-- 
gitgitgadget

Re: [PATCH 1/3] diff: have submodule_format logic avoid additional diff headers

[Junio C Hamano]

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

Re: [PATCH 1/3] diff: have submodule_format logic avoid additional diff headers

[Elijah Newren]

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.

[PATCH 2/3] diff: fix filtering of additional headers under --remerge-diff

[Elijah Newren via GitGitGadget]

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.
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 <newren@gmail.com>
---
 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 be23f660570..9535e755c73 100644
--- a/diff.c
+++ b/diff.c
@@ -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)
diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh
index e3e6fbd97b2..95a16d19aec 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 &&
 
-- 
gitgitgadget

Re: [PATCH 2/3] diff: fix filtering of additional headers under --remerge-diff

[Junio C Hamano]

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

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?

Re: [PATCH 2/3] diff: fix filtering of additional headers under --remerge-diff

[Elijah Newren]

On Wed, Aug 31, 2022 at 3:26 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.
> > 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.
>
> 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.

[PATCH 3/3] diff: fix filtering of merge commits under --remerge-diff

[Elijah Newren via GitGitGadget]

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.
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 <levraiphilippeblain@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 diff.c                  | 1 +
 t/t4069-remerge-diff.sh | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/diff.c b/diff.c
index 9535e755c73..f5c0e0599c9 100644
--- a/diff.c
+++ b/diff.c
@@ -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)));
 
diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh
index 95a16d19aec..07323ebafe0 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 &&
-- 
gitgitgadget

Re: [PATCH 0/3] Output fixes for --remerge-diff

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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

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

Re: [PATCH 0/3] Output fixes for --remerge-diff

[Elijah Newren]

On Wed, Aug 31, 2022 at 6:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.
>
> 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.

Re: [PATCH 0/3] Output fixes for --remerge-diff

[Elijah Newren]

On Wed, Aug 31, 2022 at 8:47 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Wed, Aug 31, 2022 at 6:13 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > 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.
> >
> > 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:

diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh
index e3e6fbd97b2..95a16d19aec 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


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.

Re: [PATCH 0/3] Output fixes for --remerge-diff

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Wed, Aug 31, 2022 at 6:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>> > 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.
>>
>> 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.

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.

Re: [PATCH 0/3] Output fixes for --remerge-diff

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 01 2022, Junio C Hamano wrote:

> Elijah Newren <newren@gmail.com> writes:
>
>> On Wed, Aug 31, 2022 at 6:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>
>>> > 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.
>>>
>>> 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.
>
> 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".

Re: [PATCH 0/3] Output fixes for --remerge-diff

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

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?

[PATCH v2 0/3] Output fixes for --remerge-diff

[Elijah Newren via 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 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/43cf2a1d-058a-fd79-befe-7d9bc62581ed@gmail.com/

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 <newren@gmail.com>
      
       ## 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

[PATCH v2 1/3] diff: have submodule_format logic avoid additional diff headers

[Elijah Newren via GitGitGadget]

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.

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 diff.c                  | 14 +++++++++++---
 t/t4069-remerge-diff.sh |  8 ++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index 974626a6211..d672efed7ab 100644
--- a/diff.c
+++ b/diff.c
@@ -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
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 &&
-- 
gitgitgadget

Re: [PATCH v2 1/3] diff: have submodule_format logic avoid additional diff headers

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> 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 ...

> +static int diff_filepair_is_phoney(struct diff_filespec *one,
> +				   struct diff_filespec *two)
> +{
> +	return !DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two);
> +}
> ...
> -	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

... some of what this comment said, i.e. the pair that was added by
create_filepairs_for_header_only_notifications() is what the helper
detects. 

Other than that, the whole series looks good.

[PATCH v2 2/3] diff: fix filtering of additional headers under --remerge-diff

[Elijah Newren via GitGitGadget]

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.
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 <newren@gmail.com>
---
 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 d672efed7ab..85905261ce4 100644
--- a/diff.c
+++ b/diff.c
@@ -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)
diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh
index e3e6fbd97b2..95a16d19aec 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 &&
 
-- 
gitgitgadget

[PATCH v2 3/3] diff: fix filtering of merge commits under --remerge-diff

[Elijah Newren via GitGitGadget]

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.
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 <levraiphilippeblain@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 diff.c                  | 1 +
 t/t4069-remerge-diff.sh | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/diff.c b/diff.c
index 85905261ce4..5ea9071ff07 100644
--- a/diff.c
+++ b/diff.c
@@ -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)));
 
diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh
index 95a16d19aec..07323ebafe0 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 &&
-- 
gitgitgadget

[PATCH v3 0/3] Output fixes for --remerge-diff

[Elijah Newren via 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/43cf2a1d-058a-fd79-befe-7d9bc62581ed@gmail.com/

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).
      
          Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
     @@ 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

[PATCH v3 1/3] diff: have submodule_format logic avoid additional diff headers

[Elijah Newren via GitGitGadget]

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 "phoney"
filepairs (i.e. filepairs with both modes being 0).

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 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 974626a6211..7f8637f3006 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 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 &&
-- 
gitgitgadget

[PATCH v3 2/3] diff: fix filtering of additional headers under --remerge-diff

[Elijah Newren via GitGitGadget]

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.
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 <newren@gmail.com>
---
 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 7f8637f3006..2439310ae1f 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 e3e6fbd97b2..95a16d19aec 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 &&
 
-- 
gitgitgadget

[PATCH v3 3/3] diff: fix filtering of merge commits under --remerge-diff

[Elijah Newren via GitGitGadget]

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.
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 <levraiphilippeblain@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 diff.c                  | 1 +
 t/t4069-remerge-diff.sh | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/diff.c b/diff.c
index 2439310ae1f..e1f9cef2f38 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 95a16d19aec..07323ebafe0 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 &&
-- 
gitgitgadget