From: Phillip Wood Date: Sat, 31 Oct 2020 10:55:52 GMT Subject: Re: [PATCH 1/4] rebase -i: stop overwriting ORIG_HEAD buffer Message-ID: In-Reply-To: On 27/10/2020 21:10, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" writes: > >> From: Phillip Wood > > My initial impression after seeing the recent report about ORIG_HEAD > was "hmph, these days, rebasing is done on detached HEAD and the > final step updates the target branch only once, so @{1} is much > easier to use---perhaps it is time to deprecate use of ORIG_HEAD?". > After all, ORIG_HEAD was invented way before we had reflog, and > given that one of the goal of reflog was to give more general > recovery mechanism than going back one-step like ORIG_HEAD allowed > us to, and "rebase" were taught to work on detached HEAD to make > @{1} more useful, it would not be too bad to eventually retire > ORIG_HEAD in a distant future, I thought. Thanks for filling in the history. As reset sets ORIG_HEAD as well as rebase it can be confusing so retiring it in the future maybe a good idea. > But it is a good initiative anyway to make ORIG_HEAD again work as > documented. Thanks for working on it. > >> After rebasing ORIG_HEAD is supposed to point to the old HEAD of the > > A comma after "rebasing". Sure >> rebased branch. Unfortunately the buffer storing the oid was >> overwritten with a new oid before ORIG_HEAD was created. The buffer is >> also used when writing .git/rebase-merge/orig-head which is used by >> `rebase --abort` to restore the previous head. Luckily that file is >> written before the buffer is overwritten. As we want the full oid >> find_unique_abbrev() is replaced with oid_to_hex_r() rather than >> find_unique_abbrev_r(). > > The above is hard to read and understand. It is unclear where the > observation of the current behaviour (which is often the explanation > of the cause of the bug) ends and the description of new behaviour > begins. > > ... old HEAD of the rebased branch. The code used > find_unique_abbrev() to obtain the object name of the old HEAD > and wrote to both .git/rebase-merge/orig-head (used by `rebase > --abort` to go back to the previous state) and to ORIG_HEAD. > The buffer find_unique_abbrev() gives back is volatile, > unfortunately, and was overwritten after the former file is > written but before ORIG_FILE is written, leaving an incorrect > object name in it. > > Up to that point is the observation of the current code, which > explains where the bug comes from. Please have a paragraph break > after that, before explaining the solution, e.g. > > Avoid relying on the volatile buffer of find_unique_abbrev(), > and instead supply our own buffer to keep the object name. > Because we want to use the full object name, use oid_to_hex_r() > instead of find_unique_abbrev_r() to do so. That's much clearer thanks. I wonder if swapping to oid_to_hex_r() rather than find_unique_abbev_r() is complicating this commit unnecessarily as the code that is changed here is deleted in patch 4, maybe we should do the switch there. >> I think that all of the users of head_hash should actually be using >> opts->orig_head instead as passing a string rather than a struct >> object_id around is a hang over from the scripted implementation. This >> patch just fixes the immediate bug and adds a regression test based on >> Caspar's reproduction example. The users will be converted to use >> struct object_id and head_hash removed in the next few commits. > > Makes sense. > >> >> Reported-by: Caspar Duregger >> Signed-off-by: Phillip Wood >> --- > > If you cite "Caspar's repro example" like that in the log message, > it is incomplete to leave out a URL to the mail archive. Good point I'll add a url Thanks for your comments Phillip >> builtin/rebase.c | 10 +++++----- >> t/t3404-rebase-interactive.sh | 11 +++++++++++ >> 2 files changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/builtin/rebase.c b/builtin/rebase.c >> index eeca53382f..6def28a533 100644 >> --- a/builtin/rebase.c >> +++ b/builtin/rebase.c >> @@ -270,15 +270,15 @@ static int edit_todo_file(unsigned flags) >> } >> >> static int get_revision_ranges(struct commit *upstream, struct commit *onto, >> - struct object_id *orig_head, const char **head_hash, >> + struct object_id *orig_head, char *head_hash, >> char **revisions, char **shortrevisions) >> { >> struct commit *base_rev = upstream ? upstream : onto; >> const char *shorthead; >> >> - *head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ); >> + oid_to_hex_r(head_hash, orig_head); >> *revisions = xstrfmt("%s...%s", oid_to_hex(&base_rev->object.oid), >> - *head_hash); >> + head_hash); >> >> shorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV); >> >> @@ -327,7 +327,7 @@ static void split_exec_commands(const char *cmd, struct string_list *commands) >> static int do_interactive_rebase(struct rebase_options *opts, unsigned flags) >> { >> int ret; >> - const char *head_hash = NULL; >> + char head_hash[GIT_MAX_HEXSZ]; >> char *revisions = NULL, *shortrevisions = NULL; >> struct strvec make_script_args = STRVEC_INIT; >> struct todo_list todo_list = TODO_LIST_INIT; >> @@ -335,7 +335,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags) >> struct string_list commands = STRING_LIST_INIT_DUP; >> >> if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head, >> - &head_hash, &revisions, &shortrevisions)) >> + head_hash, &revisions, &shortrevisions)) >> return -1; >> >> if (init_basic_state(&replay, >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh >> index 07a1617351..1e56696e4f 100755 >> --- a/t/t3404-rebase-interactive.sh >> +++ b/t/t3404-rebase-interactive.sh >> @@ -1797,6 +1797,17 @@ test_expect_success 'todo has correct onto hash' ' >> test_i18ngrep "^# Rebase ..* onto $onto" actual >> ' >> >> +test_expect_success 'ORIG_HEAD is updated correctly' ' >> + test_when_finished "git checkout master && git branch -D test-orig-head" && >> + git checkout -b test-orig-head A && >> + git commit --allow-empty -m A1 && >> + git commit --allow-empty -m A2 && >> + git commit --allow-empty -m A3 && >> + git commit --allow-empty -m A4 && >> + git rebase master && >> + test_cmp_rev ORIG_HEAD test-orig-head@{1} >> +' >> + >> # This must be the last test in this file >> test_expect_success '$EDITOR and friends are unchanged' ' >> test_editor_unchanged