Re-fix rebase -i with SHA-1 collisions
#529
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dscho
force-pushed
the
re-fix-rebase-i-with-sha-collisions
branch
3 times, most recently
from
890fb1c
to
e7d9ea8
Compare
|
/submit |
|
Submitted as pull.529.git.1579209506.gitgitgadget@gmail.com |
|
On the Git mailing list, "brian m. carlson" wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -2118,18 +2118,19 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, | |||
| saved = *end_of_object_name; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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 <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index b9dbf1adb0..7c30dad59c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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...
> 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;
...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.
> 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 is not
explained, though).
OK.
| @@ -2118,18 +2118,19 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, | |||
| saved = *end_of_object_name; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> 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.
Ouch. The analysis above is quite understandable.
> @@ -5128,6 +5128,18 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> return -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"));
> + }
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?
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ae6e55ce79..1cc9f36bc7 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -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' '
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Fri, 17 Jan 2020, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > + /* 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 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.
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.
> 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()?
It should.
Will fix,
Dscho
> Or am I somehow missing a hunk that removes an existing call to
> parse&check?
>
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > index ae6e55ce79..1cc9f36bc7 100755
> > --- a/t/t3404-rebase-interactive.sh
> > +++ b/t/t3404-rebase-interactive.sh
> > @@ -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' '
>
>
e7d9ea8
to
486e941
Compare
|
On the Git mailing list, "brian m. carlson" wrote (reply to this): |
|
/submit |
|
Submitted as pull.529.v2.git.1579304283.gitgitgadget@gmail.com |
| @@ -2118,18 +2118,19 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, | |||
| saved = *end_of_object_name; | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> @@ -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.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Eric,
On Mon, 20 Jan 2020, Eric Sunshine wrote:
> On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > @@ -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
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Mon, Jan 20, 2020 at 3:04 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Mon, 20 Jan 2020, Eric Sunshine wrote:
> > On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > > + 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.
Unless the git command crashes _after_ it produces the correct output...
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Eric,
On Mon, 20 Jan 2020, Eric Sunshine wrote:
> On Mon, Jan 20, 2020 at 3:04 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Mon, 20 Jan 2020, Eric Sunshine wrote:
> > > On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget
> > > <gitgitgadget@gmail.com> wrote:
> > > > + 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.
>
> Unless the git command crashes _after_ it produces the correct output...
Yes, that is true. In a very hypothetical way, of course.
:-)
Ciao,
Dscho
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Mon, Jan 20, 2020 at 3:04 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> On Mon, 20 Jan 2020, Eric Sunshine wrote:
>> > On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget
>> > <gitgitgadget@gmail.com> wrote:
>> > > + 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.
>
> 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?
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Tue, 21 Jan 2020, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > On Mon, Jan 20, 2020 at 3:04 PM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> >> On Mon, 20 Jan 2020, Eric Sunshine wrote:
> >> > On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget
> >> > <gitgitgadget@gmail.com> wrote:
> >> > > + 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.
> >
> > 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
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 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`.
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.
> 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 ;-)
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.
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
| @@ -2118,18 +2118,19 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, | |||
| saved = *end_of_object_name; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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 <johannes.schindelin@gmx.de>
> ...
> @@ -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.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Tue, 21 Jan 2020, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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 <johannes.schindelin@gmx.de>
> > ...
> > @@ -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.
You're right. I amended the commit message. Will send out a v3 soon.
Thanks,
Dscho
|
This patch series was integrated into pu via git@76318f9. |
|
This branch is now known as |
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. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In 66ae9a5 (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 75c6976 (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 393adf7 (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. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
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. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
re-fix-rebase-i-with-sha-collisions
branch
from
486e941
to
b7b0634
Compare
|
/submit |
|
Submitted as pull.529.v3.git.1579782499.gitgitgadget@gmail.com |
|
This patch series was integrated into pu via git@ac924fe. |
|
This patch series was integrated into pu via git@5b403db. |
|
This patch series was integrated into pu via git@f16ef31. |
|
This patch series was integrated into pu via git@8c0352b. |
|
This patch series was integrated into pu via git@ac03faf. |
|
This patch series was integrated into pu via git@ca6cd45. |
|
This patch series was integrated into next via git@670ba07. |
|
This patch series was integrated into pu via git@306348a. |
|
This patch series was integrated into pu via git@ed3c8e8. |
|
This patch series was integrated into pu via git@2511870. |
|
This patch series was integrated into next via git@2511870. |
|
This patch series was integrated into master via git@2511870. |
|
Closed via 2511870. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.missingCommitsCheckfeature: 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-todoreported 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 bk2204#1.
Changes since v2:
Changes since v1:
BUG().Cc: brian m. carlson sandals@crustytoothpaste.net, Alban Gruin alban.gruin@gmail.com, Eric Sunshine sunshine@sunshineco.com, Junio C Hamano gitster@pobox.com