Re: [PATCH v5 3/3] replay: add replay.refAction config option
- From
Siddharth Asthana <siddharthasthana31@gmail.com>
- Date
- Oct 29, 2025, 17:00 UTC
- 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.
Show 11 quoted lines
> > (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
Show 18 quoted lines
> >> + # 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 >> +'