diff mbox series

[4/7] rebase --apply: respect GIT_REFLOG_ACTION

Message ID 58c560d0e19a3fa6dd5de547821c9642089de3fb.1645441854.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series rebase: make reflog messages independent of the backend | expand

Commit Message

Phillip Wood Feb. 21, 2022, 11:10 a.m. UTC
From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog messages when finishing a rebase hard code "rebase" rather
than using GIT_REFLOG_ACTION.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 7 ++++---
 t/t3406-rebase-message.sh | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Christian Couder April 7, 2022, 1:59 p.m. UTC | #1
On Tue, Feb 22, 2022 at 6:41 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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" ...

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c          | 7 ++++---
>  t/t3406-rebase-message.sh | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 36863117fba..e50361fc2a9 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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?

> -       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 of
GIT_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?

>         ropts.branch = opts->head_name;
>         ropts.flags = RESET_HEAD_REFS_ONLY;
>         ropts.branch_msg = branch_reflog.buf;
Phillip Wood April 15, 2022, 2:03 p.m. UTC | #2
Hi Christian

On 07/04/2022 14:59, Christian Couder wrote:
> On Tue, Feb 22, 2022 at 6:41 AM Phillip Wood via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> 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" ...

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

>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   builtin/rebase.c          | 7 ++++---
>>   t/t3406-rebase-message.sh | 2 +-
>>   2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 36863117fba..e50361fc2a9 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -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?
> 
>> -       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 of
> GIT_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?
> 
>>          ropts.branch = opts->head_name;
>>          ropts.flags = RESET_HEAD_REFS_ONLY;
>>          ropts.branch_msg = branch_reflog.buf;
diff mbox series

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 36863117fba..e50361fc2a9 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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;
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 8aa6a79acc1..bb2a4949abc 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -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