From: Christian Couder Date: Fri, 03 Oct 2025 07:59:48 GMT Subject: Re: [PATCH v2 1/1] replay: make atomic ref updates the default behavior Message-ID: In-Reply-To: <0fba2f5e-03cd-439b-90bd-f613fcc4ae23@gmail.com> On Fri, Oct 3, 2025 at 1:27 AM Siddharth Asthana 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. > >> @@ -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. > >> + > >> + /* 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.