On Thu, Jun 09 2022, Junio C Hamano wrote: > Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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. > 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. > 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.
