Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Nov 26, 2025, 19:31 UTC
Message-ID: <cc5cc77d-5d78-4a16-b4b5-91a903436788@gmail.com>
In-Reply-To: <xmqqfra1ri5n.fsf@gitster.g>

On 26/11/25 01:36, Junio C Hamano wrote:

Show 46 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> By the way, I probably would not be queuing this version today, as
>> this has obvious conflict with a large code movement made by
>> Patrick's "history" series, which itself is expecting a reroll.
>>
>> Perhaps collect review comments on this iteration a bit more and
>> wait for that other topic to be rerolled, and if it turns out to be
>> solid enough, base a v2 of this patch on top of it?
> While I cannot test it with other topics, I had a chance to run
> tests after applying the patch directly on top of 'master':
>
>      $ make CC=clang SANITIZE=address,leak test
>      ...
>      Test Summary Report
>      -------------------
>      t3650-replay-basics.sh                           (Wstat: 256 (exited 1) Tests: 31 Failed: 5)
>        Failed tests:  23-25, 27, 31
>        Non-zero exit status: 1
>
> The first failure was this one
>
>      expecting success of 3650.23 'using replay with --revert to revert a commit':
>              # Revert commits D and E from topic2
>              git replay --revert --onto topic1 topic1..topic2 >result &&
>
>              test_line_count = 1 result &&
>              NEW_TOPIC2=$(cut -f 3 -d " " result) &&
>
>              # Verify the result updates the topic2 branch
>              printf "update refs/heads/topic2 " >expect &&
>              printf "%s " $NEW_TOPIC2 >>expect &&
>              git rev-parse topic2 >>expect &&
>
>              test_cmp expect result &&
>
>              # Verify the commit messages contain "Revert"
>              # topic1..topic2 contains D and E, so we get 2 reverts on top of topic1 (which has F, C, B, A)
>              git log --format=%s $NEW_TOPIC2 >actual &&
>              test_line_count = 6 actual &&
>              head -n 1 actual >first-line &&
>              test_grep "^Revert" first-line
>
>      test_line_count: line count for result != 1
>
> The "result" file has 0 bytes (hence 0 lines).

Ah, this is because my patch was based on a tree that had atomic ref updates as the default (REF_ACTION_UPDATE), which produces no stdout output. The tests were written for --ref-action=print behavior.

I have fixed the tests to either:
1. Use --ref-action=print explicitly when expecting output, or
2. Check the ref state directly rather than parsing stdout

The test failures you saw should be fixed in v2.

Thanks, Siddharth

Show 7 quoted lines

>
> Actually, address or leak sanitizing build is not needed to
> reproduce this problem, it seems.
>
>      $ make CC=clang test
>
> Was sufficient to see the same first failure.