rebase: make reflog messages independent of the backend
cover rebase: make reflog messages independent of the backend
This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.
* improve rebase reflog test coverage
* rebase --merge: fix reflog messages for --continue and --skip
* rebase --apply: respect GIT_REFLOG_ACTION
* rebase --abort: improve reflog message
* unify reflog messages between the two rebase backends
This series is based on pw/use-inprocess-checkout-in-rebase
Phillip Wood (7):
rebase --apply: remove duplicated code
rebase --merge: fix reflog when continuing rebase --merge: fix reflog message after skipping rebase --apply: respect GIT_REFLOG_ACTION
rebase --apply: make reflog messages match rebase --merge rebase --abort: improve reflog message
rebase: cleanup action handling
builtin/rebase.c | 144 ++++++++++++----------------- sequencer.c | 5 ++ t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++------- 3 files changed, 214 insertions(+), 120 deletions(-)
base-commit: 38c541ce94048cf72aa4f465be9314423a57f445
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Signature · 1 line
gitgitgadget
PATCH 1/7 rebase --apply: remove duplicated code
From: Phillip Wood <[email protected]>
When we are reattaching HEAD after a fast-forward we can use move_to_original_branch() rather than open coding a copy of that code. The old code appears to handle the case where the rebase is started from a detached HEAD but in fact in that case there is nothing to do as we have already updated HEAD.
Note that the removal of "strbuf_release(&msg)" is safe as there is an identical call just above this hunk.
--- builtin/rebase.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
builtin/rebase.c +1 -10
@@ -1782,19 +1782,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)* If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ -strbuf_reset (&msg);if (oideq (&merge_base, &options.orig_head)) {printf (_ ("Fast-forwarded %s to %s.\n" ), branch_name, options.onto_name); -strbuf_addf (&msg,"rebase finished: %s onto %s" , - options.head_name ? options.head_name :"detached HEAD" , -oid_to_hex (&options.onto->object.oid)); -memset (&ropts,0 ,sizeof (ropts)); - ropts.branch = options.head_name; - ropts.flags =RESET_HEAD_REFS_ONLY ; - ropts.head_msg = msg.buf; -reset_head (the_repository, &ropts); -strbuf_release (&msg); +move_to_original_branch (&options); ret =finish_rebase (&options); goto cleanup; }
Signature · 1 line
gitgitgadget
Re: [PATCH 1/7] rebase --apply: remove duplicated code
Quoted 6 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
When we are reattaching HEAD after a fast-forward we can use
move_to_original_branch()rather than open coding a copy of that code.
Nit: maybe "rebase --apply: reuse move_to_original_branch()" would be a better subject for this patch, as it's more specific.
Quoted 6 lines
The old code appears to handle the case where the rebase is started from a detached HEAD but in fact in that case there is nothing to do as we have already updated HEAD.
Note that the removal of "
strbuf_release(&msg)" is safe as there is an identical call just above this hunk.
It looks like the removed call is using strbuf_reset(), not strbuf_release().
Actually a call to strbuf_release(&msg) is also removed inside the if (oideq(&merge_base, &options.orig_head)) { ... } clause, but it doesn't look like you are talking about this one.
Quoted 10 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)builtin/rebase.c +0
-0
builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1785
Quoted 4 lines
builtin/rebase.c +0
-1@@ -1782,19 +1782,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)* If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ -strbuf_reset (&msg);
Yeah, it looks like msg isn't used in the function anymore so this call can be removed.
Quoted 6 lines
builtin/rebase.c +0
-3if (oideq (&merge_base, &options.orig_head)) {printf (_ ("Fast-forwarded %s to %s.\n" ), branch_name, options.onto_name); -strbuf_addf (&msg,"rebase finished: %s onto %s" , - options.head_name ? options.head_name :"detached HEAD" , -oid_to_hex (&options.onto->object.oid));
builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1791
Quoted 10 lines
builtin/rebase.c +1
-6-memset (&ropts,0 ,sizeof (ropts)); - ropts.branch = options.head_name; - ropts.flags =RESET_HEAD_REFS_ONLY ; - ropts.head_msg = msg.buf; -reset_head (the_repository, &ropts); -strbuf_release (&msg); +move_to_original_branch (&options); ret =finish_rebase (&options); goto cleanup; }
Nice cleanup!
Re: [PATCH 1/7] rebase --apply: remove duplicated code
Quoted 17 lines · Christian Couder
Quoted 6 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
When we are reattaching HEAD after a fast-forward we can use
move_to_original_branch()rather than open coding a copy of that code.Nit: maybe "rebase --apply: reuse
move_to_original_branch()" would be a better subject for this patch, as it's more specific.Quoted 4 lines
The old code appears to handle the case where the rebase is started from a detached HEAD but in fact in that case there is nothing to do as we have already updated HEAD.
Quoted 8 lines
Quoted 2 lines
Note that the removal of "
strbuf_release(&msg)" is safe as there is an identical call just above this hunk.It looks like the removed call is using
strbuf_reset(), notstrbuf_release().Actually a call to
strbuf_release(&msg)is also removed inside theif (oideq(&merge_base, &options.orig_head)) { ... }clause, but it doesn't look like you are talking about this one.
I think he is talking about that one and specifying why it's okay to drop that one, and omitted why it was okay to drop the strbuf_reset() because that one seemed more obvious.
Quoted 35 lines
Quoted 10 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)builtin/rebase.c +0
-0builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1785
Quoted 4 lines
builtin/rebase.c +0
-1@@ -1782,19 +1782,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)* If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ -strbuf_reset (&msg);Yeah, it looks like
msgisn't used in the function anymore so this call can be removed.Quoted 6 lines
builtin/rebase.c +0
-3if (oideq (&merge_base, &options.orig_head)) {printf (_ ("Fast-forwarded %s to %s.\n" ), branch_name, options.onto_name); -strbuf_addf (&msg,"rebase finished: %s onto %s" , - options.head_name ? options.head_name :"detached HEAD" , -oid_to_hex (&options.onto->object.oid));builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1791
Quoted 10 lines
builtin/rebase.c +1
-6-memset (&ropts,0 ,sizeof (ropts)); - ropts.branch = options.head_name; - ropts.flags =RESET_HEAD_REFS_ONLY ; - ropts.head_msg = msg.buf; -reset_head (the_repository, &ropts); -strbuf_release (&msg); +move_to_original_branch (&options); ret =finish_rebase (&options); goto cleanup; }
Quoted 1 line
Nice cleanup!
Agreed.
PATCH 2/7 rebase --merge: fix reflog when continuing
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The reflog message for a conflict resolution committed by "rebase --continue" looks like
rebase (continue): commit subject line
Unfortunately the reflog message each subsequent pick look like
rebase (continue) (pick): commit subject line
Fix this by setting the reflog message for "rebase --continue" in sequencer_continue() so it does not affect subsequent commits. This introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION in pick_commits(). Both of these will be fixed in a future series that stops the sequencer calling setenv().
--- builtin/rebase.c | 2 - sequencer.c | 5 ++ t/t3406-rebase-message.sh | 120 +++++++++++++++++++++++++------------- 3 files changed, 86 insertions(+), 41 deletions(-)
builtin/rebase.c +0 -2
@@ -1247,8 +1247,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)int fd; options.action ="continue" ; -set_reflog_action (&options); -/* Sanity check */ if (get_oid ("HEAD" , &head))die (_ ("Cannot read HEAD" ));
sequencer.c +5 -0
@@ -4777,6 +4777,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)if (read_populate_opts (opts))return -1 ;if (is_rebase_i (opts)) { +char *previous_reflog_action; +if ((res =read_populate_todo (r, &todo_list, opts))) goto release_todo_list;
@@ -4787,10 +4789,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)unlink (rebase_path_dropped ()); } + previous_reflog_action =xstrdup (getenv (GIT_REFLOG_ACTION )); +setenv (GIT_REFLOG_ACTION ,reflog_message (opts,"continue" ,NULL ),1 );if (commit_staged_changes (r, opts, &todo_list)) { res =-1 ; goto release_todo_list; } +setenv (GIT_REFLOG_ACTION , previous_reflog_action,1 ); }else if (!file_exists (get_todo_path (opts)))return continue_single_pick (r, opts);else if ((res =read_populate_todo (r, &todo_list, opts)))
t/t3406-rebase-message.sh +81 -39
145 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH 2/7] rebase --merge: fix reflog when continuing
Quoted 10 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
The reflog message for a conflict resolution committed by "rebase --continue" looks like
rebase (continue): commit subject line
Unfortunately the reflog message each subsequent pick look like
Quoted 7 lines
rebase (continue) (pick): commit subject line
Fix this by setting the reflog message for "rebase --continue" in
sequencer_continue()so it does not affect subsequent commits. This introduces a memory leak similar to the one leakingGIT_REFLOG_ACTIONinpick_commits(). Both of these will be fixed in a future series that stops the sequencer callingsetenv().
Yeah, it looks like we will leak only a small string.
Quoted 5 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 2 - sequencer.c | 5 ++ t/t3406-rebase-message.sh | 120 +++++++++++++++++++++++++-------------
The changes to the test script look a bit involved and aren't explained in the commit message. I wonder if some of those changes could have been made in a preparatory commit.
Quoted 1 line
3 files changed, 86 insertions(+), 41 deletions(-)
Re: [PATCH 2/7] rebase --merge: fix reflog when continuing
Hi Chirstian
Thanks for taking a look at this series
Quoted 28 lines · Christian Couder
Quoted 10 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
The reflog message for a conflict resolution committed by "rebase --continue" looks like
rebase (continue): commit subject line
Unfortunately the reflog message each subsequent pick look like
Quoted 7 lines
rebase (continue) (pick): commit subject line
Fix this by setting the reflog message for "rebase --continue" in
sequencer_continue()so it does not affect subsequent commits. This introduces a memory leak similar to the one leakingGIT_REFLOG_ACTIONinpick_commits(). Both of these will be fixed in a future series that stops the sequencer callingsetenv().Yeah, it looks like we will leak only a small string.
Quoted 5 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 2 - sequencer.c | 5 ++ t/t3406-rebase-message.sh | 120 +++++++++++++++++++++++++-------------
Quoted 3 lines
The changes to the test script look a bit involved and aren't explained in the commit message. I wonder if some of those changes could have been made in a preparatory commit.
That's a good point. For some reason when I put the series together I thought it would be tricky to do that without the fixes in this commit but that is not actually the case so I'll split the test changes out.
Best Wishes
Phillip
Quoted 2 lines
Quoted 2 lines
3 files changed, 86 insertions(+), 41 deletions(-)
Re: [PATCH 2/7] rebase --merge: fix reflog when continuing
Quoted 34 lines · Phillip Wood
Hi Chirstian
Thanks for taking a look at this series
Quoted 28 lines · Christian Couder
Quoted 10 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
The reflog message for a conflict resolution committed by "rebase --continue" looks like
rebase (continue): commit subject line
Unfortunately the reflog message each subsequent pick look like
Quoted 7 lines
rebase (continue) (pick): commit subject line
Fix this by setting the reflog message for "rebase --continue" in
sequencer_continue()so it does not affect subsequent commits. This introduces a memory leak similar to the one leakingGIT_REFLOG_ACTIONinpick_commits(). Both of these will be fixed in a future series that stops the sequencer callingsetenv().Yeah, it looks like we will leak only a small string.
Quoted 5 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 2 - sequencer.c | 5 ++ t/t3406-rebase-message.sh | 120 +++++++++++++++++++++++++-------------
Quoted 7 lines
Quoted 3 lines
The changes to the test script look a bit involved and aren't explained in the commit message. I wonder if some of those changes could have been made in a preparatory commit.
That's a good point. For some reason when I put the series together I thought it would be tricky to do that without the fixes in this commit but that is not actually the case so I'll split the test changes out.
That would be very nice; I'd like to see it split out too.
PATCH 3/7 rebase --merge: fix reflog message after skipping
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The reflog message for every pick after running "rebase --skip" looks like
rebase (skip) (pick): commit subject line
Fix this by not appending " (skip)" to the reflog action.
--- builtin/rebase.c | 2 -- t/t3406-rebase-message.sh | 24 ++++++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-)
builtin/rebase.c +0 -2
@@ -1273,8 +1273,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct string_list merge_rr =STRING_LIST_INIT_DUP ; options.action ="skip" ; -set_reflog_action (&options); -rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 ); ropts.flags =RESET_HEAD_HARD ;
t/t3406-rebase-message.sh +24 -0
@@ -163,6 +163,30 @@ test_reflog () { # check there is only one new entry in the branch reflog test_cmp_rev fast-forward@{1} X ' + + test_expect_success "rebase $mode --skip reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout conflicts && + test_when_finished "git reset --hard Q" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + test_must_fail git rebase $mode main && + git rebase --skip + ) && + + git log -g --format=%gs -4 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts + ${reflog_action:-rebase} (pick): Q + ${reflog_action:-rebase} (pick): P + ${reflog_action:-rebase} (start): checkout main + EOF + test_cmp expect actual + ' } test_reflog --merge
Signature · 1 line
gitgitgadget
Re: [PATCH 3/7] rebase --merge: fix reflog message after skipping
Quoted 9 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
The reflog message for every pick after running "rebase --skip" looks like
rebase (skip) (pick): commit subject line
Fix this by not appending " (skip)" to the reflog action.
Nice catch, and cool that the fix was so simple.
Quoted 57 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 2 -- t/t3406-rebase-message.sh | 24 ++++++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-)builtin/rebase.c +0
-2@@ -1273,8 +1273,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct string_list merge_rr =STRING_LIST_INIT_DUP ; options.action ="skip" ; -set_reflog_action (&options); -rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 ); ropts.flags =RESET_HEAD_HARD ;t/t3406-rebase-message.sh +24
-1@@ -163,6 +163,30 @@ test_reflog () { # check there is only one new entry in the branch reflog test_cmp_rev fast-forward@{1} X ' + + test_expect_success "rebase $mode --skip reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout conflicts && + test_when_finished "git reset --hard Q" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + test_must_fail git rebase $mode main && + git rebase --skip + ) && + + git log -g --format=%gs -4 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts + ${reflog_action:-rebase} (pick): Q + ${reflog_action:-rebase} (pick): P + ${reflog_action:-rebase} (start): checkout main + EOF + test_cmp expect actual + ' } test_reflog --merge --gitgitgadget
PATCH 4/7 rebase --apply: respect GIT_REFLOG_ACTION
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The reflog messages when finishing a rebase hard code "rebase" rather than using GIT_REFLOG_ACTION.
--- builtin/rebase.c | 7 ++++--- t/t3406-rebase-message.sh | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)
builtin/rebase.c +4 -3
@@ -580,10 +580,11 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"rebase finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid)); -strbuf_addf (&head_reflog,"rebase finished: returning to %s" , - opts->head_name); +strbuf_addf (&head_reflog,"%s finished: returning to %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name); ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ; ropts.branch_msg = branch_reflog.buf;
t/t3406-rebase-message.sh +1 -1
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' write_reflog_expect () { if test $mode = --apply then - sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//' + sed 's/(finish)/finished/; s/ ([^)]*)//' else cat fi >expect
Signature · 1 line
gitgitgadget
Re: [PATCH 4/7] rebase --apply: respect GIT_REFLOG_ACTION
Quoted 5 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
The reflog messages when finishing a rebase hard code "rebase" rather than using
GIT_REFLOG_ACTION.
Yeah, but GIT_REFLOG_ACTION is something like "pick" or "continue" ...
Quoted 11 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 7 ++++--- t/t3406-rebase-message.sh | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)builtin/rebase.c +0
-0
builtin/rebase.c static int move_to_original_branch(struct rebase_options *opts) · line 585
Quoted 7 lines
builtin/rebase.c +2
-1@@ -580,10 +580,11 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"rebase finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid));
... so this will say "pick finished..." instead of "rebase finished..." while it may be better to have something like "rebase (pick) finished...", or am I missing something?
builtin/rebase.c static int move_to_original_branch(struct rebase_options *opts) · line 587
Quoted 4 lines
builtin/rebase.c +2
-2-strbuf_addf (&head_reflog,"rebase finished: returning to %s" , - opts->head_name); +strbuf_addf (&head_reflog,"%s finished: returning to %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name);
In patch 2/7 GIT_REFLOG_ACTION, instead ofGIT_REFLOG_ACTION_ENVIRONMENT, is used.
It looks like GIT_REFLOG_ACTION_ENVIRONMENT is defined in reset.h while GIT_REFLOG_ACTION is defined in sequencer.c. Maybe we could get rid of one of these two, and use the same everywhere?
Quoted 3 lines
builtin/rebase.c +0
-0ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ; ropts.branch_msg = branch_reflog.buf;
Re: [PATCH 4/7] rebase --apply: respect GIT_REFLOG_ACTION
Hi Christian
Quoted 9 lines · Christian Couder
Quoted 5 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
The reflog messages when finishing a rebase hard code "rebase" rather than using
GIT_REFLOG_ACTION.Yeah, but
GIT_REFLOG_ACTIONis something like "pick" or "continue" ...
GIT_REFLOG_ACTION can be set in the environment by the user (or more likely a script) to change the command name in reflog messages created by rebase. At the moment we respect that for all the messages apart from the ones being changed here.
Best Wishes
Phillip
Quoted 38 lines
Quoted 11 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 7 ++++--- t/t3406-rebase-message.sh | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)builtin/rebase.c +0
-0builtin/rebase.c static int move_to_original_branch(struct rebase_options *opts) · line 585
Quoted 7 lines
builtin/rebase.c +2
-1@@ -580,10 +580,11 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"rebase finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid));... so this will say "pick finished..." instead of "rebase finished..." while it may be better to have something like "rebase (pick) finished...", or am I missing something?
builtin/rebase.c static int move_to_original_branch(struct rebase_options *opts) · line 587
Quoted 4 lines
builtin/rebase.c +2
-2-strbuf_addf (&head_reflog,"rebase finished: returning to %s" , - opts->head_name); +strbuf_addf (&head_reflog,"%s finished: returning to %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name);In patch 2/7
GIT_REFLOG_ACTION, instead ofGIT_REFLOG_ACTION_ENVIRONMENT, is used.It looks like
GIT_REFLOG_ACTION_ENVIRONMENTis defined inreset.hwhileGIT_REFLOG_ACTIONis defined insequencer.c. Maybe we could get rid of one of these two, and use the same everywhere?Quoted 3 lines
builtin/rebase.c +0
-0ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ; ropts.branch_msg = branch_reflog.buf;
PATCH 5/7 rebase --apply: make reflog messages match rebase --merge
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The apply backend creates slightly different reflog messages to the merge backend when starting and finishing a rebase and when picking commits. The choice of backend is really an implementation detail so it is confusing to have the same command create different messages depending on which backend is selected. Change the apply backend so the reflog messages from the two backends match as closely as possible. Note that there is still a difference when committing a conflict resolution - the merge backend will use "(continue)" rather than "(pick)" in that case as it does not know which command created the conflict that it is committing.
--- builtin/rebase.c | 9 +++++---- t/t3406-rebase-message.sh | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-)
builtin/rebase.c +5 -4
@@ -580,10 +580,10 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s (finish): %s onto %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid)); -strbuf_addf (&head_reflog,"%s finished: returning to %s" , +strbuf_addf (&head_reflog,"%s (finish): returning to %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name); ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ;
@@ -613,7 +613,8 @@ static int run_am(struct rebase_options *opts) am.git_cmd =1 ;strvec_push (&am.args,"am" ); - +strvec_pushf (&am.env_array,GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ));if (opts->action && !strcmp ("continue" , opts->action)) {strvec_push (&am.args,"--resolved" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg);
@@ -1763,7 +1764,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)printf (_ ("First, rewinding head to replay your work on top of " "it...\n" )); -strbuf_addf (&msg,"%s: checkout %s" , +strbuf_addf (&msg,"%s (start): checkout %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), options.onto_name); ropts.oid = &options.onto->object.oid; ropts.orig_head = &options.orig_head,
t/t3406-rebase-message.sh +1 -1
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' write_reflog_expect () { if test $mode = --apply then - sed 's/(finish)/finished/; s/ ([^)]*)//' + sed 's/(continue)/(pick)/' else cat fi >expect
Signature · 1 line
gitgitgadget
Re: [PATCH 5/7] rebase --apply: make reflog messages match rebase --merge
Quoted 13 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
The apply backend creates slightly different reflog messages to the merge backend when starting and finishing a rebase and when picking commits. The choice of backend is really an implementation detail so it is confusing to have the same command create different messages depending on which backend is selected. Change the apply backend so the reflog messages from the two backends match as closely as possible. Note that there is still a difference when committing a conflict resolution - the merge backend will use "(continue)" rather than "(pick)" in that case as it does not know which command created the conflict that it is committing.
I've always been slightly annoyed by the various differences in these backends; it's nice to see another one disappearing.
Quoted 25 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 9 +++++---- t/t3406-rebase-message.sh | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-)builtin/rebase.c +2
-2@@ -580,10 +580,10 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s (finish): %s onto %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid)); -strbuf_addf (&head_reflog,"%s finished: returning to %s" , +strbuf_addf (&head_reflog,"%s (finish): returning to %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name); ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ;
builtin/rebase.c static int run_am(struct rebase_options *opts) · line 617
Quoted 6 lines
builtin/rebase.c +2
-1@@ -613,7 +613,8 @@ static int run_am(struct rebase_options *opts) am.git_cmd =1 ;strvec_push (&am.args,"am" ); - +strvec_pushf (&am.env_array,GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ));
Kinda sad that we're still forking subprocesses and passing things via environment variable, but that's outside the scope of this series. Fix looks fine.
Quoted 27 lines
builtin/rebase.c +1
-1if (opts->action && !strcmp ("continue" , opts->action)) {strvec_push (&am.args,"--resolved" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg);@@ -1763,7 +1764,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)printf (_ ("First, rewinding head to replay your work on top of " "it...\n" )); -strbuf_addf (&msg,"%s: checkout %s" , +strbuf_addf (&msg,"%s (start): checkout %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), options.onto_name); ropts.oid = &options.onto->object.oid; ropts.orig_head = &options.orig_head,t/t3406-rebase-message.sh +1
-2@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' write_reflog_expect () { if test $mode = --apply then - sed 's/(finish)/finished/; s/ ([^)]*)//' + sed 's/(continue)/(pick)/' else cat fi >expect --gitgitgadget
PATCH 6/7 rebase --abort: improve reflog message
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
When aborting a rebase the reflog message looks like
rebase (abort): updating HEAD
which is not very informative. Improve the message by mentioning the branch that we are returning to as we do at the end of a successful rebase so it looks like.
rebase (abort): returning to refs/heads/topic
If GIT_REFLOG_ACTION is set in the environment we no longer omit "(abort)" from the reflog message. We don't omit "(start)" and "(finish)" when starting and finishing a rebase in that case so we shouldn't omit "(abort)".
--- builtin/rebase.c | 28 ++++++--------------- t/t3406-rebase-message.sh | 51 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 20 deletions(-)
builtin/rebase.c +8 -20
@@ -992,23 +992,6 @@ static void NORETURN error_on_missing_default_upstream(void)exit (1 ); } -static void set_reflog_action (struct rebase_options *options) -{ -const char *env; -struct strbuf buf =STRBUF_INIT ; - -if (!is_merge (options)) -return ; - - env =getenv (GIT_REFLOG_ACTION_ENVIRONMENT ); -if (env &&strcmp ("rebase" , env)) -return ;/* only override it if it is "rebase" */ - -strbuf_addf (&buf,"rebase (%s)" , options->action); -setenv (GIT_REFLOG_ACTION_ENVIRONMENT , buf.buf,1 ); -strbuf_release (&buf); -} -static int check_exec_cmd (const char *cmd) {if (strchr (cmd,'\n' ))
@@ -1287,18 +1270,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }case ACTION_ABORT : {struct string_list merge_rr =STRING_LIST_INIT_DUP ; - options.action ="abort" ; -set_reflog_action (&options); +struct strbuf head_msg =STRBUF_INIT ; + options.action ="abort" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 );if (read_basic_state (&options))exit (1 ); + +strbuf_addf (&head_msg,"%s (abort): returning to %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), + options.head_name ? options.head_name + :oid_to_hex (&options.orig_head)); ropts.oid = &options.orig_head; + ropts.head_msg = head_msg.buf; ropts.branch = options.head_name; ropts.flags =RESET_HEAD_HARD ; - ropts.default_reflog_action =DEFAULT_REFLOG_ACTION ;if (reset_head (the_repository, &ropts) <0 )die (_ ("could not move back to %s" ),oid_to_hex (&options.orig_head));
t/t3406-rebase-message.sh +51 -0
57 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH 6/7] rebase --abort: improve reflog message
Quoted 5 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
When aborting a rebase the reflog message looks like
Quoted 7 lines
rebase (abort): updating HEAD
which is not very informative. Improve the message by mentioning the branch that we are returning to as we do at the end of a successful rebase so it looks like.
rebase (abort): returning to refs/heads/topic
That is _so_ much nicer.
Quoted 4 lines
If
GIT_REFLOG_ACTIONis set in the environment we no longer omit "(abort)" from the reflog message. We don't omit "(start)" and "(finish)" when starting and finishing a rebase in that case so we shouldn't omit "(abort)".
I agree.
Quoted 82 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 28 ++++++--------------- t/t3406-rebase-message.sh | 51 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 20 deletions(-)builtin/rebase.c +8
-20@@ -992,23 +992,6 @@ static void NORETURN error_on_missing_default_upstream(void)exit (1 ); } -static void set_reflog_action (struct rebase_options *options) -{ -const char *env; -struct strbuf buf =STRBUF_INIT ; - -if (!is_merge (options)) -return ; - - env =getenv (GIT_REFLOG_ACTION_ENVIRONMENT ); -if (env &&strcmp ("rebase" , env)) -return ;/* only override it if it is "rebase" */ - -strbuf_addf (&buf,"rebase (%s)" , options->action); -setenv (GIT_REFLOG_ACTION_ENVIRONMENT , buf.buf,1 ); -strbuf_release (&buf); -} -static int check_exec_cmd (const char *cmd) {if (strchr (cmd,'\n' ))@@ -1287,18 +1270,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }case ACTION_ABORT : {struct string_list merge_rr =STRING_LIST_INIT_DUP ; - options.action ="abort" ; -set_reflog_action (&options); +struct strbuf head_msg =STRBUF_INIT ; + options.action ="abort" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 );if (read_basic_state (&options))exit (1 ); + +strbuf_addf (&head_msg,"%s (abort): returning to %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), + options.head_name ? options.head_name + :oid_to_hex (&options.orig_head)); ropts.oid = &options.orig_head; + ropts.head_msg = head_msg.buf; ropts.branch = options.head_name; ropts.flags =RESET_HEAD_HARD ; - ropts.default_reflog_action =DEFAULT_REFLOG_ACTION ;if (reset_head (the_repository, &ropts) <0 )die (_ ("could not move back to %s" ),oid_to_hex (&options.orig_head));t/t3406-rebase-message.sh +13
-0@@ -187,6 +187,57 @@ test_reflog () { EOF test_cmp expect actual ' + + test_expect_success "rebase $mode --abort reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout conflicts && + test_when_finished "git reset --hard Q" && + + git log -g -1 conflicts >branch-expect && + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + test_must_fail git rebase $mode main &&
t/t3406-rebase-message.sh test_reflog () { · line 212
Quoted 10 lines
t/t3406-rebase-message.sh +10
-0+ git rebase --abort + ) && + + git log -g --format=%gs -3 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (abort): returning to refs/heads/conflicts + ${reflog_action:-rebase} (pick): P + ${reflog_action:-rebase} (start): checkout main + EOF + test_cmp expect actual &&
I was confused by the ordering at first, before I realized that git log -g is going to give you reflogs from newest to oldest instead of vice-versa. Looks good.
Quoted 19 lines
t/t3406-rebase-message.sh +19
-0+ + # check branch reflog is unchanged + git log -g -1 conflicts >branch-actual && + test_cmp branch-expect branch-actual + ' + + test_expect_success "rebase $mode --abort detached HEAD reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout Q && + test_when_finished "git reset --hard Q" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + test_must_fail git rebase $mode main && + git rebase --abort + ) &&
t/t3406-rebase-message.sh test_reflog () { · line 241
Quoted 10 lines
t/t3406-rebase-message.sh +9
-0+ + git log -g --format=%gs -3 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (abort): returning to $(git rev-parse Q) + ${reflog_action:-rebase} (pick): P + ${reflog_action:-rebase} (start): checkout main + EOF + test_cmp expect actual + ' }
And thanks for checking the detached HEAD case too.
PATCH 7/7 rebase: cleanup action handling
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Treating the action as a string is a hang over from the scripted rebase. The last commit removed the only remaining use of the action that required a string so lets convert the other action users to use the existing action enum instead. If we ever need the action name as a string in the future the action_names array exists exactly for that purpose.
--- builtin/rebase.c | 91 +++++++++++++++++++++++------------------------- 1 file changed, 43 insertions(+), 48 deletions(-)
builtin/rebase.c +43 -48
@@ -58,6 +58,26 @@ enum empty_type {EMPTY_ASK }; +enum action { +ACTION_NONE =0 , +ACTION_CONTINUE , +ACTION_SKIP , +ACTION_ABORT , +ACTION_QUIT , +ACTION_EDIT_TODO , +ACTION_SHOW_CURRENT_PATCH +}; + +static const char *action_names[] = { +"undefined" , +"continue" , +"skip" , +"abort" , +"quit" , +"edit_todo" , +"show_current_patch" +}; +struct rebase_options {enum rebase_type type;enum empty_type empty;
@@ -84,7 +104,7 @@ struct rebase_options {REBASE_INTERACTIVE_EXPLICIT =1 <<4 , } flags;struct strvec git_am_opts; -const char *action; +enum action action;int signoff;int allow_rerere_autoupdate;int keep_empty;
@@ -155,24 +175,6 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)return replay; } -enum action { -ACTION_NONE =0 , -ACTION_CONTINUE , -ACTION_SKIP , -ACTION_ABORT , -ACTION_QUIT , -ACTION_EDIT_TODO , -ACTION_SHOW_CURRENT_PATCH -}; - -static const char *action_names[] = {"undefined" , -"continue" , -"skip" , -"abort" , -"quit" , -"edit_todo" , -"show_current_patch" }; -static int edit_todo_file (unsigned flags) {const char *todo_file =rebase_path_todo ();
@@ -309,8 +311,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)return ret; } -static int run_sequencer_rebase (struct rebase_options *opts, -enum action command) +static int run_sequencer_rebase (struct rebase_options *opts) {unsigned flags =0 ;int abbreviate_commands =0 , ret =0 ;
@@ -325,7 +326,7 @@ static int run_sequencer_rebase(struct rebase_options *opts, flags |= opts->reapply_cherry_picks ?TODO_LIST_REAPPLY_CHERRY_PICKS :0 ; flags |= opts->flags &REBASE_NO_QUIET ?TODO_LIST_WARN_SKIPPED_CHERRY_PICKS :0 ; -switch (command) { +switch (opts->action) {case ACTION_NONE : {if (!opts->onto && !opts->upstream)die (_ ("a base commit must be provided with --upstream or --onto" ));
@@ -358,7 +359,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,break ; }default : -BUG ("invalid command '%d'" , command); +BUG ("invalid command '%d'" , opts->action); }return ret;
@@ -615,7 +616,7 @@ static int run_am(struct rebase_options *opts)strvec_push (&am.args,"am" );strvec_pushf (&am.env_array,GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT )); -if (opts->action && !strcmp ("continue" , opts->action)) { +if (opts->action ==ACTION_CONTINUE ) {strvec_push (&am.args,"--resolved" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg);if (opts->gpg_sign_opt)
@@ -626,7 +627,7 @@ static int run_am(struct rebase_options *opts)return move_to_original_branch (opts); } -if (opts->action && !strcmp ("skip" , opts->action)) { +if (opts->action ==ACTION_SKIP ) {strvec_push (&am.args,"--skip" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); status =run_command (&am);
@@ -635,7 +636,7 @@ static int run_am(struct rebase_options *opts)return move_to_original_branch (opts); } -if (opts->action && !strcmp ("show-current-patch" , opts->action)) { +if (opts->action ==ACTION_SHOW_CURRENT_PATCH ) {strvec_push (&am.args,"--show-current-patch" );return run_command (&am); }
@@ -728,7 +729,7 @@ static int run_am(struct rebase_options *opts)return status; } -static int run_specific_rebase (struct rebase_options *opts,enum action action) +static int run_specific_rebase (struct rebase_options *opts) {int status;
@@ -746,7 +747,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action) opts->gpg_sign_opt = tmp; } - status =run_sequencer_rebase (opts, action); + status =run_sequencer_rebase (opts); }else if (opts->type ==REBASE_APPLY ) status =run_am (opts);else
@@ -1016,7 +1017,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct strbuf buf =STRBUF_INIT ;struct object_id merge_base;int ignore_whitespace =0 ; -enum action action =ACTION_NONE ;const char *gpg_sign =NULL ;struct string_list exec =STRING_LIST_INIT_NODUP ;const char *rebase_merges =NULL ;
@@ -1065,18 +1065,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)OPT_BIT (0 ,"no-ff" , &options.flags,N_ ("cherry-pick all commits, even if unchanged" ),REBASE_FORCE ), -OPT_CMDMODE (0 ,"continue" , &action,N_ ("continue" ), +OPT_CMDMODE (0 ,"continue" , &options.action,N_ ("continue" ),ACTION_CONTINUE ), -OPT_CMDMODE (0 ,"skip" , &action, +OPT_CMDMODE (0 ,"skip" , &options.action,N_ ("skip current patch and continue" ),ACTION_SKIP ), -OPT_CMDMODE (0 ,"abort" , &action, +OPT_CMDMODE (0 ,"abort" , &options.action,N_ ("abort and check out the original branch" ),ACTION_ABORT ), -OPT_CMDMODE (0 ,"quit" , &action, +OPT_CMDMODE (0 ,"quit" , &options.action,N_ ("abort but keep HEAD where it is" ),ACTION_QUIT ), -OPT_CMDMODE (0 ,"edit-todo" , &action,N_ ("edit the todo list " +OPT_CMDMODE (0 ,"edit-todo" , &options.action,N_ ("edit the todo list " "during an interactive rebase" ),ACTION_EDIT_TODO ), -OPT_CMDMODE (0 ,"show-current-patch" , &action, +OPT_CMDMODE (0 ,"show-current-patch" , &options.action,N_ ("show the patch file being applied or merged" ),ACTION_SHOW_CURRENT_PATCH ), OPT_CALLBACK_F(0, "apply", &options, NULL,
@@ -1189,7 +1189,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (preserve_merges_selected)die (_ ("--preserve-merges was replaced by --rebase-merges" )); -if (action !=ACTION_NONE && total_argc !=2 ) { +if (options.action !=ACTION_NONE && total_argc !=2 ) {usage_with_options (builtin_rebase_usage, builtin_rebase_options); }
@@ -1208,11 +1208,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (options.root && options.fork_point >0 )die (_ ("cannot combine '--root' with '--fork-point'" )); -if (action !=ACTION_NONE && !in_progress) +if (options.action !=ACTION_NONE && !in_progress)die (_ ("No rebase in progress?" ));setenv (GIT_REFLOG_ACTION_ENVIRONMENT ,"rebase" ,0 ); -if (action ==ACTION_EDIT_TODO && !is_merge (&options)) +if (options.action ==ACTION_EDIT_TODO && !is_merge (&options))die (_ ("The --edit-todo action can only be used during " "interactive rebase." ));
@@ -1222,16 +1222,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)else if (exec.nr)trace2_cmd_mode ("interactive-exec" );else -trace2_cmd_mode (action_names[action]); +trace2_cmd_mode (action_names[options.action]); } -switch (action) { +switch (options.action) {case ACTION_CONTINUE : {struct object_id head;struct lock_file lock_file =LOCK_INIT ;int fd; - options.action ="continue" ;/* Sanity check */ if (get_oid ("HEAD" , &head))die (_ ("Cannot read HEAD" ));
@@ -1257,7 +1256,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)case ACTION_SKIP : {struct string_list merge_rr =STRING_LIST_INIT_DUP ; - options.action ="skip" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 ); ropts.flags =RESET_HEAD_HARD ;
@@ -1272,7 +1270,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct string_list merge_rr =STRING_LIST_INIT_DUP ;struct strbuf head_msg =STRBUF_INIT ; - options.action ="abort" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 );
@@ -1312,17 +1309,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) goto cleanup; }case ACTION_EDIT_TODO : - options.action ="edit-todo" ; options.dont_finish_rebase =1 ; goto run_rebase;case ACTION_SHOW_CURRENT_PATCH : - options.action ="show-current-patch" ; options.dont_finish_rebase =1 ; goto run_rebase;case ACTION_NONE :break ;default : -BUG ("action: %d" , action); +BUG ("action: %d" , options.action); }/* Make sure no rebase is in progress */
@@ -1346,7 +1341,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }if ((options.flags &REBASE_INTERACTIVE_EXPLICIT ) || - (action !=ACTION_NONE ) || + (options.action !=ACTION_NONE ) || (exec.nr >0 ) || options.autosquash) { allow_preemptive_ff =0 ;
@@ -1786,7 +1781,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.revisions = revisions.buf; run_rebase: - ret =run_specific_rebase (&options, action); + ret =run_specific_rebase (&options); cleanup:strbuf_release (&buf);
Signature · 1 line
gitgitgadget
Re: [PATCH 7/7] rebase: cleanup action handling
Quoted 282 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Treating the action as a string is a hang over from the scripted rebase. The last commit removed the only remaining use of the action that required a string so lets convert the other action users to use the existing action enum instead. If we ever need the action name as a string in the future the
action_namesarray exists exactly for that purpose.Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 91 +++++++++++++++++++++++------------------------- 1 file changed, 43 insertions(+), 48 deletions(-)builtin/rebase.c +43
-49@@ -58,6 +58,26 @@ enum empty_type {EMPTY_ASK }; +enum action { +ACTION_NONE =0 , +ACTION_CONTINUE , +ACTION_SKIP , +ACTION_ABORT , +ACTION_QUIT , +ACTION_EDIT_TODO , +ACTION_SHOW_CURRENT_PATCH +}; + +static const char *action_names[] = { +"undefined" , +"continue" , +"skip" , +"abort" , +"quit" , +"edit_todo" , +"show_current_patch" +}; +struct rebase_options {enum rebase_type type;enum empty_type empty;@@ -84,7 +104,7 @@ struct rebase_options {REBASE_INTERACTIVE_EXPLICIT =1 <<4 , } flags;struct strvec git_am_opts; -const char *action; +enum action action;int signoff;int allow_rerere_autoupdate;int keep_empty;@@ -155,24 +175,6 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)return replay; } -enum action { -ACTION_NONE =0 , -ACTION_CONTINUE , -ACTION_SKIP , -ACTION_ABORT , -ACTION_QUIT , -ACTION_EDIT_TODO , -ACTION_SHOW_CURRENT_PATCH -}; - -static const char *action_names[] = {"undefined" , -"continue" , -"skip" , -"abort" , -"quit" , -"edit_todo" , -"show_current_patch" }; -static int edit_todo_file (unsigned flags) {const char *todo_file =rebase_path_todo ();@@ -309,8 +311,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)return ret; } -static int run_sequencer_rebase (struct rebase_options *opts, -enum action command) +static int run_sequencer_rebase (struct rebase_options *opts) {unsigned flags =0 ;int abbreviate_commands =0 , ret =0 ;@@ -325,7 +326,7 @@ static int run_sequencer_rebase(struct rebase_options *opts, flags |= opts->reapply_cherry_picks ?TODO_LIST_REAPPLY_CHERRY_PICKS :0 ; flags |= opts->flags &REBASE_NO_QUIET ?TODO_LIST_WARN_SKIPPED_CHERRY_PICKS :0 ; -switch (command) { +switch (opts->action) {case ACTION_NONE : {if (!opts->onto && !opts->upstream)die (_ ("a base commit must be provided with --upstream or --onto" ));@@ -358,7 +359,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,break ; }default : -BUG ("invalid command '%d'" , command); +BUG ("invalid command '%d'" , opts->action); }return ret;@@ -615,7 +616,7 @@ static int run_am(struct rebase_options *opts)strvec_push (&am.args,"am" );strvec_pushf (&am.env_array,GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT )); -if (opts->action && !strcmp ("continue" , opts->action)) { +if (opts->action ==ACTION_CONTINUE ) {strvec_push (&am.args,"--resolved" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg);if (opts->gpg_sign_opt)@@ -626,7 +627,7 @@ static int run_am(struct rebase_options *opts)return move_to_original_branch (opts); } -if (opts->action && !strcmp ("skip" , opts->action)) { +if (opts->action ==ACTION_SKIP ) {strvec_push (&am.args,"--skip" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); status =run_command (&am);@@ -635,7 +636,7 @@ static int run_am(struct rebase_options *opts)return move_to_original_branch (opts); } -if (opts->action && !strcmp ("show-current-patch" , opts->action)) { +if (opts->action ==ACTION_SHOW_CURRENT_PATCH ) {strvec_push (&am.args,"--show-current-patch" );return run_command (&am); }@@ -728,7 +729,7 @@ static int run_am(struct rebase_options *opts)return status; } -static int run_specific_rebase (struct rebase_options *opts,enum action action) +static int run_specific_rebase (struct rebase_options *opts) {int status;@@ -746,7 +747,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action) opts->gpg_sign_opt = tmp; } - status =run_sequencer_rebase (opts, action); + status =run_sequencer_rebase (opts); }else if (opts->type ==REBASE_APPLY ) status =run_am (opts);else @@ -1016,7 +1017,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct strbuf buf =STRBUF_INIT ;struct object_id merge_base;int ignore_whitespace =0 ; -enum action action =ACTION_NONE ;const char *gpg_sign =NULL ;struct string_list exec =STRING_LIST_INIT_NODUP ;const char *rebase_merges =NULL ;@@ -1065,18 +1065,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)OPT_BIT (0 ,"no-ff" , &options.flags,N_ ("cherry-pick all commits, even if unchanged" ),REBASE_FORCE ), -OPT_CMDMODE (0 ,"continue" , &action,N_ ("continue" ), +OPT_CMDMODE (0 ,"continue" , &options.action,N_ ("continue" ),ACTION_CONTINUE ), -OPT_CMDMODE (0 ,"skip" , &action, +OPT_CMDMODE (0 ,"skip" , &options.action,N_ ("skip current patch and continue" ),ACTION_SKIP ), -OPT_CMDMODE (0 ,"abort" , &action, +OPT_CMDMODE (0 ,"abort" , &options.action,N_ ("abort and check out the original branch" ),ACTION_ABORT ), -OPT_CMDMODE (0 ,"quit" , &action, +OPT_CMDMODE (0 ,"quit" , &options.action,N_ ("abort but keep HEAD where it is" ),ACTION_QUIT ), -OPT_CMDMODE (0 ,"edit-todo" , &action,N_ ("edit the todo list " +OPT_CMDMODE (0 ,"edit-todo" , &options.action,N_ ("edit the todo list " "during an interactive rebase" ),ACTION_EDIT_TODO ), -OPT_CMDMODE (0 ,"show-current-patch" , &action, +OPT_CMDMODE (0 ,"show-current-patch" , &options.action,N_ ("show the patch file being applied or merged" ),ACTION_SHOW_CURRENT_PATCH ), OPT_CALLBACK_F(0, "apply", &options, NULL,@@ -1189,7 +1189,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (preserve_merges_selected)die (_ ("--preserve-merges was replaced by --rebase-merges" )); -if (action !=ACTION_NONE && total_argc !=2 ) { +if (options.action !=ACTION_NONE && total_argc !=2 ) {usage_with_options (builtin_rebase_usage, builtin_rebase_options); }@@ -1208,11 +1208,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (options.root && options.fork_point >0 )die (_ ("cannot combine '--root' with '--fork-point'" )); -if (action !=ACTION_NONE && !in_progress) +if (options.action !=ACTION_NONE && !in_progress)die (_ ("No rebase in progress?" ));setenv (GIT_REFLOG_ACTION_ENVIRONMENT ,"rebase" ,0 ); -if (action ==ACTION_EDIT_TODO && !is_merge (&options)) +if (options.action ==ACTION_EDIT_TODO && !is_merge (&options))die (_ ("The --edit-todo action can only be used during " "interactive rebase." ));@@ -1222,16 +1222,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)else if (exec.nr)trace2_cmd_mode ("interactive-exec" );else -trace2_cmd_mode (action_names[action]); +trace2_cmd_mode (action_names[options.action]); } -switch (action) { +switch (options.action) {case ACTION_CONTINUE : {struct object_id head;struct lock_file lock_file =LOCK_INIT ;int fd; - options.action ="continue" ;/* Sanity check */ if (get_oid ("HEAD" , &head))die (_ ("Cannot read HEAD" ));@@ -1257,7 +1256,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)case ACTION_SKIP : {struct string_list merge_rr =STRING_LIST_INIT_DUP ; - options.action ="skip" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 ); ropts.flags =RESET_HEAD_HARD ;@@ -1272,7 +1270,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct string_list merge_rr =STRING_LIST_INIT_DUP ;struct strbuf head_msg =STRBUF_INIT ; - options.action ="abort" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 );@@ -1312,17 +1309,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) goto cleanup; }case ACTION_EDIT_TODO : - options.action ="edit-todo" ; options.dont_finish_rebase =1 ; goto run_rebase;case ACTION_SHOW_CURRENT_PATCH : - options.action ="show-current-patch" ; options.dont_finish_rebase =1 ; goto run_rebase;case ACTION_NONE :break ;default : -BUG ("action: %d" , action); +BUG ("action: %d" , options.action); }/* Make sure no rebase is in progress */ @@ -1346,7 +1341,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }if ((options.flags &REBASE_INTERACTIVE_EXPLICIT ) || - (action !=ACTION_NONE ) || + (options.action !=ACTION_NONE ) || (exec.nr >0 ) || options.autosquash) { allow_preemptive_ff =0 ;@@ -1786,7 +1781,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.revisions = revisions.buf; run_rebase: - ret =run_specific_rebase (&options, action); + ret =run_specific_rebase (&options); cleanup:strbuf_release (&buf); --
Quoted 1 line
gitgitgadget
Thanks for the nice cleanup!
Review Request (was Re: [PATCH 0/7] rebase: make reflog messages independent of the backend)
In reply to Phillip Wood via GitGitGadgetIf anyone has time to review these patches I'd be very grateful. I've cc'd this message to some list regulars who have reviewed rebase patches before but if anyone else fancies taking a look at them that would be great.
Thanks
Phillip
Quoted 30 lines · Phillip Wood via GitGitGadget
This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.
* improve rebase reflog test coverage
* rebase --merge: fix reflog messages for --continue and --skip
* rebase --apply: respectGIT_REFLOG_ACTION
* rebase --abort: improve reflog message
* unify reflog messages between the two rebase backendsThis series is based on pw/use-inprocess-checkout-in-rebase
Phillip Wood (7):
rebase --apply: remove duplicated code
rebase --merge: fix reflog when continuing rebase --merge: fix reflog message after skipping rebase --apply: respectGIT_REFLOG_ACTIONrebase --apply: make reflog messages match rebase --merge rebase --abort: improve reflog message
rebase: cleanup action handlingbuiltin/rebase.c | 144 ++++++++++++----------------- sequencer.c | 5 ++ t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++------- 3 files changed, 214 insertions(+), 120 deletions(-)base-commit:
38c541ce94048cf72aa4f465be9314423a57f445
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Re: Review Request (was Re: [PATCH 0/7] rebase: make reflog messages independent of the backend)
Quoted 4 lines · Phillip Wood
If anyone has time to review these patches I'd be very grateful. I've cc'd this message to some list regulars who have reviewed rebase patches before but if anyone else fancies taking a look at them that would be great.
I apologize for the delay here...especially when it came after you had already waited a month and a half. (This is the first git thing I've done in over a month, sadly.) Anyway, I just read through the series and left some comments; mostly looks good but I second the request to split up one of the patches.
Quoted 31 lines · Phillip Wood via GitGitGadget
Quoted 30 lines
This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.
* improve rebase reflog test coverage
* rebase --merge: fix reflog messages for --continue and --skip
* rebase --apply: respectGIT_REFLOG_ACTION
* rebase --abort: improve reflog message
* unify reflog messages between the two rebase backendsThis series is based on pw/use-inprocess-checkout-in-rebase
Phillip Wood (7):
rebase --apply: remove duplicated code
rebase --merge: fix reflog when continuing rebase --merge: fix reflog message after skipping rebase --apply: respectGIT_REFLOG_ACTIONrebase --apply: make reflog messages match rebase --merge rebase --abort: improve reflog message
rebase: cleanup action handlingbuiltin/rebase.c | 144 ++++++++++++----------------- sequencer.c | 5 ++ t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++------- 3 files changed, 214 insertions(+), 120 deletions(-)base-commit:
38c541ce94048cf72aa4f465be9314423a57f445
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Re: Review Request (was Re: [PATCH 0/7] rebase: make reflog messages independent of the backend)
Hi Elijah
Quoted 11 lines · Elijah Newren
Quoted 4 lines · Phillip Wood
If anyone has time to review these patches I'd be very grateful. I've cc'd this message to some list regulars who have reviewed rebase patches before but if anyone else fancies taking a look at them that would be great.
I apologize for the delay here...especially when it came after you had already waited a month and a half. (This is the first git thing I've done in over a month, sadly.) Anyway, I just read through the series and left some comments; mostly looks good but I second the request to split up one of the patches.
There's no need to apologize, I'm very grateful for you comments. I'll re-roll with that patch split up.
Thanks
Phillip
Quoted 32 lines
Quoted 32 lines · Phillip Wood via GitGitGadget
Quoted 30 lines
This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.
* improve rebase reflog test coverage
* rebase --merge: fix reflog messages for --continue and --skip
* rebase --apply: respectGIT_REFLOG_ACTION
* rebase --abort: improve reflog message
* unify reflog messages between the two rebase backendsThis series is based on pw/use-inprocess-checkout-in-rebase
Phillip Wood (7):
rebase --apply: remove duplicated code
rebase --merge: fix reflog when continuing rebase --merge: fix reflog message after skipping rebase --apply: respectGIT_REFLOG_ACTIONrebase --apply: make reflog messages match rebase --merge rebase --abort: improve reflog message
rebase: cleanup action handlingbuiltin/rebase.c | 144 ++++++++++++----------------- sequencer.c | 5 ++ t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++------- 3 files changed, 214 insertions(+), 120 deletions(-)base-commit:
38c541ce94048cf72aa4f465be9314423a57f445
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Quoted 3 lines · Phillip Wood via GitGitGadget
This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.
I took a look but might have been lost or confused in the middle, so I didn't look much at the last patches. I think this has a worthwhile goal though.
Quoted 27 lines · Phillip Wood via GitGitGadget
This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.
* improve rebase reflog test coverage
* rebase --merge: fix reflog messages for --continue and --skip
* rebase --apply: respectGIT_REFLOG_ACTION
* rebase --abort: improve reflog message
* unify reflog messages between the two rebase backendsThis series is based on pw/use-inprocess-checkout-in-rebase
Phillip Wood (7):
rebase --apply: remove duplicated code
rebase --merge: fix reflog when continuing rebase --merge: fix reflog message after skipping rebase --apply: respectGIT_REFLOG_ACTION
rebase --apply: make reflog messages match rebase --merge rebase --abort: improve reflog message
rebase: cleanup action handlingbuiltin/rebase.c | 144 ++++++++++++----------------- sequencer.c | 5 ++ t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++------- 3 files changed, 214 insertions(+), 120 deletions(-)
Quoted 4 lines
base-commit:
38c541ce94048cf72aa4f465be9314423a57f445
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1150
I read through the seven patches, then went back and commented on all of them. Hard to find problems really, looks nicely written. I would second Christian's request to have the second patch split with a preparatory testcase cleanup, which it appears you already plan to do. Other than patch, the series looks good to me.
v2 cover rebase: make reflog messages independent of the backend
In reply to Phillip Wood via GitGitGadgetThanks to Christian and Elijah for their comments on V1.
I've updated commit message for patch 1 to try and be clearer about the removal of a call to strbuf_release() and spilt out the test changes from the old patch 2 into a separate preparatory patch.
V1 Cover Letter:
This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.
* improve rebase reflog test coverage
* rebase --merge: fix reflog messages for --continue and --skip
* rebase --apply: respect GIT_REFLOG_ACTION
* rebase --abort: improve reflog message
* unify reflog messages between the two rebase backends
This series is based on pw/use-inprocess-checkout-in-rebase
Phillip Wood (8):
rebase --apply: remove duplicated code
t3406: rework rebase reflog tests
rebase --merge: fix reflog when continuing rebase --merge: fix reflog message after skipping rebase --apply: respect GIT_REFLOG_ACTION
rebase --apply: make reflog messages match rebase --merge rebase --abort: improve reflog message
rebase: cleanup action handling
builtin/rebase.c | 144 ++++++++++++----------------- sequencer.c | 5 ++ t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++------- 3 files changed, 214 insertions(+), 120 deletions(-)
base-commit: 38c541ce94048cf72aa4f465be9314423a57f445
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Range-diff vs v1:
1: 243e7b0c372 ! 1: a4320f2fcf3 rebase --apply: remove duplicated code
@@ Commit message
to do as we have already updated HEAD.
Note that the removal of "strbuf_release(&msg)" is safe as there is an
- identical call just above this hunk.
+ identical call just above this hunk which can be seen by viewing the
+ diff with -U6.
2: 493254ffbb8 ! 2: 0904b50a377 rebase --merge: fix reflog when continuing
@@ Metadata
Author: Phillip Wood <[email protected]>
## Commit message ##
- rebase --merge: fix reflog when continuing
+ t3406: rework rebase reflog tests
- The reflog message for a conflict resolution committed by "rebase
- --continue" looks like
+ Refactor the tests in preparation for adding more tests in the next
+ few commits. The reworked tests use the same function for testing both
+ the "merge" and "apply" backends. The test coverage for the "apply"
+ backend now includes setting GIT_REFLOG_ACTION.
- rebase (continue): commit subject line
-
- Unfortunately the reflog message each subsequent pick look like
-
- rebase (continue) (pick): commit subject line
-
- Fix this by setting the reflog message for "rebase --continue" in
- sequencer_continue() so it does not affect subsequent commits. This
- introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION
- in pick_commits(). Both of these will be fixed in a future series that
- stops the sequencer calling setenv().
+ Note that rebasing the "conflicts" branch does not create any
+ conflicts yet. A commit to do that will be added in the next commit
+ and the diff ends up smaller if we have don't rename the branch when
+ it is added.
- ## builtin/rebase.c ##
-@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- int fd;
-
- options.action = "continue";
-- set_reflog_action(&options);
--
- /* Sanity check */
- if (get_oid("HEAD", &head))
- die(_("Cannot read HEAD"));
-
- ## sequencer.c ##
-@@ sequencer.c: int sequencer_continue(struct repository *r, struct replay_opts *opts)
- if (read_populate_opts(opts))
- return -1;
- if (is_rebase_i(opts)) {
-+ char *previous_reflog_action;
-+
- if ((res = read_populate_todo(r, &todo_list, opts)))
- goto release_todo_list;
-
-@@ sequencer.c: int sequencer_continue(struct repository *r, struct replay_opts *opts)
- unlink(rebase_path_dropped());
- }
-
-+ previous_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
-+ setenv(GIT_REFLOG_ACTION, reflog_message(opts, "continue", NULL), 1);
- if (commit_staged_changes(r, opts, &todo_list)) {
- res = -1;
- goto release_todo_list;
- }
-+ setenv(GIT_REFLOG_ACTION, previous_reflog_action, 1);
- } else if (!file_exists(get_todo_path(opts)))
- return continue_single_pick(r, opts);
- else if ((res = read_populate_todo(r, &todo_list, opts)))
-
## t/t3406-rebase-message.sh ##
@@ t/t3406-rebase-message.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
test_expect_success 'setup' '
@@ t/t3406-rebase-message.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+ git checkout -b conflicts O &&
+ test_commit P &&
-+ test_commit conflict-X fileX &&
+ test_commit Q &&
+
git checkout -b topic O &&
@@ t/t3406-rebase-message.sh: test_expect_success 'error out early upon -C<n> or --
+ GIT_REFLOG_ACTION="$reflog_action" &&
+ export GIT_REFLOG_ACTION
+ fi &&
-+ test_must_fail git rebase $mode main &&
-+ echo resolved >fileX &&
-+ git add fileX &&
-+ git rebase --continue
++ git rebase $mode main
+ ) &&
+
-+ git log -g --format=%gs -5 >actual &&
++ git log -g --format=%gs -4 >actual &&
+ write_reflog_expect <<-EOF &&
+ ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
+ ${reflog_action:-rebase} (pick): Q
-+ ${reflog_action:-rebase} (continue): conflict-X
+ ${reflog_action:-rebase} (pick): P
+ ${reflog_action:-rebase} (start): checkout main
EOF
-: ----------- > 3: 6c15f00e170 rebase --merge: fix reflog when continuing 3: f4e6e94bc2c = 4: d3afa85ffc5 rebase --merge: fix reflog message after skipping 4: 58c560d0e19 = 5: afa67abe01a rebase --apply: respect GIT_REFLOG_ACTION 5: 1c3ec165422 = 6: 95161f21e00 rebase --apply: make reflog messages match rebase --merge 6: c863cebbdc8 = 7: d2c1dfbcd5e rebase --abort: improve reflog message 7: d26a221a79d = 8: b0d21affa78 rebase: cleanup action handling
Signature · 1 line
gitgitgadget
PATCH v2 1/8 rebase --apply: remove duplicated code
From: Phillip Wood <[email protected]>
When we are reattaching HEAD after a fast-forward we can use move_to_original_branch() rather than open coding a copy of that code. The old code appears to handle the case where the rebase is started from a detached HEAD but in fact in that case there is nothing to do as we have already updated HEAD.
Note that the removal of "strbuf_release(&msg)" is safe as there is an identical call just above this hunk which can be seen by viewing the diff with -U6.
--- builtin/rebase.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
builtin/rebase.c +1 -10
@@ -1782,19 +1782,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)* If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ -strbuf_reset (&msg);if (oideq (&merge_base, &options.orig_head)) {printf (_ ("Fast-forwarded %s to %s.\n" ), branch_name, options.onto_name); -strbuf_addf (&msg,"rebase finished: %s onto %s" , - options.head_name ? options.head_name :"detached HEAD" , -oid_to_hex (&options.onto->object.oid)); -memset (&ropts,0 ,sizeof (ropts)); - ropts.branch = options.head_name; - ropts.flags =RESET_HEAD_REFS_ONLY ; - ropts.head_msg = msg.buf; -reset_head (the_repository, &ropts); -strbuf_release (&msg); +move_to_original_branch (&options); ret =finish_rebase (&options); goto cleanup; }
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/8] rebase --apply: remove duplicated code
Quoted 11 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
When we are reattaching HEAD after a fast-forward we can use
move_to_original_branch()rather than open coding a copy of that code. The old code appears to handle the case where the rebase is started from a detached HEAD but in fact in that case there is nothing to do as we have already updated HEAD.Note that the removal of "
strbuf_release(&msg)" is safe as there is an identical call just above this hunk which can be seen by viewing the diff with -U6.
Instead of assuring the reader that this is OK, perhaps just squash this in:
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4a664964c30..5108679e816 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1029,7 +1029,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
int ret, flags, total_argc, in_progress = 0;
int keep_base = 0;
int ok_to_skip_pre_rebase = 0;
- struct strbuf msg = STRBUF_INIT;
struct strbuf revisions = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
struct object_id merge_base;
@@ -1769,17 +1768,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
printf(_("First, rewinding head to replay your work on top of "
"it...\n"));
- strbuf_addf(&msg, "%s: checkout %s",
- getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
- ropts.oid = &options.onto->object.oid;
- ropts.orig_head = &options.orig_head,
- ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
+ {
+ struct strbuf msg = STRBUF_INIT;
+
+ strbuf_addf(&msg, "%s: checkout %s",
+ getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
+ options.onto_name);
+ ropts.oid = &options.onto->object.oid;
+ ropts.orig_head = &options.orig_head,
+ ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
- ropts.head_msg = msg.buf;
- ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
- if (reset_head(the_repository, &ropts))
- die(_("Could not detach HEAD"));
- strbuf_release(&msg);
+ ropts.head_msg = msg.buf;
+ ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
+ if (reset_head(the_repository, &ropts))
+ die(_("Could not detach HEAD"));
+ strbuf_release(&msg);
+ }
/*
* If the onto is a proper descendant of the tip of the branch, thenThat's a 2-line change (aside from the scope addition) if viewed with -w. I.e. before your change below we needed &msg in two places, now that we only need it in one it seems better just to move the variable to be scoped with its only user.
And maybe it's getting too much into unrelated cleanup, but this change also leaves the loose end that the "ropts" variable no longer needs to be shared. You added it in 6ae8086161d (reset_head(): take struct rebase_head_opts, 2022-01-26) along whith the memset() removed here, but (on top of the proposed squash) we could also do this:
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5108679e816..8e2dc74c834 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1043,7 +1043,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
int reschedule_failed_exec = -1;
int allow_preemptive_ff = 1;
int preserve_merges_selected = 0;
- struct reset_head_opts ropts = { 0 };
struct option builtin_rebase_options[] = {
OPT_STRING(0, "onto", &options.onto_name,
N_("revision"),
@@ -1275,7 +1274,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
}
case ACTION_SKIP: {
struct string_list merge_rr = STRING_LIST_INIT_DUP;
-
+ struct reset_head_opts ropts = { 0 };
+
options.action = "skip";
set_reflog_action(&options);
@@ -1291,6 +1291,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
}
case ACTION_ABORT: {
struct string_list merge_rr = STRING_LIST_INIT_DUP;
+ struct reset_head_opts ropts = { 0 };
+
options.action = "abort";
set_reflog_action(&options);
@@ -1770,6 +1772,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
{
struct strbuf msg = STRBUF_INIT;
+ struct reset_head_opts ropts = { 0 };
strbuf_addf(&msg, "%s: checkout %s",
getenv(GIT_REFLOG_ACTION_ENVIRONMENT),I.e. the "ropts" earlier in the function wasn't shared with the code below, we'd "goto" past it...
Quoted 31 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)builtin/rebase.c +1
-10@@ -1782,19 +1782,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)* If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ -strbuf_reset (&msg);if (oideq (&merge_base, &options.orig_head)) {printf (_ ("Fast-forwarded %s to %s.\n" ), branch_name, options.onto_name); -strbuf_addf (&msg,"rebase finished: %s onto %s" , - options.head_name ? options.head_name :"detached HEAD" , -oid_to_hex (&options.onto->object.oid)); -memset (&ropts,0 ,sizeof (ropts)); - ropts.branch = options.head_name; - ropts.flags =RESET_HEAD_REFS_ONLY ; - ropts.head_msg = msg.buf; -reset_head (the_repository, &ropts); -strbuf_release (&msg); +move_to_original_branch (&options); ret =finish_rebase (&options); goto cleanup; }
Re: [PATCH v2 1/8] rebase --apply: remove duplicated code
In reply to Phillip Wood via GitGitGadgetbuiltin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1785
Quoted 1 line · Phillip Wood via GitGitGadget
builtin/rebase.c +0
-1-strbuf_reset (&msg);
This is unnecessary, because we have released immediately before.
Quoted 7 lines
From: Phillip Wood <[email protected]>
When we are reattaching HEAD after a fast-forward we can use
move_to_original_branch()rather than open coding a copy of that code. The old code appears to handle the case where the rebase is started from a detached HEAD but in fact in that case there is nothing to do as we have already updated HEAD.
At the beginning of move_to_original_branch(), there is an early return to do nothing when the head is detached. But the code before the pre-context of the hunk already detached the head at the "onto", and the fast-forward case wants to leave the detached head pointing at that "onto" commit, so the early return without doing anything is exactly what we want to see.
Does that mean that the original code had left two reflog entries for HEAD, one to detach at the "onto" commit, and then in this block to say "rebase finished" here? With the new code, because we return early from move_to_original_branch(), we no longer leave the "rebase finished" record in the reflog of HEAD?
Is it done deliberately as an improvement, or an oversight that led to a possible regression?
Or do we still add the reflog entry for HEAD that says "rebase finished" somewhere else when we trigger the early return and I am misreading the code?
Quoted 16 lines
Quoted 1 line
if (
oideq(&merge_base, &options.orig_head)) {builtin/rebase.c +1
-9printf (_ ("Fast-forwarded %s to %s.\n" ), branch_name, options.onto_name); -strbuf_addf (&msg,"rebase finished: %s onto %s" , - options.head_name ? options.head_name :"detached HEAD" , -oid_to_hex (&options.onto->object.oid)); -memset (&ropts,0 ,sizeof (ropts)); - ropts.branch = options.head_name; - ropts.flags =RESET_HEAD_REFS_ONLY ; - ropts.head_msg = msg.buf; -reset_head (the_repository, &ropts); -strbuf_release (&msg); +move_to_original_branch (&options); ret =finish_rebase (&options); goto cleanup; }
Re: [PATCH v2 1/8] rebase --apply: remove duplicated code
Quoted 5 lines · Junio C Hamano
builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1785
Quoted 1 line · Phillip Wood via GitGitGadget
builtin/rebase.c +0
-1-strbuf_reset (&msg);This is unnecessary, because we have released immediately before.
Sorry. I meant "reset is unnecessary and it is the right thing to remove it here", but I just realized it can be misread to say "this change is unnecessary".
PATCH v2 2/8 t3406: rework rebase reflog tests
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Refactor the tests in preparation for adding more tests in the next few commits. The reworked tests use the same function for testing both the "merge" and "apply" backends. The test coverage for the "apply" backend now includes setting GIT_REFLOG_ACTION.
Note that rebasing the "conflicts" branch does not create any conflicts yet. A commit to do that will be added in the next commit and the diff ends up smaller if we have don't rename the branch when it is added.
--- t/t3406-rebase-message.sh | 115 +++++++++++++++++++++++++------------- 1 file changed, 76 insertions(+), 39 deletions(-)
t/t3406-rebase-message.sh +76 -39
@@ -10,10 +10,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME test_expect_success 'setup' ' test_commit O fileO && test_commit X fileX && + git branch fast-forward && test_commit A fileA && test_commit B fileB && test_commit Y fileY && + git checkout -b conflicts O && + test_commit P && + test_commit Q && + git checkout -b topic O && git cherry-pick A B && test_commit Z fileZ &&
@@ -79,54 +84,86 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' test_i18ngrep "Invalid whitespace option" err ' -test_expect_success 'GIT_REFLOG_ACTION' ' - git checkout start && - test_commit reflog-onto && - git checkout -b reflog-topic start && - test_commit reflog-to-rebase && - - git rebase reflog-onto && - git log -g --format=%gs -3 >actual && - cat >expect <<-\EOF && - rebase (finish): returning to refs/heads/reflog-topic - rebase (pick): reflog-to-rebase - rebase (start): checkout reflog-onto +write_reflog_expect () { + if test $mode = --apply + then + sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//' + else + cat + fi >expect +} + +test_reflog () { + mode=$1 + reflog_action="$2" + + test_expect_success "rebase $mode reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout conflicts && + test_when_finished "git reset --hard Q" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + git rebase $mode main + ) && + + git log -g --format=%gs -4 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts + ${reflog_action:-rebase} (pick): Q + ${reflog_action:-rebase} (pick): P + ${reflog_action:-rebase} (start): checkout main EOF test_cmp expect actual && - git checkout -b reflog-prefix reflog-to-rebase && - GIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto && - git log -g --format=%gs -3 >actual && - cat >expect <<-\EOF && - change-the-reflog (finish): returning to refs/heads/reflog-prefix - change-the-reflog (pick): reflog-to-rebase - change-the-reflog (start): checkout reflog-onto + git log -g --format=%gs -1 conflicts >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): refs/heads/conflicts onto $(git rev-parse main) EOF - test_cmp expect actual -' - -test_expect_success 'rebase --apply reflog' ' - git checkout -b reflog-apply start && - old_head_reflog="$(git log -g --format=%gs -1 HEAD)" && - - git rebase --apply Y && + test_cmp expect actual && - git log -g --format=%gs -4 HEAD >actual && - cat >expect <<-EOF && - rebase finished: returning to refs/heads/reflog-apply - rebase: Z - rebase: checkout Y - $old_head_reflog + # check there is only one new entry in the branch reflog + test_cmp_rev conflicts@{1} Q + ' + + test_expect_success "rebase $mode fast-forward reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout fast-forward && + test_when_finished "git reset --hard X" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + git rebase $mode main + ) && + + git log -g --format=%gs -2 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward + ${reflog_action:-rebase} (start): checkout main EOF test_cmp expect actual && - git log -g --format=%gs -2 reflog-apply >actual && - cat >expect <<-EOF && - rebase finished: refs/heads/reflog-apply onto $(git rev-parse Y) - branch: Created from start + git log -g --format=%gs -1 fast-forward >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): refs/heads/fast-forward onto $(git rev-parse main) EOF - test_cmp expect actual -' + test_cmp expect actual && + + # check there is only one new entry in the branch reflog + test_cmp_rev fast-forward@{1} X + ' +} + +test_reflog --merge +test_reflog --merge my-reflog-action +test_reflog --apply +test_reflog --apply my-reflog-action test_expect_success 'rebase -i onto unrelated history' ' git init unrelated &&
Signature · 1 line
gitgitgadget
Re: [PATCH v2 2/8] t3406: rework rebase reflog tests
Quoted 11 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Refactor the tests in preparation for adding more tests in the next few commits. The reworked tests use the same function for testing both the "merge" and "apply" backends. The test coverage for the "apply" backend now includes setting
GIT_REFLOG_ACTION.Note that rebasing the "conflicts" branch does not create any conflicts yet. A commit to do that will be added in the next commit and the diff ends up smaller if we have don't rename the branch when it is added.
"if we have don't rename"? Ecannotparse.
Quoted 16 lines
Signed-off-by Phillip Wood <[email protected]>--- t/t3406-rebase-message.sh | 115 +++++++++++++++++++++++++------------- 1 file changed, 76 insertions(+), 39 deletions(-)t/t3406-rebase-message.sh +1
-0@@ -10,10 +10,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME test_expect_success 'setup' ' test_commit O fileO && test_commit X fileX && + git branch fast-forward && test_commit A fileA &&
t/t3406-rebase-message.sh export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME · line 24
Quoted 10 lines
t/t3406-rebase-message.sh +4
-0test_commit B fileB && test_commit Y fileY && + git checkout -b conflicts O && + test_commit P && + test_commit Q && + git checkout -b topic O && git cherry-pick A B && test_commit Z fileZ &&
So, we'd create
O---X---A---B---Y (main)
|\
| P---Q (conflicts)
\
A'--B'--Z (topic)with the above. The "fast-forward" branch is not yet used.
Without reading the rest of this patch (or the series), especially because you said you'll test multiple rebase strategies (and presumably make sure they result in the same state), my gut says that it may not buy us much to create such an unused branch. If we successfully run a fast-forward test on the branch for one strategy, we have to rewind the result before testing another strategy anyway, so forking a branch to be tested at X when we start testing each rebase strategy would be fine. But we'll see.
Quoted 18 lines
t/t3406-rebase-message.sh +2
-12@@ -79,54 +84,86 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' test_i18ngrep "Invalid whitespace option" err ' -test_expect_success 'GIT_REFLOG_ACTION' ' - git checkout start && - test_commit reflog-onto && - git checkout -b reflog-topic start && - test_commit reflog-to-rebase && - - git rebase reflog-onto && - git log -g --format=%gs -3 >actual && - cat >expect <<-\EOF && - rebase (finish): returning to refs/heads/reflog-topic - rebase (pick): reflog-to-rebase - rebase (start): checkout reflog-onto +write_reflog_expect () { + if test $mode = --apply
t/t3406-rebase-message.sh test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' · line 98
Quoted 10 lines
t/t3406-rebase-message.sh +10
-0+ then + sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//' + else + cat + fi >expect +} + +test_reflog () { + mode=$1 + reflog_action="$2"
$1 being left unquoted while $2 being quoted makes them look incoherent. It is not wrong to enclose $2 in double quotes, but it is not necessary.
t/t3406-rebase-message.sh test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' · line 111
Quoted 12 lines
t/t3406-rebase-message.sh +12
-0+ test_expect_success "rebase $mode reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout conflicts && + test_when_finished "git reset --hard Q" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + git rebase $mode main + ) &&
Is the subshell because you may want to export the envirohnment, or is there anything more subtle going on?
t/t3406-rebase-message.sh test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' · line 120
Quoted 8 lines
t/t3406-rebase-message.sh +6
-0+ git log -g --format=%gs -4 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts + ${reflog_action:-rebase} (pick): Q + ${reflog_action:-rebase} (pick): P + ${reflog_action:-rebase} (start): checkout main EOF test_cmp expect actual &&
We reset to Q and rebase it onto main, which would mean that we'd replay P and then Q on top of Y. The 4 most recent events on HEAD are that we start at 'main', picking P and Q and then updating the conflicts branch with the result. OK.
It is not what we used to test, but with reference to Q that was established at the beginning, though.
t/t3406-rebase-message.sh test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' · line 125
Quoted 11 lines
t/t3406-rebase-message.sh +3
-7- git checkout -b reflog-prefix reflog-to-rebase && - GIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto && - git log -g --format=%gs -3 >actual && - cat >expect <<-\EOF && - change-the-reflog (finish): returning to refs/heads/reflog-prefix - change-the-reflog (pick): reflog-to-rebase - change-the-reflog (start): checkout reflog-onto + git log -g --format=%gs -1 conflicts >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): refs/heads/conflicts onto $(git rev-parse main) EOF
And after doing that, there only is one reflog entry for the conflicts branch itself, i.e. record of finishing to rebase.
For both of the above two tests, I wonder if we want to make sure these four and one entries are the ONLY entries added by the operation. I.e. take the topmost reflog entry _before_ starting to run rebase for both conflicts branch and HEAD, grab five and two reflog entries after the operation from conflicts and HEAD and make sure the four and one are the truly new entries added by the operation, or something like that. Even easier would be to merely count the number of reflog entries of HEAD and conflicts before rebasing, and count them after. We can do subtraction in the shell just fine.
t/t3406-rebase-message.sh test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' · line 128
Quoted 3 lines
t/t3406-rebase-message.sh +2
-0+ test_cmp expect actual && + # check there is only one new entry in the branch reflog
It is a valid thing to try verifying, but ...
t/t3406-rebase-message.sh test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' · line 129
Quoted 1 line
t/t3406-rebase-message.sh +1
-0+ test_cmp_rev conflicts@{1} Q
... it is unclear how this can count the new reflog entries. If we added two reflog entries, one pointing at Y (i.e. tip of main), another that points at Q (i.e. tip of conflicts before the rebasing), conflicts@{1} would be the same as Q.
Quoted 11 lines
t/t3406-rebase-message.sh +11
-0+ ' + + test_expect_success "rebase $mode fast-forward reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout fast-forward && + test_when_finished "git reset --hard X" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION
t/t3406-rebase-message.sh test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' · line 150
Quoted 10 lines
t/t3406-rebase-message.sh +8
-0+ fi && + git rebase $mode main + ) && + + git log -g --format=%gs -2 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward + ${reflog_action:-rebase} (start): checkout main EOF test_cmp expect actual &&
Likewise. This only makes sure that the topmost two are what you expect without checking if it added anything extra before these two. Actually counting the entries would give you an easy fix.
Quoted 4 lines
t/t3406-rebase-message.sh +0
-4- git log -g --format=%gs -2 reflog-apply >actual && - cat >expect <<-EOF && - rebase finished: refs/heads/reflog-apply onto $(git rev-parse Y) - branch: Created from start
t/t3406-rebase-message.sh test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' · line 159
Quoted 10 lines
t/t3406-rebase-message.sh +7
-2+ git log -g --format=%gs -1 fast-forward >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): refs/heads/fast-forward onto $(git rev-parse main) EOF - test_cmp expect actual -' + test_cmp expect actual && + + # check there is only one new entry in the branch reflog + test_cmp_rev fast-forward@{1} X
The same. This does not ensure there is only one new entry. It merely says that the set of entries you cared about (which is a set of one) is preceded by an entry that happened to point at X.
t/t3406-rebase-message.sh test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' · line 169
Quoted 10 lines
t/t3406-rebase-message.sh +7
-0+ ' +} + +test_reflog --merge +test_reflog --merge my-reflog-action +test_reflog --apply +test_reflog --apply my-reflog-action test_expect_success 'rebase -i onto unrelated history' ' git init unrelated &&
Thanks.
PATCH v2 3/8 rebase --merge: fix reflog when continuing
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The reflog message for a conflict resolution committed by "rebase --continue" looks like
rebase (continue): commit subject line
Unfortunately the reflog message each subsequent pick look like
rebase (continue) (pick): commit subject line
Fix this by setting the reflog message for "rebase --continue" in sequencer_continue() so it does not affect subsequent commits. This introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION in pick_commits(). Both of these will be fixed in a future series that stops the sequencer calling setenv().
--- builtin/rebase.c | 2 -- sequencer.c | 5 +++++ t/t3406-rebase-message.sh | 9 +++++++-- 3 files changed, 12 insertions(+), 4 deletions(-)
builtin/rebase.c +0 -2
@@ -1247,8 +1247,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)int fd; options.action ="continue" ; -set_reflog_action (&options); -/* Sanity check */ if (get_oid ("HEAD" , &head))die (_ ("Cannot read HEAD" ));
sequencer.c +5 -0
@@ -4777,6 +4777,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)if (read_populate_opts (opts))return -1 ;if (is_rebase_i (opts)) { +char *previous_reflog_action; +if ((res =read_populate_todo (r, &todo_list, opts))) goto release_todo_list;
@@ -4787,10 +4789,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)unlink (rebase_path_dropped ()); } + previous_reflog_action =xstrdup (getenv (GIT_REFLOG_ACTION )); +setenv (GIT_REFLOG_ACTION ,reflog_message (opts,"continue" ,NULL ),1 );if (commit_staged_changes (r, opts, &todo_list)) { res =-1 ; goto release_todo_list; } +setenv (GIT_REFLOG_ACTION , previous_reflog_action,1 ); }else if (!file_exists (get_todo_path (opts)))return continue_single_pick (r, opts);else if ((res =read_populate_todo (r, &todo_list, opts)))
t/t3406-rebase-message.sh +7 -2
@@ -17,6 +17,7 @@ test_expect_success 'setup' ' git checkout -b conflicts O && test_commit P && + test_commit conflict-X fileX && test_commit Q && git checkout -b topic O &&
@@ -107,13 +108,17 @@ test_reflog () { GIT_REFLOG_ACTION="$reflog_action" && export GIT_REFLOG_ACTION fi && - git rebase $mode main + test_must_fail git rebase $mode main && + echo resolved >fileX && + git add fileX && + git rebase --continue ) && - git log -g --format=%gs -4 >actual && + git log -g --format=%gs -5 >actual && write_reflog_expect <<-EOF && ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts ${reflog_action:-rebase} (pick): Q + ${reflog_action:-rebase} (continue): conflict-X ${reflog_action:-rebase} (pick): P ${reflog_action:-rebase} (start): checkout main EOF
Signature · 1 line
gitgitgadget
PATCH v2 4/8 rebase --merge: fix reflog message after skipping
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The reflog message for every pick after running "rebase --skip" looks like
rebase (skip) (pick): commit subject line
Fix this by not appending " (skip)" to the reflog action.
--- builtin/rebase.c | 2 -- t/t3406-rebase-message.sh | 24 ++++++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-)
builtin/rebase.c +0 -2
@@ -1273,8 +1273,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct string_list merge_rr =STRING_LIST_INIT_DUP ; options.action ="skip" ; -set_reflog_action (&options); -rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 ); ropts.flags =RESET_HEAD_HARD ;
t/t3406-rebase-message.sh +24 -0
@@ -163,6 +163,30 @@ test_reflog () { # check there is only one new entry in the branch reflog test_cmp_rev fast-forward@{1} X ' + + test_expect_success "rebase $mode --skip reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout conflicts && + test_when_finished "git reset --hard Q" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + test_must_fail git rebase $mode main && + git rebase --skip + ) && + + git log -g --format=%gs -4 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts + ${reflog_action:-rebase} (pick): Q + ${reflog_action:-rebase} (pick): P + ${reflog_action:-rebase} (start): checkout main + EOF + test_cmp expect actual + ' } test_reflog --merge
Signature · 1 line
gitgitgadget
PATCH v2 5/8 rebase --apply: respect GIT_REFLOG_ACTION
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The reflog messages when finishing a rebase hard code "rebase" rather than using GIT_REFLOG_ACTION.
--- builtin/rebase.c | 7 ++++--- t/t3406-rebase-message.sh | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)
builtin/rebase.c +4 -3
@@ -580,10 +580,11 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"rebase finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid)); -strbuf_addf (&head_reflog,"rebase finished: returning to %s" , - opts->head_name); +strbuf_addf (&head_reflog,"%s finished: returning to %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name); ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ; ropts.branch_msg = branch_reflog.buf;
t/t3406-rebase-message.sh +1 -1
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' write_reflog_expect () { if test $mode = --apply then - sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//' + sed 's/(finish)/finished/; s/ ([^)]*)//' else cat fi >expect
Signature · 1 line
gitgitgadget
PATCH v2 6/8 rebase --apply: make reflog messages match rebase --merge
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The apply backend creates slightly different reflog messages to the merge backend when starting and finishing a rebase and when picking commits. The choice of backend is really an implementation detail so it is confusing to have the same command create different messages depending on which backend is selected. Change the apply backend so the reflog messages from the two backends match as closely as possible. Note that there is still a difference when committing a conflict resolution - the merge backend will use "(continue)" rather than "(pick)" in that case as it does not know which command created the conflict that it is committing.
--- builtin/rebase.c | 9 +++++---- t/t3406-rebase-message.sh | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-)
builtin/rebase.c +5 -4
@@ -580,10 +580,10 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s (finish): %s onto %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid)); -strbuf_addf (&head_reflog,"%s finished: returning to %s" , +strbuf_addf (&head_reflog,"%s (finish): returning to %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name); ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ;
@@ -613,7 +613,8 @@ static int run_am(struct rebase_options *opts) am.git_cmd =1 ;strvec_push (&am.args,"am" ); - +strvec_pushf (&am.env_array,GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ));if (opts->action && !strcmp ("continue" , opts->action)) {strvec_push (&am.args,"--resolved" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg);
@@ -1763,7 +1764,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)printf (_ ("First, rewinding head to replay your work on top of " "it...\n" )); -strbuf_addf (&msg,"%s: checkout %s" , +strbuf_addf (&msg,"%s (start): checkout %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), options.onto_name); ropts.oid = &options.onto->object.oid; ropts.orig_head = &options.orig_head,
t/t3406-rebase-message.sh +1 -1
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' write_reflog_expect () { if test $mode = --apply then - sed 's/(finish)/finished/; s/ ([^)]*)//' + sed 's/(continue)/(pick)/' else cat fi >expect
Signature · 1 line
gitgitgadget
Re: [PATCH v2 6/8] rebase --apply: make reflog messages match rebase --merge
Quoted 60 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
The apply backend creates slightly different reflog messages to the merge backend when starting and finishing a rebase and when picking commits. The choice of backend is really an implementation detail so it is confusing to have the same command create different messages depending on which backend is selected. Change the apply backend so the reflog messages from the two backends match as closely as possible. Note that there is still a difference when committing a conflict resolution - the merge backend will use "(continue)" rather than "(pick)" in that case as it does not know which command created the conflict that it is committing.
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 9 +++++---- t/t3406-rebase-message.sh | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-)builtin/rebase.c +5
-4@@ -580,10 +580,10 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s (finish): %s onto %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid)); -strbuf_addf (&head_reflog,"%s finished: returning to %s" , +strbuf_addf (&head_reflog,"%s (finish): returning to %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name); ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ;@@ -613,7 +613,8 @@ static int run_am(struct rebase_options *opts) am.git_cmd =1 ;strvec_push (&am.args,"am" ); - +strvec_pushf (&am.env_array,GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ));if (opts->action && !strcmp ("continue" , opts->action)) {strvec_push (&am.args,"--resolved" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg);@@ -1763,7 +1764,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)printf (_ ("First, rewinding head to replay your work on top of " "it...\n" )); -strbuf_addf (&msg,"%s: checkout %s" , +strbuf_addf (&msg,"%s (start): checkout %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), options.onto_name); ropts.oid = &options.onto->object.oid; ropts.orig_head = &options.orig_head,t/t3406-rebase-message.sh +0
-0
t/t3406-rebase-message.sh test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' · line 94
Quoted 8 lines
t/t3406-rebase-message.sh +1
-1@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' write_reflog_expect () { if test $mode = --apply then - sed 's/(finish)/finished/; s/ ([^)]*)//' + sed 's/(continue)/(pick)/' else cat fi >expect
I like the new message better, but I wonder given that the old message has been with us since 6fd2f5e60d4 (rebase: operate on a detached HEAD, 2007-11-08) whether this will break anything in the wild.
Re: [PATCH v2 6/8] rebase --apply: make reflog messages match rebase --merge
In reply to Phillip Wood via GitGitGadgetQuoted 9 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
The apply backend creates slightly different reflog messages to the merge backend when starting and finishing a rebase and when picking commits. The choice of backend is really an implementation detail so it is confusing to have the same command create different messages depending on which backend is selected. Change the apply backend so the reflog messages from the two backends match as closely as possible.
I do not agree with that line of thought.
When choosing backend, the user clearly cares which one is used to do the job, so it is far from an "implementation detail" that is better hidden from the users. In other words, the message being different is not source of confusion in this case. Producing different messages depending on the version of Git may create worse confusion, though.
Quoted 4 lines
Note that there is still a difference when committing a conflict resolution - the merge backend will use "(continue)" rather than "(pick)" in that case as it does not know which command created the conflict that it is committing.
That is OK, unless we can teach the backend to produce a more proper one (if 'continue' is considered less proper, that is, than 'pick'; I do not think I would personally care between the two words too much).
builtin/rebase.c static int move_to_original_branch(struct rebase_options *opts) · line 589
Quoted 9 lines
builtin/rebase.c +2
-2-strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s (finish): %s onto %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid)); -strbuf_addf (&head_reflog,"%s finished: returning to %s" , +strbuf_addf (&head_reflog,"%s (finish): returning to %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name); ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ;
These convey the same information so it is "Meh---I do not care too strongly about them" kind of change, unless the justification is "let's show the (word) in parehtheses consistently in these reflog entries", in which case they are welcome change.
Overall, I'd rather see us not to apply this patch at this point in time, especially ...
builtin/rebase.c static int run_am(struct rebase_options *opts) · line 616
Quoted 1 line
builtin/rebase.c +1
-0+ strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
... this new use of .env_array member semantically conflicts with the removal of .env member from the structure being proposed in another topic in flight.
builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1768
Quoted 11 lines
builtin/rebase.c +2
-1+ getenv(GIT_REFLOG_ACTION_ENVIRONMENT));if (opts->action && !strcmp ("continue" , opts->action)) {strvec_push (&am.args,"--resolved" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg);@@ -1763,7 +1764,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)printf (_ ("First, rewinding head to replay your work on top of " "it...\n" )); -strbuf_addf (&msg,"%s: checkout %s" , +strbuf_addf (&msg,"%s (start): checkout %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), options.onto_name);
I think this one is a strict improvement with or without () around the word that tells us what phase of the rebase operation we are at, and if it happens to match what the other backend produces, that would be a tolerable small regression, even though it makes it hard to tell them apart.
PATCH v2 7/8 rebase --abort: improve reflog message
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
When aborting a rebase the reflog message looks like
rebase (abort): updating HEAD
which is not very informative. Improve the message by mentioning the branch that we are returning to as we do at the end of a successful rebase so it looks like.
rebase (abort): returning to refs/heads/topic
If GIT_REFLOG_ACTION is set in the environment we no longer omit "(abort)" from the reflog message. We don't omit "(start)" and "(finish)" when starting and finishing a rebase in that case so we shouldn't omit "(abort)".
--- builtin/rebase.c | 28 ++++++--------------- t/t3406-rebase-message.sh | 51 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 20 deletions(-)
builtin/rebase.c +8 -20
@@ -992,23 +992,6 @@ static void NORETURN error_on_missing_default_upstream(void)exit (1 ); } -static void set_reflog_action (struct rebase_options *options) -{ -const char *env; -struct strbuf buf =STRBUF_INIT ; - -if (!is_merge (options)) -return ; - - env =getenv (GIT_REFLOG_ACTION_ENVIRONMENT ); -if (env &&strcmp ("rebase" , env)) -return ;/* only override it if it is "rebase" */ - -strbuf_addf (&buf,"rebase (%s)" , options->action); -setenv (GIT_REFLOG_ACTION_ENVIRONMENT , buf.buf,1 ); -strbuf_release (&buf); -} -static int check_exec_cmd (const char *cmd) {if (strchr (cmd,'\n' ))
@@ -1287,18 +1270,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }case ACTION_ABORT : {struct string_list merge_rr =STRING_LIST_INIT_DUP ; - options.action ="abort" ; -set_reflog_action (&options); +struct strbuf head_msg =STRBUF_INIT ; + options.action ="abort" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 );if (read_basic_state (&options))exit (1 ); + +strbuf_addf (&head_msg,"%s (abort): returning to %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), + options.head_name ? options.head_name + :oid_to_hex (&options.orig_head)); ropts.oid = &options.orig_head; + ropts.head_msg = head_msg.buf; ropts.branch = options.head_name; ropts.flags =RESET_HEAD_HARD ; - ropts.default_reflog_action =DEFAULT_REFLOG_ACTION ;if (reset_head (the_repository, &ropts) <0 )die (_ ("could not move back to %s" ),oid_to_hex (&options.orig_head));
t/t3406-rebase-message.sh +51 -0
57 lines not on this page · raw message
Signature · 1 line
gitgitgadget
PATCH v2 8/8 rebase: cleanup action handling
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Treating the action as a string is a hang over from the scripted rebase. The last commit removed the only remaining use of the action that required a string so lets convert the other action users to use the existing action enum instead. If we ever need the action name as a string in the future the action_names array exists exactly for that purpose.
--- builtin/rebase.c | 91 +++++++++++++++++++++++------------------------- 1 file changed, 43 insertions(+), 48 deletions(-)
builtin/rebase.c +43 -48
@@ -58,6 +58,26 @@ enum empty_type {EMPTY_ASK }; +enum action { +ACTION_NONE =0 , +ACTION_CONTINUE , +ACTION_SKIP , +ACTION_ABORT , +ACTION_QUIT , +ACTION_EDIT_TODO , +ACTION_SHOW_CURRENT_PATCH +}; + +static const char *action_names[] = { +"undefined" , +"continue" , +"skip" , +"abort" , +"quit" , +"edit_todo" , +"show_current_patch" +}; +struct rebase_options {enum rebase_type type;enum empty_type empty;
@@ -84,7 +104,7 @@ struct rebase_options {REBASE_INTERACTIVE_EXPLICIT =1 <<4 , } flags;struct strvec git_am_opts; -const char *action; +enum action action;int signoff;int allow_rerere_autoupdate;int keep_empty;
@@ -155,24 +175,6 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)return replay; } -enum action { -ACTION_NONE =0 , -ACTION_CONTINUE , -ACTION_SKIP , -ACTION_ABORT , -ACTION_QUIT , -ACTION_EDIT_TODO , -ACTION_SHOW_CURRENT_PATCH -}; - -static const char *action_names[] = {"undefined" , -"continue" , -"skip" , -"abort" , -"quit" , -"edit_todo" , -"show_current_patch" }; -static int edit_todo_file (unsigned flags) {const char *todo_file =rebase_path_todo ();
@@ -309,8 +311,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)return ret; } -static int run_sequencer_rebase (struct rebase_options *opts, -enum action command) +static int run_sequencer_rebase (struct rebase_options *opts) {unsigned flags =0 ;int abbreviate_commands =0 , ret =0 ;
@@ -325,7 +326,7 @@ static int run_sequencer_rebase(struct rebase_options *opts, flags |= opts->reapply_cherry_picks ?TODO_LIST_REAPPLY_CHERRY_PICKS :0 ; flags |= opts->flags &REBASE_NO_QUIET ?TODO_LIST_WARN_SKIPPED_CHERRY_PICKS :0 ; -switch (command) { +switch (opts->action) {case ACTION_NONE : {if (!opts->onto && !opts->upstream)die (_ ("a base commit must be provided with --upstream or --onto" ));
@@ -358,7 +359,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,break ; }default : -BUG ("invalid command '%d'" , command); +BUG ("invalid command '%d'" , opts->action); }return ret;
@@ -615,7 +616,7 @@ static int run_am(struct rebase_options *opts)strvec_push (&am.args,"am" );strvec_pushf (&am.env_array,GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT )); -if (opts->action && !strcmp ("continue" , opts->action)) { +if (opts->action ==ACTION_CONTINUE ) {strvec_push (&am.args,"--resolved" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg);if (opts->gpg_sign_opt)
@@ -626,7 +627,7 @@ static int run_am(struct rebase_options *opts)return move_to_original_branch (opts); } -if (opts->action && !strcmp ("skip" , opts->action)) { +if (opts->action ==ACTION_SKIP ) {strvec_push (&am.args,"--skip" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); status =run_command (&am);
@@ -635,7 +636,7 @@ static int run_am(struct rebase_options *opts)return move_to_original_branch (opts); } -if (opts->action && !strcmp ("show-current-patch" , opts->action)) { +if (opts->action ==ACTION_SHOW_CURRENT_PATCH ) {strvec_push (&am.args,"--show-current-patch" );return run_command (&am); }
@@ -728,7 +729,7 @@ static int run_am(struct rebase_options *opts)return status; } -static int run_specific_rebase (struct rebase_options *opts,enum action action) +static int run_specific_rebase (struct rebase_options *opts) {int status;
@@ -746,7 +747,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action) opts->gpg_sign_opt = tmp; } - status =run_sequencer_rebase (opts, action); + status =run_sequencer_rebase (opts); }else if (opts->type ==REBASE_APPLY ) status =run_am (opts);else
@@ -1016,7 +1017,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct strbuf buf =STRBUF_INIT ;struct object_id merge_base;int ignore_whitespace =0 ; -enum action action =ACTION_NONE ;const char *gpg_sign =NULL ;struct string_list exec =STRING_LIST_INIT_NODUP ;const char *rebase_merges =NULL ;
@@ -1065,18 +1065,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)OPT_BIT (0 ,"no-ff" , &options.flags,N_ ("cherry-pick all commits, even if unchanged" ),REBASE_FORCE ), -OPT_CMDMODE (0 ,"continue" , &action,N_ ("continue" ), +OPT_CMDMODE (0 ,"continue" , &options.action,N_ ("continue" ),ACTION_CONTINUE ), -OPT_CMDMODE (0 ,"skip" , &action, +OPT_CMDMODE (0 ,"skip" , &options.action,N_ ("skip current patch and continue" ),ACTION_SKIP ), -OPT_CMDMODE (0 ,"abort" , &action, +OPT_CMDMODE (0 ,"abort" , &options.action,N_ ("abort and check out the original branch" ),ACTION_ABORT ), -OPT_CMDMODE (0 ,"quit" , &action, +OPT_CMDMODE (0 ,"quit" , &options.action,N_ ("abort but keep HEAD where it is" ),ACTION_QUIT ), -OPT_CMDMODE (0 ,"edit-todo" , &action,N_ ("edit the todo list " +OPT_CMDMODE (0 ,"edit-todo" , &options.action,N_ ("edit the todo list " "during an interactive rebase" ),ACTION_EDIT_TODO ), -OPT_CMDMODE (0 ,"show-current-patch" , &action, +OPT_CMDMODE (0 ,"show-current-patch" , &options.action,N_ ("show the patch file being applied or merged" ),ACTION_SHOW_CURRENT_PATCH ), OPT_CALLBACK_F(0, "apply", &options, NULL,
@@ -1189,7 +1189,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (preserve_merges_selected)die (_ ("--preserve-merges was replaced by --rebase-merges" )); -if (action !=ACTION_NONE && total_argc !=2 ) { +if (options.action !=ACTION_NONE && total_argc !=2 ) {usage_with_options (builtin_rebase_usage, builtin_rebase_options); }
@@ -1208,11 +1208,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (options.root && options.fork_point >0 )die (_ ("cannot combine '--root' with '--fork-point'" )); -if (action !=ACTION_NONE && !in_progress) +if (options.action !=ACTION_NONE && !in_progress)die (_ ("No rebase in progress?" ));setenv (GIT_REFLOG_ACTION_ENVIRONMENT ,"rebase" ,0 ); -if (action ==ACTION_EDIT_TODO && !is_merge (&options)) +if (options.action ==ACTION_EDIT_TODO && !is_merge (&options))die (_ ("The --edit-todo action can only be used during " "interactive rebase." ));
@@ -1222,16 +1222,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)else if (exec.nr)trace2_cmd_mode ("interactive-exec" );else -trace2_cmd_mode (action_names[action]); +trace2_cmd_mode (action_names[options.action]); } -switch (action) { +switch (options.action) {case ACTION_CONTINUE : {struct object_id head;struct lock_file lock_file =LOCK_INIT ;int fd; - options.action ="continue" ;/* Sanity check */ if (get_oid ("HEAD" , &head))die (_ ("Cannot read HEAD" ));
@@ -1257,7 +1256,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)case ACTION_SKIP : {struct string_list merge_rr =STRING_LIST_INIT_DUP ; - options.action ="skip" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 ); ropts.flags =RESET_HEAD_HARD ;
@@ -1272,7 +1270,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct string_list merge_rr =STRING_LIST_INIT_DUP ;struct strbuf head_msg =STRBUF_INIT ; - options.action ="abort" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 );
@@ -1312,17 +1309,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) goto cleanup; }case ACTION_EDIT_TODO : - options.action ="edit-todo" ; options.dont_finish_rebase =1 ; goto run_rebase;case ACTION_SHOW_CURRENT_PATCH : - options.action ="show-current-patch" ; options.dont_finish_rebase =1 ; goto run_rebase;case ACTION_NONE :break ;default : -BUG ("action: %d" , action); +BUG ("action: %d" , options.action); }/* Make sure no rebase is in progress */
@@ -1346,7 +1341,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }if ((options.flags &REBASE_INTERACTIVE_EXPLICIT ) || - (action !=ACTION_NONE ) || + (options.action !=ACTION_NONE ) || (exec.nr >0 ) || options.autosquash) { allow_preemptive_ff =0 ;
@@ -1786,7 +1781,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.revisions = revisions.buf; run_rebase: - ret =run_specific_rebase (&options, action); + ret =run_specific_rebase (&options); cleanup:strbuf_release (&buf);
Signature · 1 line
gitgitgadget
Re: [PATCH v2 8/8] rebase: cleanup action handling
Quoted 8 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Treating the action as a string is a hang over from the scripted rebase. The last commit removed the only remaining use of the action that required a string so lets convert the other action users to use the existing action enum instead. If we ever need the action name as a string in the future the
action_namesarray exists exactly for that purpose.
This is very much an existing issue, just noting this since I read this
over: We really should just carry that "action_names" in the "switch" statement immediately below the trace2_cmd_mode() call, see "git grep trace2_cmd_mode". I.e. usually we do it in the "callback".
Or actually just add a OPT_CMDMODE_TRACE2(), which would be an OPT_CALLBACK (see parse_opt_show_current_patch for such an example for a cmdmode callback) that would take the "--continue" string, log "continue" etc (do we really need to munge "edit-todo" to "edit_todo").
But I digress, this code looks fine, thanks.
v3 cover rebase: make reflog messages independent of the backend
In reply to Phillip Wood via GitGitGadgetThis series fixes some bugs in the reflog messages when rebasing and changes the reflog messages of "rebase --apply" to match "rebase --merge" with the aim of making the reflog easier to parse.
Thanks to everyone who commented on V2, I've added the review club participants that I've got address for to the cc list. I've rebased onto pw/rebase-keep-base-fixes.
Change since V2:
* Patch 1: Reworded the commit message to address the concerns in [1,2] about the behavior when head_name is NULL. There is also a small change due to being rebased.
* Patch 2: Unchanged. There wasn't much love for parameterized tests in review club but we want to ensure both backends produce the same messages I think this is the safest way to achieve that. Using separate tests makes it too easy to introduce subtle differences in the testing of the two backends.
* Patch 3: Added a note to the commit message to address the concerns in
[1] about not resetting GIT_REFLOG_ACTION when we return early.
* Patches 4 & 5: Unchanged.
* Patch 6: Reworded the commit message to make a stronger argument for this change. There are concerns about backwards compatibility in [1,3,4] but
(i) we have made similar changes in the past without complaints and (ii) we're changing the message to an existing format. There is also a small change due to being rebased.
* Patches 7 & 8: Small changes due to rebase.
[1]
https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?pli=1#heading=h.t6g5l2t5ibzw
[2] https://lore.kernel.org/git/[email protected]/ [3]
https://lore.kernel.org/git/[email protected]/ [4]
https://lore.kernel.org/git/[email protected]/
V2 Cover Letter:
Thanks to Christian and Elijah for their comments on V1.
I've updated commit message for patch 1 to try and be clearer about the removal of a call to strbuf_release() and spilt out the test changes from the old patch 2 into a separate preparatory patch.
V1 Cover Letter:
This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.
* improve rebase reflog test coverage
* rebase --merge: fix reflog messages for --continue and --skip
* rebase --apply: respect GIT_REFLOG_ACTION
* rebase --abort: improve reflog message
* unify reflog messages between the two rebase backends
This series is based on pw/use-inprocess-checkout-in-rebase
Phillip Wood (8):
rebase --apply: remove duplicated code
t3406: rework rebase reflog tests
rebase --merge: fix reflog when continuing rebase --merge: fix reflog message after skipping rebase --apply: respect GIT_REFLOG_ACTION
rebase --apply: make reflog messages match rebase --merge rebase --abort: improve reflog message
rebase: cleanup action handling
builtin/rebase.c | 146 ++++++++++++------------------ sequencer.c | 5 ++ t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++------- 3 files changed, 215 insertions(+), 121 deletions(-)
base-commit: 0203c679871112c78adc6428a8ed6c04c30ccad9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Range-diff vs v2:
1: a4320f2fcf3 ! 1: a84cf971a75 rebase --apply: remove duplicated code
@@ Metadata
## Commit message ##
rebase --apply: remove duplicated code
- When we are reattaching HEAD after a fast-forward we can use
- move_to_original_branch() rather than open coding a copy of that
- code. The old code appears to handle the case where the rebase is
- started from a detached HEAD but in fact in that case there is nothing
- to do as we have already updated HEAD.
+ Use move_to_original_branch() when reattaching HEAD after a fast-forward
+ rather than open coding a copy of that code. move_to_original_branch()
+ does not call reset_head() if head_name is NULL but there should be no
+ user visible changes even though we currently call reset_head() in that
+ case. The reason for this is that the reset_head() call does not add a
+ message to the reflog because we're not changing the commit that HEAD
+ points to and so lock_ref_for_update() elides the update. When head_name
+ is not NULL then reset_head() behaves like "git symbolic-ref" and so the
+ reflog is updated.
Note that the removal of "strbuf_release(&msg)" is safe as there is an identical call just above this hunk which can be seen by viewing the
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
* we just fast-forwarded.
*/
- strbuf_reset(&msg);
- if (oideq(&merge_base, &options.orig_head)) {
+ if (oideq(&branch_base, &options.orig_head->object.oid)) {
printf(_("Fast-forwarded %s to %s.\n"),
branch_name, options.onto_name);
- strbuf_addf(&msg, "rebase finished: %s onto %s",
2: 0904b50a377 = 2: b9255ad35d2 t3406: rework rebase reflog tests 3: 6c15f00e170 ! 3: ea4da25a19c rebase --merge: fix reflog when continuing
@@ Commit message
in pick_commits(). Both of these will be fixed in a future series that stops the sequencer calling setenv().
+ If we fail to commit the staged changes then we error out so
+ GIT_REFLOG_ACTION does not need to be reset in that case.
+
## builtin/rebase.c ##
4: d3afa85ffc5 = 4: 225ff4baef7 rebase --merge: fix reflog message after skipping 5: afa67abe01a = 5: 1094681eb11 rebase --apply: respect GIT_REFLOG_ACTION 6: 95161f21e00 ! 6: a5338e6bdd8 rebase --apply: make reflog messages match rebase --merge
@@ Commit message
rebase --apply: make reflog messages match rebase --merge
The apply backend creates slightly different reflog messages to the
- merge backend when starting and finishing a rebase and when picking
- commits. The choice of backend is really an implementation detail so
- it is confusing to have the same command create different messages
- depending on which backend is selected. Change the apply backend so
- the reflog messages from the two backends match as closely as
- possible. Note that there is still a difference when committing a
- conflict resolution - the merge backend will use "(continue)" rather
- than "(pick)" in that case as it does not know which command created
- the conflict that it is committing.
+ merge backend when starting or finishing a rebase and when picking
+ commits. These differences make it harder than it needs to be to parse
+ the reflog (I have a script that reads the finishing messages from
+ rebase and it is a pain to have to accommodate two different message
+ formats). While it is possible to determine the backend used for a
+ rebase from the reflog messages, the differences are not designed for
+ that purpose. c2417d3af7 (rebase: drop '-i' from the reflog for
+ interactive-based rebases, 2020-02-15) removed the clear distinction
+ between the reflog messages of the two backends without complaint.
+
+ As the merge backend is the default it is likely to be the format most
+ common in existing reflogs. For that reason the apply backend is changed
+ to format its reflog messages to match the merge backend as closely as
+ possible. Note that there is still a difference as when committing a
+ conflict resolution the apply backend will use "(pick)" rather than
+ "(continue)" because it is not currently possible to change the message
+ for a single commit.
+
+ In addition to c2417d3af7 we also changed the reflog messages in
+ 68aa495b59 (rebase: implement --merge via the interactive machinery,
+ 2018-12-11) and 2ac0d6273f (rebase: change the default backend from "am"
+ to "merge", 2020-02-15). This commit makes the same change to "git
+ rebase --apply" that 2ac0d6273f made to "git rebase" without any backend
+ specific options. As the messages are changed to use an existing format
+ any scripts that can parse the reflog messages of the default rebase
+ backend should be unaffected by this change.
+
+ There are existing tests for the messages from both backends which are
+ adjusted to ensure that they do not get out of sync in the future.
@@ builtin/rebase.c: static int run_am(struct rebase_options *opts)
am.git_cmd = 1;
strvec_push(&am.args, "am");
-
-+ strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
++ strvec_pushf(&am.env, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
+ getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
if (opts->action && !strcmp("continue", opts->action)) {
strvec_push(&am.args, "--resolved");
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
+ strbuf_addf(&msg, "%s (start): checkout %s",
getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
ropts.oid = &options.onto->object.oid;
- ropts.orig_head = &options.orig_head,
+ ropts.orig_head = &options.orig_head->object.oid,
## t/t3406-rebase-message.sh ##
@@ t/t3406-rebase-message.sh: test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
7: d2c1dfbcd5e ! 7: aa808725fb8 rebase --abort: improve reflog message
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
+ strbuf_addf(&head_msg, "%s (abort): returning to %s",
+ getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
+ options.head_name ? options.head_name
-+ : oid_to_hex(&options.orig_head));
- ropts.oid = &options.orig_head;
++ : oid_to_hex(&options.orig_head->object.oid));
+ ropts.oid = &options.orig_head->object.oid;
+ ropts.head_msg = head_msg.buf;
ropts.branch = options.head_name;
ropts.flags = RESET_HEAD_HARD;
- ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
if (reset_head(the_repository, &ropts) < 0)
die(_("could not move back to %s"),
- oid_to_hex(&options.orig_head));
+ oid_to_hex(&options.orig_head->object.oid));
## t/t3406-rebase-message.sh ##
@@ t/t3406-rebase-message.sh: test_reflog () { 8: b0d21affa78 ! 8: f9c8664b883 rebase: cleanup action handling
@@ builtin/rebase.c: static int run_sequencer_rebase(struct rebase_options *opts,
return ret;
@@ builtin/rebase.c: static int run_am(struct rebase_options *opts)
strvec_push(&am.args, "am");
- strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
+ strvec_pushf(&am.env, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
- if (opts->action && !strcmp("continue", opts->action)) {
+ if (opts->action == ACTION_CONTINUE) {
@@ builtin/rebase.c: static int run_specific_rebase(struct rebase_options *opts, en
else
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
struct strbuf buf = STRBUF_INIT;
- struct object_id merge_base;
+ struct object_id branch_base;
int ignore_whitespace = 0;
- enum action action = ACTION_NONE;
const char *gpg_sign = NULL;
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
ACTION_SHOW_CURRENT_PATCH),
OPT_CALLBACK_F(0, "apply", &options, NULL,
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- if (preserve_merges_selected)
- die(_("--preserve-merges was replaced by --rebase-merges"));
+ } else if (is_directory(merge_dir())) {
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "%s/rewritten", merge_dir());
+- if (!(action == ACTION_ABORT) && is_directory(buf.buf)) {
++ if (!(options.action == ACTION_ABORT) && is_directory(buf.buf)) {
+ die(_("rebase --preserve-merges (-p) is no longer supported.\n"
+ "Use git rebase --abort to terminate current rebase.\n"
+ "Or downgrade to v2.33, or earlier, to complete the rebase."));
+@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
+ "Note: Your pull.rebase configuration may also be set to 'preserve',\n"
+ "which is no longer supported; use 'merges' instead"));
- if (action != ACTION_NONE && total_argc != 2) {
+ if (options.action != ACTION_NONE && total_argc != 2) {
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
}
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.root && options.fork_point > 0)
- die(_("cannot combine '--root' with '--fork-point'"));
+ die(_("options '%s' and '%s' cannot be used together"), "--root", "--fork-point");
- if (action != ACTION_NONE && !in_progress)
+ if (options.action != ACTION_NONE && !in_progress)
Signature · 1 line
gitgitgadget
PATCH v3 1/8 rebase --apply: remove duplicated code
From: Phillip Wood <[email protected]>
Use move_to_original_branch() when reattaching HEAD after a fast-forward rather than open coding a copy of that code. move_to_original_branch() does not call reset_head() if head_name is NULL but there should be no user visible changes even though we currently call reset_head() in that case. The reason for this is that the reset_head() call does not add a message to the reflog because we're not changing the commit that HEAD points to and so lock_ref_for_update() elides the update. When head_name is not NULL then reset_head() behaves like "git symbolic-ref" and so the reflog is updated.
Note that the removal of "strbuf_release(&msg)" is safe as there is an identical call just above this hunk which can be seen by viewing the diff with -U6.
--- builtin/rebase.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
builtin/rebase.c +1 -10
@@ -1808,19 +1808,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)* If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ -strbuf_reset (&msg);if (oideq (&branch_base, &options.orig_head->object.oid)) {printf (_ ("Fast-forwarded %s to %s.\n" ), branch_name, options.onto_name); -strbuf_addf (&msg,"rebase finished: %s onto %s" , - options.head_name ? options.head_name :"detached HEAD" , -oid_to_hex (&options.onto->object.oid)); -memset (&ropts,0 ,sizeof (ropts)); - ropts.branch = options.head_name; - ropts.flags =RESET_HEAD_REFS_ONLY ; - ropts.head_msg = msg.buf; -reset_head (the_repository, &ropts); -strbuf_release (&msg); +move_to_original_branch (&options); ret =finish_rebase (&options); goto cleanup; }
Signature · 1 line
gitgitgadget
Re: [PATCH v3 1/8] rebase --apply: remove duplicated code
Quoted 7 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Use
move_to_original_branch()when reattaching HEAD after a fast-forward rather than open coding a copy of that code.move_to_original_branch()does not callreset_head()ifhead_nameis NULL but there should be no user visible changes even though we currently callreset_head()in that case.
move_to_original_branch() uses both .head_msg and .branch_msg and uses different messages for them, but the original code below only feeds .head_msg while .branch_msg leaves NULL, which leads reset.c::update_refs() to use the same message as .head_msg when it wants to use .branch_msg (i.e. the message recorded in the reflog of the branch).
Doesn't this difference result in a different behaviour?
Quoted 7 lines
The reason for this is that the
reset_head()call does not add a message to the reflog because we're not changing the commit that HEAD points to and solock_ref_for_update()elides the update. Whenhead_nameis not NULL thenreset_head()behaves like "git symbolic-ref" and so the reflog is updated.Note that the removal of "
strbuf_release(&msg)" is safe as there is an
The patch is removing strbuf_reset(), not _release(), here, though.
We have released already so there is no point to reset it again, so the removal is still safe.
Quoted 33 lines
identical call just above this hunk which can be seen by viewing the diff with -U6.
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)builtin/rebase.c +1
-10@@ -1808,19 +1808,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)* If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ -strbuf_reset (&msg);if (oideq (&branch_base, &options.orig_head->object.oid)) {printf (_ ("Fast-forwarded %s to %s.\n" ), branch_name, options.onto_name); -strbuf_addf (&msg,"rebase finished: %s onto %s" , - options.head_name ? options.head_name :"detached HEAD" , -oid_to_hex (&options.onto->object.oid)); -memset (&ropts,0 ,sizeof (ropts)); - ropts.branch = options.head_name; - ropts.flags =RESET_HEAD_REFS_ONLY ; - ropts.head_msg = msg.buf; -reset_head (the_repository, &ropts); -strbuf_release (&msg); +move_to_original_branch (&options); ret =finish_rebase (&options); goto cleanup; }
Re: [PATCH v3 1/8] rebase --apply: remove duplicated code
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Use
move_to_original_branch()when reattaching HEAD after a fast-forward rather than open coding a copy of that code.move_to_original_branch()does not callreset_head()ifhead_nameis NULL but there should be no user visible changes even though we currently callreset_head()in that case.
Quoted 8 lines
move_to_original_branch()uses both .head_msg and .branch_msg and uses different messages for them, but the original code below only feeds .head_msg while .branch_msg leaves NULL, which leadsreset.c::update_refs()to use the same message as .head_msg when it wants to use .branch_msg (i.e. the message recorded in the reflog of the branch).Doesn't this difference result in a different behaviour?
I think "git rebase --apply A B" when B is already an descendant of A with a single strand of pearls would trigger the new logic, and instead of the old "rebase finished: %s onto %s" message used for both reflogs, calling move_to_original_branch() will give us "rebase
finished: %s onto %s" in the branch reflog, while "rebase finished:
returning to %s" in the HEAD reflog.
Note that I am not saying we should not change the behaviour. Saying "returning to X" in the reflog of HEAD may arguably be better than using the same "rebased X onto Y" for reflogs for both HEAD and the underlying branch.
But if that is what is going on, we should record it as improving the reflog message, not removing duplicated code.
Also, it would be good to have a test that demonstrates how the contents of the reflog changes with this change. It took me some time to figure out how to reach that codepath, even though it was relatively easy to see how the reflog message(s) used before and after the patch are different.
Quoted 6 lines
Quoted 5 lines
The reason for this is that the
reset_head()call does not add a message to the reflog because we're not changing the commit that HEAD points to and solock_ref_for_update()elides the update. Whenhead_nameis not NULL thenreset_head()behaves like "git symbolic-ref" and so the reflog is updated.
Quoted 6 lines
Quoted 1 line
Note that the removal of "
strbuf_release(&msg)" is safe as there is anThe patch is removing
strbuf_reset(), not_release(), here, though.We have released already so there is no point to reset it again, so the removal is still safe.
Thanks.
Re: [PATCH v3 1/8] rebase --apply: remove duplicated code
Hi Junio
Quoted 12 lines · Junio C Hamano
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Use
move_to_original_branch()when reattaching HEAD after a fast-forward rather than open coding a copy of that code.move_to_original_branch()does not callreset_head()ifhead_nameis NULL but there should be no user visible changes even though we currently callreset_head()in that case.
Quoted 8 lines
Quoted 8 lines
move_to_original_branch()uses both .head_msg and .branch_msg and uses different messages for them, but the original code below only feeds .head_msg while .branch_msg leaves NULL, which leadsreset.c::update_refs()to use the same message as .head_msg when it wants to use .branch_msg (i.e. the message recorded in the reflog of the branch).Doesn't this difference result in a different behaviour?
Yes, you're right
Quoted 12 lines
I think "git rebase --apply A B" when B is already an descendant of A with a single strand of pearls would trigger the new logic, and instead of the old "rebase finished: %s onto %s" message used for both reflogs, calling
move_to_original_branch()will give us "rebase
finished: %s onto %s" in the branch reflog, while "rebase finished:
returning to %s" in the HEAD reflog.Note that I am not saying we should not change the behaviour. Saying "returning to X" in the reflog of HEAD may arguably be better than using the same "rebased X onto Y" for reflogs for both HEAD and the underlying branch.
Quoted 8 lines
But if that is what is going on, we should record it as improving the reflog message, not removing duplicated code.
Also, it would be good to have a test that demonstrates how the contents of the reflog changes with this change. It took me some time to figure out how to reach that codepath, even though it was relatively easy to see how the reflog message(s) used before and after the patch are different.
I've just checked and the tests added in the next patch do test this path (they fail if I revert this commit). I should be able to swap the two patches round to demonstrate the change in behavior and rework the commit commit message for this patch.
Best Wishes
Phillip
Quoted 14 lines
Quoted 6 lines
Quoted 5 lines
The reason for this is that the
reset_head()call does not add a message to the reflog because we're not changing the commit that HEAD points to and solock_ref_for_update()elides the update. Whenhead_nameis not NULL thenreset_head()behaves like "git symbolic-ref" and so the reflog is updated.Quoted 6 lines
Quoted 1 line
Note that the removal of "
strbuf_release(&msg)" is safe as there is anThe patch is removing
strbuf_reset(), not_release(), here, though.We have released already so there is no point to reset it again, so the removal is still safe.
Thanks.
PATCH v3 2/8 t3406: rework rebase reflog tests
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Refactor the tests in preparation for adding more tests in the next few commits. The reworked tests use the same function for testing both the "merge" and "apply" backends. The test coverage for the "apply" backend now includes setting GIT_REFLOG_ACTION.
Note that rebasing the "conflicts" branch does not create any conflicts yet. A commit to do that will be added in the next commit and the diff ends up smaller if we have don't rename the branch when it is added.
--- t/t3406-rebase-message.sh | 115 +++++++++++++++++++++++++------------- 1 file changed, 76 insertions(+), 39 deletions(-)
t/t3406-rebase-message.sh +76 -39
@@ -10,10 +10,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME test_expect_success 'setup' ' test_commit O fileO && test_commit X fileX && + git branch fast-forward && test_commit A fileA && test_commit B fileB && test_commit Y fileY && + git checkout -b conflicts O && + test_commit P && + test_commit Q && + git checkout -b topic O && git cherry-pick A B && test_commit Z fileZ &&
@@ -79,54 +84,86 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' test_i18ngrep "Invalid whitespace option" err ' -test_expect_success 'GIT_REFLOG_ACTION' ' - git checkout start && - test_commit reflog-onto && - git checkout -b reflog-topic start && - test_commit reflog-to-rebase && - - git rebase reflog-onto && - git log -g --format=%gs -3 >actual && - cat >expect <<-\EOF && - rebase (finish): returning to refs/heads/reflog-topic - rebase (pick): reflog-to-rebase - rebase (start): checkout reflog-onto +write_reflog_expect () { + if test $mode = --apply + then + sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//' + else + cat + fi >expect +} + +test_reflog () { + mode=$1 + reflog_action="$2" + + test_expect_success "rebase $mode reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout conflicts && + test_when_finished "git reset --hard Q" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + git rebase $mode main + ) && + + git log -g --format=%gs -4 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts + ${reflog_action:-rebase} (pick): Q + ${reflog_action:-rebase} (pick): P + ${reflog_action:-rebase} (start): checkout main EOF test_cmp expect actual && - git checkout -b reflog-prefix reflog-to-rebase && - GIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto && - git log -g --format=%gs -3 >actual && - cat >expect <<-\EOF && - change-the-reflog (finish): returning to refs/heads/reflog-prefix - change-the-reflog (pick): reflog-to-rebase - change-the-reflog (start): checkout reflog-onto + git log -g --format=%gs -1 conflicts >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): refs/heads/conflicts onto $(git rev-parse main) EOF - test_cmp expect actual -' - -test_expect_success 'rebase --apply reflog' ' - git checkout -b reflog-apply start && - old_head_reflog="$(git log -g --format=%gs -1 HEAD)" && - - git rebase --apply Y && + test_cmp expect actual && - git log -g --format=%gs -4 HEAD >actual && - cat >expect <<-EOF && - rebase finished: returning to refs/heads/reflog-apply - rebase: Z - rebase: checkout Y - $old_head_reflog + # check there is only one new entry in the branch reflog + test_cmp_rev conflicts@{1} Q + ' + + test_expect_success "rebase $mode fast-forward reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout fast-forward && + test_when_finished "git reset --hard X" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + git rebase $mode main + ) && + + git log -g --format=%gs -2 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward + ${reflog_action:-rebase} (start): checkout main EOF test_cmp expect actual && - git log -g --format=%gs -2 reflog-apply >actual && - cat >expect <<-EOF && - rebase finished: refs/heads/reflog-apply onto $(git rev-parse Y) - branch: Created from start + git log -g --format=%gs -1 fast-forward >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): refs/heads/fast-forward onto $(git rev-parse main) EOF - test_cmp expect actual -' + test_cmp expect actual && + + # check there is only one new entry in the branch reflog + test_cmp_rev fast-forward@{1} X + ' +} + +test_reflog --merge +test_reflog --merge my-reflog-action +test_reflog --apply +test_reflog --apply my-reflog-action test_expect_success 'rebase -i onto unrelated history' ' git init unrelated &&
Signature · 1 line
gitgitgadget
PATCH v3 3/8 rebase --merge: fix reflog when continuing
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The reflog message for a conflict resolution committed by "rebase --continue" looks like
rebase (continue): commit subject line
Unfortunately the reflog message each subsequent pick look like
rebase (continue) (pick): commit subject line
Fix this by setting the reflog message for "rebase --continue" in sequencer_continue() so it does not affect subsequent commits. This introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION in pick_commits(). Both of these will be fixed in a future series that stops the sequencer calling setenv().
If we fail to commit the staged changes then we error out so GIT_REFLOG_ACTION does not need to be reset in that case.
--- builtin/rebase.c | 2 -- sequencer.c | 5 +++++ t/t3406-rebase-message.sh | 9 +++++++-- 3 files changed, 12 insertions(+), 4 deletions(-)
builtin/rebase.c +0 -2
@@ -1271,8 +1271,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)int fd; options.action ="continue" ; -set_reflog_action (&options); -/* Sanity check */ if (get_oid ("HEAD" , &head))die (_ ("Cannot read HEAD" ));
sequencer.c +5 -0
@@ -4785,6 +4785,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)if (read_populate_opts (opts))return -1 ;if (is_rebase_i (opts)) { +char *previous_reflog_action; +if ((res =read_populate_todo (r, &todo_list, opts))) goto release_todo_list;
@@ -4795,10 +4797,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)unlink (rebase_path_dropped ()); } + previous_reflog_action =xstrdup (getenv (GIT_REFLOG_ACTION )); +setenv (GIT_REFLOG_ACTION ,reflog_message (opts,"continue" ,NULL ),1 );if (commit_staged_changes (r, opts, &todo_list)) { res =-1 ; goto release_todo_list; } +setenv (GIT_REFLOG_ACTION , previous_reflog_action,1 ); }else if (!file_exists (get_todo_path (opts)))return continue_single_pick (r, opts);else if ((res =read_populate_todo (r, &todo_list, opts)))
t/t3406-rebase-message.sh +7 -2
@@ -17,6 +17,7 @@ test_expect_success 'setup' ' git checkout -b conflicts O && test_commit P && + test_commit conflict-X fileX && test_commit Q && git checkout -b topic O &&
@@ -107,13 +108,17 @@ test_reflog () { GIT_REFLOG_ACTION="$reflog_action" && export GIT_REFLOG_ACTION fi && - git rebase $mode main + test_must_fail git rebase $mode main && + echo resolved >fileX && + git add fileX && + git rebase --continue ) && - git log -g --format=%gs -4 >actual && + git log -g --format=%gs -5 >actual && write_reflog_expect <<-EOF && ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts ${reflog_action:-rebase} (pick): Q + ${reflog_action:-rebase} (continue): conflict-X ${reflog_action:-rebase} (pick): P ${reflog_action:-rebase} (start): checkout main EOF
Signature · 1 line
gitgitgadget
PATCH v3 4/8 rebase --merge: fix reflog message after skipping
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The reflog message for every pick after running "rebase --skip" looks like
rebase (skip) (pick): commit subject line
Fix this by not appending " (skip)" to the reflog action.
--- builtin/rebase.c | 2 -- t/t3406-rebase-message.sh | 24 ++++++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-)
builtin/rebase.c +0 -2
@@ -1297,8 +1297,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct string_list merge_rr =STRING_LIST_INIT_DUP ; options.action ="skip" ; -set_reflog_action (&options); -rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 ); ropts.flags =RESET_HEAD_HARD ;
t/t3406-rebase-message.sh +24 -0
@@ -163,6 +163,30 @@ test_reflog () { # check there is only one new entry in the branch reflog test_cmp_rev fast-forward@{1} X ' + + test_expect_success "rebase $mode --skip reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout conflicts && + test_when_finished "git reset --hard Q" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + test_must_fail git rebase $mode main && + git rebase --skip + ) && + + git log -g --format=%gs -4 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts + ${reflog_action:-rebase} (pick): Q + ${reflog_action:-rebase} (pick): P + ${reflog_action:-rebase} (start): checkout main + EOF + test_cmp expect actual + ' } test_reflog --merge
Signature · 1 line
gitgitgadget
PATCH v3 5/8 rebase --apply: respect GIT_REFLOG_ACTION
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The reflog messages when finishing a rebase hard code "rebase" rather than using GIT_REFLOG_ACTION.
--- builtin/rebase.c | 7 ++++--- t/t3406-rebase-message.sh | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)
builtin/rebase.c +4 -3
@@ -582,10 +582,11 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"rebase finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid)); -strbuf_addf (&head_reflog,"rebase finished: returning to %s" , - opts->head_name); +strbuf_addf (&head_reflog,"%s finished: returning to %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name); ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ; ropts.branch_msg = branch_reflog.buf;
t/t3406-rebase-message.sh +1 -1
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' write_reflog_expect () { if test $mode = --apply then - sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//' + sed 's/(finish)/finished/; s/ ([^)]*)//' else cat fi >expect
Signature · 1 line
gitgitgadget
PATCH v3 6/8 rebase --apply: make reflog messages match rebase --merge
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The apply backend creates slightly different reflog messages to the merge backend when starting or finishing a rebase and when picking commits. These differences make it harder than it needs to be to parse the reflog (I have a script that reads the finishing messages from rebase and it is a pain to have to accommodate two different message formats). While it is possible to determine the backend used for a rebase from the reflog messages, the differences are not designed for that purpose. c2417d3af7 (rebase: drop '-i' from the reflog for interactive-based rebases, 2020-02-15) removed the clear distinction between the reflog messages of the two backends without complaint.
As the merge backend is the default it is likely to be the format most common in existing reflogs. For that reason the apply backend is changed to format its reflog messages to match the merge backend as closely as possible. Note that there is still a difference as when committing a conflict resolution the apply backend will use "(pick)" rather than "(continue)" because it is not currently possible to change the message for a single commit.
In addition to c2417d3af7 we also changed the reflog messages in 68aa495b59 (rebase: implement --merge via the interactive machinery, 2018-12-11) and 2ac0d6273f (rebase: change the default backend from "am" to "merge", 2020-02-15). This commit makes the same change to "git rebase --apply" that 2ac0d6273f made to "git rebase" without any backend specific options. As the messages are changed to use an existing format any scripts that can parse the reflog messages of the default rebase backend should be unaffected by this change.
There are existing tests for the messages from both backends which are adjusted to ensure that they do not get out of sync in the future.
--- builtin/rebase.c | 9 +++++---- t/t3406-rebase-message.sh | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-)
builtin/rebase.c +5 -4
@@ -582,10 +582,10 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s (finish): %s onto %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid)); -strbuf_addf (&head_reflog,"%s finished: returning to %s" , +strbuf_addf (&head_reflog,"%s (finish): returning to %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name); ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ;
@@ -615,7 +615,8 @@ static int run_am(struct rebase_options *opts) am.git_cmd =1 ;strvec_push (&am.args,"am" ); - +strvec_pushf (&am.env,GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ));if (opts->action && !strcmp ("continue" , opts->action)) {strvec_push (&am.args,"--resolved" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg);
@@ -1789,7 +1790,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)printf (_ ("First, rewinding head to replay your work on top of " "it...\n" )); -strbuf_addf (&msg,"%s: checkout %s" , +strbuf_addf (&msg,"%s (start): checkout %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), options.onto_name); ropts.oid = &options.onto->object.oid; ropts.orig_head = &options.orig_head->object.oid,
t/t3406-rebase-message.sh +1 -1
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' write_reflog_expect () { if test $mode = --apply then - sed 's/(finish)/finished/; s/ ([^)]*)//' + sed 's/(continue)/(pick)/' else cat fi >expect
Signature · 1 line
gitgitgadget
PATCH v3 7/8 rebase --abort: improve reflog message
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
When aborting a rebase the reflog message looks like
rebase (abort): updating HEAD
which is not very informative. Improve the message by mentioning the branch that we are returning to as we do at the end of a successful rebase so it looks like.
rebase (abort): returning to refs/heads/topic
If GIT_REFLOG_ACTION is set in the environment we no longer omit "(abort)" from the reflog message. We don't omit "(start)" and "(finish)" when starting and finishing a rebase in that case so we shouldn't omit "(abort)".
--- builtin/rebase.c | 28 ++++++--------------- t/t3406-rebase-message.sh | 51 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 20 deletions(-)
builtin/rebase.c +8 -20
@@ -1001,23 +1001,6 @@ static void NORETURN error_on_missing_default_upstream(void)exit (1 ); } -static void set_reflog_action (struct rebase_options *options) -{ -const char *env; -struct strbuf buf =STRBUF_INIT ; - -if (!is_merge (options)) -return ; - - env =getenv (GIT_REFLOG_ACTION_ENVIRONMENT ); -if (env &&strcmp ("rebase" , env)) -return ;/* only override it if it is "rebase" */ - -strbuf_addf (&buf,"rebase (%s)" , options->action); -setenv (GIT_REFLOG_ACTION_ENVIRONMENT , buf.buf,1 ); -strbuf_release (&buf); -} -static int check_exec_cmd (const char *cmd) {if (strchr (cmd,'\n' ))
@@ -1311,18 +1294,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }case ACTION_ABORT : {struct string_list merge_rr =STRING_LIST_INIT_DUP ; - options.action ="abort" ; -set_reflog_action (&options); +struct strbuf head_msg =STRBUF_INIT ; + options.action ="abort" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 );if (read_basic_state (&options))exit (1 ); + +strbuf_addf (&head_msg,"%s (abort): returning to %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), + options.head_name ? options.head_name + :oid_to_hex (&options.orig_head->object.oid)); ropts.oid = &options.orig_head->object.oid; + ropts.head_msg = head_msg.buf; ropts.branch = options.head_name; ropts.flags =RESET_HEAD_HARD ; - ropts.default_reflog_action =DEFAULT_REFLOG_ACTION ;if (reset_head (the_repository, &ropts) <0 )die (_ ("could not move back to %s" ),oid_to_hex (&options.orig_head->object.oid));
t/t3406-rebase-message.sh +51 -0
57 lines not on this page · raw message
Signature · 1 line
gitgitgadget
PATCH v3 8/8 rebase: cleanup action handling
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Treating the action as a string is a hang over from the scripted rebase. The last commit removed the only remaining use of the action that required a string so lets convert the other action users to use the existing action enum instead. If we ever need the action name as a string in the future the action_names array exists exactly for that purpose.
--- builtin/rebase.c | 93 +++++++++++++++++++++++------------------------- 1 file changed, 44 insertions(+), 49 deletions(-)
builtin/rebase.c +44 -49
@@ -59,6 +59,26 @@ enum empty_type {EMPTY_ASK }; +enum action { +ACTION_NONE =0 , +ACTION_CONTINUE , +ACTION_SKIP , +ACTION_ABORT , +ACTION_QUIT , +ACTION_EDIT_TODO , +ACTION_SHOW_CURRENT_PATCH +}; + +static const char *action_names[] = { +"undefined" , +"continue" , +"skip" , +"abort" , +"quit" , +"edit_todo" , +"show_current_patch" +}; +struct rebase_options {enum rebase_type type;enum empty_type empty;
@@ -85,7 +105,7 @@ struct rebase_options {REBASE_INTERACTIVE_EXPLICIT =1 <<4 , } flags;struct strvec git_am_opts; -const char *action; +enum action action;int signoff;int allow_rerere_autoupdate;int keep_empty;
@@ -156,24 +176,6 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)return replay; } -enum action { -ACTION_NONE =0 , -ACTION_CONTINUE , -ACTION_SKIP , -ACTION_ABORT , -ACTION_QUIT , -ACTION_EDIT_TODO , -ACTION_SHOW_CURRENT_PATCH -}; - -static const char *action_names[] = {"undefined" , -"continue" , -"skip" , -"abort" , -"quit" , -"edit_todo" , -"show_current_patch" }; -static int edit_todo_file (unsigned flags) {const char *todo_file =rebase_path_todo ();
@@ -310,8 +312,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)return ret; } -static int run_sequencer_rebase (struct rebase_options *opts, -enum action command) +static int run_sequencer_rebase (struct rebase_options *opts) {unsigned flags =0 ;int abbreviate_commands =0 , ret =0 ;
@@ -326,7 +327,7 @@ static int run_sequencer_rebase(struct rebase_options *opts, flags |= opts->reapply_cherry_picks ?TODO_LIST_REAPPLY_CHERRY_PICKS :0 ; flags |= opts->flags &REBASE_NO_QUIET ?TODO_LIST_WARN_SKIPPED_CHERRY_PICKS :0 ; -switch (command) { +switch (opts->action) {case ACTION_NONE : {if (!opts->onto && !opts->upstream)die (_ ("a base commit must be provided with --upstream or --onto" ));
@@ -359,7 +360,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,break ; }default : -BUG ("invalid command '%d'" , command); +BUG ("invalid command '%d'" , opts->action); }return ret;
@@ -617,7 +618,7 @@ static int run_am(struct rebase_options *opts)strvec_push (&am.args,"am" );strvec_pushf (&am.env,GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT )); -if (opts->action && !strcmp ("continue" , opts->action)) { +if (opts->action ==ACTION_CONTINUE ) {strvec_push (&am.args,"--resolved" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg);if (opts->gpg_sign_opt)
@@ -628,7 +629,7 @@ static int run_am(struct rebase_options *opts)return move_to_original_branch (opts); } -if (opts->action && !strcmp ("skip" , opts->action)) { +if (opts->action ==ACTION_SKIP ) {strvec_push (&am.args,"--skip" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); status =run_command (&am);
@@ -637,7 +638,7 @@ static int run_am(struct rebase_options *opts)return move_to_original_branch (opts); } -if (opts->action && !strcmp ("show-current-patch" , opts->action)) { +if (opts->action ==ACTION_SHOW_CURRENT_PATCH ) {strvec_push (&am.args,"--show-current-patch" );return run_command (&am); }
@@ -730,7 +731,7 @@ static int run_am(struct rebase_options *opts)return status; } -static int run_specific_rebase (struct rebase_options *opts,enum action action) +static int run_specific_rebase (struct rebase_options *opts) {int status;
@@ -748,7 +749,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action) opts->gpg_sign_opt = tmp; } - status =run_sequencer_rebase (opts, action); + status =run_sequencer_rebase (opts); }else if (opts->type ==REBASE_APPLY ) status =run_am (opts);else
@@ -1025,7 +1026,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct strbuf buf =STRBUF_INIT ;struct object_id branch_base;int ignore_whitespace =0 ; -enum action action =ACTION_NONE ;const char *gpg_sign =NULL ;struct string_list exec =STRING_LIST_INIT_NODUP ;const char *rebase_merges =NULL ;
@@ -1074,18 +1074,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)OPT_BIT (0 ,"no-ff" , &options.flags,N_ ("cherry-pick all commits, even if unchanged" ),REBASE_FORCE ), -OPT_CMDMODE (0 ,"continue" , &action,N_ ("continue" ), +OPT_CMDMODE (0 ,"continue" , &options.action,N_ ("continue" ),ACTION_CONTINUE ), -OPT_CMDMODE (0 ,"skip" , &action, +OPT_CMDMODE (0 ,"skip" , &options.action,N_ ("skip current patch and continue" ),ACTION_SKIP ), -OPT_CMDMODE (0 ,"abort" , &action, +OPT_CMDMODE (0 ,"abort" , &options.action,N_ ("abort and check out the original branch" ),ACTION_ABORT ), -OPT_CMDMODE (0 ,"quit" , &action, +OPT_CMDMODE (0 ,"quit" , &options.action,N_ ("abort but keep HEAD where it is" ),ACTION_QUIT ), -OPT_CMDMODE (0 ,"edit-todo" , &action,N_ ("edit the todo list " +OPT_CMDMODE (0 ,"edit-todo" , &options.action,N_ ("edit the todo list " "during an interactive rebase" ),ACTION_EDIT_TODO ), -OPT_CMDMODE (0 ,"show-current-patch" , &action, +OPT_CMDMODE (0 ,"show-current-patch" , &options.action,N_ ("show the patch file being applied or merged" ),ACTION_SHOW_CURRENT_PATCH ), OPT_CALLBACK_F(0, "apply", &options, NULL,
@@ -1174,7 +1174,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }else if (is_directory (merge_dir ())) {strbuf_reset (&buf);strbuf_addf (&buf,"%s/rewritten" ,merge_dir ()); -if (!(action ==ACTION_ABORT ) &&is_directory (buf.buf)) { +if (!(options.action ==ACTION_ABORT ) &&is_directory (buf.buf)) {die (_ ("`rebase --preserve-merges` (-p) is no longer supported.\n" "Use `git rebase --abort` to terminate current rebase.\n" "Or downgrade to v2.33, or earlier, to complete the rebase." ));
@@ -1201,7 +1201,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)"Note: Your `pull.rebase` configuration may also be set to 'preserve',\n" "which is no longer supported; use 'merges' instead")); -if (action !=ACTION_NONE && total_argc !=2 ) { +if (options.action !=ACTION_NONE && total_argc !=2 ) {usage_with_options (builtin_rebase_usage, builtin_rebase_options); }
@@ -1232,11 +1232,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (options.root && options.fork_point >0 )die (_ ("options '%s' and '%s' cannot be used together" ),"--root" ,"--fork-point" ); -if (action !=ACTION_NONE && !in_progress) +if (options.action !=ACTION_NONE && !in_progress)die (_ ("No rebase in progress?" ));setenv (GIT_REFLOG_ACTION_ENVIRONMENT ,"rebase" ,0 ); -if (action ==ACTION_EDIT_TODO && !is_merge (&options)) +if (options.action ==ACTION_EDIT_TODO && !is_merge (&options))die (_ ("The --edit-todo action can only be used during " "interactive rebase." ));
@@ -1246,16 +1246,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)else if (exec.nr)trace2_cmd_mode ("interactive-exec" );else -trace2_cmd_mode (action_names[action]); +trace2_cmd_mode (action_names[options.action]); } -switch (action) { +switch (options.action) {case ACTION_CONTINUE : {struct object_id head;struct lock_file lock_file =LOCK_INIT ;int fd; - options.action ="continue" ;/* Sanity check */ if (get_oid ("HEAD" , &head))die (_ ("Cannot read HEAD" ));
@@ -1281,7 +1280,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)case ACTION_SKIP : {struct string_list merge_rr =STRING_LIST_INIT_DUP ; - options.action ="skip" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 ); ropts.flags =RESET_HEAD_HARD ;
@@ -1296,7 +1294,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct string_list merge_rr =STRING_LIST_INIT_DUP ;struct strbuf head_msg =STRBUF_INIT ; - options.action ="abort" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 );
@@ -1336,17 +1333,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) goto cleanup; }case ACTION_EDIT_TODO : - options.action ="edit-todo" ; options.dont_finish_rebase =1 ; goto run_rebase;case ACTION_SHOW_CURRENT_PATCH : - options.action ="show-current-patch" ; options.dont_finish_rebase =1 ; goto run_rebase;case ACTION_NONE :break ;default : -BUG ("action: %d" , action); +BUG ("action: %d" , options.action); }/* Make sure no rebase is in progress */
@@ -1370,7 +1365,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }if ((options.flags &REBASE_INTERACTIVE_EXPLICIT ) || - (action !=ACTION_NONE ) || + (options.action !=ACTION_NONE ) || (exec.nr >0 ) || options.autosquash) { allow_preemptive_ff =0 ;
@@ -1812,7 +1807,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.revisions = revisions.buf; run_rebase: - ret =run_specific_rebase (&options, action); + ret =run_specific_rebase (&options); cleanup:strbuf_release (&buf);
Signature · 1 line
gitgitgadget
Quoted 7 lines · Phillip Wood via GitGitGadget
This series fixes some bugs in the reflog messages when rebasing and changes the reflog messages of "rebase --apply" to match "rebase --merge" with the aim of making the reflog easier to parse.
Thanks to everyone who commented on V2, I've added the review club participants that I've got address for to the cc list. I've rebased onto pw/rebase-keep-base-fixes.
Thanks for identifying the base clearly.
If "What's cooking" is correct, the base topic is waiting for an update? I'd prefer to see us build new stuff on top of solidified ground.
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Phillip Wood via GitGitGadget
This series fixes some bugs in the reflog messages when rebasing and changes the reflog messages of "rebase --apply" to match "rebase --merge" with the aim of making the reflog easier to parse.
Thanks to everyone who commented on V2, I've added the review club participants that I've got address for to the cc list. I've rebased onto pw/rebase-keep-base-fixes.
Quoted 5 lines
Thanks for identifying the base clearly.
If "What's cooking" is correct, the base topic is waiting for an update? I'd prefer to see us build new stuff on top of solidified ground.
Oh sorry, I thought I'd submitted a new version last week but it seems I pushed it to gitgitgadet, updated the cover letter but forgot to say "/submit". I'm hoping that will be the final version and provide a stable base for this series.
Best Wishes
Phillip
Quoted 3 lines · Phillip Wood
I pushed it to gitgitgadet, updated the cover letter but forgot to say "/submit". I'm hoping that will be the final version and provide a stable base for this series.
Thanks, will look forward to.
v4 cover rebase: make reflog messages independent of the backend
In reply to Phillip Wood via GitGitGadgetThis series fixes some bugs in the reflog messages when rebasing and changes the reflog messages of "rebase --apply" to match "rebase --merge" with the aim of making the reflog easier to parse.
Thanks to Junio for his comments, here are the changes since V3:
* Patch 1: Swapped with patch 2. Small change to fast-forward tests to make them pass
* Patch 2: Swapped with patch 1. Reworded commit message and added a change to the fast-forward test to reflect the change in reflog message introduced by this patch
The rest of the patches are unchanged.
V2 Cover Letter
Thanks to everyone who commented on V2, I've added the review club participants that I've got address for to the cc list. I've rebased onto pw/rebase-keep-base-fixes.
Change since V2:
* Patch 1: Reworded the commit message to address the concerns in [1,2] about the behavior when head_name is NULL. There is also a small change due to being rebased.
* Patch 2: Unchanged. There wasn't much love for parameterized tests in review club but we want to ensure both backends produce the same messages I think this is the safest way to achieve that. Using separate tests makes it too easy to introduce subtle differences in the testing of the two backends.
* Patch 3: Added a note to the commit message to address the concerns in
[1] about not resetting GIT_REFLOG_ACTION when we return early.
* Patches 4 & 5: Unchanged.
* Patch 6: Reworded the commit message to make a stronger argument for this change. There are concerns about backwards compatibility in [1,3,4] but
(i) we have made similar changes in the past without complaints and (ii) we're changing the message to an existing format. There is also a small change due to being rebased.
* Patches 7 & 8: Small changes due to rebase.
[1]
https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?pli=1#heading=h.t6g5l2t5ibzw
[2] https://lore.kernel.org/git/[email protected]/ [3]
https://lore.kernel.org/git/[email protected]/ [4]
https://lore.kernel.org/git/[email protected]/
V2 Cover Letter:
Thanks to Christian and Elijah for their comments on V1.
I've updated commit message for patch 1 to try and be clearer about the removal of a call to strbuf_release() and spilt out the test changes from the old patch 2 into a separate preparatory patch.
V1 Cover Letter:
This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.
* improve rebase reflog test coverage
* rebase --merge: fix reflog messages for --continue and --skip
* rebase --apply: respect GIT_REFLOG_ACTION
* rebase --abort: improve reflog message
* unify reflog messages between the two rebase backends
This series is based on pw/use-inprocess-checkout-in-rebase
Phillip Wood (8):
t3406: rework rebase reflog tests
rebase --apply: improve fast-forward reflog message rebase --merge: fix reflog when continuing rebase --merge: fix reflog message after skipping rebase --apply: respect GIT_REFLOG_ACTION
rebase --apply: make reflog messages match rebase --merge rebase --abort: improve reflog message
rebase: cleanup action handling
builtin/rebase.c | 146 ++++++++++++------------------ sequencer.c | 5 ++ t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++------- 3 files changed, 215 insertions(+), 121 deletions(-)
base-commit: aa1df8146d70bb85c63b0999868fe29aebc1173e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Range-diff vs v3:
2: b9255ad35d2 ! 1: 8d5ae067ce3 t3406: rework rebase reflog tests
@@ Commit message
the "merge" and "apply" backends. The test coverage for the "apply" backend now includes setting GIT_REFLOG_ACTION.
- Note that rebasing the "conflicts" branch does not create any
- conflicts yet. A commit to do that will be added in the next commit
- and the diff ends up smaller if we have don't rename the branch when
- it is added.
+ Note that rebasing the "conflicts" branch does not create any conflicts
+ yet. A commit to do that will be added shortly and the diff ends up
+ smaller if we have don't rename the branch when it is added.
@@ t/t3406-rebase-message.sh: test_expect_success 'error out early upon -C<n> or --
+ ) &&
+
+ git log -g --format=%gs -2 >actual &&
++ if test "$mode" = "--apply"
++ then
++ finish_msg="refs/heads/fast-forward onto $(git rev-parse main)"
++ else
++ finish_msg="returning to refs/heads/fast-forward"
++ fi &&
+ write_reflog_expect <<-EOF &&
-+ ${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward
++ ${reflog_action:-rebase} (finish): $finish_msg
+ ${reflog_action:-rebase} (start): checkout main
EOF
test_cmp expect actual &&
1: a84cf971a75 ! 2: 12701ef7c7c rebase --apply: remove duplicated code
@@ Metadata
Author: Phillip Wood <[email protected]>
## Commit message ##
- rebase --apply: remove duplicated code
+ rebase --apply: improve fast-forward reflog message
- Use move_to_original_branch() when reattaching HEAD after a fast-forward
- rather than open coding a copy of that code. move_to_original_branch()
- does not call reset_head() if head_name is NULL but there should be no
- user visible changes even though we currently call reset_head() in that
- case. The reason for this is that the reset_head() call does not add a
- message to the reflog because we're not changing the commit that HEAD
- points to and so lock_ref_for_update() elides the update. When head_name
- is not NULL then reset_head() behaves like "git symbolic-ref" and so the
- reflog is updated.
+ Using move_to_original_branch() when reattaching HEAD after a
+ fast-forward improves HEAD's reflog message when rebasing a branch. This
+ is because it uses separate reflog messages for "HEAD" and
+ "branch". HEAD's reflog will now record which branch we're returning to.
- Note that the removal of "strbuf_release(&msg)" is safe as there is an
- identical call just above this hunk which can be seen by viewing the
- diff with -U6.
+ When rebasing a detached HEAD there is no change in behavior. We
+ currently call reset_head() when head_name is NULL but
+ move_to_original_branch() does not. However the existing call does not
+ add a message to the reflog because we're not changing the commit that
+ HEAD points to and so lock_ref_for_update() skips the update.
+ Note that the removal of "strbuf_reset(&msg)" is safe as there is a call
+ to strbuf_release(&msf) just above this hunk which can be seen by
+ viewing the diff with -U6.
+
+ Helped-by: Junio C Hamano <[email protected]>
## builtin/rebase.c ##
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
ret = finish_rebase(&options);
goto cleanup;
}
+
+ ## t/t3406-rebase-message.sh ##
+@@ t/t3406-rebase-message.sh: test_reflog () {
+ ) &&
+
+ git log -g --format=%gs -2 >actual &&
+- if test "$mode" = "--apply"
+- then
+- finish_msg="refs/heads/fast-forward onto $(git rev-parse main)"
+- else
+- finish_msg="returning to refs/heads/fast-forward"
+- fi &&
+ write_reflog_expect <<-EOF &&
+- ${reflog_action:-rebase} (finish): $finish_msg
++ ${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward
+ ${reflog_action:-rebase} (start): checkout main
+ EOF
+ test_cmp expect actual &&
3: ea4da25a19c = 3: 2c965f4b97c rebase --merge: fix reflog when continuing 4: 225ff4baef7 = 4: 17138d910f0 rebase --merge: fix reflog message after skipping 5: 1094681eb11 = 5: 0c71c732904 rebase --apply: respect GIT_REFLOG_ACTION 6: a5338e6bdd8 = 6: 3f6b2e39f40 rebase --apply: make reflog messages match rebase --merge 7: aa808725fb8 = 7: c8fa57f129d rebase --abort: improve reflog message 8: f9c8664b883 = 8: ed800844ba1 rebase: cleanup action handling
Signature · 1 line
gitgitgadget
PATCH v4 1/8 t3406: rework rebase reflog tests
From: Phillip Wood <[email protected]>
Refactor the tests in preparation for adding more tests in the next few commits. The reworked tests use the same function for testing both the "merge" and "apply" backends. The test coverage for the "apply" backend now includes setting GIT_REFLOG_ACTION.
Note that rebasing the "conflicts" branch does not create any conflicts yet. A commit to do that will be added shortly and the diff ends up smaller if we have don't rename the branch when it is added.
--- t/t3406-rebase-message.sh | 121 ++++++++++++++++++++++++++------------ 1 file changed, 82 insertions(+), 39 deletions(-)
t/t3406-rebase-message.sh +82 -39
@@ -10,10 +10,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME test_expect_success 'setup' ' test_commit O fileO && test_commit X fileX && + git branch fast-forward && test_commit A fileA && test_commit B fileB && test_commit Y fileY && + git checkout -b conflicts O && + test_commit P && + test_commit Q && + git checkout -b topic O && git cherry-pick A B && test_commit Z fileZ &&
@@ -79,54 +84,92 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' test_i18ngrep "Invalid whitespace option" err ' -test_expect_success 'GIT_REFLOG_ACTION' ' - git checkout start && - test_commit reflog-onto && - git checkout -b reflog-topic start && - test_commit reflog-to-rebase && - - git rebase reflog-onto && - git log -g --format=%gs -3 >actual && - cat >expect <<-\EOF && - rebase (finish): returning to refs/heads/reflog-topic - rebase (pick): reflog-to-rebase - rebase (start): checkout reflog-onto +write_reflog_expect () { + if test $mode = --apply + then + sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//' + else + cat + fi >expect +} + +test_reflog () { + mode=$1 + reflog_action="$2" + + test_expect_success "rebase $mode reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout conflicts && + test_when_finished "git reset --hard Q" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + git rebase $mode main + ) && + + git log -g --format=%gs -4 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts + ${reflog_action:-rebase} (pick): Q + ${reflog_action:-rebase} (pick): P + ${reflog_action:-rebase} (start): checkout main EOF test_cmp expect actual && - git checkout -b reflog-prefix reflog-to-rebase && - GIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto && - git log -g --format=%gs -3 >actual && - cat >expect <<-\EOF && - change-the-reflog (finish): returning to refs/heads/reflog-prefix - change-the-reflog (pick): reflog-to-rebase - change-the-reflog (start): checkout reflog-onto + git log -g --format=%gs -1 conflicts >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): refs/heads/conflicts onto $(git rev-parse main) EOF - test_cmp expect actual -' - -test_expect_success 'rebase --apply reflog' ' - git checkout -b reflog-apply start && - old_head_reflog="$(git log -g --format=%gs -1 HEAD)" && - - git rebase --apply Y && + test_cmp expect actual && - git log -g --format=%gs -4 HEAD >actual && - cat >expect <<-EOF && - rebase finished: returning to refs/heads/reflog-apply - rebase: Z - rebase: checkout Y - $old_head_reflog + # check there is only one new entry in the branch reflog + test_cmp_rev conflicts@{1} Q + ' + + test_expect_success "rebase $mode fast-forward reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout fast-forward && + test_when_finished "git reset --hard X" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + git rebase $mode main + ) && + + git log -g --format=%gs -2 >actual && + if test "$mode" = "--apply" + then + finish_msg="refs/heads/fast-forward onto $(git rev-parse main)" + else + finish_msg="returning to refs/heads/fast-forward" + fi && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): $finish_msg + ${reflog_action:-rebase} (start): checkout main EOF test_cmp expect actual && - git log -g --format=%gs -2 reflog-apply >actual && - cat >expect <<-EOF && - rebase finished: refs/heads/reflog-apply onto $(git rev-parse Y) - branch: Created from start + git log -g --format=%gs -1 fast-forward >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): refs/heads/fast-forward onto $(git rev-parse main) EOF - test_cmp expect actual -' + test_cmp expect actual && + + # check there is only one new entry in the branch reflog + test_cmp_rev fast-forward@{1} X + ' +} + +test_reflog --merge +test_reflog --merge my-reflog-action +test_reflog --apply +test_reflog --apply my-reflog-action test_expect_success 'rebase -i onto unrelated history' ' git init unrelated &&
Signature · 1 line
gitgitgadget
PATCH v4 2/8 rebase --apply: improve fast-forward reflog message
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Using move_to_original_branch() when reattaching HEAD after a fast-forward improves HEAD's reflog message when rebasing a branch. This is because it uses separate reflog messages for "HEAD" and "branch". HEAD's reflog will now record which branch we're returning to.
When rebasing a detached HEAD there is no change in behavior. We currently call reset_head() when head_name is NULL but move_to_original_branch() does not. However the existing call does not add a message to the reflog because we're not changing the commit that HEAD points to and so lock_ref_for_update() skips the update.
Note that the removal of "strbuf_reset(&msg)" is safe as there is a call to strbuf_release(&msf) just above this hunk which can be seen by viewing the diff with -U6.
--- builtin/rebase.c | 11 +---------- t/t3406-rebase-message.sh | 8 +------- 2 files changed, 2 insertions(+), 17 deletions(-)
builtin/rebase.c +1 -10
@@ -1811,19 +1811,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)* If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ -strbuf_reset (&msg);if (oideq (&branch_base, &options.orig_head->object.oid)) {printf (_ ("Fast-forwarded %s to %s.\n" ), branch_name, options.onto_name); -strbuf_addf (&msg,"rebase finished: %s onto %s" , - options.head_name ? options.head_name :"detached HEAD" , -oid_to_hex (&options.onto->object.oid)); -memset (&ropts,0 ,sizeof (ropts)); - ropts.branch = options.head_name; - ropts.flags =RESET_HEAD_REFS_ONLY ; - ropts.head_msg = msg.buf; -reset_head (the_repository, &ropts); -strbuf_release (&msg); +move_to_original_branch (&options); ret =finish_rebase (&options); goto cleanup; }
t/t3406-rebase-message.sh +1 -7
@@ -143,14 +143,8 @@ test_reflog () { ) && git log -g --format=%gs -2 >actual && - if test "$mode" = "--apply" - then - finish_msg="refs/heads/fast-forward onto $(git rev-parse main)" - else - finish_msg="returning to refs/heads/fast-forward" - fi && write_reflog_expect <<-EOF && - ${reflog_action:-rebase} (finish): $finish_msg + ${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward ${reflog_action:-rebase} (start): checkout main EOF test_cmp expect actual &&
Signature · 1 line
gitgitgadget
PATCH v4 3/8 rebase --merge: fix reflog when continuing
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The reflog message for a conflict resolution committed by "rebase --continue" looks like
rebase (continue): commit subject line
Unfortunately the reflog message each subsequent pick look like
rebase (continue) (pick): commit subject line
Fix this by setting the reflog message for "rebase --continue" in sequencer_continue() so it does not affect subsequent commits. This introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION in pick_commits(). Both of these will be fixed in a future series that stops the sequencer calling setenv().
If we fail to commit the staged changes then we error out so GIT_REFLOG_ACTION does not need to be reset in that case.
--- builtin/rebase.c | 2 -- sequencer.c | 5 +++++ t/t3406-rebase-message.sh | 9 +++++++-- 3 files changed, 12 insertions(+), 4 deletions(-)
builtin/rebase.c +0 -2
@@ -1271,8 +1271,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)int fd; options.action ="continue" ; -set_reflog_action (&options); -/* Sanity check */ if (get_oid ("HEAD" , &head))die (_ ("Cannot read HEAD" ));
sequencer.c +5 -0
@@ -4785,6 +4785,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)if (read_populate_opts (opts))return -1 ;if (is_rebase_i (opts)) { +char *previous_reflog_action; +if ((res =read_populate_todo (r, &todo_list, opts))) goto release_todo_list;
@@ -4795,10 +4797,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)unlink (rebase_path_dropped ()); } + previous_reflog_action =xstrdup (getenv (GIT_REFLOG_ACTION )); +setenv (GIT_REFLOG_ACTION ,reflog_message (opts,"continue" ,NULL ),1 );if (commit_staged_changes (r, opts, &todo_list)) { res =-1 ; goto release_todo_list; } +setenv (GIT_REFLOG_ACTION , previous_reflog_action,1 ); }else if (!file_exists (get_todo_path (opts)))return continue_single_pick (r, opts);else if ((res =read_populate_todo (r, &todo_list, opts)))
t/t3406-rebase-message.sh +7 -2
@@ -17,6 +17,7 @@ test_expect_success 'setup' ' git checkout -b conflicts O && test_commit P && + test_commit conflict-X fileX && test_commit Q && git checkout -b topic O &&
@@ -107,13 +108,17 @@ test_reflog () { GIT_REFLOG_ACTION="$reflog_action" && export GIT_REFLOG_ACTION fi && - git rebase $mode main + test_must_fail git rebase $mode main && + echo resolved >fileX && + git add fileX && + git rebase --continue ) && - git log -g --format=%gs -4 >actual && + git log -g --format=%gs -5 >actual && write_reflog_expect <<-EOF && ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts ${reflog_action:-rebase} (pick): Q + ${reflog_action:-rebase} (continue): conflict-X ${reflog_action:-rebase} (pick): P ${reflog_action:-rebase} (start): checkout main EOF
Signature · 1 line
gitgitgadget
Re: [PATCH v4 3/8] rebase --merge: fix reflog when continuing
Quoted 3 lines · Phillip Wood via GitGitGadget
... This
introduces a memory leak similar to the one leakingGIT_REFLOG_ACTIONinpick_commits().
Is it just the matter of freeing previous_reflog_action after you call setenv(), or does it take much more involved changes?
Quoted 2 lines
Both of these will be fixed in a future series that stops the sequencer calling
setenv().
If it gets fixed in a future step in the same series, that is a different matter, but if it is easy enough not to deliberately introduce a new leak, we'd prefer to do so.
Re: [PATCH v4 3/8] rebase --merge: fix reflog when continuing
Hi Junio
Quoted 8 lines · Junio C Hamano
Quoted 3 lines · Phillip Wood via GitGitGadget
... This
introduces a memory leak similar to the one leakingGIT_REFLOG_ACTIONinpick_commits().Is it just the matter of freeing
previous_reflog_actionafter you callsetenv(), or does it take much more involved changes?
We should be freeing previous_reflog_action - I had misremembered a previous discussion about setenv() but the manual page makes it quite clear that it copies the strings passed to it. However we call setenv() each time we pick a commit and that leaks the previous value. The solution is to avoid calling setenv() at all and instead use the env member of struct child_process when we run "git commit".
Quoted 6 lines
Quoted 2 lines
Both of these will be fixed in a future series that stops the sequencer calling
setenv().If it gets fixed in a future step in the same series, that is a different matter, but if it is easy enough not to deliberately introduce a new leak, we'd prefer to do so.
It's a couple of patches to fix which are more or less finished, I'm planning to send them once this series is in next.
Best Wishes
Phillip
Re: [PATCH v4 3/8] rebase --merge: fix reflog when continuing
Quoted 8 lines · Phillip Wood
Quoted 5 lines
Quoted 2 lines
Both of these will be fixed in a future series that stops the sequencer calling
setenv().If it gets fixed in a future step in the same series, that is a different matter, but if it is easy enough not to deliberately introduce a new leak, we'd prefer to do so.
It's a couple of patches to fix which are more or less finished, I'm planning to send them once this series is in next.
So we will do the "add a known breakage of the same kind as there exists others, and then later fix them all up, including the one that is added by this series, because fixes are non-trivial and this topic is easier to finish if we allowed to add a known breakage" approach? Just making sure it is what you plan to do.
Thanks.
Re: [PATCH v4 3/8] rebase --merge: fix reflog when continuing
Hi Junio
Quoted 8 lines · Junio C Hamano
Quoted 6 lines · Phillip Wood
Quoted 5 lines
Quoted 2 lines
Both of these will be fixed in a future series that stops the sequencer calling
setenv().If it gets fixed in a future step in the same series, that is a different matter, but if it is easy enough not to deliberately introduce a new leak, we'd prefer to do so.
Quoted 8 lines
Quoted 2 lines
It's a couple of patches to fix which are more or less finished, I'm planning to send them once this series is in next.
So we will do the "add a known breakage of the same kind as there exists others, and then later fix them all up, including the one that is added by this series, because fixes are non-trivial and this topic is easier to finish if we allowed to add a known breakage" approach? Just making sure it is what you plan to do.
Yes, that's right
Thanks
Phillip
Quoted 1 line
Thanks.
Re: [PATCH v4 3/8] rebase --merge: fix reflog when continuing
Quoted 13 lines · Phillip Wood
Hi Junio
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Phillip Wood
Quoted 5 lines
Quoted 2 lines
Both of these will be fixed in a future series that stops the sequencer calling
setenv().If it gets fixed in a future step in the same series, that is a different matter, but if it is easy enough not to deliberately introduce a new leak, we'd prefer to do so.
It's a couple of patches to fix which are more or less finished, I'm planning to send them once this series is in next.
Quoted 7 lines
Quoted 5 lines
So we will do the "add a known breakage of the same kind as there exists others, and then later fix them all up, including the one that is added by this series, because fixes are non-trivial and this topic is easier to finish if we allowed to add a known breakage" approach? Just making sure it is what you plan to do.
Yes, that's right
OK, I do not mind as long as we leave a NEEDSWORK note to tell others that we know the leak and promise to fix it soon (so they do not waste their effort to fix it independently).
Thanks.
PATCH v4 4/8 rebase --merge: fix reflog message after skipping
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The reflog message for every pick after running "rebase --skip" looks like
rebase (skip) (pick): commit subject line
Fix this by not appending " (skip)" to the reflog action.
--- builtin/rebase.c | 2 -- t/t3406-rebase-message.sh | 24 ++++++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-)
builtin/rebase.c +0 -2
@@ -1297,8 +1297,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct string_list merge_rr =STRING_LIST_INIT_DUP ; options.action ="skip" ; -set_reflog_action (&options); -rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 ); ropts.flags =RESET_HEAD_HARD ;
t/t3406-rebase-message.sh +24 -0
@@ -163,6 +163,30 @@ test_reflog () { # check there is only one new entry in the branch reflog test_cmp_rev fast-forward@{1} X ' + + test_expect_success "rebase $mode --skip reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" ' + git checkout conflicts && + test_when_finished "git reset --hard Q" && + + ( + if test -n "$reflog_action" + then + GIT_REFLOG_ACTION="$reflog_action" && + export GIT_REFLOG_ACTION + fi && + test_must_fail git rebase $mode main && + git rebase --skip + ) && + + git log -g --format=%gs -4 >actual && + write_reflog_expect <<-EOF && + ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts + ${reflog_action:-rebase} (pick): Q + ${reflog_action:-rebase} (pick): P + ${reflog_action:-rebase} (start): checkout main + EOF + test_cmp expect actual + ' } test_reflog --merge
Signature · 1 line
gitgitgadget
PATCH v4 5/8 rebase --apply: respect GIT_REFLOG_ACTION
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The reflog messages when finishing a rebase hard code "rebase" rather than using GIT_REFLOG_ACTION.
--- builtin/rebase.c | 7 ++++--- t/t3406-rebase-message.sh | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)
builtin/rebase.c +4 -3
@@ -582,10 +582,11 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"rebase finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid)); -strbuf_addf (&head_reflog,"rebase finished: returning to %s" , - opts->head_name); +strbuf_addf (&head_reflog,"%s finished: returning to %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name); ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ; ropts.branch_msg = branch_reflog.buf;
t/t3406-rebase-message.sh +1 -1
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' write_reflog_expect () { if test $mode = --apply then - sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//' + sed 's/(finish)/finished/; s/ ([^)]*)//' else cat fi >expect
Signature · 1 line
gitgitgadget
Re: [PATCH v4 5/8] rebase --apply: respect GIT_REFLOG_ACTION
Quoted 4 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
The reflog messages when finishing a rebase hard code "rebase" rather than using
GIT_REFLOG_ACTION.
Makes sense.
Quoted 39 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 7 ++++--- t/t3406-rebase-message.sh | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)builtin/rebase.c +4
-3@@ -582,10 +582,11 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"rebase finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid)); -strbuf_addf (&head_reflog,"rebase finished: returning to %s" , - opts->head_name); +strbuf_addf (&head_reflog,"%s finished: returning to %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name); ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ; ropts.branch_msg = branch_reflog.buf;t/t3406-rebase-message.sh +1
-1@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' write_reflog_expect () { if test $mode = --apply then - sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//' + sed 's/(finish)/finished/; s/ ([^)]*)//' else cat fi >expect
PATCH v4 6/8 rebase --apply: make reflog messages match rebase --merge
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The apply backend creates slightly different reflog messages to the merge backend when starting or finishing a rebase and when picking commits. These differences make it harder than it needs to be to parse the reflog (I have a script that reads the finishing messages from rebase and it is a pain to have to accommodate two different message formats). While it is possible to determine the backend used for a rebase from the reflog messages, the differences are not designed for that purpose. c2417d3af7 (rebase: drop '-i' from the reflog for interactive-based rebases, 2020-02-15) removed the clear distinction between the reflog messages of the two backends without complaint.
As the merge backend is the default it is likely to be the format most common in existing reflogs. For that reason the apply backend is changed to format its reflog messages to match the merge backend as closely as possible. Note that there is still a difference as when committing a conflict resolution the apply backend will use "(pick)" rather than "(continue)" because it is not currently possible to change the message for a single commit.
In addition to c2417d3af7 we also changed the reflog messages in 68aa495b59 (rebase: implement --merge via the interactive machinery, 2018-12-11) and 2ac0d6273f (rebase: change the default backend from "am" to "merge", 2020-02-15). This commit makes the same change to "git rebase --apply" that 2ac0d6273f made to "git rebase" without any backend specific options. As the messages are changed to use an existing format any scripts that can parse the reflog messages of the default rebase backend should be unaffected by this change.
There are existing tests for the messages from both backends which are adjusted to ensure that they do not get out of sync in the future.
--- builtin/rebase.c | 9 +++++---- t/t3406-rebase-message.sh | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-)
builtin/rebase.c +5 -4
@@ -582,10 +582,10 @@ static int move_to_original_branch(struct rebase_options *opts)if (!opts->onto)BUG ("move_to_original_branch without onto" ); -strbuf_addf (&branch_reflog,"%s finished: %s onto %s" , +strbuf_addf (&branch_reflog,"%s (finish): %s onto %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name,oid_to_hex (&opts->onto->object.oid)); -strbuf_addf (&head_reflog,"%s finished: returning to %s" , +strbuf_addf (&head_reflog,"%s (finish): returning to %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), opts->head_name); ropts.branch = opts->head_name; ropts.flags =RESET_HEAD_REFS_ONLY ;
@@ -615,7 +615,8 @@ static int run_am(struct rebase_options *opts) am.git_cmd =1 ;strvec_push (&am.args,"am" ); - +strvec_pushf (&am.env,GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ));if (opts->action && !strcmp ("continue" , opts->action)) {strvec_push (&am.args,"--resolved" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg);
@@ -1792,7 +1793,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)printf (_ ("First, rewinding head to replay your work on top of " "it...\n" )); -strbuf_addf (&msg,"%s: checkout %s" , +strbuf_addf (&msg,"%s (start): checkout %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), options.onto_name); ropts.oid = &options.onto->object.oid; ropts.orig_head = &options.orig_head->object.oid,
t/t3406-rebase-message.sh +1 -1
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' write_reflog_expect () { if test $mode = --apply then - sed 's/(finish)/finished/; s/ ([^)]*)//' + sed 's/(continue)/(pick)/' else cat fi >expect
Signature · 1 line
gitgitgadget
Re: [PATCH v4 6/8] rebase --apply: make reflog messages match rebase --merge
Quoted 7 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
The apply backend creates slightly different reflog messages to the merge backend when starting or finishing a rebase and when picking commits. ...
There are existing tests for the messages from both backends which are adjusted to ensure that they do not get out of sync in the future.
Nice.
PATCH v4 7/8 rebase --abort: improve reflog message
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
When aborting a rebase the reflog message looks like
rebase (abort): updating HEAD
which is not very informative. Improve the message by mentioning the branch that we are returning to as we do at the end of a successful rebase so it looks like.
rebase (abort): returning to refs/heads/topic
If GIT_REFLOG_ACTION is set in the environment we no longer omit "(abort)" from the reflog message. We don't omit "(start)" and "(finish)" when starting and finishing a rebase in that case so we shouldn't omit "(abort)".
--- builtin/rebase.c | 28 ++++++--------------- t/t3406-rebase-message.sh | 51 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 20 deletions(-)
builtin/rebase.c +8 -20
@@ -1001,23 +1001,6 @@ static void NORETURN error_on_missing_default_upstream(void)exit (1 ); } -static void set_reflog_action (struct rebase_options *options) -{ -const char *env; -struct strbuf buf =STRBUF_INIT ; - -if (!is_merge (options)) -return ; - - env =getenv (GIT_REFLOG_ACTION_ENVIRONMENT ); -if (env &&strcmp ("rebase" , env)) -return ;/* only override it if it is "rebase" */ - -strbuf_addf (&buf,"rebase (%s)" , options->action); -setenv (GIT_REFLOG_ACTION_ENVIRONMENT , buf.buf,1 ); -strbuf_release (&buf); -} -static int check_exec_cmd (const char *cmd) {if (strchr (cmd,'\n' ))
@@ -1311,18 +1294,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }case ACTION_ABORT : {struct string_list merge_rr =STRING_LIST_INIT_DUP ; - options.action ="abort" ; -set_reflog_action (&options); +struct strbuf head_msg =STRBUF_INIT ; + options.action ="abort" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 );if (read_basic_state (&options))exit (1 ); + +strbuf_addf (&head_msg,"%s (abort): returning to %s" , +getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), + options.head_name ? options.head_name + :oid_to_hex (&options.orig_head->object.oid)); ropts.oid = &options.orig_head->object.oid; + ropts.head_msg = head_msg.buf; ropts.branch = options.head_name; ropts.flags =RESET_HEAD_HARD ; - ropts.default_reflog_action =DEFAULT_REFLOG_ACTION ;if (reset_head (the_repository, &ropts) <0 )die (_ ("could not move back to %s" ),oid_to_hex (&options.orig_head->object.oid));
t/t3406-rebase-message.sh +51 -0
57 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v4 7/8] rebase --abort: improve reflog message
Quoted 10 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
When aborting a rebase the reflog message looks like
rebase (abort): updating HEAD
which is not very informative. Improve the message by mentioning the branch that we are returning to as we do at the end of a successful rebase so it looks like.
Quoted 6 lines
rebase (abort): returning to refs/heads/topic
If
GIT_REFLOG_ACTIONis set in the environment we no longer omit "(abort)" from the reflog message. We don't omit "(start)" and "(finish)" when starting and finishing a rebase in that case so we shouldn't omit "(abort)".
Super nice.
PATCH v4 8/8 rebase: cleanup action handling
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Treating the action as a string is a hang over from the scripted rebase. The last commit removed the only remaining use of the action that required a string so lets convert the other action users to use the existing action enum instead. If we ever need the action name as a string in the future the action_names array exists exactly for that purpose.
--- builtin/rebase.c | 93 +++++++++++++++++++++++------------------------- 1 file changed, 44 insertions(+), 49 deletions(-)
builtin/rebase.c +44 -49
@@ -59,6 +59,26 @@ enum empty_type {EMPTY_ASK }; +enum action { +ACTION_NONE =0 , +ACTION_CONTINUE , +ACTION_SKIP , +ACTION_ABORT , +ACTION_QUIT , +ACTION_EDIT_TODO , +ACTION_SHOW_CURRENT_PATCH +}; + +static const char *action_names[] = { +"undefined" , +"continue" , +"skip" , +"abort" , +"quit" , +"edit_todo" , +"show_current_patch" +}; +struct rebase_options {enum rebase_type type;enum empty_type empty;
@@ -85,7 +105,7 @@ struct rebase_options {REBASE_INTERACTIVE_EXPLICIT =1 <<4 , } flags;struct strvec git_am_opts; -const char *action; +enum action action;int signoff;int allow_rerere_autoupdate;int keep_empty;
@@ -156,24 +176,6 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)return replay; } -enum action { -ACTION_NONE =0 , -ACTION_CONTINUE , -ACTION_SKIP , -ACTION_ABORT , -ACTION_QUIT , -ACTION_EDIT_TODO , -ACTION_SHOW_CURRENT_PATCH -}; - -static const char *action_names[] = {"undefined" , -"continue" , -"skip" , -"abort" , -"quit" , -"edit_todo" , -"show_current_patch" }; -static int edit_todo_file (unsigned flags) {const char *todo_file =rebase_path_todo ();
@@ -310,8 +312,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)return ret; } -static int run_sequencer_rebase (struct rebase_options *opts, -enum action command) +static int run_sequencer_rebase (struct rebase_options *opts) {unsigned flags =0 ;int abbreviate_commands =0 , ret =0 ;
@@ -326,7 +327,7 @@ static int run_sequencer_rebase(struct rebase_options *opts, flags |= opts->reapply_cherry_picks ?TODO_LIST_REAPPLY_CHERRY_PICKS :0 ; flags |= opts->flags &REBASE_NO_QUIET ?TODO_LIST_WARN_SKIPPED_CHERRY_PICKS :0 ; -switch (command) { +switch (opts->action) {case ACTION_NONE : {if (!opts->onto && !opts->upstream)die (_ ("a base commit must be provided with --upstream or --onto" ));
@@ -359,7 +360,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,break ; }default : -BUG ("invalid command '%d'" , command); +BUG ("invalid command '%d'" , opts->action); }return ret;
@@ -617,7 +618,7 @@ static int run_am(struct rebase_options *opts)strvec_push (&am.args,"am" );strvec_pushf (&am.env,GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT )); -if (opts->action && !strcmp ("continue" , opts->action)) { +if (opts->action ==ACTION_CONTINUE ) {strvec_push (&am.args,"--resolved" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg);if (opts->gpg_sign_opt)
@@ -628,7 +629,7 @@ static int run_am(struct rebase_options *opts)return move_to_original_branch (opts); } -if (opts->action && !strcmp ("skip" , opts->action)) { +if (opts->action ==ACTION_SKIP ) {strvec_push (&am.args,"--skip" );strvec_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); status =run_command (&am);
@@ -637,7 +638,7 @@ static int run_am(struct rebase_options *opts)return move_to_original_branch (opts); } -if (opts->action && !strcmp ("show-current-patch" , opts->action)) { +if (opts->action ==ACTION_SHOW_CURRENT_PATCH ) {strvec_push (&am.args,"--show-current-patch" );return run_command (&am); }
@@ -730,7 +731,7 @@ static int run_am(struct rebase_options *opts)return status; } -static int run_specific_rebase (struct rebase_options *opts,enum action action) +static int run_specific_rebase (struct rebase_options *opts) {int status;
@@ -748,7 +749,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action) opts->gpg_sign_opt = tmp; } - status =run_sequencer_rebase (opts, action); + status =run_sequencer_rebase (opts); }else if (opts->type ==REBASE_APPLY ) status =run_am (opts);else
@@ -1025,7 +1026,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct strbuf buf =STRBUF_INIT ;struct object_id branch_base;int ignore_whitespace =0 ; -enum action action =ACTION_NONE ;const char *gpg_sign =NULL ;struct string_list exec =STRING_LIST_INIT_NODUP ;const char *rebase_merges =NULL ;
@@ -1074,18 +1074,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)OPT_BIT (0 ,"no-ff" , &options.flags,N_ ("cherry-pick all commits, even if unchanged" ),REBASE_FORCE ), -OPT_CMDMODE (0 ,"continue" , &action,N_ ("continue" ), +OPT_CMDMODE (0 ,"continue" , &options.action,N_ ("continue" ),ACTION_CONTINUE ), -OPT_CMDMODE (0 ,"skip" , &action, +OPT_CMDMODE (0 ,"skip" , &options.action,N_ ("skip current patch and continue" ),ACTION_SKIP ), -OPT_CMDMODE (0 ,"abort" , &action, +OPT_CMDMODE (0 ,"abort" , &options.action,N_ ("abort and check out the original branch" ),ACTION_ABORT ), -OPT_CMDMODE (0 ,"quit" , &action, +OPT_CMDMODE (0 ,"quit" , &options.action,N_ ("abort but keep HEAD where it is" ),ACTION_QUIT ), -OPT_CMDMODE (0 ,"edit-todo" , &action,N_ ("edit the todo list " +OPT_CMDMODE (0 ,"edit-todo" , &options.action,N_ ("edit the todo list " "during an interactive rebase" ),ACTION_EDIT_TODO ), -OPT_CMDMODE (0 ,"show-current-patch" , &action, +OPT_CMDMODE (0 ,"show-current-patch" , &options.action,N_ ("show the patch file being applied or merged" ),ACTION_SHOW_CURRENT_PATCH ), OPT_CALLBACK_F(0, "apply", &options, NULL,
@@ -1174,7 +1174,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }else if (is_directory (merge_dir ())) {strbuf_reset (&buf);strbuf_addf (&buf,"%s/rewritten" ,merge_dir ()); -if (!(action ==ACTION_ABORT ) &&is_directory (buf.buf)) { +if (!(options.action ==ACTION_ABORT ) &&is_directory (buf.buf)) {die (_ ("`rebase --preserve-merges` (-p) is no longer supported.\n" "Use `git rebase --abort` to terminate current rebase.\n" "Or downgrade to v2.33, or earlier, to complete the rebase." ));
@@ -1201,7 +1201,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)"Note: Your `pull.rebase` configuration may also be set to 'preserve',\n" "which is no longer supported; use 'merges' instead")); -if (action !=ACTION_NONE && total_argc !=2 ) { +if (options.action !=ACTION_NONE && total_argc !=2 ) {usage_with_options (builtin_rebase_usage, builtin_rebase_options); }
@@ -1232,11 +1232,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (options.root && options.fork_point >0 )die (_ ("options '%s' and '%s' cannot be used together" ),"--root" ,"--fork-point" ); -if (action !=ACTION_NONE && !in_progress) +if (options.action !=ACTION_NONE && !in_progress)die (_ ("No rebase in progress?" ));setenv (GIT_REFLOG_ACTION_ENVIRONMENT ,"rebase" ,0 ); -if (action ==ACTION_EDIT_TODO && !is_merge (&options)) +if (options.action ==ACTION_EDIT_TODO && !is_merge (&options))die (_ ("The --edit-todo action can only be used during " "interactive rebase." ));
@@ -1246,16 +1246,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)else if (exec.nr)trace2_cmd_mode ("interactive-exec" );else -trace2_cmd_mode (action_names[action]); +trace2_cmd_mode (action_names[options.action]); } -switch (action) { +switch (options.action) {case ACTION_CONTINUE : {struct object_id head;struct lock_file lock_file =LOCK_INIT ;int fd; - options.action ="continue" ;/* Sanity check */ if (get_oid ("HEAD" , &head))die (_ ("Cannot read HEAD" ));
@@ -1281,7 +1280,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)case ACTION_SKIP : {struct string_list merge_rr =STRING_LIST_INIT_DUP ; - options.action ="skip" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 ); ropts.flags =RESET_HEAD_HARD ;
@@ -1296,7 +1294,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct string_list merge_rr =STRING_LIST_INIT_DUP ;struct strbuf head_msg =STRBUF_INIT ; - options.action ="abort" ;rerere_clear (the_repository, &merge_rr);string_list_clear (&merge_rr,1 );
@@ -1336,17 +1333,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) goto cleanup; }case ACTION_EDIT_TODO : - options.action ="edit-todo" ; options.dont_finish_rebase =1 ; goto run_rebase;case ACTION_SHOW_CURRENT_PATCH : - options.action ="show-current-patch" ; options.dont_finish_rebase =1 ; goto run_rebase;case ACTION_NONE :break ;default : -BUG ("action: %d" , action); +BUG ("action: %d" , options.action); }/* Make sure no rebase is in progress */
@@ -1370,7 +1365,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }if ((options.flags &REBASE_INTERACTIVE_EXPLICIT ) || - (action !=ACTION_NONE ) || + (options.action !=ACTION_NONE ) || (exec.nr >0 ) || options.autosquash) { allow_preemptive_ff =0 ;
@@ -1815,7 +1810,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.revisions = revisions.buf; run_rebase: - ret =run_specific_rebase (&options, action); + ret =run_specific_rebase (&options); cleanup:strbuf_release (&buf);
Signature · 1 line
gitgitgadget
Re: [PATCH v4 8/8] rebase: cleanup action handling
Quoted 8 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Treating the action as a string is a hang over from the scripted rebase. The last commit removed the only remaining use of the action that required a string so lets convert the other action users to use the existing action enum instead. If we ever need the action name as a string in the future the
action_namesarray exists exactly for that purpose.
Nice.
#leftoverbit
Perhaps a clean-up worth making after the dust settles from this series would be to use designated initialisers to avoid names and their string values going out of sync, perhaps like
static const char *action_names[] = {
[ACTION_NONE] = "undefined",
[ACTION_CONTINUE] = "continue",
...
[ACTION_SHOW_CURRENT_PATCH] = "show_current_patch",
};Unless the final element is something that must stay at the end even when adding new member to a collection, it is a good idea to keep a (seemingly unnecessary) comma at the end. That would make it easier to add a new member without unnecessary patch noise.