git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Alban Gruin <alban.gruin@gmail.com>
To: git@vger.kernel.org
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Phillip Wood <phillip.wood@dunelm.org.uk>,
	Junio C Hamano <gitster@pobox.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	Alban Gruin <alban.gruin@gmail.com>
Subject: [PATCH v3 0/5] Use complete_action's todo list to do the rebase
Date: Sat, 23 Nov 2019 15:37:00 +0100	[thread overview]
Message-ID: <20191123143705.17280-1-alban.gruin@gmail.com> (raw)
In-Reply-To: <20191007092641.12661-1-alban.gruin@gmail.com>

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@gmail.com/
[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


  parent reply	other threads:[~2019-11-23 14:46 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190925201315.19722-1-alban.gruin@gmail.com>
2019-09-25 20:13 ` [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-10-02  2:10   ` Junio C Hamano
2019-10-02  8:06     ` Johannes Schindelin
2019-10-02  8:59       ` Junio C Hamano
2019-10-02  9:48         ` Johannes Schindelin
2019-10-02 18:03         ` Alban Gruin
2019-09-25 20:13 ` [PATCH v1 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-09-25 20:13 ` [PATCH v1 3/5] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-09-25 20:13 ` [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
2019-09-27 13:30   ` Phillip Wood
2019-10-02  8:16     ` Johannes Schindelin
2019-10-02  9:32       ` Phillip Wood
2019-10-02 12:06         ` Johannes Schindelin
2019-09-25 20:13 ` [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
2019-09-27 13:26   ` Phillip Wood
2019-10-02  8:20     ` Johannes Schindelin
2019-10-02 18:03       ` Alban Gruin
2019-10-02  2:38   ` Junio C Hamano
2019-10-02 18:37     ` Alban Gruin
2019-10-26  7:47   ` René Scharfe
2019-10-26 11:27     ` Alban Gruin
2019-10-28  1:39       ` Junio C Hamano
2019-10-28  3:20         ` Junio C Hamano
2019-09-25 20:20 ` [PATCH v1 0/5] Use complete_action's todo list to do the rebase Alban Gruin
2019-09-27 13:32 ` [PATCH v1 0/5] Use complete_action’s " Phillip Wood
2019-10-07  9:26 ` [PATCH v2 0/5] Use complete_action's " Alban Gruin
2019-10-07  9:26   ` [PATCH v2 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-10-07  9:26   ` [PATCH v2 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-10-07  9:26   ` [PATCH v2 3/5] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-10-07  9:26   ` [PATCH v2 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
2019-10-07  9:26   ` [PATCH v2 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
2019-10-08  2:45   ` [PATCH v2 0/5] Use complete_action's todo list to do the rebase Junio C Hamano
2019-10-08 16:18     ` Alban Gruin
2019-10-14 12:49   ` Johannes Schindelin
2019-11-23 14:37   ` Alban Gruin [this message]
2019-11-23 14:37     ` [PATCH v3 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-11-23 14:37     ` [PATCH v3 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-11-23 14:37     ` [PATCH v3 3/5] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-11-23 14:37     ` [PATCH v3 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
2019-11-23 14:37     ` [PATCH v3 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
2019-11-24 17:43     ` [PATCH v4 0/5] Use complete_action's todo list to do the rebase Alban Gruin
2019-11-24 17:43       ` [PATCH v4 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-11-24 17:43       ` [PATCH v4 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-11-24 17:43       ` [PATCH v4 3/5] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-11-24 17:43       ` [PATCH v4 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
2019-11-26 18:27         ` Jonathan Tan
2019-11-24 17:43       ` [PATCH v4 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
2019-11-26 18:41         ` Jonathan Tan
2019-11-27 19:56           ` Alban Gruin
2019-11-27 20:09             ` Alban Gruin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191123143705.17280-1-alban.gruin@gmail.com \
    --to=alban.gruin@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).