Re: [PATCH] connected: distinguish local/remote bad objects

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 9, 2022, 18:00 UTC
Message-ID: <220609.86zgilsofm.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqwndpn5dz.fsf@gitster.g>

On Thu, Jun 09 2022, Junio C Hamano wrote:

Show 15 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
>
>>  builtin/fetch.c              |  2 +-
>>  connected.c                  |  1 +
>>  revision.c                   | 16 ++++++++++++--
>>  revision.h                   |  3 +++
>>  t/t5518-fetch-exit-status.sh | 43 ++++++++++++++++++++++++++++++++++++
>>  5 files changed, 62 insertions(+), 3 deletions(-)
>
> This seems to break linux-leaks CI job by making 5518, which was
> marked in some topic in flight to expect to be leak-free, fail.
>
> Because of the way linux-leaks test framework is done, it is not
> easy to tell if the code changes essential to this topic introduced
> new leaks, in which case we would want to fix that.

I think this is just an existing leak that happens to be exposed by a new (in this file) test, i.e. transport_get() leaks via an xmalloc() for transport_helper.

Show 9 quoted lines

> Note that this may not the fault of the code changes in this patch.
> If the tests added by the patch started using git commands that are
> known to leak (i.e. not ready to be subjected to the "leaks" test)
> in order to prepare the scenario or to inspect the result, even if
> the code changes in this topic did not introduce any leak, we can
> see the same breakage in linux-leaks CI job.  An easy way out would
> be to disable leak-check CI for the entire 5518, but that is not
> very satisfactory, as the earlier part of that script should still
> be leak-free.

I think doing that would be fine in this case. It will get easier to fix leaks now that "struct rev_info" is out of the way (and I've got a lot of pending patches), but I can always loop back & re-mark this particular test as leak-free at some future date.

Show 5 quoted lines

> Another way out might be to add these two tests in a
> new script, which is not marked as not-leaking.  After all, what the
> new topic adds is not about exit status but how that exit status
> comes about, so it might not be a bad idea even without the CI leak
> stuff anyway.

Yeah, that sounds especially good in this case, as if we can't run httpd we'll print a meaningful "skip" message in that case. See 0a2bfccb9c8 (t0051: use "skip_all" under !MINGW in single-test file, 2022-02-04)

> Ævar, does the internal state used for revision walking count as
> leaking when it is still held by the time we hit die() in
> bad_object(), or anything on stack when we die() are still reachable
> and won't be reported as a failure?

No, but in this case the variable containing the leaked data isn't in scope by the time we exit, i.e. it was used by fetch_one() which had it malloc'd, but the struct it lived in went away, and now we're exiting from cmd_fetch() etc.