From: Elijah Newren Date: Tue, 30 Dec 2025 22:50:58 GMT Subject: Re: [PATCH v2 1/5] replay: remove dead code and rearrange Message-ID: In-Reply-To: On Tue, Dec 30, 2025 at 7:03 AM wrote: > > From: Kristoffer Haugsbakk > > 22d99f01 (replay: add --advance or 'cherry-pick' mode, 2023-11-24) both > added `--advance` and made one of `--onto` or `--advance` mandatory. > But `determine_replay_mode` claims that there is a third alternative; > neither of `--onto` or `--advance` were given: > > if (onto_name) { > ... > } else if (*advance_name) { > ... > } else { > ... > } > > But this is false—the fallthrough else-block is dead code. > > Commit 22d99f01 was iterated upon by several people.[1] The initial > author wrote code for a sort of *guess mode*, allowing for shorter > commands when that was possible. But the next person instead made one > of the aforementioned options mandatory. In turn this code was dead on > arrival in git.git. > > [1]: https://lore.kernel.org/git/CABPp-BEcJqjD4ztsZo2FTZgWT5ZOADKYEyiZtda+d0mSd1quPQ@mail.gmail.com/ > > Let’s remove this code. We can also join the if-block with the > condition `!*advance_name` into the `*onto` block since we do not set > `*advance_name` in this function. It only looked like we might set it > since the dead code has this line: > > *advance_name = xstrdup_or_null(last_key); > > Let’s also rename the function since we do not determine > the replay mode here. We simply populate data structures. > > Note that there might be more dead code caused by this *guess mode*. > We only concern ourselves with this function for now. > > Helped-by: Elijah Newren > Signed-off-by: Kristoffer Haugsbakk > --- > > Notes (series): > v2: [new] > > See the link in the commit message. > > Not strictly needed for this series but I think it makes sense to fix it > here. > > builtin/replay.c | 70 +++++++++++------------------------------------- > 1 file changed, 16 insertions(+), 54 deletions(-) > > diff --git a/builtin/replay.c b/builtin/replay.c > index 6172c8aacc9..54849f65c87 100644 > --- a/builtin/replay.c > +++ b/builtin/replay.c > @@ -154,16 +154,16 @@ static void get_ref_information(struct repository *repo, > > free(fullname); > } > } > > -static void determine_replay_mode(struct repository *repo, > - struct rev_cmdline_info *cmd_info, > - const char *onto_name, > - char **advance_name, > - struct commit **onto, > - struct strset **update_refs) > +static void populate_for_onto_or_advance_mode(struct repository *repo, > + struct rev_cmdline_info *cmd_info, > + const char *onto_name, > + char **advance_name, > + struct commit **onto, > + struct strset **update_refs) Renaming makes sense, but the new name is quite the mouthful, and it feels slightly odd because "onto" is both a command line flag and a variable -- and the variable value is used regardless of which command line flag is used. Since the variable is used either way, there's a risk someone might be confused by this function name. Maybe just setup_replay_mode() ? Or maybe others have other suggestions? > { > struct ref_info rinfo; > > get_ref_information(repo, cmd_info, &rinfo); > if (!rinfo.positive_refexprs) > @@ -174,69 +174,30 @@ static void determine_replay_mode(struct repository *repo, > if (onto_name) { > *onto = peel_committish(repo, onto_name); > if (rinfo.positive_refexprs < > strset_get_size(&rinfo.positive_refs)) > die(_("all positive revisions given must be references")); > - } else if (*advance_name) { > + *update_refs = xcalloc(1, sizeof(**update_refs)); > + **update_refs = rinfo.positive_refs; > + memset(&rinfo.positive_refs, 0, sizeof(**update_refs)); > + } else { > struct object_id oid; > char *fullname = NULL; > > + if (!*advance_name) > + BUG("expected either onto_name or *advance_name in this function"); > + > *onto = peel_committish(repo, *advance_name); > if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name), > &oid, &fullname, 0) == 1) { > free(*advance_name); > *advance_name = fullname; > } else { > die(_("argument to --advance must be a reference")); > } > if (rinfo.positive_refexprs > 1) > die(_("cannot advance target with multiple sources because ordering would be ill-defined")); > - } else { > - int positive_refs_complete = ( > - rinfo.positive_refexprs == > - strset_get_size(&rinfo.positive_refs)); > - int negative_refs_complete = ( > - rinfo.negative_refexprs == > - strset_get_size(&rinfo.negative_refs)); > - /* > - * We need either positive_refs_complete or > - * negative_refs_complete, but not both. > - */ > - if (rinfo.negative_refexprs > 0 && > - positive_refs_complete == negative_refs_complete) > - die(_("cannot implicitly determine whether this is an --advance or --onto operation")); > - if (negative_refs_complete) { > - struct hashmap_iter iter; > - struct strmap_entry *entry; > - const char *last_key = NULL; > - > - if (rinfo.negative_refexprs == 0) > - die(_("all positive revisions given must be references")); > - else if (rinfo.negative_refexprs > 1) > - die(_("cannot implicitly determine whether this is an --advance or --onto operation")); > - else if (rinfo.positive_refexprs > 1) > - die(_("cannot advance target with multiple source branches because ordering would be ill-defined")); > - > - /* Only one entry, but we have to loop to get it */ > - strset_for_each_entry(&rinfo.negative_refs, > - &iter, entry) { > - last_key = entry->key; > - } > - > - free(*advance_name); > - *advance_name = xstrdup_or_null(last_key); > - } else { /* positive_refs_complete */ > - if (rinfo.negative_refexprs > 1) > - die(_("cannot implicitly determine correct base for --onto")); > - if (rinfo.negative_refexprs == 1) > - *onto = rinfo.onto; > - } > - } > - if (!*advance_name) { > - *update_refs = xcalloc(1, sizeof(**update_refs)); > - **update_refs = rinfo.positive_refs; > - memset(&rinfo.positive_refs, 0, sizeof(**update_refs)); > } > strset_clear(&rinfo.negative_refs); > strset_clear(&rinfo.positive_refs); > } > > @@ -384,12 +345,13 @@ int cmd_replay(int argc, > "'%s' bit in 'struct rev_info' will be forced"), > "simplify_history"); > revs.simplify_history = 0; > } > > - determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name, > - &onto, &update_refs); > + populate_for_onto_or_advance_mode(repo, &revs.cmdline, > + onto_name, &advance_name, > + &onto, &update_refs); > > if (!onto) /* FIXME: Should handle replaying down to root commit */ > die("Replaying down to root commit is not supported yet!"); > > if (prepare_revision_walk(&revs) < 0) { > -- > 2.52.0.10.g08704017180 Looks fine otherwise.