Re-fix rebase -i with SHA-1 collisions
cover Re-fix rebase -i with SHA-1 collisions
Triggered by one of brian's SHA-256 patch series, I looked at the reason why the SHA-1 collision test case passed when it shouldn't. Turns out that the regression test was not quite thorough enough, and the interactive rebase did regress recently.
While in the area, I realized that the same bug exists in the code backing the rebase.missingCommitsCheck feature: the backed-up todo list uses shortened commit IDs that may very well become ambiguous during the rebase. For good measure, this patch series fixes that, too.
Finally, I saw that git rebase --edit-todo reported the line in an awkward, maybe even incorrect, way when there was an ambiguous commit ID, and I also fixed that.
To make sure that the code can be easily adapted to SHA-256 after these patches, I actually already made those adjustments on top and offered them up at https://github.com/bk2204/git/pull/1.
Johannes Schindelin (3):
parse_insn_line(): improve error message when parsing failed rebase -i: re-fix short SHA-1 collision
rebase -i: also avoid SHA-1 collisions with missingCommitsCheck
rebase-interactive.c | 8 +++++--- sequencer.c | 21 +++++++++++++++++---- t/t3404-rebase-interactive.sh | 17 +++++++++++++++-- 3 files changed, 37 insertions(+), 9 deletions(-)
base-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-529%2Fdscho%2Fre-fix-rebase-i-with-sha-collisions-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-529/dscho/re-fix-rebase-i-with-sha-collisions-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/529
Signature · 1 line
gitgitgadget
PATCH 1/3 parse_insn_line(): improve error message when parsing failed
From: Johannes Schindelin <[email protected]>
In the case that a get_oid() call failed, we showed some rather bogus part of the line instead of the precise string we sent to said function. That makes it rather hard for users to understand what is going wrong, so let's fix that.
--- sequencer.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
sequencer.c +4 -3
@@ -2118,6 +2118,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, saved = *end_of_object_name; *end_of_object_name ='\0' ; status =get_oid (bol, &commit_oid); +if (status <0 ) +error (_ ("could not parse '%s'" ), bol);/* return later */ *end_of_object_name = saved; bol = end_of_object_name +strspn (end_of_object_name," \t" );
@@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, item->arg_len = (int )(eol - bol);if (status <0 ) -return error (_ ("could not parse '%.*s'" ), - (int )(end_of_object_name - bol), bol); +return status; item->commit =lookup_commit_reference (r, &commit_oid); -return !item->commit; +return item->commit ?0 :-1 ; }int sequencer_get_last_command (struct repository *r,enum replay_action *action)
Signature · 1 line
gitgitgadget
Re: [PATCH 1/3] parse_insn_line(): improve error message when parsing failed
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 17 lines · writes:
From: Johannes Schindelin <[email protected]>
In the case that a
get_oid()call failed, we showed some rather bogus part of the line instead of the precise string we sent to said function. That makes it rather hard for users to understand what is going wrong, so let's fix that.Signed-off-by Johannes Schindelin <[email protected]>--- sequencer.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)sequencer.c +0
-0
sequencer.c static int parse_insn_line(struct repository *r, struct todo_item *item, · line 2123
Quoted 6 lines
sequencer.c +2
-0@@ -2118,6 +2118,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, saved = *end_of_object_name; *end_of_object_name ='\0' ; status =get_oid (bol, &commit_oid); +if (status <0 ) +error (_ ("could not parse '%s'" ), bol);/* return later */ *end_of_object_name = saved;
OK, so after making sure the line begins a command that takes an object name, we first NUL terminated the token after the command but it turns out the string is not a valid oid. We show the part we thought was the object name before reverting the NUL termination.
Makes sense. And then later...
sequencer.c static int parse_insn_line(struct repository *r, struct todo_item *item, · line 2130
Quoted 8 lines
sequencer.c +1
-2bol = end_of_object_name +strspn (end_of_object_name," \t" );@@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, item->arg_len = (int )(eol - bol);if (status <0 ) -return error (_ ("could not parse '%.*s'" ), - (int )(end_of_object_name - bol), bol); +return status;
...this is the *only* code that uses the status that was assigned to the return value of get_oid().
Because the implementation of this function (mis)uses "bol", which stands for "beginning of line", as a moving pointer of "beginning of the token we are looking at", the value of "bol" at this point of the control in the original code is not where the "bol" pointer used to be when end-of-object-name was computed (if it were, the '%.*s' argument would have been correct) and shows a token after where the object name would have been, which may help the user identify the line but does not directly show what token we had trouble with parsing.
And the updated code will avoid the issue by using the "bol" pointer when it is still pointing at the right place.
sequencer.c static int parse_insn_line(struct repository *r, struct todo_item *item, · line 2133
Quoted 3 lines
sequencer.c +1
-1item->commit =lookup_commit_reference (r, &commit_oid); -return !item->commit; +return item->commit ?0 :-1 ;
This changes the polarity of the error exit from positive 1 to negative 1. The only caller of this function takes anything non-zero as a failure so this would not cause behaviour change, but returning negative is more in line with the practice so it is an improvement (it is unrelated to the theme of this patch and is not explained, though).
OK.
PATCH 2/3 rebase -i: re-fix short SHA-1 collision
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
In 66ae9a57b88 (t3404: rebase -i: demonstrate short SHA-1 collision, 2013-08-23), we added a test case that demonstrated how it is possible that a previously unambiguous short commit ID could become ambiguous *during* a rebase.
In 75c69766554 (rebase -i: fix short SHA-1 collision, 2013-08-23), we fixed that problem simply by writing out the todo list with expanded commit IDs (except *right* before letting the user edit the todo list, in which case we shorten them, but we expand them right after the file was edited).
However, the bug resurfaced as a side effect of 393adf7a6f6 (sequencer:
directly call pick_commits() from complete_action(), 2019-11-24): as of this commit, the sequencer no longer re-reads the todo list after writing it out with expanded commit IDs.
The only redeeming factor is that the todo list is already parsed at that stage, including all the commits corresponding to the commands, therefore the sequencer can continue even if the internal todo list has short commit IDs.
That does not prevent problems, though: the sequencer writes out the done and git-rebase-todo files incrementally (i.e. overwriting the todo list with a version that has _short_ commit IDs), and if a merge conflict happens, or if an edit or a break command is encountered, a subsequent git rebase --continue _will_ re-read the todo list, opening an opportunity for the "short SHA-1 collision" bug again.
To avoid that, let's make sure that we do expand the commit IDs in the todo list as soon as we have parsed it after letting the user edit it.
Additionally, we improve the 'short SHA-1 collide' test case in t3404 to test specifically for the case where the rebase is resumed. We also hard-code the expected colliding short SHA-1s, to document the expectation (and to make it easier on future readers).
Note that we specifically test that the short commit ID is used in the git-rebase-todo.tmp file: this file is created by the fake editor in the test script and reflects the state that would have been presented to the user to edit.
--- sequencer.c | 14 +++++++++++++- t/t3404-rebase-interactive.sh | 15 +++++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-)
sequencer.c +13 -1
@@ -5076,7 +5076,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla {const char *shortonto, *todo_file =rebase_path_todo ();struct todo_list new_todo =TODO_LIST_INIT ; -struct strbuf *buf = &todo_list->buf; +struct strbuf *buf = &todo_list->buf, buf2 =STRBUF_INIT ;struct object_id oid = onto->object.oid;int res;
@@ -5128,6 +5128,18 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned flareturn -1 ; } +/* Expand the commit IDs */ +todo_list_to_strbuf (r, &new_todo, &buf2,-1 ,0 ); +strbuf_swap (&new_todo.buf, &buf2); +strbuf_release (&buf2); + new_todo.total_nr -= new_todo.nr; +if (todo_list_parse_insn_buffer (r, new_todo.buf.buf, &new_todo) <0 ) { +fprintf (stderr,_ (edit_todo_list_advice)); +checkout_onto (r, opts, onto_name, &onto->object.oid, orig_head); +todo_list_release (&new_todo); +return error (_ ("invalid todo list after expanding IDs" )); + } +if (opts->allow_ff &&skip_unnecessary_picks (r, &new_todo, &oid)) {todo_list_release (&new_todo);return error (_ ("could not skip unnecessary pick commands" ));
t/t3404-rebase-interactive.sh +13 -2
@@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' ' test_expect_success SHA1 'short SHA-1 collide' ' test_when_finished "reset_rebase && git checkout master" && git checkout collide && + colliding_sha1=6bcda37 && + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" && ( unset test_tick && test_tick && set_fake_editor && FAKE_COMMIT_MESSAGE="collide2 ac4f2ee" \ - FAKE_LINES="reword 1 2" git rebase -i HEAD~2 - ) + FAKE_LINES="reword 1 break 2" git rebase -i HEAD~2 && + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" && + grep "^pick $colliding_sha1 " \ + .git/rebase-merge/git-rebase-todo.tmp && + grep "^pick [0-9a-f]\{40\}" \ + .git/rebase-merge/git-rebase-todo && + git rebase --continue + ) && + collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" && + collide3="$(git rev-parse collide3 | cut -c 1-4)" && + test "$collide2" = "$collide3" ' test_expect_success 'respect core.abbrev' '
Signature · 1 line
gitgitgadget
Re: [PATCH 2/3] rebase -i: re-fix short SHA-1 collision
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 4 lines · writes:
However, the bug resurfaced as a side effect of
393adf7a6f6(sequencer:
directly callpick_commits()fromcomplete_action(), 2019-11-24): as of this commit, the sequencer no longer re-reads the todo list after writing it out with expanded commit IDs.
Ouch. The analysis above is quite understandable.
Quoted 5 lines
sequencer.c +1
-0@@ -5128,6 +5128,18 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned flareturn -1 ; } +/* Expand the commit IDs */
sequencer.c int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla · line 5141
Quoted 10 lines
sequencer.c +10
-0+todo_list_to_strbuf (r, &new_todo, &buf2,-1 ,0 ); +strbuf_swap (&new_todo.buf, &buf2); +strbuf_release (&buf2); + new_todo.total_nr -= new_todo.nr; +if (todo_list_parse_insn_buffer (r, new_todo.buf.buf, &new_todo) <0 ) { +fprintf (stderr,_ (edit_todo_list_advice)); +checkout_onto (r, opts, onto_name, &onto->object.oid, orig_head); +todo_list_release (&new_todo); +return error (_ ("invalid todo list after expanding IDs" )); + }
The above happens after edit_todo_list() returns and then the resulting data (i.e. new_todo) that came from the on-disk file has been parsed with an existing call to todo_lsit_parse_insn_buffer()?
I am wondering when this if() statement would trigger, iow, under what condition the result of "Expand the commit IDs" operation fails to be parsed correctly, and what the user can do to remedy it. Especially given that incoming new_todo has passed the existing parse and check without hitting "return -1" we see above in the context, I am not sure if there is anything, other than any potential glitch in the added code above, that can cause this second parse_insn_buffer() to fail. Shouldn't the body of if() be simply a BUG()?
Or am I somehow missing a hunk that removes an existing call to parse&check?
Quoted 31 lines
t/t3404-rebase-interactive.sh +13
-2@@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' ' test_expect_success SHA1 'short SHA-1 collide' ' test_when_finished "reset_rebase && git checkout master" && git checkout collide && + colliding_sha1=6bcda37 && + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" && ( unset test_tick && test_tick && set_fake_editor && FAKE_COMMIT_MESSAGE="collide2 ac4f2ee" \ - FAKE_LINES="reword 1 2" git rebase -i HEAD~2 - ) + FAKE_LINES="reword 1 break 2" git rebase -i HEAD~2 && + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" && + grep "^pick $colliding_sha1 " \ + .git/rebase-merge/git-rebase-todo.tmp && + grep "^pick [0-9a-f]\{40\}" \ + .git/rebase-merge/git-rebase-todo && + git rebase --continue + ) && + collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" && + collide3="$(git rev-parse collide3 | cut -c 1-4)" && + test "$collide2" = "$collide3" ' test_expect_success 'respect core.abbrev' '
Re: [PATCH 2/3] rebase -i: re-fix short SHA-1 collision
Hi Junio,
Quoted 19 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
sequencer.c int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla · line 5141
Quoted 11 lines · writes:
sequencer.c +11
-0+/* Expand the commit IDs */ +todo_list_to_strbuf (r, &new_todo, &buf2,-1 ,0 ); +strbuf_swap (&new_todo.buf, &buf2); +strbuf_release (&buf2); + new_todo.total_nr -= new_todo.nr; +if (todo_list_parse_insn_buffer (r, new_todo.buf.buf, &new_todo) <0 ) { +fprintf (stderr,_ (edit_todo_list_advice)); +checkout_onto (r, opts, onto_name, &onto->object.oid, orig_head); +todo_list_release (&new_todo); +return error (_ ("invalid todo list after expanding IDs" )); + }The above happens after
edit_todo_list()returns and then the resulting data (i.e.new_todo) that came from the on-disk file has been parsed with an existing call totodo_lsit_parse_insn_buffer()?
Quoted 3 lines
I am wondering when this
if()statement would trigger, iow, under what condition the result of "Expand the commit IDs" operation fails to be parsed correctly, and what the user can do to remedy it.
You're right, this is defensive code to guard against bugs, but the error message suggests that it might be a user error: it isn't.
Quoted 6 lines
Especially given that incoming
new_todohas passed the existing parse and check without hitting "return -1" we see above in the context, I am not sure if there is anything, other than any potential glitch in the added code above, that can cause this secondparse_insn_buffer()to fail. Shouldn't the body ofif()be simply aBUG()?
It should.
Will fix,
Dscho
Quoted 36 lines
Or am I somehow missing a hunk that removes an existing call to parse&check?
Quoted 31 lines
t/t3404-rebase-interactive.sh +13
-2@@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' ' test_expect_success SHA1 'short SHA-1 collide' ' test_when_finished "reset_rebase && git checkout master" && git checkout collide && + colliding_sha1=6bcda37 && + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" && ( unset test_tick && test_tick && set_fake_editor && FAKE_COMMIT_MESSAGE="collide2 ac4f2ee" \ - FAKE_LINES="reword 1 2" git rebase -i HEAD~2 - ) + FAKE_LINES="reword 1 break 2" git rebase -i HEAD~2 && + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" && + grep "^pick $colliding_sha1 " \ + .git/rebase-merge/git-rebase-todo.tmp && + grep "^pick [0-9a-f]\{40\}" \ + .git/rebase-merge/git-rebase-todo && + git rebase --continue + ) && + collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" && + collide3="$(git rev-parse collide3 | cut -c 1-4)" && + test "$collide2" = "$collide3" ' test_expect_success 'respect core.abbrev' '
PATCH 3/3 rebase -i: also avoid SHA-1 collisions with missingCommitsCheck
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
When rebase.missingCommitsCheck is in effect, we use the backup of the todo list that was copied just before the user was allowed to edit it.
That backup is, of course, just as susceptible to the hash collision as the todo list itself: a reworded commit could make a previously unambiguous short commit ID ambiguous all of a sudden.
So let's not just copy the todo list, but let's instead write out the backup with expanded commit IDs.
--- rebase-interactive.c | 8 +++++--- t/t3404-rebase-interactive.sh | 2 ++ 2 files changed, 7 insertions(+), 3 deletions(-)
rebase-interactive.c +5 -3
@@ -104,9 +104,11 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list, -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))return error_errno (_ ("could not write '%s'" ), todo_file); -if (initial &©_file (rebase_path_todo_backup (), todo_file,0666 )) -return error (_ ("could not copy '%s' to '%s'." ), todo_file, -rebase_path_todo_backup ()); +if (initial && +todo_list_write_to_file (r, todo_list,rebase_path_todo_backup (), + shortrevisions, shortonto,-1 , + (flags |TODO_LIST_APPEND_TODO_HELP ) & ~TODO_LIST_SHORTEN_IDS ) <0 ) +return error (_ ("could not write '%s'." ),rebase_path_todo_backup ());if (launch_sequence_editor (todo_file, &new_todo->buf,NULL ))return -2 ;
t/t3404-rebase-interactive.sh +2 -0
@@ -1277,6 +1277,8 @@ test_expect_success SHA1 'short SHA-1 collide' ' .git/rebase-merge/git-rebase-todo.tmp && grep "^pick [0-9a-f]\{40\}" \ .git/rebase-merge/git-rebase-todo && + grep "^pick [0-9a-f]\{40\}" \ + .git/rebase-merge/git-rebase-todo.backup && git rebase --continue ) && collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" &&
Signature · 1 line
gitgitgadget
Quoted 10 lines · Johannes Schindelin via GitGitGadget
Triggered by one of brian's SHA-256 patch series, I looked at the reason why the SHA-1 collision test case passed when it shouldn't. Turns out that the regression test was not quite thorough enough, and the interactive rebase did regress recently.
While in the area, I realized that the same bug exists in the code backing the rebase.missingCommitsCheck feature: the backed-up todo list uses shortened commit IDs that may very well become ambiguous during the rebase. For good measure, this patch series fixes that, too.
Quoted 7 lines
Finally, I saw that git rebase --edit-todo reported the line in an awkward, maybe even incorrect, way when there was an ambiguous commit ID, and I also fixed that.
To make sure that the code can be easily adapted to SHA-256 after these patches, I actually already made those adjustments on top and offered them up at https://github.com/bk2204/git/pull/1.
This series looks great to me, and thanks for fixing this.
As mentioned in the PR, I'm happy for you to drop the SHA-256 patch into this series if you like, or I can carry it in a future series. Either way is fine with me.
Signature · 2 lines
brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Hi brian,
Quoted 19 lines · brian m. carlson
Quoted 10 lines · Johannes Schindelin via GitGitGadget
Triggered by one of brian's SHA-256 patch series, I looked at the reason why the SHA-1 collision test case passed when it shouldn't. Turns out that the regression test was not quite thorough enough, and the interactive rebase did regress recently.
While in the area, I realized that the same bug exists in the code backing the rebase.missingCommitsCheck feature: the backed-up todo list uses shortened commit IDs that may very well become ambiguous during the rebase. For good measure, this patch series fixes that, too.
Quoted 7 lines
Finally, I saw that git rebase --edit-todo reported the line in an awkward, maybe even incorrect, way when there was an ambiguous commit ID, and I also fixed that.
To make sure that the code can be easily adapted to SHA-256 after these patches, I actually already made those adjustments on top and offered them up at https://github.com/bk2204/git/pull/1.
Quoted 5 lines
This series looks great to me, and thanks for fixing this.
As mentioned in the PR, I'm happy for you to drop the SHA-256 patch into this series if you like, or I can carry it in a future series. Either way is fine with me.
Excellent. Given that the re-fix to avoid short commit ID collisions has little to do with supporting SHA-256, I would like to keep the patch series separate, then.
The question whether to move the SHA-256 support patch into your series is more a question to Junio, i.e. which patch series will be merged down faster.
Junio, what is your preference here?
Ciao,
Dscho
Quoted 6 lines · Johannes Schindelin
Quoted 5 lines
This series looks great to me, and thanks for fixing this.
As mentioned in the PR, I'm happy for you to drop the SHA-256 patch into this series if you like, or I can carry it in a future series. Either way is fine with me.
Quoted 7 lines
Excellent. Given that the re-fix to avoid short commit ID collisions has little to do with supporting SHA-256, I would like to keep the patch series separate, then.
The question whether to move the SHA-256 support patch into your series is more a question to Junio, i.e. which patch series will be merged down faster.
I need to do a reroll of part 8, so I'll pick it into a future series (part 9) of test fixes and drop my patch. That way it won't interfere with either series making progress, but it will still be included at the end.
Signature · 2 lines
brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Quoted 8 lines · brian m. carlson
Quoted 3 lines
The question whether to move the SHA-256 support patch into your series is more a question to Junio, i.e. which patch series will be merged down faster.
I need to do a reroll of part 8, so I'll pick it into a future series (part 9) of test fixes and drop my patch. That way it won't interfere with either series making progress, but it will still be included at the end.
Alright. Thanks for coordinating between you two.
v2 cover Re-fix rebase -i with SHA-1 collisions
In reply to Johannes Schindelin via GitGitGadgetTriggered by one of brian's SHA-256 patch series, I looked at the reason why the SHA-1 collision test case passed when it shouldn't. Turns out that the regression test was not quite thorough enough, and the interactive rebase did regress recently.
While in the area, I realized that the same bug exists in the code backing the rebase.missingCommitsCheck feature: the backed-up todo list uses shortened commit IDs that may very well become ambiguous during the rebase. For good measure, this patch series fixes that, too.
Finally, I saw that git rebase --edit-todo reported the line in an awkward, maybe even incorrect, way when there was an ambiguous commit ID, and I also fixed that.
To make sure that the code can be easily adapted to SHA-256 after these patches, I actually already made those adjustments on top and offered them up at https://github.com/bk2204/git/pull/1.
Changes since v1:
* Turned the error condition when parsing the todo list with just-expanded commit IDs failed into a BUG().
Johannes Schindelin (3):
parse_insn_line(): improve error message when parsing failed rebase -i: re-fix short SHA-1 collision
rebase -i: also avoid SHA-1 collisions with missingCommitsCheck
rebase-interactive.c | 8 +++++--- sequencer.c | 18 ++++++++++++++---- t/t3404-rebase-interactive.sh | 17 +++++++++++++++-- 3 files changed, 34 insertions(+), 9 deletions(-)
base-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-529%2Fdscho%2Fre-fix-rebase-i-with-sha-collisions-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-529/dscho/re-fix-rebase-i-with-sha-collisions-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/529
Range-diff vs v1:
1: 2ae2e435b0 = 1: 2ae2e435b0 parse_insn_line(): improve error message when parsing failed
2: ad50cd1b92 ! 2: 102fa568dc rebase -i: re-fix short SHA-1 collision
@@ -66,12 +66,9 @@
+ strbuf_swap(&new_todo.buf, &buf2);
+ strbuf_release(&buf2);
+ new_todo.total_nr -= new_todo.nr;
-+ if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0) {
-+ fprintf(stderr, _(edit_todo_list_advice));
-+ checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
-+ todo_list_release(&new_todo);
-+ return error(_("invalid todo list after expanding IDs"));
-+ }
++ if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
++ BUG("invalid todo list after expanding IDs:\n%s",
++ new_todo.buf.buf);
+
if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {
todo_list_release(&new_todo); 3: e7d9ea8992 = 3: 486e9413a6 rebase -i: also avoid SHA-1 collisions with missingCommitsCheck
Signature · 1 line
gitgitgadget
PATCH v2 1/3 parse_insn_line(): improve error message when parsing failed
From: Johannes Schindelin <[email protected]>
In the case that a get_oid() call failed, we showed some rather bogus part of the line instead of the precise string we sent to said function. That makes it rather hard for users to understand what is going wrong, so let's fix that.
--- sequencer.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
sequencer.c +4 -3
@@ -2118,6 +2118,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, saved = *end_of_object_name; *end_of_object_name ='\0' ; status =get_oid (bol, &commit_oid); +if (status <0 ) +error (_ ("could not parse '%s'" ), bol);/* return later */ *end_of_object_name = saved; bol = end_of_object_name +strspn (end_of_object_name," \t" );
@@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, item->arg_len = (int )(eol - bol);if (status <0 ) -return error (_ ("could not parse '%.*s'" ), - (int )(end_of_object_name - bol), bol); +return status; item->commit =lookup_commit_reference (r, &commit_oid); -return !item->commit; +return item->commit ?0 :-1 ; }int sequencer_get_last_command (struct repository *r,enum replay_action *action)
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/3] parse_insn_line(): improve error message when parsing failed
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 10 lines · writes:
From: Johannes Schindelin <[email protected]>
In the case that a
get_oid()call failed, we showed some rather bogus part of the line instead of the precise string we sent to said function. That makes it rather hard for users to understand what is going wrong, so let's fix that.Signed-off-by Johannes Schindelin <[email protected]>...
sequencer.c static int parse_insn_line(struct repository *r, struct todo_item *item, · line 2133
Quoted 10 lines
sequencer.c +2
-3@@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, item->arg_len = (int )(eol - bol);if (status <0 ) -return error (_ ("could not parse '%.*s'" ), - (int )(end_of_object_name - bol), bol); +return status; item->commit =lookup_commit_reference (r, &commit_oid); -return !item->commit; +return item->commit ?0 :-1 ;
This changes the polarity of the error exit from positive 1 to negative 1. The only caller of this function takes anything non-zero as a failure so this would not cause behaviour change, but returning negative is more in line with the practice so it is an improvement.
It is unrelated to the theme of this patch, and the proposed log message does not even mention it, though.
Re: [PATCH v2 1/3] parse_insn_line(): improve error message when parsing failed
Hi Junio,
Quoted 30 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 10 lines · writes:
From: Johannes Schindelin <[email protected]>
In the case that a
get_oid()call failed, we showed some rather bogus part of the line instead of the precise string we sent to said function. That makes it rather hard for users to understand what is going wrong, so let's fix that.Signed-off-by Johannes Schindelin <[email protected]>...
sequencer.c static int parse_insn_line(struct repository *r, struct todo_item *item, · line 2133
Quoted 10 lines
sequencer.c +2
-3@@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, item->arg_len = (int )(eol - bol);if (status <0 ) -return error (_ ("could not parse '%.*s'" ), - (int )(end_of_object_name - bol), bol); +return status; item->commit =lookup_commit_reference (r, &commit_oid); -return !item->commit; +return item->commit ?0 :-1 ;This changes the polarity of the error exit from positive 1 to negative 1. The only caller of this function takes anything non-zero as a failure so this would not cause behaviour change, but returning negative is more in line with the practice so it is an improvement.
Quoted 2 lines
It is unrelated to the theme of this patch, and the proposed log message does not even mention it, though.
You're right. I amended the commit message. Will send out a v3 soon.
Thanks,
Dscho
PATCH v2 2/3 rebase -i: re-fix short SHA-1 collision
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
In 66ae9a57b88 (t3404: rebase -i: demonstrate short SHA-1 collision, 2013-08-23), we added a test case that demonstrated how it is possible that a previously unambiguous short commit ID could become ambiguous *during* a rebase.
In 75c69766554 (rebase -i: fix short SHA-1 collision, 2013-08-23), we fixed that problem simply by writing out the todo list with expanded commit IDs (except *right* before letting the user edit the todo list, in which case we shorten them, but we expand them right after the file was edited).
However, the bug resurfaced as a side effect of 393adf7a6f6 (sequencer:
directly call pick_commits() from complete_action(), 2019-11-24): as of this commit, the sequencer no longer re-reads the todo list after writing it out with expanded commit IDs.
The only redeeming factor is that the todo list is already parsed at that stage, including all the commits corresponding to the commands, therefore the sequencer can continue even if the internal todo list has short commit IDs.
That does not prevent problems, though: the sequencer writes out the done and git-rebase-todo files incrementally (i.e. overwriting the todo list with a version that has _short_ commit IDs), and if a merge conflict happens, or if an edit or a break command is encountered, a subsequent git rebase --continue _will_ re-read the todo list, opening an opportunity for the "short SHA-1 collision" bug again.
To avoid that, let's make sure that we do expand the commit IDs in the todo list as soon as we have parsed it after letting the user edit it.
Additionally, we improve the 'short SHA-1 collide' test case in t3404 to test specifically for the case where the rebase is resumed. We also hard-code the expected colliding short SHA-1s, to document the expectation (and to make it easier on future readers).
Note that we specifically test that the short commit ID is used in the git-rebase-todo.tmp file: this file is created by the fake editor in the test script and reflects the state that would have been presented to the user to edit.
--- sequencer.c | 11 ++++++++++- t/t3404-rebase-interactive.sh | 15 +++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-)
sequencer.c +10 -1
@@ -5076,7 +5076,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla {const char *shortonto, *todo_file =rebase_path_todo ();struct todo_list new_todo =TODO_LIST_INIT ; -struct strbuf *buf = &todo_list->buf; +struct strbuf *buf = &todo_list->buf, buf2 =STRBUF_INIT ;struct object_id oid = onto->object.oid;int res;
@@ -5128,6 +5128,15 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned flareturn -1 ; } +/* Expand the commit IDs */ +todo_list_to_strbuf (r, &new_todo, &buf2,-1 ,0 ); +strbuf_swap (&new_todo.buf, &buf2); +strbuf_release (&buf2); + new_todo.total_nr -= new_todo.nr; +if (todo_list_parse_insn_buffer (r, new_todo.buf.buf, &new_todo) <0 ) +BUG ("invalid todo list after expanding IDs:\n%s" , + new_todo.buf.buf); +if (opts->allow_ff &&skip_unnecessary_picks (r, &new_todo, &oid)) {todo_list_release (&new_todo);return error (_ ("could not skip unnecessary pick commands" ));
t/t3404-rebase-interactive.sh +13 -2
@@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' ' test_expect_success SHA1 'short SHA-1 collide' ' test_when_finished "reset_rebase && git checkout master" && git checkout collide && + colliding_sha1=6bcda37 && + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" && ( unset test_tick && test_tick && set_fake_editor && FAKE_COMMIT_MESSAGE="collide2 ac4f2ee" \ - FAKE_LINES="reword 1 2" git rebase -i HEAD~2 - ) + FAKE_LINES="reword 1 break 2" git rebase -i HEAD~2 && + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" && + grep "^pick $colliding_sha1 " \ + .git/rebase-merge/git-rebase-todo.tmp && + grep "^pick [0-9a-f]\{40\}" \ + .git/rebase-merge/git-rebase-todo && + git rebase --continue + ) && + collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" && + collide3="$(git rev-parse collide3 | cut -c 1-4)" && + test "$collide2" = "$collide3" ' test_expect_success 'respect core.abbrev' '
Signature · 1 line
gitgitgadget
Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision
t/t3404-rebase-interactive.sh test_expect_success SHA1 'short SHA-1 setup' ' · line 1268
Quoted 7 lines · Johannes Schindelin via GitGitGadget
t/t3404-rebase-interactive.sh +2
-0@@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' ' test_expect_success SHA1 'short SHA-1 collide' ' test_when_finished "reset_rebase && git checkout master" && git checkout collide && + colliding_sha1=6bcda37 && + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
How much do we care that this is introducing new code with git upstream of a pipe (considering recent efforts to eradicate such usage)? Same question regarding several other new instances introduce by this patch.
Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision
Hi Eric,
Quoted 14 lines · Eric Sunshine
t/t3404-rebase-interactive.sh test_expect_success SHA1 'short SHA-1 setup' ' · line 1268
Quoted 7 lines · Johannes Schindelin via GitGitGadget
t/t3404-rebase-interactive.sh +2
-0@@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' ' test_expect_success SHA1 'short SHA-1 collide' ' test_when_finished "reset_rebase && git checkout master" && git checkout collide && + colliding_sha1=6bcda37 && + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&How much do we care that this is introducing new code with git upstream of a pipe (considering recent efforts to eradicate such usage)? Same question regarding several other new instances introduce by this patch.
I would argue that the test case will fail if the git call fails. So I am not overly concerned if that git call is upstream of a pipe.
Ciao,
Dscho
Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision
Quoted 5 lines · Johannes Schindelin
Quoted 4 lines · Eric Sunshine
Quoted 1 line · Johannes Schindelin via GitGitGadget
+ test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
Quoted 7 lines
Quoted 4 lines
How much do we care that this is introducing new code with git upstream of a pipe (considering recent efforts to eradicate such usage)? Same question regarding several other new instances introduce by this patch.
I would argue that the test case will fail if the
gitcall fails. So I am not overly concerned if thatgitcall is upstream of a pipe.
Unless the git command crashes _after_ it produces the correct output...
Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision
Hi Eric,
Quoted 12 lines · Eric Sunshine
Quoted 10 lines · Johannes Schindelin
Quoted 8 lines · Eric Sunshine
Quoted 1 line · Johannes Schindelin via GitGitGadget
+ test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
How much do we care that this is introducing new code with git upstream of a pipe (considering recent efforts to eradicate such usage)? Same question regarding several other new instances introduce by this patch.
Quoted 4 lines
Quoted 2 lines
I would argue that the test case will fail if the
gitcall fails. So I am not overly concerned if thatgitcall is upstream of a pipe.Unless the git command crashes _after_ it produces the correct output...
Yes, that is true. In a very hypothetical way, of course.
:-)
Ciao,
Dscho
Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision
In reply to Eric SunshineQuoted 12 lines · Eric Sunshine
Quoted 10 lines · Johannes Schindelin
Quoted 8 lines · Eric Sunshine
Quoted 1 line · Johannes Schindelin via GitGitGadget
+ test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
How much do we care that this is introducing new code with git upstream of a pipe (considering recent efforts to eradicate such usage)? Same question regarding several other new instances introduce by this patch.
Quoted 4 lines
Quoted 2 lines
I would argue that the test case will fail if the
gitcall fails. So I am not overly concerned if thatgitcall is upstream of a pipe.Unless the git command crashes _after_ it produces the correct output...
True. Doesn't rev-parse have an appropriate option for this kind of thing that gets rid of the need for "cut" in the first place?
Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision
Hi Junio,
Quoted 14 lines · Junio C Hamano
Quoted 12 lines · Eric Sunshine
Quoted 10 lines · Johannes Schindelin
Quoted 8 lines · Eric Sunshine
Quoted 1 line · Johannes Schindelin via GitGitGadget
+ test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
How much do we care that this is introducing new code with git upstream of a pipe (considering recent efforts to eradicate such usage)? Same question regarding several other new instances introduce by this patch.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
I would argue that the test case will fail if the
gitcall fails. So I am not overly concerned if thatgitcall is upstream of a pipe.Unless the git command crashes _after_ it produces the correct output...
True. Doesn't rev-parse have an appropriate option for this kind of thing that gets rid of the need for "cut" in the first place?
You mean git rev-parse --short=4? That does something _sligthly_
different: it tries to shorten the OID to 4 characters _unless that would be ambiguous_. In our case, it _will_ be ambiguous. That's why I use cut.
As to the crash in rev-parse _after_ printing out the OID: yes, there is a possibility for that. But that regression test is not about rev-parse, so it is actually a good thing that it would not trigger on such a bug ;-)
Ciao,
Dscho
Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision
Quoted 7 lines · Johannes Schindelin
Quoted 2 lines
True. Doesn't rev-parse have an appropriate option for this kind of thing that gets rid of the need for "cut" in the first place?
You mean
git rev-parse --short=4? That does something _sligthly_
different: it tries to shorten the OID to 4 characters_unlessthat would beambiguous_. In our case, it _will_ be ambiguous. That's why I usecut.
Ah, yes of course; we want ambiguous prefix. I think a more thorough test would be to see that the output with --short=$n (where n is the length of the abbreviated object name in $colliding_sha1) is longer than $colliding_sha1 and the output prefix-matches $colliding_sha1 iow, something like
abbreviated=$(git rev-parse --short=7 HEAD) && case "$abbreviated" in "$colliding_sha1"?*) : happy ;; *) false ;; esac && ...
which would make sure that we are testing colliding case.
Quoted 3 lines
As to the crash in
rev-parse_after_ printing out the OID: yes, there is a possibility for that. But that regression test is not aboutrev-parse, so it is actually a good thing that it would not trigger on such a bug ;-)
No, I do not think this test should be about rev-parse working correctly---just that if it is easy enough to make the test robust enough against such a breakage, it would be nice to do so, that's all.
I'm not Eric but I suspect his primary point was not about worrying about rev-parse crashing but more about avoiding to add a pattern less experienced developers can copy&paste without thinking enough to realize why it would be OK here and not OK in the context of the tests they are adding. That would be what I would worry about more than rev-parse crashing in the part of the test under discussion.
Thanks.
PATCH v2 3/3 rebase -i: also avoid SHA-1 collisions with missingCommitsCheck
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
When rebase.missingCommitsCheck is in effect, we use the backup of the todo list that was copied just before the user was allowed to edit it.
That backup is, of course, just as susceptible to the hash collision as the todo list itself: a reworded commit could make a previously unambiguous short commit ID ambiguous all of a sudden.
So let's not just copy the todo list, but let's instead write out the backup with expanded commit IDs.
--- rebase-interactive.c | 8 +++++--- t/t3404-rebase-interactive.sh | 2 ++ 2 files changed, 7 insertions(+), 3 deletions(-)
rebase-interactive.c +5 -3
@@ -104,9 +104,11 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list, -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))return error_errno (_ ("could not write '%s'" ), todo_file); -if (initial &©_file (rebase_path_todo_backup (), todo_file,0666 )) -return error (_ ("could not copy '%s' to '%s'." ), todo_file, -rebase_path_todo_backup ()); +if (initial && +todo_list_write_to_file (r, todo_list,rebase_path_todo_backup (), + shortrevisions, shortonto,-1 , + (flags |TODO_LIST_APPEND_TODO_HELP ) & ~TODO_LIST_SHORTEN_IDS ) <0 ) +return error (_ ("could not write '%s'." ),rebase_path_todo_backup ());if (launch_sequence_editor (todo_file, &new_todo->buf,NULL ))return -2 ;
t/t3404-rebase-interactive.sh +2 -0
@@ -1277,6 +1277,8 @@ test_expect_success SHA1 'short SHA-1 collide' ' .git/rebase-merge/git-rebase-todo.tmp && grep "^pick [0-9a-f]\{40\}" \ .git/rebase-merge/git-rebase-todo && + grep "^pick [0-9a-f]\{40\}" \ + .git/rebase-merge/git-rebase-todo.backup && git rebase --continue ) && collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" &&
Signature · 1 line
gitgitgadget
v3 cover Re-fix rebase -i with SHA-1 collisions
In reply to Johannes Schindelin via GitGitGadgetTriggered by one of brian's SHA-256 patch series, I looked at the reason why the SHA-1 collision test case passed when it shouldn't. Turns out that the regression test was not quite thorough enough, and the interactive rebase did regress recently.
While in the area, I realized that the same bug exists in the code backing the rebase.missingCommitsCheck feature: the backed-up todo list uses shortened commit IDs that may very well become ambiguous during the rebase. For good measure, this patch series fixes that, too.
Finally, I saw that git rebase --edit-todo reported the line in an awkward, maybe even incorrect, way when there was an ambiguous commit ID, and I also fixed that.
To make sure that the code can be easily adapted to SHA-256 after these patches, I actually already made those adjustments on top and offered them up at https://github.com/bk2204/git/pull/1.
Changes since v2:
* Clarified in the first commit message that the change from a positive return value to a negative return value (to indicate failure, in both cases) both was intentional, and does not require any change in the corresponding function's only caller.
Changes since v1:
* Turned the error condition when parsing the todo list with just-expanded commit IDs failed into a BUG().
Johannes Schindelin (3):
parse_insn_line(): improve error message when parsing failed rebase -i: re-fix short SHA-1 collision
rebase -i: also avoid SHA-1 collisions with missingCommitsCheck
rebase-interactive.c | 8 +++++--- sequencer.c | 18 ++++++++++++++---- t/t3404-rebase-interactive.sh | 17 +++++++++++++++-- 3 files changed, 34 insertions(+), 9 deletions(-)
base-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-529%2Fdscho%2Fre-fix-rebase-i-with-sha-collisions-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-529/dscho/re-fix-rebase-i-with-sha-collisions-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/529
Range-diff vs v2:
1: 2ae2e435b0 ! 1: 6f5c7b0325 parse_insn_line(): improve error message when parsing failed
@@ -7,6 +7,11 @@
That makes it rather hard for users to understand what is going wrong, so let's fix that.
+ While at it, return a negative value from parse_insn_line() in case of
+ an error, as per our convention. This function's only caller,
+ todo_list_parse_insn_buffer(), cares only whether that return value is
+ non-zero or not, i.e. does not need to be changed.
+
diff --git a/sequencer.c b/sequencer.c
2: 102fa568dc = 2: 595ba81e09 rebase -i: re-fix short SHA-1 collision 3: 486e9413a6 = 3: b7b063408f rebase -i: also avoid SHA-1 collisions with missingCommitsCheck
Signature · 1 line
gitgitgadget
PATCH v3 1/3 parse_insn_line(): improve error message when parsing failed
From: Johannes Schindelin <[email protected]>
In the case that a get_oid() call failed, we showed some rather bogus part of the line instead of the precise string we sent to said function. That makes it rather hard for users to understand what is going wrong, so let's fix that.
While at it, return a negative value from parse_insn_line() in case of an error, as per our convention. This function's only caller, todo_list_parse_insn_buffer(), cares only whether that return value is non-zero or not, i.e. does not need to be changed.
--- sequencer.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
sequencer.c +4 -3
@@ -2118,6 +2118,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, saved = *end_of_object_name; *end_of_object_name ='\0' ; status =get_oid (bol, &commit_oid); +if (status <0 ) +error (_ ("could not parse '%s'" ), bol);/* return later */ *end_of_object_name = saved; bol = end_of_object_name +strspn (end_of_object_name," \t" );
@@ -2125,11 +2127,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, item->arg_len = (int )(eol - bol);if (status <0 ) -return error (_ ("could not parse '%.*s'" ), - (int )(end_of_object_name - bol), bol); +return status; item->commit =lookup_commit_reference (r, &commit_oid); -return !item->commit; +return item->commit ?0 :-1 ; }int sequencer_get_last_command (struct repository *r,enum replay_action *action)
Signature · 1 line
gitgitgadget
PATCH v3 2/3 rebase -i: re-fix short SHA-1 collision
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
In 66ae9a57b88 (t3404: rebase -i: demonstrate short SHA-1 collision, 2013-08-23), we added a test case that demonstrated how it is possible that a previously unambiguous short commit ID could become ambiguous *during* a rebase.
In 75c69766554 (rebase -i: fix short SHA-1 collision, 2013-08-23), we fixed that problem simply by writing out the todo list with expanded commit IDs (except *right* before letting the user edit the todo list, in which case we shorten them, but we expand them right after the file was edited).
However, the bug resurfaced as a side effect of 393adf7a6f6 (sequencer:
directly call pick_commits() from complete_action(), 2019-11-24): as of this commit, the sequencer no longer re-reads the todo list after writing it out with expanded commit IDs.
The only redeeming factor is that the todo list is already parsed at that stage, including all the commits corresponding to the commands, therefore the sequencer can continue even if the internal todo list has short commit IDs.
That does not prevent problems, though: the sequencer writes out the done and git-rebase-todo files incrementally (i.e. overwriting the todo list with a version that has _short_ commit IDs), and if a merge conflict happens, or if an edit or a break command is encountered, a subsequent git rebase --continue _will_ re-read the todo list, opening an opportunity for the "short SHA-1 collision" bug again.
To avoid that, let's make sure that we do expand the commit IDs in the todo list as soon as we have parsed it after letting the user edit it.
Additionally, we improve the 'short SHA-1 collide' test case in t3404 to test specifically for the case where the rebase is resumed. We also hard-code the expected colliding short SHA-1s, to document the expectation (and to make it easier on future readers).
Note that we specifically test that the short commit ID is used in the git-rebase-todo.tmp file: this file is created by the fake editor in the test script and reflects the state that would have been presented to the user to edit.
--- sequencer.c | 11 ++++++++++- t/t3404-rebase-interactive.sh | 15 +++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-)
sequencer.c +10 -1
@@ -5076,7 +5076,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla {const char *shortonto, *todo_file =rebase_path_todo ();struct todo_list new_todo =TODO_LIST_INIT ; -struct strbuf *buf = &todo_list->buf; +struct strbuf *buf = &todo_list->buf, buf2 =STRBUF_INIT ;struct object_id oid = onto->object.oid;int res;
@@ -5128,6 +5128,15 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned flareturn -1 ; } +/* Expand the commit IDs */ +todo_list_to_strbuf (r, &new_todo, &buf2,-1 ,0 ); +strbuf_swap (&new_todo.buf, &buf2); +strbuf_release (&buf2); + new_todo.total_nr -= new_todo.nr; +if (todo_list_parse_insn_buffer (r, new_todo.buf.buf, &new_todo) <0 ) +BUG ("invalid todo list after expanding IDs:\n%s" , + new_todo.buf.buf); +if (opts->allow_ff &&skip_unnecessary_picks (r, &new_todo, &oid)) {todo_list_release (&new_todo);return error (_ ("could not skip unnecessary pick commands" ));
t/t3404-rebase-interactive.sh +13 -2
@@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' ' test_expect_success SHA1 'short SHA-1 collide' ' test_when_finished "reset_rebase && git checkout master" && git checkout collide && + colliding_sha1=6bcda37 && + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" && ( unset test_tick && test_tick && set_fake_editor && FAKE_COMMIT_MESSAGE="collide2 ac4f2ee" \ - FAKE_LINES="reword 1 2" git rebase -i HEAD~2 - ) + FAKE_LINES="reword 1 break 2" git rebase -i HEAD~2 && + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" && + grep "^pick $colliding_sha1 " \ + .git/rebase-merge/git-rebase-todo.tmp && + grep "^pick [0-9a-f]\{40\}" \ + .git/rebase-merge/git-rebase-todo && + git rebase --continue + ) && + collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" && + collide3="$(git rev-parse collide3 | cut -c 1-4)" && + test "$collide2" = "$collide3" ' test_expect_success 'respect core.abbrev' '
Signature · 1 line
gitgitgadget
PATCH v3 3/3 rebase -i: also avoid SHA-1 collisions with missingCommitsCheck
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
When rebase.missingCommitsCheck is in effect, we use the backup of the todo list that was copied just before the user was allowed to edit it.
That backup is, of course, just as susceptible to the hash collision as the todo list itself: a reworded commit could make a previously unambiguous short commit ID ambiguous all of a sudden.
So let's not just copy the todo list, but let's instead write out the backup with expanded commit IDs.
--- rebase-interactive.c | 8 +++++--- t/t3404-rebase-interactive.sh | 2 ++ 2 files changed, 7 insertions(+), 3 deletions(-)
rebase-interactive.c +5 -3
@@ -104,9 +104,11 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list, -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))return error_errno (_ ("could not write '%s'" ), todo_file); -if (initial &©_file (rebase_path_todo_backup (), todo_file,0666 )) -return error (_ ("could not copy '%s' to '%s'." ), todo_file, -rebase_path_todo_backup ()); +if (initial && +todo_list_write_to_file (r, todo_list,rebase_path_todo_backup (), + shortrevisions, shortonto,-1 , + (flags |TODO_LIST_APPEND_TODO_HELP ) & ~TODO_LIST_SHORTEN_IDS ) <0 ) +return error (_ ("could not write '%s'." ),rebase_path_todo_backup ());if (launch_sequence_editor (todo_file, &new_todo->buf,NULL ))return -2 ;
t/t3404-rebase-interactive.sh +2 -0
@@ -1277,6 +1277,8 @@ test_expect_success SHA1 'short SHA-1 collide' ' .git/rebase-merge/git-rebase-todo.tmp && grep "^pick [0-9a-f]\{40\}" \ .git/rebase-merge/git-rebase-todo && + grep "^pick [0-9a-f]\{40\}" \ + .git/rebase-merge/git-rebase-todo.backup && git rebase --continue ) && collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" &&
Signature · 1 line
gitgitgadget