From: Siddharth Asthana <siddharthasthana31@gmail•com>
To: Christian Couder <christian.couder@gmail•com>
Cc: Elijah Newren <newren@gmail•com>,
git@vger•kernel.org, gitster@pobox•com, ps@pks•im,
code@khaugsbakk•name, rybak.a.v@gmail•com, karthik.188@gmail•com,
jltobler@gmail•com, toon@iotcl•com, johncai86@gmail•com,
johannes.schindelin@gmx•de
Subject: Re: [PATCH v2 1/1] replay: make atomic ref updates the default behavior
Date: Thu, 9 Oct 2025 01:29:38 +0530 [thread overview]
Message-ID: <b3369f52-5391-4b00-8051-57617f998734@gmail.com> (raw)
In-Reply-To: <CAP8UFD1JBeGxV65DFCs9dSkYwMpSBhWCZoj6dXCwmKgZnR_=KA@mail.gmail.com>
On 03/10/25 13:29, Christian Couder wrote:
> On Fri, Oct 3, 2025 at 1:27 AM Siddharth Asthana
> <siddharthasthana31@gmail•com> wrote:
>
>>>> For users needing the traditional pipeline workflow, --output-commands
>>>> preserves the original behavior:
>>>>
>>>> git replay --output-commands --onto main topic1..topic2 | git update-ref --stdin
>>> This is good. Did you also add a config option so that someone can
>>> just set that option once and use the old behavior? (as per the
>>> suggestion at https://lore.kernel.org/git/xmqq5xdrvand.fsf@gitster.g/
>>> ?)
>>
>> I didn't, but I should have. I will add a config option for v3.
> You don't need to add that configuration option in the main patch. I
> would suggest adding it in a separate patch after the main one (which
> changes the default behavior of the command).
>
> Note that in the commit message of the main patch, it's nice to say
> that a following commit will add a configuration option for users who
> prefer the previous default behavior.
>
>> For naming, I am thinking either:
>> - replay.updateRefs (boolean: true = update, false = output-commands)
>> - replay.defaultOutput (string: "update" | "commands")
> If the command line option is called `--output-commands` then I would
> suggest naming it "replay.outputCommands" and making it a boolean.
That makes sense - replay.outputCommands matches the command line option
name directly. Much clearer than my replay.defaultOutput idea.
So the pattern would be:
- replay.outputCommands = false (default): atomic ref updates
- replay.outputCommands = true: traditional pipeline output
I will implement this in a separate patch after the main one, as you
suggested.
>
>>>> @@ -330,9 +361,12 @@ int cmd_replay(int argc,
>>>> usage_with_options(replay_usage, replay_options);
>>>> }
>>>>
>>>> - if (advance_name_opt && contained)
>>>> - die(_("options '%s' and '%s' cannot be used together"),
>>>> - "--advance", "--contained");
>>>> + die_for_incompatible_opt2(!!advance_name_opt, "--advance",
>>>> + contained, "--contained");
>>> Broken indentation. Also, should this have been done as a preparatory
>>> cleanup patch?
>>
>> Good catches. I will fix the indentation.
>>
>> On making it a preparatory patch: should I split it out as a separate
>> cleanup commit, or is it minor enough to fold into the main change? I am
>> leaning toward folding it in since it's directly related to the option
>> handling changes
> If there is only this additional small cleanup change in the main
> commit, and this small cleanup change is clearly mentioned in the
> commit message as a "while at it small cleanup change", I think it's
> OK.
Got it. Since it's just the one die_for_incompatible_opt2() change and
it's directly related to option handling, I will fold it into the main
patch
with a "while at it" note in the commit message.
>
> If you find out that other additional small cleanup changes would be
> nice too, then they should definitely all go into a preparatory patch
> before the main patch.
>
>
>>>> +
>>>> + /* Handle empty ranges: if no commits were processed, treat as success */
>>>> + if (!commits_processed)
>>>> + ret = 1; /* Success - no commits to replay is not an error */
>>>> + else
>>>> + ret = result.clean;
>>> The change to treat empty ranges as success is an orthogonal change
>>> that I think at a minimum belongs in a separate patch. Out of
>>> curiosity, how did you discover the exit status with an empty commit
>>> range? Why does someone specify such a range, and what form or forms
>>> might it come in? And is merely returning a successful result enough,
>>> or is there more that needs to be done for correctness?
>>
>> I was thinking about automated scripts that compute ranges dynamically -
>> they might generate A..B where it turns out A==B, and treating that as
>> "no work needed, success" seemed reasonable for scripting.
>>
>> But you raise a good point: A..A seems like obvious user error (why would
>> anyone do that intentionally?), and B..A where B contains A is likely a
>> mistake that maybe should error rather than silently succeed.
>>
>> I am inclined to drop it entirely from this series. If there's real demand
>> for specific empty-range handling, we can add it later with proper
>> discussion of the actual use cases. Does that sound reasonable?
> Yeah, I think dropping it from this series is fine.
Thanks Christian. I will drop the empty range handling from this series.
On documenting the current behavior for empty ranges: should that go in
this series or separately? If the current behavior is just "returns
failure for empty ranges", maybe a simple doc note is enough. But if we
want to discuss what the behavior *should* be, that probably deserves its
own focused series.
What do you think?
>
> What happens in those cases should be documented if it isn't already
> though. Those documentation changes should probably be in a separate
> patch.
>
> Thanks.
next prev parent reply other threads:[~2025-10-08 19:59 UTC|newest]
Thread overview: 125+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-08 4:36 [PATCH 0/2] replay: add --update-refs option Siddharth Asthana
2025-09-08 4:36 ` [PATCH 1/2] " Siddharth Asthana
2025-09-08 9:54 ` Patrick Steinhardt
2025-09-09 6:58 ` Siddharth Asthana
2025-09-09 9:00 ` Patrick Steinhardt
2025-09-09 7:32 ` Elijah Newren
2025-09-10 17:58 ` Siddharth Asthana
2025-09-08 4:36 ` [PATCH 2/2] replay: document --update-refs and --batch options Siddharth Asthana
2025-09-08 6:00 ` Christian Couder
2025-09-09 6:36 ` Siddharth Asthana
2025-09-09 7:26 ` Christian Couder
2025-09-10 20:26 ` Siddharth Asthana
2025-09-08 14:40 ` Kristoffer Haugsbakk
2025-09-09 7:06 ` Siddharth Asthana
2025-09-09 19:20 ` Andrei Rybak
2025-09-10 20:28 ` Siddharth Asthana
2025-09-08 6:07 ` [PATCH 0/2] replay: add --update-refs option Christian Couder
2025-09-09 6:36 ` Siddharth Asthana
2025-09-08 14:33 ` Kristoffer Haugsbakk
2025-09-09 7:04 ` Siddharth Asthana
2025-09-09 7:13 ` Elijah Newren
2025-09-09 7:47 ` Christian Couder
2025-09-09 9:19 ` Elijah Newren
2025-09-09 16:44 ` Junio C Hamano
2025-09-09 19:52 ` Elijah Newren
2025-09-26 23:08 ` [PATCH v2 0/1] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-09-26 23:08 ` [PATCH v2 1/1] " Siddharth Asthana
2025-09-30 8:23 ` Christian Couder
2025-10-02 22:16 ` Siddharth Asthana
2025-10-03 7:30 ` Christian Couder
2025-10-02 22:55 ` Elijah Newren
2025-10-03 7:05 ` Christian Couder
2025-09-30 10:05 ` Phillip Wood
2025-10-02 10:00 ` Karthik Nayak
2025-10-02 22:20 ` Siddharth Asthana
2025-10-02 22:20 ` Siddharth Asthana
2025-10-08 14:01 ` Phillip Wood
2025-10-08 20:09 ` Siddharth Asthana
2025-10-08 20:59 ` Elijah Newren
2025-10-08 21:16 ` Siddharth Asthana
2025-10-09 9:40 ` Phillip Wood
2025-10-02 16:32 ` Elijah Newren
2025-10-02 18:27 ` Junio C Hamano
2025-10-02 23:42 ` Siddharth Asthana
2025-10-02 23:27 ` Siddharth Asthana
2025-10-03 7:59 ` Christian Couder
2025-10-08 19:59 ` Siddharth Asthana [this message]
2025-10-03 19:48 ` Elijah Newren
2025-10-03 20:32 ` Junio C Hamano
2025-10-08 20:06 ` Siddharth Asthana
2025-10-08 20:59 ` Junio C Hamano
2025-10-08 21:10 ` Siddharth Asthana
2025-10-08 21:30 ` Elijah Newren
2025-10-08 20:05 ` Siddharth Asthana
2025-10-02 17:14 ` [PATCH v2 0/1] " Kristoffer Haugsbakk
2025-10-02 23:36 ` Siddharth Asthana
2025-10-03 19:05 ` Kristoffer Haugsbakk
2025-10-08 20:02 ` Siddharth Asthana
2025-10-08 20:56 ` Elijah Newren
2025-10-08 21:16 ` Kristoffer Haugsbakk
2025-10-08 21:18 ` Siddharth Asthana
2025-10-13 18:33 ` [PATCH v3 0/3] replay: make atomic ref updates the default Siddharth Asthana
2025-10-13 18:33 ` [PATCH v3 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-10-13 18:33 ` [PATCH v3 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-10-13 22:05 ` Junio C Hamano
2025-10-15 5:01 ` Siddharth Asthana
2025-10-13 18:33 ` [PATCH v3 3/3] replay: add replay.defaultAction config option Siddharth Asthana
2025-10-13 19:39 ` [PATCH v3 0/3] replay: make atomic ref updates the default Junio C Hamano
2025-10-15 4:57 ` Siddharth Asthana
2025-10-15 10:33 ` Christian Couder
2025-10-15 14:45 ` Junio C Hamano
2025-10-22 18:50 ` [PATCH v4 " Siddharth Asthana
2025-10-22 18:50 ` [PATCH v4 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-10-22 18:50 ` [PATCH v4 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-10-22 21:19 ` Junio C Hamano
2025-10-28 19:03 ` Siddharth Asthana
2025-10-24 10:37 ` Christian Couder
2025-10-24 15:23 ` Junio C Hamano
2025-10-28 20:18 ` Siddharth Asthana
2025-10-28 19:39 ` Siddharth Asthana
2025-10-22 18:50 ` [PATCH v4 3/3] replay: add replay.refAction config option Siddharth Asthana
2025-10-24 11:01 ` Christian Couder
2025-10-24 15:30 ` Junio C Hamano
2025-10-28 20:08 ` Siddharth Asthana
2025-10-28 19:26 ` Siddharth Asthana
2025-10-24 13:28 ` Phillip Wood
2025-10-24 13:36 ` Phillip Wood
2025-10-28 19:47 ` Siddharth Asthana
2025-10-28 19:46 ` Siddharth Asthana
2025-10-23 18:47 ` [PATCH v4 0/3] replay: make atomic ref updates the default Junio C Hamano
2025-10-25 16:57 ` Junio C Hamano
2025-10-28 20:19 ` Siddharth Asthana
2025-10-24 9:39 ` Christian Couder
2025-10-28 21:46 ` [PATCH v5 " Siddharth Asthana
2025-10-28 21:46 ` [PATCH v5 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-10-28 21:46 ` [PATCH v5 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-10-28 21:46 ` [PATCH v5 3/3] replay: add replay.refAction config option Siddharth Asthana
2025-10-29 16:19 ` Christian Couder
2025-10-29 17:00 ` Siddharth Asthana
2025-10-30 19:19 ` [PATCH v6 0/3] replay: make atomic ref updates the default Siddharth Asthana
2025-10-30 19:19 ` [PATCH v6 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-10-31 18:47 ` Elijah Newren
2025-11-05 18:39 ` Siddharth Asthana
2025-10-30 19:19 ` [PATCH v6 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-10-31 18:49 ` Elijah Newren
2025-10-31 19:59 ` Junio C Hamano
2025-11-05 19:07 ` Siddharth Asthana
2025-11-03 16:25 ` Phillip Wood
2025-11-03 19:32 ` Siddharth Asthana
2025-11-04 16:15 ` Phillip Wood
2025-10-30 19:19 ` [PATCH v6 3/3] replay: add replay.refAction config option Siddharth Asthana
2025-10-31 7:08 ` Christian Couder
2025-11-05 19:03 ` Siddharth Asthana
2025-10-31 18:49 ` Elijah Newren
2025-11-05 19:10 ` Siddharth Asthana
2025-10-31 18:51 ` [PATCH v6 0/3] replay: make atomic ref updates the default Elijah Newren
2025-11-05 19:15 ` [PATCH v7 " Siddharth Asthana
2025-11-05 19:15 ` [PATCH v7 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-11-05 19:16 ` [PATCH v7 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-11-05 19:16 ` [PATCH v7 3/3] replay: add replay.refAction config option Siddharth Asthana
2025-11-06 19:32 ` [PATCH v7 0/3] replay: make atomic ref updates the default Elijah Newren
2025-11-08 13:22 ` Siddharth Asthana
2025-11-08 17:11 ` Elijah Newren
2025-11-07 15:48 ` Phillip Wood
2025-11-08 13:23 ` Siddharth Asthana
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b3369f52-5391-4b00-8051-57617f998734@gmail.com \
--to=siddharthasthana31@gmail$(echo .)com \
--cc=christian.couder@gmail$(echo .)com \
--cc=code@khaugsbakk$(echo .)name \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=jltobler@gmail$(echo .)com \
--cc=johannes.schindelin@gmx$(echo .)de \
--cc=johncai86@gmail$(echo .)com \
--cc=karthik.188@gmail$(echo .)com \
--cc=newren@gmail$(echo .)com \
--cc=ps@pks$(echo .)im \
--cc=rybak.a.v@gmail$(echo .)com \
--cc=toon@iotcl$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox