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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Elijah Newren <newren@gmail.com>,
	Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Philippe Blain <levraiphilippeblain@gmail.com>
Subject: Re: [PATCH 0/3] Output fixes for --remerge-diff
Date: Thu, 01 Sep 2022 20:46:44 +0200	[thread overview]
Message-ID: <220901.86tu5rhqtg.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqo7vzce39.fsf@gitster.g>

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