From: Elijah Newren Date: Fri, 03 Oct 2025 19:48:39 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 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? (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. > >> @@ -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.