Re: [PATCH v2 10/14] rebase: cleanup reset_head() calls

From: Phillip Wood <phillip.wood123@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
	Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	"Philippe Blain" <levraiphilippeblain@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Elijah Newren" <newren@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"René Scharfe" <l.s.r@web.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Phillip Wood" <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v2 10/14] rebase: cleanup reset_head() calls
Date: Tue, 25 Jan 2022 11:07:57 +0000	[thread overview]
Message-ID: <74ef1b90-0d7a-fa28-9c5a-4c328674384e@gmail.com> (raw)
In-Reply-To: <xmqqczm5r34h.fsf@gitster.g>

On 09/12/2021 19:26, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> If ORIG_HEAD is not set by passing RESET_ORIG_HEAD then there is no
>> need to pass anything for reflog_orig_head. In addition to the callers
>> fixed in this commit move_to_original_branch() also passes
>> reflog_orig_head without setting ORIG_HEAD. That caller is mistakenly
>> passing the message it wants to put in the branch reflog which is not
>> currently possible so we delay fixing that caller until we can pass
>> the message as the branch reflog.
> 
> As I hinted elsewhere, these rules should be spelled out in a
> comment before "int reset_head(...)" either in reset.[ch].
> 
> For this particular one, I wonder if
> 
>   (A) we can lose RESET_ORIG_HEAD bit and use the presence of
>       reflog_orig_head to mean what that bit currently means, or
> 
>   (B) we keep the current code strucure, but make it a BUG() if
>       a non-NULL reflog_orig_head is given without RESET_ORIG_HEAD
>       bit set.

We do (B) later in the series, it cannot be done in this commit as we 
leave move_to_original_branch() unchanged here and that would BUG() out. 
I've added a comment it the commit message to explain this.

Best Wishes

Phillip

> 
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   builtin/rebase.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 3d78b5c8bef..fdd822c470f 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -675,7 +675,7 @@ static int run_am(struct rebase_options *opts)
>>   
>>   		reset_head(the_repository, &opts->orig_head,
>>   			   opts->head_name, 0,
>> -			   "HEAD", NULL, DEFAULT_REFLOG_ACTION);
>> +			   NULL, NULL, DEFAULT_REFLOG_ACTION);
>>   		error(_("\ngit encountered an error while preparing the "
>>   			"patches to replay\n"
>>   			"these revisions:\n"
>> @@ -1777,7 +1777,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   			options.head_name ? options.head_name : "detached HEAD",
>>   			oid_to_hex(&options.onto->object.oid));
>>   		reset_head(the_repository, NULL, options.head_name,
>> -			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf, NULL);
>> +			   RESET_HEAD_REFS_ONLY, NULL, msg.buf, NULL);
>>   		strbuf_release(&msg);
>>   		ret = finish_rebase(&options);
>>   		goto cleanup;