git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Mar 26, 2023, 14:28 UTC
Message-ID
<8a188876-c456-7269-28de-9ff406204030@dunelm.org.uk>
In-Reply-To
<20230323162235.995574-9-oswald.buddenhagen@gmx.de>
Hi Oswald
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
Show 12 quoted lines
> When the user butchers the todo file during rebase -i setup, the
> --continue which would follow --edit-todo would have skipped the last
> steps of the setup. Notably, this would bypass the fast-forward over
> untouched picks (though the actual picking loop would still fast-forward
> the commits, one by one).
> 
> Fix this by splitting off the tail of complete_action() to a new
> start_rebase() function and call that from sequencer_continue() when no
> commands have been executed yet.
> 
> More or less as a side effect, we no longer checkout `onto` before exiting
> when the todo file is bad.

I think the implications of this change deserve to be discussed in the commit message. Three things spring to mind but there may be others I haven't thought of

  - Previously when rebase stopped and handed control back to the user
    HEAD would have already been detached. This patch changes that
    meaning we can have an active rebase of a branch while that branch is
    checked out. What does "git status" show in this case? What does the
    shell prompt show? Will it confuse users?
  - Previously if the user created a commit before running "rebase
    --continue" we'd rebase on to that commit. Now that commit will be
    silently dropped.
  - Previously if the user checkout out another commit before running
    "rebase --continue" we'd rebase on to that commit. Now we we rebase
    on to the original "onto" commit.
 > This makes aborting cheaper and will simplify
 > things in a later change.

Given that we're stopping so the user can fix the problem and continue the rebase I don't think optimizing for aborting is a convincing reason for this change on its own.

Show 11 quoted lines
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 62986a7b1b..00d3e19c62 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>   		return ret;
>   	}
>   	if (cmd == 'c')
> -		return sequencer_continue(the_repository, opts);
> +		return sequencer_continue(the_repository, opts,
> +					  0, NULL, NULL, NULL);

It's a bit unfortunate that we have to start passing all these extra parameters, could the sequencer read them itself in read_populate_opts()?

> -int sequencer_continue(struct repository *r, struct replay_opts *opts)
> +static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags,
> +			const char *onto_name, const struct object_id *onto,
> +			const struct object_id *orig_head, struct todo_list *todo_list);

It would be nice to avoid this forward declaration. I think you could do that by adding a preparatory patch that moves either checkout_onto() or sequencer_continue()

Show 18 quoted lines
> @@ -6142,49 +6154,52 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   
>   		return error(_("nothing to do"));
>   	} else if (res == EDIT_TODO_INCORRECT) {
> -		checkout_onto(r, opts, onto_name, onto, orig_head);
>   		todo_list_release(&new_todo);
>   
>   		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);

I don't think we need to move this code. If start_rebase() is called from sequencer_continue() the initial edit of the todo list failed and has been fixed by running "git rebase --edit-todo". In that case the oids have already been expanded on disc.

Show 29 quoted lines
> -	if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {
> -		todo_list_release(&new_todo);
> -		return error(_("could not skip unnecessary pick commands"));
> -	}
> -
> -	if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,
> -				    flags & ~(TODO_LIST_SHORTEN_IDS), action)) {
> -		todo_list_release(&new_todo);
> -		return error_errno(_("could not write '%s'"), todo_file);
> -	}
> -
> -	res = -1;
> -
> -	if (checkout_onto(r, opts, onto_name, onto, orig_head))
> -		goto cleanup;
> -
> -	if (require_clean_work_tree(r, "rebase", NULL, 1, 1))
> -		goto cleanup;
> -
> -	todo_list_write_total_nr(&new_todo);
> -	res = pick_commits(r, &new_todo, opts);
> -
> -cleanup:
> +	res = start_rebase(r, opts, flags, onto_name, onto, orig_head, &new_todo);
>   	todo_list_release(&new_todo);
>   
>   	return res;
>   }
>   
> +test_expect_success 'continue after bad first command' '
> +	test_when_finished "git rebase --abort ||:" &&
> +	git checkout primary^0 &&

If you want a specific commit it's better to use a tag name as those are fixed whereas the branches get rebased all over the place in this test file.

> +	git reflog expire --expire=all HEAD &&
Is this really necessary, can you pass -n to "git reflog" below?
Show 10 quoted lines
> +	(
> +		set_fake_editor &&
> +		test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \
> +			git rebase -i HEAD~3 &&
> +		test_cmp_rev HEAD primary &&
> +		FAKE_LINES="pick 2 pick 3 reword 4" git rebase --edit-todo &&
> +		FAKE_COMMIT_MESSAGE="E_reworded" git rebase --continue
> +	) &&
> +	git reflog > reflog &&
> +	test $(grep -c fast-forward reflog) = 1 &&
Using test_line_count would make test failures easier to debug.
> +	test_cmp_rev HEAD~1 primary~1 &&
> +	test "$(git log -1 --format=%B)" = "E_reworded"

It is slightly more work, but please use test_cmp for things like this as it makes it so much easier to debug test failures.

Best Wishes
Phillip
Show 17 quoted lines
> +'
> +
> +test_expect_success 'abort after bad first command' '
> +	test_when_finished "git rebase --abort ||:" &&
> +	git checkout primary^0 &&
> +	(
> +		set_fake_editor &&
> +		test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \
> +			git rebase -i HEAD~3
> +	) &&
> +	git rebase --abort &&
> +	test_cmp_rev HEAD primary
> +'
> +
>   test_expect_success 'tabs and spaces are accepted in the todolist' '
>   	rebase_setup_and_clean indented-comment &&
>   	write_script add-indent.sh <<-\EOF &&
Previous: Oswald BuddenhagenNext: Oswald Buddenhagen
Message 3 of 49 in “sequencer refactoring”
  1. 0/8 sequencer refactoringOswald Buddenhagen, Mar 23, 2023
  2. 8/8 rebase: improve resumption from incorrect initial todo listOswald Buddenhagen, Mar 23, 2023
  3. Phillip WoodMar 26, 2023
  4. Oswald BuddenhagenApr 26, 2023
  5. Phillip WoodMay 17, 2023
  6. Oswald BuddenhagenAug 24, 2023
  7. 1/8 rebase: simplify code related to imply_merge()Oswald Buddenhagen, Mar 23, 2023
  8. Phillip WoodMar 23, 2023
  9. Junio C HamanoMar 23, 2023
  10. Felipe ContrerasMar 23, 2023
  11. 0/3 rebase refactoringOswald Buddenhagen, Aug 9, 2023
  12. 1/3 rebase: simplify code related to imply_merge()Oswald Buddenhagen, Aug 9, 2023
  13. 2/3 rebase: handle --strategy via imply_merge() as wellOswald Buddenhagen, Aug 9, 2023
  14. 3/3 rebase: move parse_opt_keep_empty() downOswald Buddenhagen, Aug 9, 2023
  15. Phillip WoodAug 15, 2023
  16. 0/3 rebase refactoringOswald Buddenhagen, Oct 20, 2023
  17. 2/3 rebase: handle --strategy via imply_merge() as wellOswald Buddenhagen, Oct 20, 2023
  18. Junio C HamanoOct 20, 2023
  19. 3/3 rebase: move parse_opt_keep_empty() downOswald Buddenhagen, Oct 20, 2023
  20. 1/3 rebase: simplify code related to imply_merge()Oswald Buddenhagen, Oct 20, 2023
  21. Junio C HamanoOct 20, 2023
  22. Phillip WoodOct 23, 2023
  23. Junio C HamanoOct 23, 2023
  24. 2/8 rebase: move parse_opt_keep_empty() downOswald Buddenhagen, Mar 23, 2023
  25. Phillip WoodMar 23, 2023
  26. 4/8 sequencer: create enum for edit_todo_list() return valueOswald Buddenhagen, Mar 23, 2023
  27. Phillip WoodMar 23, 2023
  28. 5/8 rebase: preserve interactive todo file on checkout failureOswald Buddenhagen, Mar 23, 2023
  29. Phillip WoodMar 23, 2023
  30. Oswald BuddenhagenMar 23, 2023
  31. Phillip WoodMar 24, 2023
  32. Oswald BuddenhagenMar 24, 2023
  33. Junio C HamanoMar 23, 2023
  34. Oswald BuddenhagenMar 23, 2023
  35. Junio C HamanoMar 24, 2023
  36. 3/8 sequencer: pass around rebase action explicitlyOswald Buddenhagen, Mar 23, 2023
  37. Phillip WoodMar 23, 2023
  38. Oswald BuddenhagenMar 23, 2023
  39. 6/8 sequencer: simplify allocation of result array in todo_list_rearrange_squash()Oswald Buddenhagen, Mar 23, 2023
  40. Phillip WoodMar 23, 2023
  41. Oswald BuddenhagenMar 23, 2023
  42. 7/8 sequencer: pass `onto` to complete_action() as object-idOswald Buddenhagen, Mar 23, 2023
  43. Phillip WoodMar 23, 2023
  44. Oswald BuddenhagenMar 23, 2023
  45. Phillip WoodMar 24, 2023
  46. Phillip WoodMar 23, 2023
  47. Phillip WoodMar 25, 2023
  48. Phillip WoodApr 6, 2023
  49. Phillip WoodMay 17, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.