From: Siddharth Asthana Date: Wed, 08 Oct 2025 20:05:18 GMT Subject: Re: [PATCH v2 1/1] replay: make atomic ref updates the default behavior Message-ID: In-Reply-To: On 04/10/25 01:18, Elijah Newren wrote: > On Thu, Oct 2, 2025 at 4:27 PM 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. >> >> For naming, I am thinking either: >> - replay.updateRefs (boolean: true = update, false = output-commands) >> - replay.defaultOutput (string: "update" | "commands") >> >> The boolean feels simpler, but the string might be more extensible if we >> add other output modes later. Which pattern feels more consistent with >> existing Git config conventions? Looking at rebase.* they're mostly >> boolean toggles, but am I missing a better example to follow? > replay.updateRefs sounds better to me. defaultOutput with "update" > doesn't make sense to me. > >> You are right - I don't have a concrete use case. I was trying to >> anticipate potential needs but ended up adding unjustified complexity. >> >> I will remove --allow-partial entirely from v3. This simplifies to exactly >> two modes with clear purposes: >> 1. Default: atomic ref updates (all-or-nothing) >> 2. --output-commands: traditional pipeline for special cases >> >> Much cleaner design. > Note that once you add a config option, you'll also need an additional > command line flag (or make it possible to invert an existing one), so > that users can override the config and get the default behavior. > Maybe --[no-]update-refs would make sense after all, where > --update-refs is the default and --no-update-refs is your current > --output-commands? That's a good point. With a config option, users need a way to override it. The --[no-]update-refs pattern makes sense: - --update-refs (default): atomic ref updates - --no-update-refs: output commands (equivalent to --output-commands) This is cleaner than having both --output-commands and needing a separate --no-output-commands. And you're right about the rebase naming - it's not really a collision since the concepts are similar enough. Should I go with --[no-]update-refs and drop --output-commands entirely, or keep --output-commands as an alias for --no-update-refs for clarity? > > (I know you all talked elsewhere in this thread about "avoiding a name > collision" with rebase, but I don't quite see it as a collision. When > Stolee suggested the flag for rebase, I pointed out it's roughly what > I'm doing in replay, so it doesn't feel like a conflict to me. I'm > also open to an alternative flag name if it makes sense, but we > probably want whatever the command line flag is to be similar to the > config name and "defaultOutput"/--default-output don't make sense as a > name to me.) > >>>> @@ -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 > Given that it was directly adjacent to the other > die_for_incompatible_opt2() call, if that were still the case, I could > see making it part of the same commit. However, dropping the > --allow-partial flag means you don't need to add that other call > anymore, so it makes this remaining die_for_incompataible_opt2() call > an entirely orthogonal change to the rest of your patch. As such, I > think it belongs in a separate patch; it could either be a preparatory > patch or a follow-up. Good point. With --allow-partial gone, the die_for_incompatible_opt2() change stands alone. I will make it a preparatory cleanup patch before the main change. > >>>> @@ -407,6 +452,8 @@ int cmd_replay(int argc, >>>> khint_t pos; >>>> int hr; >>>> >>>> + commits_processed = 1; >>>> + >>>> if (!commit->parents) >>>> die(_("replaying down to root commit is not supported yet!")); >>>> if (commit->parents->next) >>>> @@ -457,9 +535,17 @@ int cmd_replay(int argc, >>>> strset_clear(update_refs); >>>> free(update_refs); >>>> } >>>> - ret = result.clean; >>>> + >>>> + /* 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? > Yep, dropping it makes sense to me. Alternatively, documenting what > happens in the case of empty ranges, as Christian suggests, also makes > sense to me though I might suggest that it be done in an entirely > separate series rather than just a separate patch of this series. I will drop it from this series. Documenting the current empty range behavior can be a separate follow-up if there's interest, but I don't think it needs to block this change.