From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Tue, 28 Oct 2025 19:46:34 GMT
Subject: Re: [PATCH v4 3/3] replay: add replay.refAction config option
Message-ID: <359f1d65-b5b9-451a-95cc-c62343798c60@gmail.com>
In-Reply-To: <a4cd31ad-7086-4d05-ba00-db65ec24b45a@gmail.com>


On 24/10/25 18:58, Phillip Wood wrote:
> On 22/10/2025 19:50, Siddharth Asthana wrote:
>
> This is looking pretty nice now, I've left some on he tests comments 
> below


Thanks for the test improvements!


>
>> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
>> index 54c86b87d8..307beb667e 100755
>> --- a/t/t3650-replay-basics.sh
>> +++ b/t/t3650-replay-basics.sh
>> @@ -217,4 +217,46 @@ test_expect_success 
>> 'merge.directoryRenames=false' '
>>           --onto rename-onto rename-onto..rename-from
>>   '
>>   +test_expect_success 'replay.refAction config option' '
>> +    # Store original state
>> +    START=$(git rev-parse topic2) &&
>
> Isn't there a tag we can use here from the initial setup?


Good point - I'll use `topic1` instead of `$(git rev-parse topic2)` for
consistency with the existing test patterns.


>
>> +    test_when_finished "git branch -f topic2 $START && git config 
>> --unset replay.refAction" &&
>> +
>> +    # Set config to print
>> +    git config replay.refAction print &&
> I think it would be better to use test_config here rather than having 
> to clear the config manually with test_when_finished() above.


Absolutely, `test_config` is much cleaner and handles the cleanup 
automatically.
I will refactor all the config tests to use this pattern.


>
>> +    git replay --onto main topic1..topic2 >output &&
>> +    test_line_count = 1 output &&
>> +    grep "^update refs/heads/topic2 " output &&
>
> Rather than test_line_count and grep it would be better to use 
> test_cmp here.


Will switch to `test_cmp` where appropriate, and definitely change 
`grep` to
`test_grep` for better error reporting.

Thanks,
Siddharth


>
> The same comments apply to the rest of the tests
>
> Thanks
>
> Phillip
>
>> +
>> +    # Reset and test update mode
>> +    git branch -f topic2 $START &&
>> +    git config replay.refAction update &&
>> +    git replay --onto main topic1..topic2 >output &&
>> +    test_must_be_empty output &&
>> +
>> +    # Verify ref was updated
>> +    git log --format=%s topic2 >actual &&
>> +    test_write_lines E D M L B A >expect &&
>> +    test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'command-line --ref-action overrides config' '
>> +    # Store original state
>> +    START=$(git rev-parse topic2) &&
>> +    test_when_finished "git branch -f topic2 $START && git config 
>> --unset replay.refAction" &&
>> +
>> +    # Set config to update but use --ref-action=print
>> +    git config replay.refAction update &&
>> +    git replay --ref-action=print --onto main topic1..topic2 >output &&
>> +    test_line_count = 1 output &&
>> +    grep "^update refs/heads/topic2 " output
>> +'
>> +
>> +test_expect_success 'invalid replay.refAction value' '
>> +    test_when_finished "git config --unset replay.refAction" &&
>> +    git config replay.refAction invalid &&
>> +    test_must_fail git replay --onto main topic1..topic2 2>error &&
>> +    grep "invalid value for replay.refAction" error
>> +'
>> +
>>   test_done
>
>