{"thread":{"id":"54515","subject":"[PATCH 4/4] rebase -i: simplify get_revision_ranges()","startedAt":"2020-10-27T14:02:29Z","lastAt":"2020-11-04T15:30:04Z","messageCount":15,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","Phillip Wood","herr.kaste"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"408509","messageId":"ed78f8628a67b346ffebaab50ac52cf2a83a6131.1603807338.git.gitgitgadget@gmail.com","threadId":"54515","inReplyTo":"pull.773.git.1603807337.gitgitgadget@gmail.com","subject":"[PATCH 4/4] rebase -i: simplify get_revision_ranges()","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-27T14:02:17Z","receivedAt":"2020-10-27T14:02:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nNow that all the external users of head_hash have been converted to\nuse a opts->orig_head instead we can stop returning head_hash from\nget_revision_ranges().\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex f94f9fe307..17450e9d9e 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -270,15 +270,14 @@ static int edit_todo_file(unsigned flags)\n }\n \n static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n-\t\t\t struct object_id *orig_head, char *head_hash,\n-\t\t\t char **revisions, char **shortrevisions)\n+\t\t\t struct object_id *orig_head, char **revisions,\n+\t\t\t char **shortrevisions)\n {\n \tstruct commit *base_rev = upstream ? upstream : onto;\n \tconst char *shorthead;\n \n-\toid_to_hex_r(head_hash, orig_head);\n \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n-\t\t\t\t\t\t head_hash);\n+\t\t\t oid_to_hex(orig_head));\n \n \tshorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV);\n \n@@ -328,7 +327,6 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)\n static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n {\n \tint ret;\n-\tchar head_hash[GIT_MAX_HEXSZ];\n \tchar *revisions = NULL, *shortrevisions = NULL;\n \tstruct strvec make_script_args = STRVEC_INIT;\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n@@ -336,12 +334,12 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \tstruct string_list commands = STRING_LIST_INIT_DUP;\n \n \tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,\n-\t\t\t\thead_hash, &revisions, &shortrevisions))\n+\t\t\t\t&revisions, &shortrevisions))\n \t\treturn -1;\n \n \tif (init_basic_state(&replay,\n \t\t\t opts->head_name ? opts->head_name : \"detached HEAD\",\n-\t\t\t opts->onto, head_hash)) {\n+\t\t\t opts->onto, &opts->orig_head)) {\n \t\tfree(revisions);\n \t\tfree(shortrevisions);\n \n-- \ngitgitgadget\n"},{"id":"408513","messageId":"11494a7b79cd5b6fbef51720e7111a94e7bd650e.1603807338.git.gitgitgadget@gmail.com","threadId":"54515","inReplyTo":"pull.773.git.1603807337.gitgitgadget@gmail.com","subject":"[PATCH 3/4] rebase -i: use struct object_id when writing state","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-27T14:02:16Z","receivedAt":"2020-10-27T18:24:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nRather than passing a string around pass the struct object_id that the\nstring was created from call oid_hex() when we write the file.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 3 ++-\n sequencer.c | 5 +++--\n sequencer.h | 2 +-\n 3 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex d975918de6..f94f9fe307 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -296,7 +296,8 @@ static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n }\n \n static int init_basic_state(struct replay_opts *opts, const char *head_name,\n-\t\t\t struct commit *onto, const char *orig_head)\n+\t\t\t struct commit *onto,\n+\t\t\t const struct object_id *orig_head)\n {\n \tFILE *interactive;\n \ndiff --git a/sequencer.c b/sequencer.c\nindex f79c3df861..2037f0ba66 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2692,7 +2692,7 @@ static void write_strategy_opts(struct replay_opts *opts)\n }\n \n int write_basic_state(struct replay_opts *opts, const char *head_name,\n-\t\t struct commit *onto, const char *orig_head)\n+\t\t struct commit *onto, const struct object_id *orig_head)\n {\n \tif (head_name)\n \t\twrite_file(rebase_path_head_name(), \"%s\\n\", head_name);\n@@ -2700,7 +2700,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\twrite_file(rebase_path_onto(), \"%s\\n\",\n \t\t\t oid_to_hex(&onto->object.oid));\n \tif (orig_head)\n-\t\twrite_file(rebase_path_orig_head(), \"%s\\n\", orig_head);\n+\t\twrite_file(rebase_path_orig_head(), \"%s\\n\",\n+\t\t\t oid_to_hex(orig_head));\n \n \tif (opts->quiet)\n \t\twrite_file(rebase_path_quiet(), \"%s\", \"\");\ndiff --git a/sequencer.h b/sequencer.h\nindex ea56825488..cf201f2406 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -227,7 +227,7 @@ int read_author_script(const char *path, char **name, char **email, char **date,\n \t\t int allow_missing);\n void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);\n int write_basic_state(struct replay_opts *opts, const char *head_name,\n-\t\t struct commit *onto, const char *orig_head);\n+\t\t struct commit *onto, const struct object_id *orig_head);\n void sequencer_post_commit_cleanup(struct repository *r, int verbose);\n int sequencer_get_last_command(struct repository* r,\n \t\t\t enum replay_action *action);\n-- \ngitgitgadget\n\n"},{"id":"408514","messageId":"24f2c4a62317231f4eabed23bb24d345abc9d67e.1603807338.git.gitgitgadget@gmail.com","threadId":"54515","inReplyTo":"pull.773.git.1603807337.gitgitgadget@gmail.com","subject":"[PATCH 1/4] rebase -i: stop overwriting ORIG_HEAD buffer","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-27T14:02:14Z","receivedAt":"2020-10-27T18:25:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAfter rebasing ORIG_HEAD is supposed to point to the old HEAD of the\nrebased branch. Unfortunately the buffer storing the oid was\noverwritten with a new oid before ORIG_HEAD was created. The buffer is\nalso used when writing .git/rebase-merge/orig-head which is used by\n`rebase --abort` to restore the previous head. Luckily that file is\nwritten before the buffer is overwritten. As we want the full oid\nfind_unique_abbrev() is replaced with oid_to_hex_r() rather than\nfind_unique_abbrev_r().\n\nI think that all of the users of head_hash should actually be using\nopts->orig_head instead as passing a string rather than a struct\nobject_id around is a hang over from the scripted implementation. This\npatch just fixes the immediate bug and adds a regression test based on\nCaspar's reproduction example. The users will be converted to use\nstruct object_id and head_hash removed in the next few commits.\n\nReported-by: Caspar Duregger \nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 10 +++++-----\n t/t3404-rebase-interactive.sh | 11 +++++++++++\n 2 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex eeca53382f..6def28a533 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -270,15 +270,15 @@ static int edit_todo_file(unsigned flags)\n }\n \n static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n-\t\t\t struct object_id *orig_head, const char **head_hash,\n+\t\t\t struct object_id *orig_head, char *head_hash,\n \t\t\t char **revisions, char **shortrevisions)\n {\n \tstruct commit *base_rev = upstream ? upstream : onto;\n \tconst char *shorthead;\n \n-\t*head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ);\n+\toid_to_hex_r(head_hash, orig_head);\n \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n-\t\t\t\t\t\t *head_hash);\n+\t\t\t\t\t\t head_hash);\n \n \tshorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV);\n \n@@ -327,7 +327,7 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)\n static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n {\n \tint ret;\n-\tconst char *head_hash = NULL;\n+\tchar head_hash[GIT_MAX_HEXSZ];\n \tchar *revisions = NULL, *shortrevisions = NULL;\n \tstruct strvec make_script_args = STRVEC_INIT;\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n@@ -335,7 +335,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \tstruct string_list commands = STRING_LIST_INIT_DUP;\n \n \tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,\n-\t\t\t\t&head_hash, &revisions, &shortrevisions))\n+\t\t\t\thead_hash, &revisions, &shortrevisions))\n \t\treturn -1;\n \n \tif (init_basic_state(&replay,\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 07a1617351..1e56696e4f 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1797,6 +1797,17 @@ test_expect_success 'todo has correct onto hash' '\n \ttest_i18ngrep \"^# Rebase ..* onto $onto\" actual\n '\n \n+test_expect_success 'ORIG_HEAD is updated correctly' '\n+\ttest_when_finished \"git checkout master && git branch -D test-orig-head\" &&\n+\tgit checkout -b test-orig-head A &&\n+\tgit commit --allow-empty -m A1 &&\n+\tgit commit --allow-empty -m A2 &&\n+\tgit commit --allow-empty -m A3 &&\n+\tgit commit --allow-empty -m A4 &&\n+\tgit rebase master &&\n+\ttest_cmp_rev ORIG_HEAD test-orig-head@{1}\n+'\n+\n # This must be the last test in this file\n test_expect_success '$EDITOR and friends are unchanged' '\n \ttest_editor_unchanged\n-- \ngitgitgadget\n\n"},{"id":"408516","messageId":"589aed08f18c50979c210ca97c6dc0a01eac76c3.1603807338.git.gitgitgadget@gmail.com","threadId":"54515","inReplyTo":"pull.773.git.1603807337.gitgitgadget@gmail.com","subject":"[PATCH 2/4] rebase -i: use struct object_id rather than looking up commit","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-27T14:02:15Z","receivedAt":"2020-10-27T18:25:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWe already have a struct object_id containing the oid that we want to\nset ORIG_HEAD to so use that rather than converting it to a string and\nthen calling get_oid() on that string.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 5 +++--\n sequencer.c | 10 +++-------\n sequencer.h | 5 +++--\n 3 files changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 6def28a533..d975918de6 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -370,8 +370,9 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \n \t\tsplit_exec_commands(opts->cmd, &commands);\n \t\tret = complete_action(the_repository, &replay, flags,\n-\t\t\tshortrevisions, opts->onto_name, opts->onto, head_hash,\n-\t\t\t&commands, opts->autosquash, &todo_list);\n+\t\t\tshortrevisions, opts->onto_name, opts->onto,\n+\t\t\t&opts->orig_head, &commands, opts->autosquash,\n+\t\t\t&todo_list);\n \t}\n \n \tstring_list_clear(&commands, 0);\ndiff --git a/sequencer.c b/sequencer.c\nindex 00acb12496..f79c3df861 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3965,21 +3965,17 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n \n static int checkout_onto(struct repository *r, struct replay_opts *opts,\n \t\t\t const char *onto_name, const struct object_id *onto,\n-\t\t\t const char *orig_head)\n+\t\t\t const struct object_id *orig_head)\n {\n-\tstruct object_id oid;\n \tconst char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n \n-\tif (get_oid(orig_head, &oid))\n-\t\treturn error(_(\"%s: not a valid OID\"), orig_head);\n-\n \tif (run_git_checkout(r, opts, oid_to_hex(onto), action)) {\n \t\tapply_autostash(rebase_path_autostash());\n \t\tsequencer_remove_state(opts);\n \t\treturn error(_(\"could not detach HEAD\"));\n \t}\n \n-\treturn update_ref(NULL, \"ORIG_HEAD\", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n+\treturn update_ref(NULL, \"ORIG_HEAD\", orig_head, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n }\n \n static int stopped_at_head(struct repository *r)\n@@ -5314,7 +5310,7 @@ static int skip_unnecessary_picks(struct repository *r,\n \n int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,\n \t\t const char *shortrevisions, const char *onto_name,\n-\t\t struct commit *onto, const char *orig_head,\n+\t\t struct commit *onto, const struct object_id *orig_head,\n \t\t struct string_list *commands, unsigned autosquash,\n \t\t struct todo_list *todo_list)\n {\ndiff --git a/sequencer.h b/sequencer.h\nindex b2a501e445..ea56825488 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -163,8 +163,9 @@ void todo_list_add_exec_commands(struct todo_list *todo_list,\n \t\t\t\t struct string_list *commands);\n int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,\n \t\t const char *shortrevisions, const char *onto_name,\n-\t\t struct commit *onto, const char *orig_head, struct string_list *commands,\n-\t\t unsigned autosquash, struct todo_list *todo_list);\n+\t\t struct commit *onto, const struct object_id *orig_head,\n+\t\t struct string_list *commands, unsigned autosquash,\n+\t\t struct todo_list *todo_list);\n int todo_list_rearrange_squash(struct todo_list *todo_list);\n \n /*\n-- \ngitgitgadget\n\n"},{"id":"408515","messageId":"pull.773.git.1603807337.gitgitgadget@gmail.com","threadId":"54515","inReplyTo":null,"subject":"[PATCH 0/4] rebase -i: fix ORIG_HEAD handling","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-27T14:02:13Z","receivedAt":"2020-10-27T18:25:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"The buffer containing the oid for ORIG_HEAD is overwritten before ORIG_HEAD\nis created. This series fixes that bug and then converts the code to use\nstruct object_id rather than passing around strings.\n\nThanks to Caspar for reporting the bug and providing a reproducible example\n\nPhillip Wood (4):\n rebase -i: stop overwriting ORIG_HEAD buffer\n rebase -i: use struct object_id rather than looking up commit\n rebase -i: use struct object_id when writing state\n rebase -i: simplify get_revision_ranges()\n\n builtin/rebase.c | 20 ++++++++++----------\n sequencer.c | 15 ++++++---------\n sequencer.h | 7 ++++---\n t/t3404-rebase-interactive.sh | 11 +++++++++++\n 4 files changed, 31 insertions(+), 22 deletions(-)\n\n\nbase-commit: 2e673356aefa8ed19be3c878f966ad6189ecb510\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-773%2Fphillipwood%2Fwip%2Frebase-fix-orig_head-handling-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-773/phillipwood/wip/rebase-fix-orig_head-handling-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/773\n-- \ngitgitgadget\n"},{"id":"408532","messageId":"xmqq7drbbcj5.fsf@gitster.c.googlers.com","threadId":"54515","inReplyTo":"24f2c4a62317231f4eabed23bb24d345abc9d67e.1603807338.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] rebase -i: stop overwriting ORIG_HEAD buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-27T21:10:38Z","receivedAt":"2020-10-27T21:10:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> From: Phillip Wood \n\nMy initial impression after seeing the recent report about ORIG_HEAD\nwas \"hmph, these days, rebasing is done on detached HEAD and the\nfinal step updates the target branch only once, so @{1} is much\neasier to use---perhaps it is time to deprecate use of ORIG_HEAD?\".\nAfter all, ORIG_HEAD was invented way before we had reflog, and\ngiven that one of the goal of reflog was to give more general\nrecovery mechanism than going back one-step like ORIG_HEAD allowed\nus to, and \"rebase\" were taught to work on detached HEAD to make\n@{1} more useful, it would not be too bad to eventually retire\nORIG_HEAD in a distant future, I thought.\n\nBut it is a good initiative anyway to make ORIG_HEAD again work as\ndocumented. Thanks for working on it.\n\n> After rebasing ORIG_HEAD is supposed to point to the old HEAD of the\n\nA comma after \"rebasing\".\n\n> rebased branch. Unfortunately the buffer storing the oid was\n> overwritten with a new oid before ORIG_HEAD was created. The buffer is\n> also used when writing .git/rebase-merge/orig-head which is used by\n> `rebase --abort` to restore the previous head. Luckily that file is\n> written before the buffer is overwritten. As we want the full oid\n> find_unique_abbrev() is replaced with oid_to_hex_r() rather than\n> find_unique_abbrev_r().\n\nThe above is hard to read and understand. It is unclear where the\nobservation of the current behaviour (which is often the explanation\nof the cause of the bug) ends and the description of new behaviour\nbegins.\n\n ... old HEAD of the rebased branch. The code used\n find_unique_abbrev() to obtain the object name of the old HEAD\n and wrote to both .git/rebase-merge/orig-head (used by `rebase\n --abort` to go back to the previous state) and to ORIG_HEAD.\n The buffer find_unique_abbrev() gives back is volatile,\n unfortunately, and was overwritten after the former file is\n written but before ORIG_FILE is written, leaving an incorrect\n object name in it.\n\nUp to that point is the observation of the current code, which\nexplains where the bug comes from. Please have a paragraph break\nafter that, before explaining the solution, e.g.\n\n Avoid relying on the volatile buffer of find_unique_abbrev(),\n and instead supply our own buffer to keep the object name.\n Because we want to use the full object name, use oid_to_hex_r()\n instead of find_unique_abbrev_r() to do so.\n\n> I think that all of the users of head_hash should actually be using\n> opts->orig_head instead as passing a string rather than a struct\n> object_id around is a hang over from the scripted implementation. This\n> patch just fixes the immediate bug and adds a regression test based on\n> Caspar's reproduction example. The users will be converted to use\n> struct object_id and head_hash removed in the next few commits.\n\nMakes sense.\n\n>\n> Reported-by: Caspar Duregger \n> Signed-off-by: Phillip Wood \n> ---\n\nIf you cite \"Caspar's repro example\" like that in the log message,\nit is incomplete to leave out a URL to the mail archive.\n\n\n> builtin/rebase.c | 10 +++++-----\n> t/t3404-rebase-interactive.sh | 11 +++++++++++\n> 2 files changed, 16 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index eeca53382f..6def28a533 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -270,15 +270,15 @@ static int edit_todo_file(unsigned flags)\n> }\n> \n> static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n> -\t\t\t struct object_id *orig_head, const char **head_hash,\n> +\t\t\t struct object_id *orig_head, char *head_hash,\n> \t\t\t char **revisions, char **shortrevisions)\n> {\n> \tstruct commit *base_rev = upstream ? upstream : onto;\n> \tconst char *shorthead;\n> \n> -\t*head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ);\n> +\toid_to_hex_r(head_hash, orig_head);\n> \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n> -\t\t\t\t\t\t *head_hash);\n> +\t\t\t\t\t\t head_hash);\n> \n> \tshorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV);\n> \n> @@ -327,7 +327,7 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)\n> static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n> {\n> \tint ret;\n> -\tconst char *head_hash = NULL;\n> +\tchar head_hash[GIT_MAX_HEXSZ];\n> \tchar *revisions = NULL, *shortrevisions = NULL;\n> \tstruct strvec make_script_args = STRVEC_INIT;\n> \tstruct todo_list todo_list = TODO_LIST_INIT;\n> @@ -335,7 +335,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n> \tstruct string_list commands = STRING_LIST_INIT_DUP;\n> \n> \tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,\n> -\t\t\t\t&head_hash, &revisions, &shortrevisions))\n> +\t\t\t\thead_hash, &revisions, &shortrevisions))\n> \t\treturn -1;\n> \n> \tif (init_basic_state(&replay,\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 07a1617351..1e56696e4f 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1797,6 +1797,17 @@ test_expect_success 'todo has correct onto hash' '\n> \ttest_i18ngrep \"^# Rebase ..* onto $onto\" actual\n> '\n> \n> +test_expect_success 'ORIG_HEAD is updated correctly' '\n> +\ttest_when_finished \"git checkout master && git branch -D test-orig-head\" &&\n> +\tgit checkout -b test-orig-head A &&\n> +\tgit commit --allow-empty -m A1 &&\n> +\tgit commit --allow-empty -m A2 &&\n> +\tgit commit --allow-empty -m A3 &&\n> +\tgit commit --allow-empty -m A4 &&\n> +\tgit rebase master &&\n> +\ttest_cmp_rev ORIG_HEAD test-orig-head@{1}\n> +'\n> +\n> # This must be the last test in this file\n> test_expect_success '$EDITOR and friends are unchanged' '\n> \ttest_editor_unchanged\n"},{"id":"408781","messageId":"fa2d75ee-e692-a400-e9f7-8f13a5c220e8@gmail.com","threadId":"54515","inReplyTo":"xmqq7drbbcj5.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/4] rebase -i: stop overwriting ORIG_HEAD buffer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-31T10:55:52Z","receivedAt":"2020-10-31T10:55:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 27/10/2020 21:10, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> \n>> From: Phillip Wood \n> \n> My initial impression after seeing the recent report about ORIG_HEAD\n> was \"hmph, these days, rebasing is done on detached HEAD and the\n> final step updates the target branch only once, so @{1} is much\n> easier to use---perhaps it is time to deprecate use of ORIG_HEAD?\".\n> After all, ORIG_HEAD was invented way before we had reflog, and\n> given that one of the goal of reflog was to give more general\n> recovery mechanism than going back one-step like ORIG_HEAD allowed\n> us to, and \"rebase\" were taught to work on detached HEAD to make\n> @{1} more useful, it would not be too bad to eventually retire\n> ORIG_HEAD in a distant future, I thought.\n\nThanks for filling in the history. As reset sets ORIG_HEAD as well as \nrebase it can be confusing so retiring it in the future maybe a good idea.\n\n> But it is a good initiative anyway to make ORIG_HEAD again work as\n> documented. Thanks for working on it.\n> \n>> After rebasing ORIG_HEAD is supposed to point to the old HEAD of the\n> \n> A comma after \"rebasing\".\n\nSure\n\n>> rebased branch. Unfortunately the buffer storing the oid was\n>> overwritten with a new oid before ORIG_HEAD was created. The buffer is\n>> also used when writing .git/rebase-merge/orig-head which is used by\n>> `rebase --abort` to restore the previous head. Luckily that file is\n>> written before the buffer is overwritten. As we want the full oid\n>> find_unique_abbrev() is replaced with oid_to_hex_r() rather than\n>> find_unique_abbrev_r().\n> \n> The above is hard to read and understand. It is unclear where the\n> observation of the current behaviour (which is often the explanation\n> of the cause of the bug) ends and the description of new behaviour\n> begins.\n> \n> ... old HEAD of the rebased branch. The code used\n> find_unique_abbrev() to obtain the object name of the old HEAD\n> and wrote to both .git/rebase-merge/orig-head (used by `rebase\n> --abort` to go back to the previous state) and to ORIG_HEAD.\n> The buffer find_unique_abbrev() gives back is volatile,\n> unfortunately, and was overwritten after the former file is\n> written but before ORIG_FILE is written, leaving an incorrect\n> object name in it.\n> \n> Up to that point is the observation of the current code, which\n> explains where the bug comes from. Please have a paragraph break\n> after that, before explaining the solution, e.g.\n> \n> Avoid relying on the volatile buffer of find_unique_abbrev(),\n> and instead supply our own buffer to keep the object name.\n> Because we want to use the full object name, use oid_to_hex_r()\n> instead of find_unique_abbrev_r() to do so.\n\nThat's much clearer thanks. I wonder if swapping to oid_to_hex_r() \nrather than find_unique_abbev_r() is complicating this commit \nunnecessarily as the code that is changed here is deleted in patch 4, \nmaybe we should do the switch there.\n\n>> I think that all of the users of head_hash should actually be using\n>> opts->orig_head instead as passing a string rather than a struct\n>> object_id around is a hang over from the scripted implementation. This\n>> patch just fixes the immediate bug and adds a regression test based on\n>> Caspar's reproduction example. The users will be converted to use\n>> struct object_id and head_hash removed in the next few commits.\n> \n> Makes sense.\n> \n>>\n>> Reported-by: Caspar Duregger \n>> Signed-off-by: Phillip Wood \n>> ---\n> \n> If you cite \"Caspar's repro example\" like that in the log message,\n> it is incomplete to leave out a URL to the mail archive.\n\nGood point I'll add a url\n\nThanks for your comments\n\nPhillip\n\n>> builtin/rebase.c | 10 +++++-----\n>> t/t3404-rebase-interactive.sh | 11 +++++++++++\n>> 2 files changed, 16 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>> index eeca53382f..6def28a533 100644\n>> --- a/builtin/rebase.c\n>> +++ b/builtin/rebase.c\n>> @@ -270,15 +270,15 @@ static int edit_todo_file(unsigned flags)\n>> }\n>> \n>> static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n>> -\t\t\t struct object_id *orig_head, const char **head_hash,\n>> +\t\t\t struct object_id *orig_head, char *head_hash,\n>> \t\t\t char **revisions, char **shortrevisions)\n>> {\n>> \tstruct commit *base_rev = upstream ? upstream : onto;\n>> \tconst char *shorthead;\n>> \n>> -\t*head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ);\n>> +\toid_to_hex_r(head_hash, orig_head);\n>> \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n>> -\t\t\t\t\t\t *head_hash);\n>> +\t\t\t\t\t\t head_hash);\n>> \n>> \tshorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV);\n>> \n>> @@ -327,7 +327,7 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)\n>> static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n>> {\n>> \tint ret;\n>> -\tconst char *head_hash = NULL;\n>> +\tchar head_hash[GIT_MAX_HEXSZ];\n>> \tchar *revisions = NULL, *shortrevisions = NULL;\n>> \tstruct strvec make_script_args = STRVEC_INIT;\n>> \tstruct todo_list todo_list = TODO_LIST_INIT;\n>> @@ -335,7 +335,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n>> \tstruct string_list commands = STRING_LIST_INIT_DUP;\n>> \n>> \tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,\n>> -\t\t\t\t&head_hash, &revisions, &shortrevisions))\n>> +\t\t\t\thead_hash, &revisions, &shortrevisions))\n>> \t\treturn -1;\n>> \n>> \tif (init_basic_state(&replay,\n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> index 07a1617351..1e56696e4f 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -1797,6 +1797,17 @@ test_expect_success 'todo has correct onto hash' '\n>> \ttest_i18ngrep \"^# Rebase ..* onto $onto\" actual\n>> '\n>> \n>> +test_expect_success 'ORIG_HEAD is updated correctly' '\n>> +\ttest_when_finished \"git checkout master && git branch -D test-orig-head\" &&\n>> +\tgit checkout -b test-orig-head A &&\n>> +\tgit commit --allow-empty -m A1 &&\n>> +\tgit commit --allow-empty -m A2 &&\n>> +\tgit commit --allow-empty -m A3 &&\n>> +\tgit commit --allow-empty -m A4 &&\n>> +\tgit rebase master &&\n>> +\ttest_cmp_rev ORIG_HEAD test-orig-head@{1}\n>> +'\n>> +\n>> # This must be the last test in this file\n>> test_expect_success '$EDITOR and friends are unchanged' '\n>> \ttest_editor_unchanged\n\n"},{"id":"408901","messageId":"CAFzd1+7wzPZa9brWzWzSQdcMEnbEcV28zqBBAV_rsdsKNaKaFg@mail.gmail.com","threadId":"54515","inReplyTo":"xmqq7drbbcj5.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/4] rebase -i: stop overwriting ORIG_HEAD buffer","fromName":"herr.kaste","fromEmail":"herr.kaste@gmail.com","sentAt":"2020-11-02T19:40:42Z","receivedAt":"2020-11-02T19:41:10Z","isPatch":true,"sender":{"key":"herr.kaste@gmail.com","avatar":null},"body":"Am Di., 27. Okt. 2020 um 22:10 Uhr schrieb Junio C Hamano :\n>\n> My initial impression after seeing the recent report about ORIG_HEAD\n> was \"hmph, these days, rebasing is done on detached HEAD and the\n> final step updates the target branch only once, so @{1} is much\n> easier to use---perhaps it is time to deprecate use of ORIG_HEAD?\".\n> After all, ORIG_HEAD was invented way before we had reflog, and\n> given that one of the goal of reflog was to give more general\n> recovery mechanism than going back one-step like ORIG_HEAD allowed\n> us to, and \"rebase\" were taught to work on detached HEAD to make\n> @{1} more useful, it would not be too bad to eventually retire\n> ORIG_HEAD in a distant future, I thought.\n\nPhillip pointed out that ORIG_HEAD is actually not save *if* there is\na `reset` or `rebase --skip` during the rebase. Otherwise, by design,\nORIG_HEAD would be easier to use, as in the form `@{}`\ntwo things have to be decided and can go wrong.\n\nWouldn't it be better for the mid-term run to set ORIG_HEAD at the end\nof the rebase operation, basically during cleanup, and make the commitment\nthat it always points to the right thing for undoing the recent rebase\nor to compare it (`git diff ORIG_HEAD`) to see possible unwanted merge\nresolutions.\n\nI say mid-term because a `git rebase --undo` with additional safety checks\nand a `--autostash` for example is IMO the better long-run UX, in a very\ndistant future.\n\n>\n> But it is a good initiative anyway to make ORIG_HEAD again work as\n> documented. Thanks for working on it.\n>\n\nRegards\nCaspar Duregger\n"},{"id":"408962","messageId":"xmqqpn4vqogu.fsf@gitster.c.googlers.com","threadId":"54515","inReplyTo":"CAFzd1+7wzPZa9brWzWzSQdcMEnbEcV28zqBBAV_rsdsKNaKaFg@mail.gmail.com","subject":"Re: [PATCH 1/4] rebase -i: stop overwriting ORIG_HEAD buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-03T00:21:53Z","receivedAt":"2020-11-03T00:21:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"herr.kaste\" writes:\n\n> Phillip pointed out that ORIG_HEAD is actually not save *if* there is\n> a `reset` or `rebase --skip` during the rebase. Otherwise, by design,\n> ORIG_HEAD would be easier to use, as in the form `@{}`\n> two things have to be decided and can go wrong.\n\nWhat \"two\"? You should be able to just say @{1} regardless---that\nwas the whole point of performing all the intermediate steps while\non the detached HEAD so that you can rely on being 1, and @{} is a short-hand of @{} for the\ncurrent branch, and not a short-hand for HEAD@{...}, to help such a\nuse case.\n\nOr am I missing something?\n"},{"id":"408989","messageId":"CAFzd1+5D4pHhGf=K0LwaaOUjoBByqsMTpBVtR+Ots6-afBTDEA@mail.gmail.com","threadId":"54515","inReplyTo":"xmqqpn4vqogu.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/4] rebase -i: stop overwriting ORIG_HEAD buffer","fromName":"herr.kaste","fromEmail":"herr.kaste@gmail.com","sentAt":"2020-11-03T11:02:45Z","receivedAt":"2020-11-03T11:03:15Z","isPatch":true,"sender":{"key":"herr.kaste@gmail.com","avatar":null},"body":"Am Di., 3. Nov. 2020 um 01:21 Uhr schrieb Junio C Hamano :\n>\n> \"herr.kaste\" writes:\n>\n> > Phillip pointed out that ORIG_HEAD is actually not save *if* there is\n> > a `reset` or `rebase --skip` during the rebase. Otherwise, by design,\n> > ORIG_HEAD would be easier to use, as in the form `@{}`\n> > two things have to be decided and can go wrong.\n>\n> What \"two\"? You should be able to just say @{1} regardless---that\n> was the whole point of performing all the intermediate steps while\n> on the detached HEAD so that you can rely on being 1, and @{ or time>} is a short-hand of @{} for the\n> current branch, and not a short-hand for HEAD@{...}, to help such a\n> use case.\n>\n> Or am I missing something?\n\nWell, \"@{1}\" basically means: from the stream of things that happened take\nthe first. It is very natural to refer to the most recent thing differently.\nIn practice, until now, I used the {...} form only to refer to older things.\nTo put it differently, using {...} I'm researching history.\n\nFrom the docs:\n\n ORIG_HEAD is created by commands that move your HEAD in a drastic way,\n to record the position of the HEAD before their operation, so that you\n can easily change the tip of the branch back to the state before you ran\n them.\n\nThat's just humane. You do something, and then you revert. I don't need\na concept of a written history here, just of recency.\n"},{"id":"409087","messageId":"pull.773.v2.git.1604503780.gitgitgadget@gmail.com","threadId":"54515","inReplyTo":"pull.773.git.1603807337.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] rebase -i: fix ORIG_HEAD handling","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-04T15:29:36Z","receivedAt":"2020-11-04T15:29:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"The buffer containing the oid for ORIG_HEAD is overwritten before ORIG_HEAD\nis created. This series fixes that bug and then converts the code to use\nstruct object_id rather than passing around strings.\n\nThanks to Caspar for reporting the bug and providing a reproducible example\n\nChanges since v1:\n\n * Updated the commit message to patch 1 as suggested by Junio\n * Moved the conversion from find_unique_abbrev() to oid_to_hex() from patch\n 1 to patch 4\n * Fixed a compilation error in patch 3 (the required change was in patch 4\n by a mistake)\n\nCc: Johannes Schindelin Johannes.Schindelin@gmx.de\n[Johannes.Schindelin@gmx.de], Caspar Duregger herr.kaste@gmail.com\n[herr.kaste@gmail.com]\n\nPhillip Wood (4):\n rebase -i: stop overwriting ORIG_HEAD buffer\n rebase -i: use struct object_id rather than looking up commit\n rebase -i: use struct object_id when writing state\n rebase -i: simplify get_revision_ranges()\n\n builtin/rebase.c | 20 ++++++++++----------\n sequencer.c | 15 ++++++---------\n sequencer.h | 7 ++++---\n t/t3404-rebase-interactive.sh | 11 +++++++++++\n 4 files changed, 31 insertions(+), 22 deletions(-)\n\n\nbase-commit: 2e673356aefa8ed19be3c878f966ad6189ecb510\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-773%2Fphillipwood%2Fwip%2Frebase-fix-orig_head-handling-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-773/phillipwood/wip/rebase-fix-orig_head-handling-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/773\n\nRange-diff vs v1:\n\n 1: 24f2c4a623 ! 1: da05958c58 rebase -i: stop overwriting ORIG_HEAD buffer\n @@ Metadata\n ## Commit message ##\n rebase -i: stop overwriting ORIG_HEAD buffer\n \n - After rebasing ORIG_HEAD is supposed to point to the old HEAD of the\n - rebased branch. Unfortunately the buffer storing the oid was\n - overwritten with a new oid before ORIG_HEAD was created. The buffer is\n - also used when writing .git/rebase-merge/orig-head which is used by\n - `rebase --abort` to restore the previous head. Luckily that file is\n - written before the buffer is overwritten. As we want the full oid\n - find_unique_abbrev() is replaced with oid_to_hex_r() rather than\n - find_unique_abbrev_r().\n + After rebasing, ORIG_HEAD is supposed to point to the old HEAD of the\n + rebased branch. The code used find_unique_abbrev() to obtain the\n + object name of the old HEAD and wrote to both\n + .git/rebase-merge/orig-head (used by `rebase --abort` to go back to\n + the previous state) and to ORIG_HEAD. The buffer find_unique_abbrev()\n + gives back is volatile, unfortunately, and was overwritten after the\n + former file is written but before ORIG_FILE is written, leaving an\n + incorrect object name in it.\n +\n + Avoid relying on the volatile buffer of find_unique_abbrev(), and\n + instead supply our own buffer to keep the object name.\n \n I think that all of the users of head_hash should actually be using\n opts->orig_head instead as passing a string rather than a struct\n object_id around is a hang over from the scripted implementation. This\n patch just fixes the immediate bug and adds a regression test based on\n - Caspar's reproduction example. The users will be converted to use\n + Caspar's reproduction example[1]. The users will be converted to use\n struct object_id and head_hash removed in the next few commits.\n \n + [1] https://lore.kernel.org/git/CAFzd1+7PDg2PZgKw7U0kdepdYuoML9wSN4kofmB_-8NHrbbrHg@mail.gmail.com\n +\n Reported-by: Caspar Duregger \n Signed-off-by: Phillip Wood \n \n @@ builtin/rebase.c: static int edit_todo_file(unsigned flags)\n \tconst char *shorthead;\n \n -\t*head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ);\n -+\toid_to_hex_r(head_hash, orig_head);\n ++\tfind_unique_abbrev_r(head_hash, orig_head, GIT_MAX_HEXSZ);\n \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n -\t\t\t\t\t\t *head_hash);\n +\t\t\t\t\t\t head_hash);\n 2: 589aed08f1 = 2: da499f1e4c rebase -i: use struct object_id rather than looking up commit\n 3: 11494a7b79 ! 3: 076d212915 rebase -i: use struct object_id when writing state\n @@ builtin/rebase.c: static int get_revision_ranges(struct commit *upstream, struct\n {\n \tFILE *interactive;\n \n +@@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n + \n + \tif (init_basic_state(&replay,\n + \t\t\t opts->head_name ? opts->head_name : \"detached HEAD\",\n +-\t\t\t opts->onto, head_hash)) {\n ++\t\t\t opts->onto, &opts->orig_head)) {\n + \t\tfree(revisions);\n + \t\tfree(shortrevisions);\n + \n \n ## sequencer.c ##\n @@ sequencer.c: static void write_strategy_opts(struct replay_opts *opts)\n 4: ed78f8628a ! 4: faae3ccff5 rebase -i: simplify get_revision_ranges()\n @@ Commit message\n use a opts->orig_head instead we can stop returning head_hash from\n get_revision_ranges().\n \n + Because we want to pass the full object names back to the caller in\n + `revisions` the find_unique_abbrev_r() call that was used to initialize\n + `head_hash` is replaced with oid_to_hex().\n +\n Signed-off-by: Phillip Wood \n \n ## builtin/rebase.c ##\n @@ builtin/rebase.c: static int edit_todo_file(unsigned flags)\n \tstruct commit *base_rev = upstream ? upstream : onto;\n \tconst char *shorthead;\n \n --\toid_to_hex_r(head_hash, orig_head);\n +-\tfind_unique_abbrev_r(head_hash, orig_head, GIT_MAX_HEXSZ);\n \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n -\t\t\t\t\t\t head_hash);\n +\t\t\t oid_to_hex(orig_head));\n @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts,\n \t\treturn -1;\n \n \tif (init_basic_state(&replay,\n - \t\t\t opts->head_name ? opts->head_name : \"detached HEAD\",\n --\t\t\t opts->onto, head_hash)) {\n -+\t\t\t opts->onto, &opts->orig_head)) {\n - \t\tfree(revisions);\n - \t\tfree(shortrevisions);\n - \n\n-- \ngitgitgadget\n"},{"id":"409088","messageId":"da05958c58deb849def1209ac7d2b54c415c35cb.1604503780.git.gitgitgadget@gmail.com","threadId":"54515","inReplyTo":"pull.773.v2.git.1604503780.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] rebase -i: stop overwriting ORIG_HEAD buffer","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-04T15:29:37Z","receivedAt":"2020-11-04T15:29:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAfter rebasing, ORIG_HEAD is supposed to point to the old HEAD of the\nrebased branch. The code used find_unique_abbrev() to obtain the\nobject name of the old HEAD and wrote to both\n.git/rebase-merge/orig-head (used by `rebase --abort` to go back to\nthe previous state) and to ORIG_HEAD. The buffer find_unique_abbrev()\ngives back is volatile, unfortunately, and was overwritten after the\nformer file is written but before ORIG_FILE is written, leaving an\nincorrect object name in it.\n\nAvoid relying on the volatile buffer of find_unique_abbrev(), and\ninstead supply our own buffer to keep the object name.\n\nI think that all of the users of head_hash should actually be using\nopts->orig_head instead as passing a string rather than a struct\nobject_id around is a hang over from the scripted implementation. This\npatch just fixes the immediate bug and adds a regression test based on\nCaspar's reproduction example[1]. The users will be converted to use\nstruct object_id and head_hash removed in the next few commits.\n\n[1] https://lore.kernel.org/git/CAFzd1+7PDg2PZgKw7U0kdepdYuoML9wSN4kofmB_-8NHrbbrHg@mail.gmail.com\n\nReported-by: Caspar Duregger \nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 10 +++++-----\n t/t3404-rebase-interactive.sh | 11 +++++++++++\n 2 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex eeca53382f..cd101b2559 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -270,15 +270,15 @@ static int edit_todo_file(unsigned flags)\n }\n \n static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n-\t\t\t struct object_id *orig_head, const char **head_hash,\n+\t\t\t struct object_id *orig_head, char *head_hash,\n \t\t\t char **revisions, char **shortrevisions)\n {\n \tstruct commit *base_rev = upstream ? upstream : onto;\n \tconst char *shorthead;\n \n-\t*head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ);\n+\tfind_unique_abbrev_r(head_hash, orig_head, GIT_MAX_HEXSZ);\n \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n-\t\t\t\t\t\t *head_hash);\n+\t\t\t\t\t\t head_hash);\n \n \tshorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV);\n \n@@ -327,7 +327,7 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)\n static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n {\n \tint ret;\n-\tconst char *head_hash = NULL;\n+\tchar head_hash[GIT_MAX_HEXSZ];\n \tchar *revisions = NULL, *shortrevisions = NULL;\n \tstruct strvec make_script_args = STRVEC_INIT;\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n@@ -335,7 +335,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \tstruct string_list commands = STRING_LIST_INIT_DUP;\n \n \tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,\n-\t\t\t\t&head_hash, &revisions, &shortrevisions))\n+\t\t\t\thead_hash, &revisions, &shortrevisions))\n \t\treturn -1;\n \n \tif (init_basic_state(&replay,\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 07a1617351..1e56696e4f 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1797,6 +1797,17 @@ test_expect_success 'todo has correct onto hash' '\n \ttest_i18ngrep \"^# Rebase ..* onto $onto\" actual\n '\n \n+test_expect_success 'ORIG_HEAD is updated correctly' '\n+\ttest_when_finished \"git checkout master && git branch -D test-orig-head\" &&\n+\tgit checkout -b test-orig-head A &&\n+\tgit commit --allow-empty -m A1 &&\n+\tgit commit --allow-empty -m A2 &&\n+\tgit commit --allow-empty -m A3 &&\n+\tgit commit --allow-empty -m A4 &&\n+\tgit rebase master &&\n+\ttest_cmp_rev ORIG_HEAD test-orig-head@{1}\n+'\n+\n # This must be the last test in this file\n test_expect_success '$EDITOR and friends are unchanged' '\n \ttest_editor_unchanged\n-- \ngitgitgadget\n\n"},{"id":"409089","messageId":"076d212915c9a1706d18c5b1829bf0bf22bf5095.1604503780.git.gitgitgadget@gmail.com","threadId":"54515","inReplyTo":"pull.773.v2.git.1604503780.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] rebase -i: use struct object_id when writing state","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-04T15:29:39Z","receivedAt":"2020-11-04T15:29:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nRather than passing a string around pass the struct object_id that the\nstring was created from call oid_hex() when we write the file.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 5 +++--\n sequencer.c | 5 +++--\n sequencer.h | 2 +-\n 3 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 4e4a5e774e..28e7b7f5ce 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -296,7 +296,8 @@ static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n }\n \n static int init_basic_state(struct replay_opts *opts, const char *head_name,\n-\t\t\t struct commit *onto, const char *orig_head)\n+\t\t\t struct commit *onto,\n+\t\t\t const struct object_id *orig_head)\n {\n \tFILE *interactive;\n \n@@ -340,7 +341,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \n \tif (init_basic_state(&replay,\n \t\t\t opts->head_name ? opts->head_name : \"detached HEAD\",\n-\t\t\t opts->onto, head_hash)) {\n+\t\t\t opts->onto, &opts->orig_head)) {\n \t\tfree(revisions);\n \t\tfree(shortrevisions);\n \ndiff --git a/sequencer.c b/sequencer.c\nindex f79c3df861..2037f0ba66 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2692,7 +2692,7 @@ static void write_strategy_opts(struct replay_opts *opts)\n }\n \n int write_basic_state(struct replay_opts *opts, const char *head_name,\n-\t\t struct commit *onto, const char *orig_head)\n+\t\t struct commit *onto, const struct object_id *orig_head)\n {\n \tif (head_name)\n \t\twrite_file(rebase_path_head_name(), \"%s\\n\", head_name);\n@@ -2700,7 +2700,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\twrite_file(rebase_path_onto(), \"%s\\n\",\n \t\t\t oid_to_hex(&onto->object.oid));\n \tif (orig_head)\n-\t\twrite_file(rebase_path_orig_head(), \"%s\\n\", orig_head);\n+\t\twrite_file(rebase_path_orig_head(), \"%s\\n\",\n+\t\t\t oid_to_hex(orig_head));\n \n \tif (opts->quiet)\n \t\twrite_file(rebase_path_quiet(), \"%s\", \"\");\ndiff --git a/sequencer.h b/sequencer.h\nindex ea56825488..cf201f2406 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -227,7 +227,7 @@ int read_author_script(const char *path, char **name, char **email, char **date,\n \t\t int allow_missing);\n void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);\n int write_basic_state(struct replay_opts *opts, const char *head_name,\n-\t\t struct commit *onto, const char *orig_head);\n+\t\t struct commit *onto, const struct object_id *orig_head);\n void sequencer_post_commit_cleanup(struct repository *r, int verbose);\n int sequencer_get_last_command(struct repository* r,\n \t\t\t enum replay_action *action);\n-- \ngitgitgadget\n\n"},{"id":"409090","messageId":"faae3ccff53b83aac3b8998fc5259f319fd80fc4.1604503780.git.gitgitgadget@gmail.com","threadId":"54515","inReplyTo":"pull.773.v2.git.1604503780.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] rebase -i: simplify get_revision_ranges()","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-04T15:29:40Z","receivedAt":"2020-11-04T15:29:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nNow that all the external users of head_hash have been converted to\nuse a opts->orig_head instead we can stop returning head_hash from\nget_revision_ranges().\n\nBecause we want to pass the full object names back to the caller in\n`revisions` the find_unique_abbrev_r() call that was used to initialize\n`head_hash` is replaced with oid_to_hex().\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 28e7b7f5ce..17450e9d9e 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -270,15 +270,14 @@ static int edit_todo_file(unsigned flags)\n }\n \n static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n-\t\t\t struct object_id *orig_head, char *head_hash,\n-\t\t\t char **revisions, char **shortrevisions)\n+\t\t\t struct object_id *orig_head, char **revisions,\n+\t\t\t char **shortrevisions)\n {\n \tstruct commit *base_rev = upstream ? upstream : onto;\n \tconst char *shorthead;\n \n-\tfind_unique_abbrev_r(head_hash, orig_head, GIT_MAX_HEXSZ);\n \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n-\t\t\t\t\t\t head_hash);\n+\t\t\t oid_to_hex(orig_head));\n \n \tshorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV);\n \n@@ -328,7 +327,6 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)\n static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n {\n \tint ret;\n-\tchar head_hash[GIT_MAX_HEXSZ];\n \tchar *revisions = NULL, *shortrevisions = NULL;\n \tstruct strvec make_script_args = STRVEC_INIT;\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n@@ -336,7 +334,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \tstruct string_list commands = STRING_LIST_INIT_DUP;\n \n \tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,\n-\t\t\t\thead_hash, &revisions, &shortrevisions))\n+\t\t\t\t&revisions, &shortrevisions))\n \t\treturn -1;\n \n \tif (init_basic_state(&replay,\n-- \ngitgitgadget\n"},{"id":"409091","messageId":"da499f1e4cc1fc949aff0089898d0275d3997acd.1604503780.git.gitgitgadget@gmail.com","threadId":"54515","inReplyTo":"pull.773.v2.git.1604503780.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] rebase -i: use struct object_id rather than looking up commit","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-04T15:29:38Z","receivedAt":"2020-11-04T15:30:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWe already have a struct object_id containing the oid that we want to\nset ORIG_HEAD to so use that rather than converting it to a string and\nthen calling get_oid() on that string.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 5 +++--\n sequencer.c | 10 +++-------\n sequencer.h | 5 +++--\n 3 files changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex cd101b2559..4e4a5e774e 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -370,8 +370,9 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \n \t\tsplit_exec_commands(opts->cmd, &commands);\n \t\tret = complete_action(the_repository, &replay, flags,\n-\t\t\tshortrevisions, opts->onto_name, opts->onto, head_hash,\n-\t\t\t&commands, opts->autosquash, &todo_list);\n+\t\t\tshortrevisions, opts->onto_name, opts->onto,\n+\t\t\t&opts->orig_head, &commands, opts->autosquash,\n+\t\t\t&todo_list);\n \t}\n \n \tstring_list_clear(&commands, 0);\ndiff --git a/sequencer.c b/sequencer.c\nindex 00acb12496..f79c3df861 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3965,21 +3965,17 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n \n static int checkout_onto(struct repository *r, struct replay_opts *opts,\n \t\t\t const char *onto_name, const struct object_id *onto,\n-\t\t\t const char *orig_head)\n+\t\t\t const struct object_id *orig_head)\n {\n-\tstruct object_id oid;\n \tconst char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n \n-\tif (get_oid(orig_head, &oid))\n-\t\treturn error(_(\"%s: not a valid OID\"), orig_head);\n-\n \tif (run_git_checkout(r, opts, oid_to_hex(onto), action)) {\n \t\tapply_autostash(rebase_path_autostash());\n \t\tsequencer_remove_state(opts);\n \t\treturn error(_(\"could not detach HEAD\"));\n \t}\n \n-\treturn update_ref(NULL, \"ORIG_HEAD\", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n+\treturn update_ref(NULL, \"ORIG_HEAD\", orig_head, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n }\n \n static int stopped_at_head(struct repository *r)\n@@ -5314,7 +5310,7 @@ static int skip_unnecessary_picks(struct repository *r,\n \n int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,\n \t\t const char *shortrevisions, const char *onto_name,\n-\t\t struct commit *onto, const char *orig_head,\n+\t\t struct commit *onto, const struct object_id *orig_head,\n \t\t struct string_list *commands, unsigned autosquash,\n \t\t struct todo_list *todo_list)\n {\ndiff --git a/sequencer.h b/sequencer.h\nindex b2a501e445..ea56825488 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -163,8 +163,9 @@ void todo_list_add_exec_commands(struct todo_list *todo_list,\n \t\t\t\t struct string_list *commands);\n int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,\n \t\t const char *shortrevisions, const char *onto_name,\n-\t\t struct commit *onto, const char *orig_head, struct string_list *commands,\n-\t\t unsigned autosquash, struct todo_list *todo_list);\n+\t\t struct commit *onto, const struct object_id *orig_head,\n+\t\t struct string_list *commands, unsigned autosquash,\n+\t\t struct todo_list *todo_list);\n int todo_list_rearrange_squash(struct todo_list *todo_list);\n \n /*\n-- \ngitgitgadget\n\n"}]}