{"thread":{"id":"52654","subject":"[PATCH 0/3] Re-fix rebase -i with SHA-1 collisions","startedAt":"2020-01-16T21:18:31Z","lastAt":"2020-01-23T12:28:29Z","messageCount":28,"participants":["Johannes Schindelin via GitGitGadget","brian m. carlson","Johannes Schindelin","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"389931","messageId":"pull.529.git.1579209506.gitgitgadget@gmail.com","threadId":"52654","inReplyTo":null,"subject":"[PATCH 0/3] Re-fix rebase -i with SHA-1 collisions","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-16T21:18:23Z","receivedAt":"2020-01-16T21:18:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Triggered by one of brian's SHA-256 patch series, I looked at the reason why\nthe SHA-1 collision test case passed when it shouldn't. Turns out that the\nregression test was not quite thorough enough, and the interactive rebase \ndid regress recently.\n\nWhile in the area, I realized that the same bug exists in the code backing\nthe rebase.missingCommitsCheck feature: the backed-up todo list uses\nshortened commit IDs that may very well become ambiguous during the rebase.\nFor good measure, this patch series fixes that, too.\n\nFinally, I saw that git rebase --edit-todo reported the line in an awkward,\nmaybe even incorrect, way when there was an ambiguous commit ID, and I also\nfixed that.\n\nTo make sure that the code can be easily adapted to SHA-256 after these\npatches, I actually already made those adjustments on top and offered them\nup at https://github.com/bk2204/git/pull/1.\n\nJohannes Schindelin (3):\n parse_insn_line(): improve error message when parsing failed\n rebase -i: re-fix short SHA-1 collision\n rebase -i: also avoid SHA-1 collisions with missingCommitsCheck\n\n rebase-interactive.c | 8 +++++---\n sequencer.c | 21 +++++++++++++++++----\n t/t3404-rebase-interactive.sh | 17 +++++++++++++++--\n 3 files changed, 37 insertions(+), 9 deletions(-)\n\n\nbase-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-529%2Fdscho%2Fre-fix-rebase-i-with-sha-collisions-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-529/dscho/re-fix-rebase-i-with-sha-collisions-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/529\n-- \ngitgitgadget\n"},{"id":"389932","messageId":"2ae2e435b0ef6888e72defc7abee1909b29aa914.1579209506.git.gitgitgadget@gmail.com","threadId":"52654","inReplyTo":"pull.529.git.1579209506.gitgitgadget@gmail.com","subject":"[PATCH 1/3] parse_insn_line(): improve error message when parsing failed","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-16T21:18:24Z","receivedAt":"2020-01-16T21:18:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the case that a `get_oid()` call failed, we showed some rather bogus\npart of the line instead of the precise string we sent to said function.\nThat makes it rather hard for users to understand what is going wrong,\nso let's fix that.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b9dbf1adb0..7c30dad59c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2118,6 +2118,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \tsaved = *end_of_object_name;\n \t*end_of_object_name = '\\0';\n \tstatus = get_oid(bol, &commit_oid);\n+\tif (status < 0)\n+\t\terror(_(\"could not parse '%s'\"), bol); /* return later */\n \t*end_of_object_name = saved;\n \n \tbol = end_of_object_name + strspn(end_of_object_name, \" \\t\");\n@@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \titem->arg_len = (int)(eol - bol);\n \n \tif (status < 0)\n-\t\treturn error(_(\"could not parse '%.*s'\"),\n-\t\t\t (int)(end_of_object_name - bol), bol);\n+\t\treturn status;\n \n \titem->commit = lookup_commit_reference(r, &commit_oid);\n-\treturn !item->commit;\n+\treturn item->commit ? 0 : -1;\n }\n \n int sequencer_get_last_command(struct repository *r, enum replay_action *action)\n-- \ngitgitgadget\n\n"},{"id":"389933","messageId":"ad50cd1b92e3e52309536f3a84064571a224a0da.1579209506.git.gitgitgadget@gmail.com","threadId":"52654","inReplyTo":"pull.529.git.1579209506.gitgitgadget@gmail.com","subject":"[PATCH 2/3] rebase -i: re-fix short SHA-1 collision","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-16T21:18:25Z","receivedAt":"2020-01-16T21:18:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 66ae9a57b88 (t3404: rebase -i: demonstrate short SHA-1 collision,\n2013-08-23), we added a test case that demonstrated how it is possible\nthat a previously unambiguous short commit ID could become ambiguous\n*during* a rebase.\n\nIn 75c69766554 (rebase -i: fix short SHA-1 collision, 2013-08-23), we\nfixed that problem simply by writing out the todo list with expanded\ncommit IDs (except *right* before letting the user edit the todo list,\nin which case we shorten them, but we expand them right after the file\nwas edited).\n\nHowever, the bug resurfaced as a side effect of 393adf7a6f6 (sequencer:\ndirectly call pick_commits() from complete_action(), 2019-11-24): as of\nthis commit, the sequencer no longer re-reads the todo list after\nwriting it out with expanded commit IDs.\n\nThe only redeeming factor is that the todo list is already parsed at\nthat stage, including all the commits corresponding to the commands,\ntherefore the sequencer can continue even if the internal todo list has\nshort commit IDs.\n\nThat does not prevent problems, though: the sequencer writes out the\n`done` and `git-rebase-todo` files incrementally (i.e. overwriting the\ntodo list with a version that has _short_ commit IDs), and if a merge\nconflict happens, or if an `edit` or a `break` command is encountered, a\nsubsequent `git rebase --continue` _will_ re-read the todo list, opening\nan opportunity for the \"short SHA-1 collision\" bug again.\n\nTo avoid that, let's make sure that we do expand the commit IDs in the\ntodo list as soon as we have parsed it after letting the user edit it.\n\nAdditionally, we improve the 'short SHA-1 collide' test case in t3404 to\ntest specifically for the case where the rebase is resumed. We also\nhard-code the expected colliding short SHA-1s, to document the\nexpectation (and to make it easier on future readers).\n\nNote that we specifically test that the short commit ID is used in the\n`git-rebase-todo.tmp` file: this file is created by the fake editor in\nthe test script and reflects the state that would have been presented to\nthe user to edit.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 14 +++++++++++++-\n t/t3404-rebase-interactive.sh | 15 +++++++++++++--\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7c30dad59c..c2945c699d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5076,7 +5076,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n {\n \tconst char *shortonto, *todo_file = rebase_path_todo();\n \tstruct todo_list new_todo = TODO_LIST_INIT;\n-\tstruct strbuf *buf = &todo_list->buf;\n+\tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n \tstruct object_id oid = onto->object.oid;\n \tint res;\n \n@@ -5128,6 +5128,18 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn -1;\n \t}\n \n+\t/* Expand the commit IDs */\n+\ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n+\tstrbuf_swap(&new_todo.buf, &buf2);\n+\tstrbuf_release(&buf2);\n+\tnew_todo.total_nr -= new_todo.nr;\n+\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0) {\n+\t\tfprintf(stderr, _(edit_todo_list_advice));\n+\t\tcheckout_onto(r, opts, onto_name, &onto->object.oid, orig_head);\n+\t\ttodo_list_release(&new_todo);\n+\t\treturn error(_(\"invalid todo list after expanding IDs\"));\n+\t}\n+\n \tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {\n \t\ttodo_list_release(&new_todo);\n \t\treturn error(_(\"could not skip unnecessary pick commands\"));\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ae6e55ce79..1cc9f36bc7 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' '\n test_expect_success SHA1 'short SHA-1 collide' '\n \ttest_when_finished \"reset_rebase && git checkout master\" &&\n \tgit checkout collide &&\n+\tcolliding_sha1=6bcda37 &&\n+\ttest $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n \t(\n \t\tunset test_tick &&\n \t\ttest_tick &&\n \t\tset_fake_editor &&\n \t\tFAKE_COMMIT_MESSAGE=\"collide2 ac4f2ee\" \\\n-\t\tFAKE_LINES=\"reword 1 2\" git rebase -i HEAD~2\n-\t)\n+\t\tFAKE_LINES=\"reword 1 break 2\" git rebase -i HEAD~2 &&\n+\t\ttest $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n+\t\tgrep \"^pick $colliding_sha1 \" \\\n+\t\t\t.git/rebase-merge/git-rebase-todo.tmp &&\n+\t\tgrep \"^pick [0-9a-f]\\{40\\}\" \\\n+\t\t\t.git/rebase-merge/git-rebase-todo &&\n+\t\tgit rebase --continue\n+\t) &&\n+\tcollide2=\"$(git rev-parse HEAD~1 | cut -c 1-4)\" &&\n+\tcollide3=\"$(git rev-parse collide3 | cut -c 1-4)\" &&\n+\ttest \"$collide2\" = \"$collide3\"\n '\n \n test_expect_success 'respect core.abbrev' '\n-- \ngitgitgadget\n\n"},{"id":"389934","messageId":"e7d9ea8992461a16fb53efb1ab9a726530c1d315.1579209506.git.gitgitgadget@gmail.com","threadId":"52654","inReplyTo":"pull.529.git.1579209506.gitgitgadget@gmail.com","subject":"[PATCH 3/3] rebase -i: also avoid SHA-1 collisions with missingCommitsCheck","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-16T21:18:26Z","receivedAt":"2020-01-16T21:18:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen `rebase.missingCommitsCheck` is in effect, we use the backup of the\ntodo list that was copied just before the user was allowed to edit it.\n\nThat backup is, of course, just as susceptible to the hash collision as\nthe todo list itself: a reworded commit could make a previously\nunambiguous short commit ID ambiguous all of a sudden.\n\nSo let's not just copy the todo list, but let's instead write out the\nbackup with expanded commit IDs.\n\nSigned-off-by: Johannes Schindelin \n---\n rebase-interactive.c | 8 +++++---\n t/t3404-rebase-interactive.sh | 2 ++\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex aa18ae82b7..1259adc8ea 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -104,9 +104,11 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n \t\t\t\t -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))\n \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n \n-\tif (initial && copy_file(rebase_path_todo_backup(), todo_file, 0666))\n-\t\treturn error(_(\"could not copy '%s' to '%s'.\"), todo_file,\n-\t\t\t rebase_path_todo_backup());\n+\tif (initial &&\n+\t todo_list_write_to_file(r, todo_list, rebase_path_todo_backup(),\n+\t\t\t\t shortrevisions, shortonto, -1,\n+\t\t\t\t (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)\n+\t\treturn error(_(\"could not write '%s'.\"), rebase_path_todo_backup());\n \n \tif (launch_sequence_editor(todo_file, &new_todo->buf, NULL))\n \t\treturn -2;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 1cc9f36bc7..b90ea0fe44 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1277,6 +1277,8 @@ test_expect_success SHA1 'short SHA-1 collide' '\n \t\t\t.git/rebase-merge/git-rebase-todo.tmp &&\n \t\tgrep \"^pick [0-9a-f]\\{40\\}\" \\\n \t\t\t.git/rebase-merge/git-rebase-todo &&\n+\t\tgrep \"^pick [0-9a-f]\\{40\\}\" \\\n+\t\t\t.git/rebase-merge/git-rebase-todo.backup &&\n \t\tgit rebase --continue\n \t) &&\n \tcollide2=\"$(git rev-parse HEAD~1 | cut -c 1-4)\" &&\n-- \ngitgitgadget\n"},{"id":"389958","messageId":"20200116235411.GZ6570@camp.crustytoothpaste.net","threadId":"52654","inReplyTo":"pull.529.git.1579209506.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Re-fix rebase -i with SHA-1 collisions","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-01-16T23:54:11Z","receivedAt":"2020-01-16T23:54:19Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-01-16 at 21:18:23, Johannes Schindelin via GitGitGadget wrote:\n> Triggered by one of brian's SHA-256 patch series, I looked at the reason why\n> the SHA-1 collision test case passed when it shouldn't. Turns out that the\n> regression test was not quite thorough enough, and the interactive rebase \n> did regress recently.\n> \n> While in the area, I realized that the same bug exists in the code backing\n> the rebase.missingCommitsCheck feature: the backed-up todo list uses\n> shortened commit IDs that may very well become ambiguous during the rebase.\n> For good measure, this patch series fixes that, too.\n> \n> Finally, I saw that git rebase --edit-todo reported the line in an awkward,\n> maybe even incorrect, way when there was an ambiguous commit ID, and I also\n> fixed that.\n> \n> To make sure that the code can be easily adapted to SHA-256 after these\n> patches, I actually already made those adjustments on top and offered them\n> up at https://github.com/bk2204/git/pull/1.\n\nThis series looks great to me, and thanks for fixing this.\n\nAs mentioned in the PR, I'm happy for you to drop the SHA-256 patch into\nthis series if you like, or I can carry it in a future series. Either\nway is fine with me.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"389963","messageId":"nycvar.QRO.7.76.6.2001171050000.46@tvgsbejvaqbjf.bet","threadId":"52654","inReplyTo":"20200116235411.GZ6570@camp.crustytoothpaste.net","subject":"Re: [PATCH 0/3] Re-fix rebase -i with SHA-1 collisions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-17T09:51:42Z","receivedAt":"2020-01-17T09:51:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi brian,\n\nOn Thu, 16 Jan 2020, brian m. carlson wrote:\n\n> On 2020-01-16 at 21:18:23, Johannes Schindelin via GitGitGadget wrote:\n> > Triggered by one of brian's SHA-256 patch series, I looked at the reason why\n> > the SHA-1 collision test case passed when it shouldn't. Turns out that the\n> > regression test was not quite thorough enough, and the interactive rebase\n> > did regress recently.\n> >\n> > While in the area, I realized that the same bug exists in the code backing\n> > the rebase.missingCommitsCheck feature: the backed-up todo list uses\n> > shortened commit IDs that may very well become ambiguous during the rebase.\n> > For good measure, this patch series fixes that, too.\n> >\n> > Finally, I saw that git rebase --edit-todo reported the line in an awkward,\n> > maybe even incorrect, way when there was an ambiguous commit ID, and I also\n> > fixed that.\n> >\n> > To make sure that the code can be easily adapted to SHA-256 after these\n> > patches, I actually already made those adjustments on top and offered them\n> > up at https://github.com/bk2204/git/pull/1.\n>\n> This series looks great to me, and thanks for fixing this.\n>\n> As mentioned in the PR, I'm happy for you to drop the SHA-256 patch into\n> this series if you like, or I can carry it in a future series. Either\n> way is fine with me.\n\nExcellent. Given that the re-fix to avoid short commit ID collisions has\nlittle to do with supporting SHA-256, I would like to keep the patch\nseries separate, then.\n\nThe question whether to move the SHA-256 support patch into your series is\nmore a question to Junio, i.e. which patch series will be merged down\nfaster.\n\nJunio, what is your preference here?\n\nCiao,\nDscho\n"},{"id":"390003","messageId":"xmqqmuamdkuv.fsf@gitster-ct.c.googlers.com","threadId":"52654","inReplyTo":"2ae2e435b0ef6888e72defc7abee1909b29aa914.1579209506.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] parse_insn_line(): improve error message when parsing failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-17T18:00:08Z","receivedAt":"2020-01-17T18:00:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In the case that a `get_oid()` call failed, we showed some rather bogus\n> part of the line instead of the precise string we sent to said function.\n> That makes it rather hard for users to understand what is going wrong,\n> so let's fix that.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> sequencer.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index b9dbf1adb0..7c30dad59c 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2118,6 +2118,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n> \tsaved = *end_of_object_name;\n> \t*end_of_object_name = '\\0';\n> \tstatus = get_oid(bol, &commit_oid);\n> +\tif (status < 0)\n> +\t\terror(_(\"could not parse '%s'\"), bol); /* return later */\n> \t*end_of_object_name = saved;\n\nOK, so after making sure the line begins a command that takes an\nobject name, we first NUL terminated the token after the command\nbut it turns out the string is not a valid oid. We show the part\nwe thought was the object name before reverting the NUL termination.\n\nMakes sense. And then later...\n\n> \tbol = end_of_object_name + strspn(end_of_object_name, \" \\t\");\n> @@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n> \titem->arg_len = (int)(eol - bol);\n> \n> \tif (status < 0)\n> -\t\treturn error(_(\"could not parse '%.*s'\"),\n> -\t\t\t (int)(end_of_object_name - bol), bol);\n> +\t\treturn status;\n\n...this is the *only* code that uses the status that was assigned to\nthe return value of get_oid(). \n\nBecause the implementation of this function (mis)uses \"bol\", which\nstands for \"beginning of line\", as a moving pointer of \"beginning of\nthe token we are looking at\", the value of \"bol\" at this point of\nthe control in the original code is not where the \"bol\" pointer used\nto be when end-of-object-name was computed (if it were, the '%.*s'\nargument would have been correct) and shows a token after where the\nobject name would have been, which may help the user identify the\nline but does not directly show what token we had trouble with\nparsing.\n\nAnd the updated code will avoid the issue by using the \"bol\" pointer\nwhen it is still pointing at the right place.\n\n\n> \titem->commit = lookup_commit_reference(r, &commit_oid);\n> -\treturn !item->commit;\n> +\treturn item->commit ? 0 : -1;\n\nThis changes the polarity of the error exit from positive 1 to\nnegative 1. The only caller of this function takes anything\nnon-zero as a failure so this would not cause behaviour change, but\nreturning negative is more in line with the practice so it is an\nimprovement (it is unrelated to the theme of this patch and is not\nexplained, though).\n\nOK.\n\n"},{"id":"390004","messageId":"xmqqimladjh3.fsf@gitster-ct.c.googlers.com","threadId":"52654","inReplyTo":"ad50cd1b92e3e52309536f3a84064571a224a0da.1579209506.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] rebase -i: re-fix short SHA-1 collision","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-17T18:30:00Z","receivedAt":"2020-01-17T18:30:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> However, the bug resurfaced as a side effect of 393adf7a6f6 (sequencer:\n> directly call pick_commits() from complete_action(), 2019-11-24): as of\n> this commit, the sequencer no longer re-reads the todo list after\n> writing it out with expanded commit IDs.\n\nOuch. The analysis above is quite understandable.\n\n> @@ -5128,6 +5128,18 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \t\treturn -1;\n> \t}\n> \n> +\t/* Expand the commit IDs */\n> +\ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n> +\tstrbuf_swap(&new_todo.buf, &buf2);\n> +\tstrbuf_release(&buf2);\n> +\tnew_todo.total_nr -= new_todo.nr;\n> +\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0) {\n> +\t\tfprintf(stderr, _(edit_todo_list_advice));\n> +\t\tcheckout_onto(r, opts, onto_name, &onto->object.oid, orig_head);\n> +\t\ttodo_list_release(&new_todo);\n> +\t\treturn error(_(\"invalid todo list after expanding IDs\"));\n> +\t}\n\nThe above happens after edit_todo_list() returns and then the\nresulting data (i.e. new_todo) that came from the on-disk file has\nbeen parsed with an existing call to todo_lsit_parse_insn_buffer()?\n\nI am wondering when this if() statement would trigger, iow, under\nwhat condition the result of \"Expand the commit IDs\" operation fails\nto be parsed correctly, and what the user can do to remedy it.\nEspecially given that incoming new_todo has passed the existing\nparse and check without hitting \"return -1\" we see above in the\ncontext, I am not sure if there is anything, other than any\npotential glitch in the added code above, that can cause this second\nparse_insn_buffer() to fail. Shouldn't the body of if() be simply a\nBUG()?\n\nOr am I somehow missing a hunk that removes an existing call to\nparse&check?\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index ae6e55ce79..1cc9f36bc7 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' '\n> test_expect_success SHA1 'short SHA-1 collide' '\n> \ttest_when_finished \"reset_rebase && git checkout master\" &&\n> \tgit checkout collide &&\n> +\tcolliding_sha1=6bcda37 &&\n> +\ttest $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n> \t(\n> \t\tunset test_tick &&\n> \t\ttest_tick &&\n> \t\tset_fake_editor &&\n> \t\tFAKE_COMMIT_MESSAGE=\"collide2 ac4f2ee\" \\\n> -\t\tFAKE_LINES=\"reword 1 2\" git rebase -i HEAD~2\n> -\t)\n> +\t\tFAKE_LINES=\"reword 1 break 2\" git rebase -i HEAD~2 &&\n> +\t\ttest $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n> +\t\tgrep \"^pick $colliding_sha1 \" \\\n> +\t\t\t.git/rebase-merge/git-rebase-todo.tmp &&\n> +\t\tgrep \"^pick [0-9a-f]\\{40\\}\" \\\n> +\t\t\t.git/rebase-merge/git-rebase-todo &&\n> +\t\tgit rebase --continue\n> +\t) &&\n> +\tcollide2=\"$(git rev-parse HEAD~1 | cut -c 1-4)\" &&\n> +\tcollide3=\"$(git rev-parse collide3 | cut -c 1-4)\" &&\n> +\ttest \"$collide2\" = \"$collide3\"\n> '\n> \n> test_expect_success 'respect core.abbrev' '\n"},{"id":"390026","messageId":"nycvar.QRO.7.76.6.2001172229300.46@tvgsbejvaqbjf.bet","threadId":"52654","inReplyTo":"xmqqimladjh3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] rebase -i: re-fix short SHA-1 collision","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-17T21:31:16Z","receivedAt":"2020-01-17T21:31:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 17 Jan 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > +\t/* Expand the commit IDs */\n> > +\ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n> > +\tstrbuf_swap(&new_todo.buf, &buf2);\n> > +\tstrbuf_release(&buf2);\n> > +\tnew_todo.total_nr -= new_todo.nr;\n> > +\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0) {\n> > +\t\tfprintf(stderr, _(edit_todo_list_advice));\n> > +\t\tcheckout_onto(r, opts, onto_name, &onto->object.oid, orig_head);\n> > +\t\ttodo_list_release(&new_todo);\n> > +\t\treturn error(_(\"invalid todo list after expanding IDs\"));\n> > +\t}\n>\n> The above happens after edit_todo_list() returns and then the\n> resulting data (i.e. new_todo) that came from the on-disk file has\n> been parsed with an existing call to todo_lsit_parse_insn_buffer()?\n>\n> I am wondering when this if() statement would trigger, iow, under\n> what condition the result of \"Expand the commit IDs\" operation fails\n> to be parsed correctly, and what the user can do to remedy it.\n\nYou're right, this is defensive code to guard against bugs, but the error\nmessage suggests that it might be a user error: it isn't.\n\n> Especially given that incoming new_todo has passed the existing\n> parse and check without hitting \"return -1\" we see above in the\n> context, I am not sure if there is anything, other than any\n> potential glitch in the added code above, that can cause this second\n> parse_insn_buffer() to fail. Shouldn't the body of if() be simply a\n> BUG()?\n\nIt should.\n\nWill fix,\nDscho\n\n> Or am I somehow missing a hunk that removes an existing call to\n> parse&check?\n>\n> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> > index ae6e55ce79..1cc9f36bc7 100755\n> > --- a/t/t3404-rebase-interactive.sh\n> > +++ b/t/t3404-rebase-interactive.sh\n> > @@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' '\n> > test_expect_success SHA1 'short SHA-1 collide' '\n> > \ttest_when_finished \"reset_rebase && git checkout master\" &&\n> > \tgit checkout collide &&\n> > +\tcolliding_sha1=6bcda37 &&\n> > +\ttest $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n> > \t(\n> > \t\tunset test_tick &&\n> > \t\ttest_tick &&\n> > \t\tset_fake_editor &&\n> > \t\tFAKE_COMMIT_MESSAGE=\"collide2 ac4f2ee\" \\\n> > -\t\tFAKE_LINES=\"reword 1 2\" git rebase -i HEAD~2\n> > -\t)\n> > +\t\tFAKE_LINES=\"reword 1 break 2\" git rebase -i HEAD~2 &&\n> > +\t\ttest $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n> > +\t\tgrep \"^pick $colliding_sha1 \" \\\n> > +\t\t\t.git/rebase-merge/git-rebase-todo.tmp &&\n> > +\t\tgrep \"^pick [0-9a-f]\\{40\\}\" \\\n> > +\t\t\t.git/rebase-merge/git-rebase-todo &&\n> > +\t\tgit rebase --continue\n> > +\t) &&\n> > +\tcollide2=\"$(git rev-parse HEAD~1 | cut -c 1-4)\" &&\n> > +\tcollide3=\"$(git rev-parse collide3 | cut -c 1-4)\" &&\n> > +\ttest \"$collide2\" = \"$collide3\"\n> > '\n> >\n> > test_expect_success 'respect core.abbrev' '\n>\n>\n"},{"id":"390028","messageId":"20200117223734.GA6570@camp.crustytoothpaste.net","threadId":"52654","inReplyTo":"nycvar.QRO.7.76.6.2001171050000.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/3] Re-fix rebase -i with SHA-1 collisions","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-01-17T22:37:34Z","receivedAt":"2020-01-17T22:37:42Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-01-17 at 09:51:42, Johannes Schindelin wrote:\n> > This series looks great to me, and thanks for fixing this.\n> >\n> > As mentioned in the PR, I'm happy for you to drop the SHA-256 patch into\n> > this series if you like, or I can carry it in a future series. Either\n> > way is fine with me.\n> \n> Excellent. Given that the re-fix to avoid short commit ID collisions has\n> little to do with supporting SHA-256, I would like to keep the patch\n> series separate, then.\n> \n> The question whether to move the SHA-256 support patch into your series is\n> more a question to Junio, i.e. which patch series will be merged down\n> faster.\n\nI need to do a reroll of part 8, so I'll pick it into a future series\n(part 9) of test fixes and drop my patch. That way it won't interfere\nwith either series making progress, but it will still be included at the\nend.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"390029","messageId":"pull.529.v2.git.1579304283.gitgitgadget@gmail.com","threadId":"52654","inReplyTo":"pull.529.git.1579209506.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Re-fix rebase -i with SHA-1 collisions","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-17T23:38:00Z","receivedAt":"2020-01-17T23:38:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Triggered by one of brian's SHA-256 patch series, I looked at the reason why\nthe SHA-1 collision test case passed when it shouldn't. Turns out that the\nregression test was not quite thorough enough, and the interactive rebase \ndid regress recently.\n\nWhile in the area, I realized that the same bug exists in the code backing\nthe rebase.missingCommitsCheck feature: the backed-up todo list uses\nshortened commit IDs that may very well become ambiguous during the rebase.\nFor good measure, this patch series fixes that, too.\n\nFinally, I saw that git rebase --edit-todo reported the line in an awkward,\nmaybe even incorrect, way when there was an ambiguous commit ID, and I also\nfixed that.\n\nTo make sure that the code can be easily adapted to SHA-256 after these\npatches, I actually already made those adjustments on top and offered them\nup at https://github.com/bk2204/git/pull/1.\n\nChanges since v1:\n\n * Turned the error condition when parsing the todo list with just-expanded\n commit IDs failed into a BUG().\n\nJohannes Schindelin (3):\n parse_insn_line(): improve error message when parsing failed\n rebase -i: re-fix short SHA-1 collision\n rebase -i: also avoid SHA-1 collisions with missingCommitsCheck\n\n rebase-interactive.c | 8 +++++---\n sequencer.c | 18 ++++++++++++++----\n t/t3404-rebase-interactive.sh | 17 +++++++++++++++--\n 3 files changed, 34 insertions(+), 9 deletions(-)\n\n\nbase-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-529%2Fdscho%2Fre-fix-rebase-i-with-sha-collisions-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-529/dscho/re-fix-rebase-i-with-sha-collisions-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/529\n\nRange-diff vs v1:\n\n 1: 2ae2e435b0 = 1: 2ae2e435b0 parse_insn_line(): improve error message when parsing failed\n 2: ad50cd1b92 ! 2: 102fa568dc rebase -i: re-fix short SHA-1 collision\n @@ -66,12 +66,9 @@\n +\tstrbuf_swap(&new_todo.buf, &buf2);\n +\tstrbuf_release(&buf2);\n +\tnew_todo.total_nr -= new_todo.nr;\n -+\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0) {\n -+\t\tfprintf(stderr, _(edit_todo_list_advice));\n -+\t\tcheckout_onto(r, opts, onto_name, &onto->object.oid, orig_head);\n -+\t\ttodo_list_release(&new_todo);\n -+\t\treturn error(_(\"invalid todo list after expanding IDs\"));\n -+\t}\n ++\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n ++\t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n ++\t\t new_todo.buf.buf);\n +\n \tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {\n \t\ttodo_list_release(&new_todo);\n 3: e7d9ea8992 = 3: 486e9413a6 rebase -i: also avoid SHA-1 collisions with missingCommitsCheck\n\n-- \ngitgitgadget\n"},{"id":"390030","messageId":"2ae2e435b0ef6888e72defc7abee1909b29aa914.1579304283.git.gitgitgadget@gmail.com","threadId":"52654","inReplyTo":"pull.529.v2.git.1579304283.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] parse_insn_line(): improve error message when parsing failed","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-17T23:38:01Z","receivedAt":"2020-01-17T23:38:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the case that a `get_oid()` call failed, we showed some rather bogus\npart of the line instead of the precise string we sent to said function.\nThat makes it rather hard for users to understand what is going wrong,\nso let's fix that.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b9dbf1adb0..7c30dad59c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2118,6 +2118,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \tsaved = *end_of_object_name;\n \t*end_of_object_name = '\\0';\n \tstatus = get_oid(bol, &commit_oid);\n+\tif (status < 0)\n+\t\terror(_(\"could not parse '%s'\"), bol); /* return later */\n \t*end_of_object_name = saved;\n \n \tbol = end_of_object_name + strspn(end_of_object_name, \" \\t\");\n@@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \titem->arg_len = (int)(eol - bol);\n \n \tif (status < 0)\n-\t\treturn error(_(\"could not parse '%.*s'\"),\n-\t\t\t (int)(end_of_object_name - bol), bol);\n+\t\treturn status;\n \n \titem->commit = lookup_commit_reference(r, &commit_oid);\n-\treturn !item->commit;\n+\treturn item->commit ? 0 : -1;\n }\n \n int sequencer_get_last_command(struct repository *r, enum replay_action *action)\n-- \ngitgitgadget\n\n"},{"id":"390031","messageId":"486e9413a6b19d2695878fc19ffc49adabe3b00f.1579304283.git.gitgitgadget@gmail.com","threadId":"52654","inReplyTo":"pull.529.v2.git.1579304283.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] rebase -i: also avoid SHA-1 collisions with missingCommitsCheck","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-17T23:38:03Z","receivedAt":"2020-01-17T23:38:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen `rebase.missingCommitsCheck` is in effect, we use the backup of the\ntodo list that was copied just before the user was allowed to edit it.\n\nThat backup is, of course, just as susceptible to the hash collision as\nthe todo list itself: a reworded commit could make a previously\nunambiguous short commit ID ambiguous all of a sudden.\n\nSo let's not just copy the todo list, but let's instead write out the\nbackup with expanded commit IDs.\n\nSigned-off-by: Johannes Schindelin \n---\n rebase-interactive.c | 8 +++++---\n t/t3404-rebase-interactive.sh | 2 ++\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex aa18ae82b7..1259adc8ea 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -104,9 +104,11 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n \t\t\t\t -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))\n \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n \n-\tif (initial && copy_file(rebase_path_todo_backup(), todo_file, 0666))\n-\t\treturn error(_(\"could not copy '%s' to '%s'.\"), todo_file,\n-\t\t\t rebase_path_todo_backup());\n+\tif (initial &&\n+\t todo_list_write_to_file(r, todo_list, rebase_path_todo_backup(),\n+\t\t\t\t shortrevisions, shortonto, -1,\n+\t\t\t\t (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)\n+\t\treturn error(_(\"could not write '%s'.\"), rebase_path_todo_backup());\n \n \tif (launch_sequence_editor(todo_file, &new_todo->buf, NULL))\n \t\treturn -2;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 1cc9f36bc7..b90ea0fe44 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1277,6 +1277,8 @@ test_expect_success SHA1 'short SHA-1 collide' '\n \t\t\t.git/rebase-merge/git-rebase-todo.tmp &&\n \t\tgrep \"^pick [0-9a-f]\\{40\\}\" \\\n \t\t\t.git/rebase-merge/git-rebase-todo &&\n+\t\tgrep \"^pick [0-9a-f]\\{40\\}\" \\\n+\t\t\t.git/rebase-merge/git-rebase-todo.backup &&\n \t\tgit rebase --continue\n \t) &&\n \tcollide2=\"$(git rev-parse HEAD~1 | cut -c 1-4)\" &&\n-- \ngitgitgadget\n"},{"id":"390032","messageId":"102fa568dc09c1faa2d36903ccb7e1b285dd50b2.1579304283.git.gitgitgadget@gmail.com","threadId":"52654","inReplyTo":"pull.529.v2.git.1579304283.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-17T23:38:02Z","receivedAt":"2020-01-17T23:38:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 66ae9a57b88 (t3404: rebase -i: demonstrate short SHA-1 collision,\n2013-08-23), we added a test case that demonstrated how it is possible\nthat a previously unambiguous short commit ID could become ambiguous\n*during* a rebase.\n\nIn 75c69766554 (rebase -i: fix short SHA-1 collision, 2013-08-23), we\nfixed that problem simply by writing out the todo list with expanded\ncommit IDs (except *right* before letting the user edit the todo list,\nin which case we shorten them, but we expand them right after the file\nwas edited).\n\nHowever, the bug resurfaced as a side effect of 393adf7a6f6 (sequencer:\ndirectly call pick_commits() from complete_action(), 2019-11-24): as of\nthis commit, the sequencer no longer re-reads the todo list after\nwriting it out with expanded commit IDs.\n\nThe only redeeming factor is that the todo list is already parsed at\nthat stage, including all the commits corresponding to the commands,\ntherefore the sequencer can continue even if the internal todo list has\nshort commit IDs.\n\nThat does not prevent problems, though: the sequencer writes out the\n`done` and `git-rebase-todo` files incrementally (i.e. overwriting the\ntodo list with a version that has _short_ commit IDs), and if a merge\nconflict happens, or if an `edit` or a `break` command is encountered, a\nsubsequent `git rebase --continue` _will_ re-read the todo list, opening\nan opportunity for the \"short SHA-1 collision\" bug again.\n\nTo avoid that, let's make sure that we do expand the commit IDs in the\ntodo list as soon as we have parsed it after letting the user edit it.\n\nAdditionally, we improve the 'short SHA-1 collide' test case in t3404 to\ntest specifically for the case where the rebase is resumed. We also\nhard-code the expected colliding short SHA-1s, to document the\nexpectation (and to make it easier on future readers).\n\nNote that we specifically test that the short commit ID is used in the\n`git-rebase-todo.tmp` file: this file is created by the fake editor in\nthe test script and reflects the state that would have been presented to\nthe user to edit.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 11 ++++++++++-\n t/t3404-rebase-interactive.sh | 15 +++++++++++++--\n 2 files changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7c30dad59c..5f69b47e32 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5076,7 +5076,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n {\n \tconst char *shortonto, *todo_file = rebase_path_todo();\n \tstruct todo_list new_todo = TODO_LIST_INIT;\n-\tstruct strbuf *buf = &todo_list->buf;\n+\tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n \tstruct object_id oid = onto->object.oid;\n \tint res;\n \n@@ -5128,6 +5128,15 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn -1;\n \t}\n \n+\t/* Expand the commit IDs */\n+\ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n+\tstrbuf_swap(&new_todo.buf, &buf2);\n+\tstrbuf_release(&buf2);\n+\tnew_todo.total_nr -= new_todo.nr;\n+\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n+\t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n+\t\t new_todo.buf.buf);\n+\n \tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {\n \t\ttodo_list_release(&new_todo);\n \t\treturn error(_(\"could not skip unnecessary pick commands\"));\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ae6e55ce79..1cc9f36bc7 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' '\n test_expect_success SHA1 'short SHA-1 collide' '\n \ttest_when_finished \"reset_rebase && git checkout master\" &&\n \tgit checkout collide &&\n+\tcolliding_sha1=6bcda37 &&\n+\ttest $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n \t(\n \t\tunset test_tick &&\n \t\ttest_tick &&\n \t\tset_fake_editor &&\n \t\tFAKE_COMMIT_MESSAGE=\"collide2 ac4f2ee\" \\\n-\t\tFAKE_LINES=\"reword 1 2\" git rebase -i HEAD~2\n-\t)\n+\t\tFAKE_LINES=\"reword 1 break 2\" git rebase -i HEAD~2 &&\n+\t\ttest $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n+\t\tgrep \"^pick $colliding_sha1 \" \\\n+\t\t\t.git/rebase-merge/git-rebase-todo.tmp &&\n+\t\tgrep \"^pick [0-9a-f]\\{40\\}\" \\\n+\t\t\t.git/rebase-merge/git-rebase-todo &&\n+\t\tgit rebase --continue\n+\t) &&\n+\tcollide2=\"$(git rev-parse HEAD~1 | cut -c 1-4)\" &&\n+\tcollide3=\"$(git rev-parse collide3 | cut -c 1-4)\" &&\n+\ttest \"$collide2\" = \"$collide3\"\n '\n \n test_expect_success 'respect core.abbrev' '\n-- \ngitgitgadget\n\n"},{"id":"390108","messageId":"CAPig+cT8t39UvnF2i6CDoHW4kfEGr-CRFxZKOCstCTU0YzrCgQ@mail.gmail.com","threadId":"52654","inReplyTo":"102fa568dc09c1faa2d36903ccb7e1b285dd50b2.1579304283.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-20T16:49:15Z","receivedAt":"2020-01-20T16:49:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget\n wrote:\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> @@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' '\n> test_expect_success SHA1 'short SHA-1 collide' '\n> test_when_finished \"reset_rebase && git checkout master\" &&\n> git checkout collide &&\n> + colliding_sha1=6bcda37 &&\n> + test $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n\nHow much do we care that this is introducing new code with git\nupstream of a pipe (considering recent efforts to eradicate such\nusage)? Same question regarding several other new instances introduce\nby this patch.\n"},{"id":"390125","messageId":"nycvar.QRO.7.76.6.2001202103221.46@tvgsbejvaqbjf.bet","threadId":"52654","inReplyTo":"CAPig+cT8t39UvnF2i6CDoHW4kfEGr-CRFxZKOCstCTU0YzrCgQ@mail.gmail.com","subject":"Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-20T20:04:21Z","receivedAt":"2020-01-20T20:04:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Mon, 20 Jan 2020, Eric Sunshine wrote:\n\n> On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget\n> wrote:\n> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> > @@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' '\n> > test_expect_success SHA1 'short SHA-1 collide' '\n> > test_when_finished \"reset_rebase && git checkout master\" &&\n> > git checkout collide &&\n> > + colliding_sha1=6bcda37 &&\n> > + test $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n>\n> How much do we care that this is introducing new code with git\n> upstream of a pipe (considering recent efforts to eradicate such\n> usage)? Same question regarding several other new instances introduce\n> by this patch.\n\nI would argue that the test case will fail if the `git` call fails. So I\nam not overly concerned if that `git` call is upstream of a pipe.\n\nCiao,\nDscho\n"},{"id":"390126","messageId":"CAPig+cR2T26nJ1G5n82r_Xi8Y84k211n6bhBxG4Ku27vaW1pWA@mail.gmail.com","threadId":"52654","inReplyTo":"nycvar.QRO.7.76.6.2001202103221.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-20T20:08:15Z","receivedAt":"2020-01-20T20:08:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 20, 2020 at 3:04 PM Johannes Schindelin\n wrote:\n> On Mon, 20 Jan 2020, Eric Sunshine wrote:\n> > On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget\n> > wrote:\n> > > + test $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n> >\n> > How much do we care that this is introducing new code with git\n> > upstream of a pipe (considering recent efforts to eradicate such\n> > usage)? Same question regarding several other new instances introduce\n> > by this patch.\n>\n> I would argue that the test case will fail if the `git` call fails. So I\n> am not overly concerned if that `git` call is upstream of a pipe.\n\nUnless the git command crashes _after_ it produces the correct output...\n"},{"id":"390166","messageId":"nycvar.QRO.7.76.6.2001211248440.46@tvgsbejvaqbjf.bet","threadId":"52654","inReplyTo":"CAPig+cR2T26nJ1G5n82r_Xi8Y84k211n6bhBxG4Ku27vaW1pWA@mail.gmail.com","subject":"Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-21T11:49:07Z","receivedAt":"2020-01-21T11:49:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Mon, 20 Jan 2020, Eric Sunshine wrote:\n\n> On Mon, Jan 20, 2020 at 3:04 PM Johannes Schindelin\n> wrote:\n> > On Mon, 20 Jan 2020, Eric Sunshine wrote:\n> > > On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget\n> > > wrote:\n> > > > + test $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n> > >\n> > > How much do we care that this is introducing new code with git\n> > > upstream of a pipe (considering recent efforts to eradicate such\n> > > usage)? Same question regarding several other new instances introduce\n> > > by this patch.\n> >\n> > I would argue that the test case will fail if the `git` call fails. So I\n> > am not overly concerned if that `git` call is upstream of a pipe.\n>\n> Unless the git command crashes _after_ it produces the correct output...\n\nYes, that is true. In a very hypothetical way, of course.\n\n:-)\n\nCiao,\nDscho\n"},{"id":"390183","messageId":"xmqqeevsbsgm.fsf@gitster-ct.c.googlers.com","threadId":"52654","inReplyTo":"20200117223734.GA6570@camp.crustytoothpaste.net","subject":"Re: [PATCH 0/3] Re-fix rebase -i with SHA-1 collisions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-21T18:00:09Z","receivedAt":"2020-01-21T18:00:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n>> The question whether to move the SHA-256 support patch into your series is\n>> more a question to Junio, i.e. which patch series will be merged down\n>> faster.\n>\n> I need to do a reroll of part 8, so I'll pick it into a future series\n> (part 9) of test fixes and drop my patch. That way it won't interfere\n> with either series making progress, but it will still be included at the\n> end.\n\nAlright. Thanks for coordinating between you two.\n"},{"id":"390207","messageId":"xmqqpnfc8o39.fsf@gitster-ct.c.googlers.com","threadId":"52654","inReplyTo":"CAPig+cR2T26nJ1G5n82r_Xi8Y84k211n6bhBxG4Ku27vaW1pWA@mail.gmail.com","subject":"Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-21T22:02:50Z","receivedAt":"2020-01-21T22:02:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Jan 20, 2020 at 3:04 PM Johannes Schindelin\n> wrote:\n>> On Mon, 20 Jan 2020, Eric Sunshine wrote:\n>> > On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget\n>> > wrote:\n>> > > + test $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n>> >\n>> > How much do we care that this is introducing new code with git\n>> > upstream of a pipe (considering recent efforts to eradicate such\n>> > usage)? Same question regarding several other new instances introduce\n>> > by this patch.\n>>\n>> I would argue that the test case will fail if the `git` call fails. So I\n>> am not overly concerned if that `git` call is upstream of a pipe.\n>\n> Unless the git command crashes _after_ it produces the correct output...\n\nTrue. Doesn't rev-parse have an appropriate option for this kind of\nthing that gets rid of the need for \"cut\" in the first place?\n"},{"id":"390208","messageId":"xmqqlfq08nzz.fsf@gitster-ct.c.googlers.com","threadId":"52654","inReplyTo":"2ae2e435b0ef6888e72defc7abee1909b29aa914.1579304283.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] parse_insn_line(): improve error message when parsing failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-21T22:04:48Z","receivedAt":"2020-01-21T22:04:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In the case that a `get_oid()` call failed, we showed some rather bogus\n> part of the line instead of the precise string we sent to said function.\n> That makes it rather hard for users to understand what is going wrong,\n> so let's fix that.\n>\n> Signed-off-by: Johannes Schindelin \n> ...\n> @@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n> \titem->arg_len = (int)(eol - bol);\n> \n> \tif (status < 0)\n> -\t\treturn error(_(\"could not parse '%.*s'\"),\n> -\t\t\t (int)(end_of_object_name - bol), bol);\n> +\t\treturn status;\n> \n> \titem->commit = lookup_commit_reference(r, &commit_oid);\n> -\treturn !item->commit;\n> +\treturn item->commit ? 0 : -1;\n\nThis changes the polarity of the error exit from positive 1 to\nnegative 1. The only caller of this function takes anything\nnon-zero as a failure so this would not cause behaviour change, but\nreturning negative is more in line with the practice so it is an\nimprovement.\n\nIt is unrelated to the theme of this patch, and the proposed log\nmessage does not even mention it, though.\n"},{"id":"390233","messageId":"nycvar.QRO.7.76.6.2001221508180.46@tvgsbejvaqbjf.bet","threadId":"52654","inReplyTo":"xmqqpnfc8o39.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-22T14:10:15Z","receivedAt":"2020-01-22T14:10:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 21 Jan 2020, Junio C Hamano wrote:\n\n> Eric Sunshine writes:\n>\n> > On Mon, Jan 20, 2020 at 3:04 PM Johannes Schindelin\n> > wrote:\n> >> On Mon, 20 Jan 2020, Eric Sunshine wrote:\n> >> > On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget\n> >> > wrote:\n> >> > > + test $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n> >> >\n> >> > How much do we care that this is introducing new code with git\n> >> > upstream of a pipe (considering recent efforts to eradicate such\n> >> > usage)? Same question regarding several other new instances introduce\n> >> > by this patch.\n> >>\n> >> I would argue that the test case will fail if the `git` call fails. So I\n> >> am not overly concerned if that `git` call is upstream of a pipe.\n> >\n> > Unless the git command crashes _after_ it produces the correct output...\n>\n> True. Doesn't rev-parse have an appropriate option for this kind of\n> thing that gets rid of the need for \"cut\" in the first place?\n\nYou mean `git rev-parse --short=4`? That does something _sligthly_\ndifferent: it tries to shorten the OID to 4 characters _unless that would\nbe ambiguous_. In our case, it _will_ be ambiguous. That's why I use\n`cut`.\n\nAs to the crash in `rev-parse` _after_ printing out the OID: yes, there is\na possibility for that. But that regression test is not about `rev-parse`,\nso it is actually a good thing that it would not trigger on such a bug ;-)\n\nCiao,\nDscho\n"},{"id":"390237","messageId":"xmqqr1zr73yh.fsf@gitster-ct.c.googlers.com","threadId":"52654","inReplyTo":"nycvar.QRO.7.76.6.2001221508180.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-22T18:15:18Z","receivedAt":"2020-01-22T18:15:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> True. Doesn't rev-parse have an appropriate option for this kind of\n>> thing that gets rid of the need for \"cut\" in the first place?\n>\n> You mean `git rev-parse --short=4`? That does something _sligthly_\n> different: it tries to shorten the OID to 4 characters _unless that would\n> be ambiguous_. In our case, it _will_ be ambiguous. That's why I use\n> `cut`.\n\nAh, yes of course; we want ambiguous prefix. I think a more\nthorough test would be to see that the output with --short=$n (where\nn is the length of the abbreviated object name in $colliding_sha1)\nis longer than $colliding_sha1 and the output prefix-matches\n$colliding_sha1 iow, something like\n\n\tabbreviated=$(git rev-parse --short=7 HEAD) &&\n\tcase \"$abbreviated\" in\n\t\"$colliding_sha1\"?*) : happy ;;\n\t*) false ;;\n\tesac &&\n\t...\n\nwhich would make sure that we are testing colliding case.\n\n\n> As to the crash in `rev-parse` _after_ printing out the OID: yes, there is\n> a possibility for that. But that regression test is not about `rev-parse`,\n> so it is actually a good thing that it would not trigger on such a bug ;-)\n\nNo, I do not think this test should be about rev-parse working\ncorrectly---just that if it is easy enough to make the test robust\nenough against such a breakage, it would be nice to do so, that's\nall.\n\nI'm not Eric but I suspect his primary point was not about worrying\nabout rev-parse crashing but more about avoiding to add a pattern\nless experienced developers can copy&paste without thinking enough\nto realize why it would be OK here and not OK in the context of the\ntests they are adding. That would be what I would worry about more\nthan rev-parse crashing in the part of the test under discussion.\n\nThanks.\n\n"},{"id":"390265","messageId":"nycvar.QRO.7.76.6.2001231326310.46@tvgsbejvaqbjf.bet","threadId":"52654","inReplyTo":"xmqqlfq08nzz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/3] parse_insn_line(): improve error message when parsing failed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-23T12:26:59Z","receivedAt":"2020-01-23T12:27:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 21 Jan 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > In the case that a `get_oid()` call failed, we showed some rather bogus\n> > part of the line instead of the precise string we sent to said function.\n> > That makes it rather hard for users to understand what is going wrong,\n> > so let's fix that.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ...\n> > @@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n> > \titem->arg_len = (int)(eol - bol);\n> >\n> > \tif (status < 0)\n> > -\t\treturn error(_(\"could not parse '%.*s'\"),\n> > -\t\t\t (int)(end_of_object_name - bol), bol);\n> > +\t\treturn status;\n> >\n> > \titem->commit = lookup_commit_reference(r, &commit_oid);\n> > -\treturn !item->commit;\n> > +\treturn item->commit ? 0 : -1;\n>\n> This changes the polarity of the error exit from positive 1 to\n> negative 1. The only caller of this function takes anything\n> non-zero as a failure so this would not cause behaviour change, but\n> returning negative is more in line with the practice so it is an\n> improvement.\n>\n> It is unrelated to the theme of this patch, and the proposed log\n> message does not even mention it, though.\n\nYou're right. I amended the commit message. Will send out a v3 soon.\n\nThanks,\nDscho\n"},{"id":"390266","messageId":"pull.529.v3.git.1579782499.gitgitgadget@gmail.com","threadId":"52654","inReplyTo":"pull.529.v2.git.1579304283.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] Re-fix rebase -i with SHA-1 collisions","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-23T12:28:16Z","receivedAt":"2020-01-23T12:28:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Triggered by one of brian's SHA-256 patch series, I looked at the reason why\nthe SHA-1 collision test case passed when it shouldn't. Turns out that the\nregression test was not quite thorough enough, and the interactive rebase \ndid regress recently.\n\nWhile in the area, I realized that the same bug exists in the code backing\nthe rebase.missingCommitsCheck feature: the backed-up todo list uses\nshortened commit IDs that may very well become ambiguous during the rebase.\nFor good measure, this patch series fixes that, too.\n\nFinally, I saw that git rebase --edit-todo reported the line in an awkward,\nmaybe even incorrect, way when there was an ambiguous commit ID, and I also\nfixed that.\n\nTo make sure that the code can be easily adapted to SHA-256 after these\npatches, I actually already made those adjustments on top and offered them\nup at https://github.com/bk2204/git/pull/1.\n\nChanges since v2:\n\n * Clarified in the first commit message that the change from a positive\n return value to a negative return value (to indicate failure, in both\n cases) both was intentional, and does not require any change in the\n corresponding function's only caller.\n\nChanges since v1:\n\n * Turned the error condition when parsing the todo list with just-expanded\n commit IDs failed into a BUG().\n\nJohannes Schindelin (3):\n parse_insn_line(): improve error message when parsing failed\n rebase -i: re-fix short SHA-1 collision\n rebase -i: also avoid SHA-1 collisions with missingCommitsCheck\n\n rebase-interactive.c | 8 +++++---\n sequencer.c | 18 ++++++++++++++----\n t/t3404-rebase-interactive.sh | 17 +++++++++++++++--\n 3 files changed, 34 insertions(+), 9 deletions(-)\n\n\nbase-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-529%2Fdscho%2Fre-fix-rebase-i-with-sha-collisions-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-529/dscho/re-fix-rebase-i-with-sha-collisions-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/529\n\nRange-diff vs v2:\n\n 1: 2ae2e435b0 ! 1: 6f5c7b0325 parse_insn_line(): improve error message when parsing failed\n @@ -7,6 +7,11 @@\n That makes it rather hard for users to understand what is going wrong,\n so let's fix that.\n \n + While at it, return a negative value from `parse_insn_line()` in case of\n + an error, as per our convention. This function's only caller,\n + `todo_list_parse_insn_buffer()`, cares only whether that return value is\n + non-zero or not, i.e. does not need to be changed.\n +\n Signed-off-by: Johannes Schindelin \n \n diff --git a/sequencer.c b/sequencer.c\n 2: 102fa568dc = 2: 595ba81e09 rebase -i: re-fix short SHA-1 collision\n 3: 486e9413a6 = 3: b7b063408f rebase -i: also avoid SHA-1 collisions with missingCommitsCheck\n\n-- \ngitgitgadget\n"},{"id":"390267","messageId":"6f5c7b032524ed2510286caa0623fdbfa2b02eb3.1579782500.git.gitgitgadget@gmail.com","threadId":"52654","inReplyTo":"pull.529.v3.git.1579782499.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] parse_insn_line(): improve error message when parsing failed","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-23T12:28:17Z","receivedAt":"2020-01-23T12:28:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the case that a `get_oid()` call failed, we showed some rather bogus\npart of the line instead of the precise string we sent to said function.\nThat makes it rather hard for users to understand what is going wrong,\nso let's fix that.\n\nWhile at it, return a negative value from `parse_insn_line()` in case of\nan error, as per our convention. This function's only caller,\n`todo_list_parse_insn_buffer()`, cares only whether that return value is\nnon-zero or not, i.e. does not need to be changed.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b9dbf1adb0..7c30dad59c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2118,6 +2118,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \tsaved = *end_of_object_name;\n \t*end_of_object_name = '\\0';\n \tstatus = get_oid(bol, &commit_oid);\n+\tif (status < 0)\n+\t\terror(_(\"could not parse '%s'\"), bol); /* return later */\n \t*end_of_object_name = saved;\n \n \tbol = end_of_object_name + strspn(end_of_object_name, \" \\t\");\n@@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \titem->arg_len = (int)(eol - bol);\n \n \tif (status < 0)\n-\t\treturn error(_(\"could not parse '%.*s'\"),\n-\t\t\t (int)(end_of_object_name - bol), bol);\n+\t\treturn status;\n \n \titem->commit = lookup_commit_reference(r, &commit_oid);\n-\treturn !item->commit;\n+\treturn item->commit ? 0 : -1;\n }\n \n int sequencer_get_last_command(struct repository *r, enum replay_action *action)\n-- \ngitgitgadget\n\n"},{"id":"390268","messageId":"595ba81e0993b942370a2949867e5910e57fdd45.1579782500.git.gitgitgadget@gmail.com","threadId":"52654","inReplyTo":"pull.529.v3.git.1579782499.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] rebase -i: re-fix short SHA-1 collision","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-23T12:28:18Z","receivedAt":"2020-01-23T12:28:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 66ae9a57b88 (t3404: rebase -i: demonstrate short SHA-1 collision,\n2013-08-23), we added a test case that demonstrated how it is possible\nthat a previously unambiguous short commit ID could become ambiguous\n*during* a rebase.\n\nIn 75c69766554 (rebase -i: fix short SHA-1 collision, 2013-08-23), we\nfixed that problem simply by writing out the todo list with expanded\ncommit IDs (except *right* before letting the user edit the todo list,\nin which case we shorten them, but we expand them right after the file\nwas edited).\n\nHowever, the bug resurfaced as a side effect of 393adf7a6f6 (sequencer:\ndirectly call pick_commits() from complete_action(), 2019-11-24): as of\nthis commit, the sequencer no longer re-reads the todo list after\nwriting it out with expanded commit IDs.\n\nThe only redeeming factor is that the todo list is already parsed at\nthat stage, including all the commits corresponding to the commands,\ntherefore the sequencer can continue even if the internal todo list has\nshort commit IDs.\n\nThat does not prevent problems, though: the sequencer writes out the\n`done` and `git-rebase-todo` files incrementally (i.e. overwriting the\ntodo list with a version that has _short_ commit IDs), and if a merge\nconflict happens, or if an `edit` or a `break` command is encountered, a\nsubsequent `git rebase --continue` _will_ re-read the todo list, opening\nan opportunity for the \"short SHA-1 collision\" bug again.\n\nTo avoid that, let's make sure that we do expand the commit IDs in the\ntodo list as soon as we have parsed it after letting the user edit it.\n\nAdditionally, we improve the 'short SHA-1 collide' test case in t3404 to\ntest specifically for the case where the rebase is resumed. We also\nhard-code the expected colliding short SHA-1s, to document the\nexpectation (and to make it easier on future readers).\n\nNote that we specifically test that the short commit ID is used in the\n`git-rebase-todo.tmp` file: this file is created by the fake editor in\nthe test script and reflects the state that would have been presented to\nthe user to edit.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 11 ++++++++++-\n t/t3404-rebase-interactive.sh | 15 +++++++++++++--\n 2 files changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7c30dad59c..5f69b47e32 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5076,7 +5076,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n {\n \tconst char *shortonto, *todo_file = rebase_path_todo();\n \tstruct todo_list new_todo = TODO_LIST_INIT;\n-\tstruct strbuf *buf = &todo_list->buf;\n+\tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n \tstruct object_id oid = onto->object.oid;\n \tint res;\n \n@@ -5128,6 +5128,15 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn -1;\n \t}\n \n+\t/* Expand the commit IDs */\n+\ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n+\tstrbuf_swap(&new_todo.buf, &buf2);\n+\tstrbuf_release(&buf2);\n+\tnew_todo.total_nr -= new_todo.nr;\n+\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n+\t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n+\t\t new_todo.buf.buf);\n+\n \tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {\n \t\ttodo_list_release(&new_todo);\n \t\treturn error(_(\"could not skip unnecessary pick commands\"));\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ae6e55ce79..1cc9f36bc7 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' '\n test_expect_success SHA1 'short SHA-1 collide' '\n \ttest_when_finished \"reset_rebase && git checkout master\" &&\n \tgit checkout collide &&\n+\tcolliding_sha1=6bcda37 &&\n+\ttest $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n \t(\n \t\tunset test_tick &&\n \t\ttest_tick &&\n \t\tset_fake_editor &&\n \t\tFAKE_COMMIT_MESSAGE=\"collide2 ac4f2ee\" \\\n-\t\tFAKE_LINES=\"reword 1 2\" git rebase -i HEAD~2\n-\t)\n+\t\tFAKE_LINES=\"reword 1 break 2\" git rebase -i HEAD~2 &&\n+\t\ttest $colliding_sha1 = \"$(git rev-parse HEAD | cut -c 1-7)\" &&\n+\t\tgrep \"^pick $colliding_sha1 \" \\\n+\t\t\t.git/rebase-merge/git-rebase-todo.tmp &&\n+\t\tgrep \"^pick [0-9a-f]\\{40\\}\" \\\n+\t\t\t.git/rebase-merge/git-rebase-todo &&\n+\t\tgit rebase --continue\n+\t) &&\n+\tcollide2=\"$(git rev-parse HEAD~1 | cut -c 1-4)\" &&\n+\tcollide3=\"$(git rev-parse collide3 | cut -c 1-4)\" &&\n+\ttest \"$collide2\" = \"$collide3\"\n '\n \n test_expect_success 'respect core.abbrev' '\n-- \ngitgitgadget\n\n"},{"id":"390269","messageId":"b7b063408f61abbbdf88b2dbf22e6274e75a31f8.1579782500.git.gitgitgadget@gmail.com","threadId":"52654","inReplyTo":"pull.529.v3.git.1579782499.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] rebase -i: also avoid SHA-1 collisions with missingCommitsCheck","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-23T12:28:19Z","receivedAt":"2020-01-23T12:28:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen `rebase.missingCommitsCheck` is in effect, we use the backup of the\ntodo list that was copied just before the user was allowed to edit it.\n\nThat backup is, of course, just as susceptible to the hash collision as\nthe todo list itself: a reworded commit could make a previously\nunambiguous short commit ID ambiguous all of a sudden.\n\nSo let's not just copy the todo list, but let's instead write out the\nbackup with expanded commit IDs.\n\nSigned-off-by: Johannes Schindelin \n---\n rebase-interactive.c | 8 +++++---\n t/t3404-rebase-interactive.sh | 2 ++\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex aa18ae82b7..1259adc8ea 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -104,9 +104,11 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n \t\t\t\t -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))\n \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n \n-\tif (initial && copy_file(rebase_path_todo_backup(), todo_file, 0666))\n-\t\treturn error(_(\"could not copy '%s' to '%s'.\"), todo_file,\n-\t\t\t rebase_path_todo_backup());\n+\tif (initial &&\n+\t todo_list_write_to_file(r, todo_list, rebase_path_todo_backup(),\n+\t\t\t\t shortrevisions, shortonto, -1,\n+\t\t\t\t (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)\n+\t\treturn error(_(\"could not write '%s'.\"), rebase_path_todo_backup());\n \n \tif (launch_sequence_editor(todo_file, &new_todo->buf, NULL))\n \t\treturn -2;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 1cc9f36bc7..b90ea0fe44 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1277,6 +1277,8 @@ test_expect_success SHA1 'short SHA-1 collide' '\n \t\t\t.git/rebase-merge/git-rebase-todo.tmp &&\n \t\tgrep \"^pick [0-9a-f]\\{40\\}\" \\\n \t\t\t.git/rebase-merge/git-rebase-todo &&\n+\t\tgrep \"^pick [0-9a-f]\\{40\\}\" \\\n+\t\t\t.git/rebase-merge/git-rebase-todo.backup &&\n \t\tgit rebase --continue\n \t) &&\n \tcollide2=\"$(git rev-parse HEAD~1 | cut -c 1-4)\" &&\n-- \ngitgitgadget\n"}]}