From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Wed, 29 Oct 2025 17:00:21 GMT
Subject: Re: [PATCH v5 3/3] replay: add replay.refAction config option
Message-ID: <dd147777-b7d2-4271-b0ab-6d580b8f2768@gmail.com>
In-Reply-To: <CAP8UFD03fx+wKwJzDG8UZz=+S8=07hG6npNnebTmBxrcXNYqGQ@mail.gmail.com>


On 29/10/25 21:49, Christian Couder wrote:
> On Tue, Oct 28, 2025 at 10:46 PM Siddharth Asthana
> <siddharthasthana31@gmail.com> wrote:
>
>> +static enum ref_action_mode parse_ref_action_mode(const char *mode_str, const char *source)


Hi Christian,


> Nit: it's a bit strange that it's called "ref_action_str" everywhere
> except here where it's called "mode_str". I'd prefer "ref_action"
> everywhere.


You are right - that's inconsistent naming. Looking at similar patterns 
in the
codebase like `parse_sign_mode()` in gpg-interface.c, the parameter is just
called `arg`, but for clarity I should stick with `ref_action` throughout.

The inconsistency came from trying to distinguish the string parameter from
the function name, but it just makes the code harder to follow. I will 
rename
both `mode_str` parameters to `ref_action` in the helper functions.


>
> (I understand that "mode" is related to parse_ref_action_mode() having
> "mode" in its name but it's the case for get_ref_action_mode() too.)
>
>> +test_expect_success 'replay.refAction config option' '
>> +       # Store original state
>> +       START=$(git rev-parse topic2) &&
>> +       test_when_finished "git branch -f topic2 $START" &&
>> +       test_when_finished "git config --unset replay.refAction || true" &&
> Is there something preventing test_config to be used in this test
> while it's used in other tests below?


Nothing preventing it - I was being overly cautious because this test sets
config twice in sequence, but `test_config` handles that fine. Looking at
the test-lib-functions.sh implementation, `test_config` uses 
`test_when_finished`
with `test_unconfig` which properly handles multiple config operations.

The manual approach is actually more fragile since it relies on the `|| 
true`
pattern and doesn't guarantee cleanup if the test fails early. I will 
switch
to `test_config` for consistency with the other config tests.

Both fixes are straightforward - I will send them in v6.

Thanks for the careful review and keeping the code quality high!

- Siddharth


>
>> +       # Set config to print
>> +       git config replay.refAction print &&
>> +       git replay --onto main topic1..topic2 >output &&
>> +       test_line_count = 1 output &&
>> +       test_grep "^update refs/heads/topic2 " output &&
>> +
>> +       # 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
>> +'