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

From: Elijah Newren <newren@gmail.com>
Date: Sep 1, 2022, 04:01 UTC
Message-ID: <CABPp-BFJ3-MxJOuU+Rd-1AtVeC0SH3NJnpN=L0511vhkLchiJA@mail.gmail.com>
In-Reply-To: <CABPp-BHULBGAbmY1r9fpRr+MrjqOp7j-devOgkfA25jpYBGY9g@mail.gmail.com>

On Wed, Aug 31, 2022 at 8:47 PM Elijah Newren <newren@gmail.com> wrote:

Show 22 quoted lines

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