From: Phillip Wood Date: Tue, 25 Jan 2022 11:07:57 GMT Subject: Re: [PATCH v2 10/14] rebase: cleanup reset_head() calls Message-ID: <74ef1b90-0d7a-fa28-9c5a-4c328674384e@gmail.com> In-Reply-To: On 09/12/2021 19:26, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" writes: > >> From: Phillip Wood >> >> 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 >> --- >> 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;