This can be seen as a continuation of ag/reduce-rewriting-todo.
Currently, complete_action() releases its todo list before calling
sequencer_continue(), which reloads the todo list from the disk. This
series removes this useless round trip.
Patches 1, 2, and 3 originally come from a series meaning to improve
rebase.missingCommitsCheck[0]. In the original series, I wanted to
check for missing commits in read_populate_todo(), so a warning could be
issued after a `rebase --continue' or an `exec' commands. But, in the
case of the initial edit, it is already checked in complete_action(),
and would be checked a second time in sequencer_continue() (a caller of
read_populate_todo()). So I hacked up sequencer_continue() to accept a
pointer to a todo list, and if not null, would skip the call to
read_populate_todo(). (This was really ugly, to be honest.) Some
issues arose with git-prompt.sh[1], hence 1, 2 and 3.
Patch 5 is a new approach to what I did first. Instead of bolting a new
parameter to sequencer_continue(), this makes complete_action() calling
directly pick_commits().
This is based on 4c86140027 ("Third batch").
Changes since v2:
- Patch 1 has been reworded to fix a mistake in read_populate_todo()'s
name, reported by Jonathan Tan.
- Patches 4 and 5 has been reworded to improve descriptions of the code
paths, according to comments made by Jonathan Tan.
- Squashed b0537b0ec3 ("SQUASH??? tentative leakfix") into the 5th
patch to fix a memory leak reported by René Sharfe.
The tip of this series is tagged as "reduce-todo-list-cont-v3" at
https://github.com/agrn/git.
[0] http://public-inbox.org/git/20190717143918.7406-1-alban.gruin@xxxxxxxxx/
[1] http://public-inbox.org/git/1732521.CJWHkCQAay@andromeda/
Alban Gruin (5):
sequencer: update `total_nr' when adding an item to a todo list
sequencer: update `done_nr' when skipping commands in a todo list
sequencer: move the code writing total_nr on the disk to a new
function
rebase: fill `squash_onto' in get_replay_opts()
sequencer: directly call pick_commits() from complete_action()
builtin/rebase.c | 5 +++++
sequencer.c | 32 +++++++++++++++++++++++---------
2 files changed, 28 insertions(+), 9 deletions(-)
Diff-intervalle contre v2 :
1: 9215b191c7 ! 1: 11a221e82e sequencer: update `total_nr' when adding an item to a todo list
@@ Commit message
This variable is mostly used by command prompts (ie. git-prompt.sh and
the like). By forgetting to update it, the original code made it not
reflect the reality, but this flaw was masked by the code calling
- unnecessarily read_todo_list() again to update the variable to its
+ unnecessarily read_populate_todo() again to update the variable to its
correct value. At the end of this series, the unnecessary call will be
removed, and the inconsistency addressed by this patch would start to
matter.
2: 7cad541092 = 2: 76a3af70b6 sequencer: update `done_nr' when skipping commands in a todo list
3: 7c9c4ddd30 = 3: 9c5bd30465 sequencer: move the code writing total_nr on the disk to a new function
4: cd44fb4e10 ! 4: bc3d69a10e rebase: fill `squash_onto' in get_replay_opts()
@@ Metadata
## Commit message ##
rebase: fill `squash_onto' in get_replay_opts()
- Currently, the get_replay_opts() function does not initialise the
- `squash_onto' field (which is used for the `--root' mode), only
- read_populate_opts() does. That would lead to incorrect results when
- calling pick_commits() without reading the options from the disk first.
+ When sequencer_continue() is called by complete_action(), `opts' has
+ been filled by get_replay_opts(). Currently, it does not initialise the
+ `squash_onto' field (used by the `--root' mode), only
+ read_populate_opts() does. It’s not a problem yet since
+ sequencer_continue() calls it before pick_commits(), but it would lead
+ to incorrect results once complete_action() is modified to call
+ pick_commits() directly.
Let’s change that.
5: 523fdd35a1 ! 5: e7691db66b sequencer: directly call pick_commits() from complete_action()
@@ Metadata
## Commit message ##
sequencer: directly call pick_commits() from complete_action()
- Currently, complete_action() calls sequencer_continue() to do the
- rebase. Before the former calls pick_commits(), it
+ Currently, complete_action(), used by builtin/rebase.c to start a new
+ rebase, calls sequencer_continue() to do it. Before the former calls
+ pick_commits(), it
- calls read_and_refresh_cache() -- this is unnecessary here as we've
- just called require_clean_work_tree()
+ just called require_clean_work_tree() in complete_action()
- calls read_populate_opts() -- this is unnecessary as we're starting a
- new rebase, so opts is fully populated
+ new rebase, so `opts' is fully populated
- loads the todo list -- this is unnecessary as we've just populated
- the todo list
+ the todo list in complete_action()
- commits any staged changes -- this is unnecessary as we're starting a
new rebase, so there are no staged changes
- calls record_in_rewritten() -- this is unnecessary as we're starting
@@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts,
}
- todo_list_release(&new_todo);
--
++ res = -1;
+
if (checkout_onto(r, opts, onto_name, &oid, orig_head))
- return -1;
+- return -1;
++ goto cleanup;
if (require_clean_work_tree(r, "rebase", "", 1, 1))
- return -1;
+- return -1;
++ goto cleanup;
- return sequencer_continue(r, opts);
+ todo_list_write_total_nr(&new_todo);
+ res = pick_commits(r, &new_todo, opts);
++
++cleanup:
+ todo_list_release(&new_todo);
+
+ return res;
--
2.24.0
