Re: [PATCH v2 1/5] replay: remove dead code and rearrange

>
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> 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 <newren@gmail.com>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>
> 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)

>  {
>         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