Skip to content

Re-fix rebase -i with SHA-1 collisions #529

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dscho wants to merge 3 commits into from
Closed

Re-fix rebase -i with SHA-1 collisions #529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f5c7b0
parse_insn_line(): improve error message when parsing failed
dscho Jan 15, 2020
595ba81
rebase -i: re-fix short SHA-1 collision
dscho Jan 15, 2020
b7b0634
rebase -i: also avoid SHA-1 collisions with missingCommitsCheck
dscho Jan 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions rebase-interactive.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 && copy_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;
Expand Down
18 changes: 14 additions & 4 deletions sequencer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2118,18 +2118,19 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
saved = *end_of_object_name;
Copy link

@gitgitgadget gitgitgadget bot Jan 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@gitgitgadget gitgitgadget bot Jan 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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' '

Copy link

@gitgitgadget gitgitgadget bot Jan 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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' '
>
>

Copy link

@gitgitgadget gitgitgadget bot Jan 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@gitgitgadget gitgitgadget bot Jan 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@gitgitgadget gitgitgadget bot Jan 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link

@gitgitgadget gitgitgadget bot Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@gitgitgadget gitgitgadget bot Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@gitgitgadget gitgitgadget bot Jan 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@gitgitgadget gitgitgadget bot Jan 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@gitgitgadget gitgitgadget bot Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@gitgitgadget gitgitgadget bot Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*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");
item->arg_offset = bol - buf;
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)
Expand Down Expand Up @@ -5075,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;

Expand Down Expand Up @@ -5127,6 +5128,15 @@ 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)
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"));
Expand Down
17 changes: 15 additions & 2 deletions t/t3404-rebase-interactive.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1264,13 +1264,26 @@ 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 &&
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)" &&
collide3="$(git rev-parse collide3 | cut -c 1-4)" &&
test "$collide2" = "$collide3"
'

test_expect_success 'respect core.abbrev' '
Expand Down