* [PATCH v2 1/5] replay: remove dead code and rearrange
2025-12-30 15:01 ` [PATCH v2 0/5] " kristofferhaugsbakk
@ 2025-12-30 15:01 ` kristofferhaugsbakk
2025-12-30 22:50 ` Elijah Newren
0 siblings, 1 reply; 5+ messages in thread
From: kristofferhaugsbakk @ 2025-12-30 15:01 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, christian.couder, newren, Siddharth Asthana,
Phillip Wood
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/5] replay: remove dead code and rearrange
2025-12-30 15:01 ` [PATCH v2 1/5] replay: remove dead code and rearrange kristofferhaugsbakk
@ 2025-12-30 22:50 ` Elijah Newren
2025-12-30 23:37 ` Junio C Hamano
2026-01-02 9:51 ` Kristoffer Haugsbakk
0 siblings, 2 replies; 5+ messages in thread
From: Elijah Newren @ 2025-12-30 22:50 UTC (permalink / raw)
To: kristofferhaugsbakk
Cc: git, Kristoffer Haugsbakk, christian.couder, Siddharth Asthana,
Phillip Wood
On Tue, Dec 30, 2025 at 7:03 AM <kristofferhaugsbakk@fastmail•com> wrote:
>
> 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)
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/5] replay: remove dead code and rearrange
2025-12-30 22:50 ` Elijah Newren
@ 2025-12-30 23:37 ` Junio C Hamano
2026-01-02 9:51 ` Kristoffer Haugsbakk
1 sibling, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2025-12-30 23:37 UTC (permalink / raw)
To: Elijah Newren
Cc: kristofferhaugsbakk, git, Kristoffer Haugsbakk, christian.couder,
Siddharth Asthana, Phillip Wood
> > -static void determine_replay_mode(struct repository *repo,
> > ...
> > +static void populate_for_onto_or_advance_mode(struct repository *repo,
> > ...
> 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?
After reading the above, the name that came to my mind is (curiously)
determine_replay_mode() ;-).
> Looks fine otherwise.
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/5] replay: remove dead code and rearrange
2025-12-30 22:50 ` Elijah Newren
2025-12-30 23:37 ` Junio C Hamano
@ 2026-01-02 9:51 ` Kristoffer Haugsbakk
1 sibling, 0 replies; 5+ messages in thread
From: Kristoffer Haugsbakk @ 2026-01-02 9:51 UTC (permalink / raw)
To: Elijah Newren, Kristoffer Haugsbakk
Cc: git, Christian Couder, Siddharth Asthana, Phillip Wood
On Tue, Dec 30, 2025, at 23:50, Elijah Newren wrote:
> On Tue, Dec 30, 2025 at 7:03 AM <kristofferhaugsbakk@fastmail•com> wrote:
>>[snip]
>> -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?
Yeah, it is a mouthful.
I can use `set_up_replay_mode`.
>>[snip]
>
> Looks fine otherwise.
Thanks for reviewing this round!
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/5] replay: remove dead code and rearrange
@ 2026-01-28 0:04 Hassan
0 siblings, 0 replies; 5+ messages in thread
From: Hassan @ 2026-01-28 0:04 UTC (permalink / raw)
To: gitster
Cc: christian.couder, code, git, kristofferhaugsbakk, newren,
phillip.wood, siddharthasthana31
Vodafone cash
Sent from my iPhone
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-01-28 0:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-28 0:04 [PATCH v2 1/5] replay: remove dead code and rearrange Hassan
-- strict thread matches above, loose matches on Subject: below --
2025-12-22 22:04 [PATCH 0/2] replay: die descriptively when invalid commit-ish kristofferhaugsbakk
2025-12-30 15:01 ` [PATCH v2 0/5] " kristofferhaugsbakk
2025-12-30 15:01 ` [PATCH v2 1/5] replay: remove dead code and rearrange kristofferhaugsbakk
2025-12-30 22:50 ` Elijah Newren
2025-12-30 23:37 ` Junio C Hamano
2026-01-02 9:51 ` Kristoffer Haugsbakk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox