Re: [PATCH V5 2/2] git-apply: add --allow-empty flag

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 17, 2021, 04:51 UTC
Message-ID: <211217.86wnk395bz.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqee6dz5s9.fsf@gitster.g>

On Wed, Dec 15 2021, Junio C Hamano wrote:

Show 48 quoted lines

> Jerry Zhang <jerry@skydio.com> writes:
>
>>  t/t4126-apply-empty.sh      | 22 ++++++++++++++++++----
>>  4 files changed, 30 insertions(+), 7 deletions(-)
>> ...
>> diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
>> index ceb6a79fe0..949e284d14 100755
>> --- a/t/t4126-apply-empty.sh
>> +++ b/t/t4126-apply-empty.sh
>> @@ -7,10 +7,12 @@ test_description='apply empty'
>>  test_expect_success setup '
>>  	>empty &&
>>  	git add empty &&
>>  	test_tick &&
>>  	git commit -m initial &&
>> +	git commit --allow-empty -m "empty commit" &&
>> +	git format-patch --always HEAD~ >empty.patch &&
>>  	for i in a b c d e
>
> When merged with anything that has ab/mark-leak-free-tests-even-more
> topic, this will start breaking the tests, as it is my understanding
> that "git log" family hasn't been audited and converted for leak
> sanitizer.
>
> This is sort of water under the bridge, as the other topic is
> already in 'master', but come to think of it, the strategy we used
> with TEST_PASSES_SANITIZE_LEAK variable was misguided.  
>
> If the git subcommands a single test script uses were only the
> subcommands that the test script wants to test, the approach to
> default to "this subcommand has not been made leak sanitizer clean",
> and then to add TEST_PASSES mark as we sanitize the subcommand makes
> perfect sense, but most test scripts need to run git subcommands
> that are *not* the focus of the test---they run them only to prepare
> the scene in which the subcommands being tested are excersized.  In
> such a situation (which is exactly what happens here), marking that
> "right now, all the tested subcommands and also all the subcommands
> that happen to be exercised to prepare fixture are clean" would
> force us to flip-flop with "now we use a subcommand we didn't use in
> this script before to prepare the scene, and it is not yet sanitizer
> clean, so we need to unmark it", which is not quite ideal, but is
> much better than forcing the contributor who is *not* working on making
> these subcommands leak-sanitizer-clean to worry about such a breakage.
>
> I am tempted to drop the "TEST_PASSES" bit from this script for now,
> but I have to say that the "mark leak-free tests" topic took us in
> an awkward place.  We probably want to do something a bit more fine
> grained about it.

I don't see how us not having a 1=1 mapping between say a "mktag.sh" test script and that script *only* running "git mktag" makes the approach with SANITIZE=leak misguided.

You can, FWIW, mark things in a more gradual manner than un-marking the script entirely. There's the SANITIZE_LEAK prerequisite for individual "test_expect_success".

Yes it's painful that topics in-flight have this happen to them, but that pain will mostly go away one the "big leaks" are solved, i.e. checkout/commit/log etc.

I have all those patches, but they've been held up by the pace these changes have been getting integrated at.

E.g. f346fcb62a0 (Merge branch 'ab/mark-leak-free-tests-even-more', 2021-12-15) just hit master, but that series has been on-list since the 31st of October, and was picked up & noted in What's Cooking on the 2nd of November[3]. The only changes in it are adding the same "TEST_PASSES_SANITIZE_LEAK=true" marking to 104 test scripts.

Part of that delay is the release that happened mid-November, but even accounting for that I wish we could find ways to make this go faster.

I.e. I understand that a general change to git.git might take this time, but in this case really all the proof we should need is "does CI pass?". So I don't see why we couldn't make this go a bit faster.

Similarly for things that add new free()'s we can (unless the code is tricky, which is usually obvious, i.e. adding highly conditional free) count on libc/SANITIZE=[leak|address] to validate that the memory management is OK.

Anyway, I had hoped to submit the "struct rev_info" freeing sometime soon, depending on how you'd queue up other prerequisites for it.

But with an UNLEAK() for it in-flight I'll delay it even more, since it will directly conflict both textually & semantically with the changes to fix the same memory leak.

So as painful as other parts of this are I'd really like it if we could avoid taking one step back and two steps forward each step of the way by plastering over things with UNLEAK(). See [4] for earlier discussion on that.

1. https://lore.kernel.org/git/?q=ab%2Fmark-leak-free-tests-even-more
2. https://lore.kernel.org/git/cover-00.15-00000000000-20211030T221945Z-avarab@gmail.com/
3. https://lore.kernel.org/git/xmqqy267851e.fsf@gitster.g/
4. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/