From: Siddharth Asthana Date: Wed, 08 Oct 2025 19:59:38 GMT Subject: Re: [PATCH v2 1/1] replay: make atomic ref updates the default behavior Message-ID: In-Reply-To: On 03/10/25 13:29, Christian Couder wrote: > 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. 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.