Re: [PATCH v2 1/1] replay: make atomic ref updates the default behavior

From: Christian Couder <christian.couder@gmail.com>
Date: Oct 3, 2025, 07:59 UTC
Message-ID: <CAP8UFD1JBeGxV65DFCs9dSkYwMpSBhWCZoj6dXCwmKgZnR_=KA@mail.gmail.com>
In-Reply-To: <0fba2f5e-03cd-439b-90bd-f613fcc4ae23@gmail.com>

On Fri, Oct 3, 2025 at 1:27 AM Siddharth Asthana <siddharthasthana31@gmail.com> wrote:

Show 11 quoted lines

> >> 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.

Show 19 quoted lines

> >> @@ -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.

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.

Show 25 quoted lines

> >> +
> >> +       /* 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.

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.