git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list
       [not found] <20190925201315.19722-1-alban.gruin@gmail.com>
@ 2019-09-25 20:13 ` Alban Gruin
  2019-10-02  2:10   ` Junio C Hamano
  2019-09-25 20:13 ` [PATCH v1 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2019-09-25 20:13 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

`total_nr' is the total amount of items, done and toto, that are in a
todo list.  But unlike `nr', it was not updated when an item was
appended to the list.

This variable is mostly used by command prompts (ie. git-prompt.sh and
the like).

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index d648aaf416..575b852a5a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list)
 static struct todo_item *append_new_todo(struct todo_list *todo_list)
 {
 	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
+	todo_list->total_nr++;
 	return todo_list->items + todo_list->nr++;
 }
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 2/5] sequencer: update `done_nr' when skipping commands in a todo list
       [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-09-25 20:13 ` 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
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-09-25 20:13 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

In a todo list, `done_nr' is the amount of commands that were executed
or skipped, but skip_unnecessary_picks() did not update it.

This variable is mostly used by command prompts (ie. git-prompt.sh and
the like).

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index 575b852a5a..42313f8de6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5054,6 +5054,7 @@ static int skip_unnecessary_picks(struct repository *r,
 		MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i);
 		todo_list->nr -= i;
 		todo_list->current = 0;
+		todo_list->done_nr += i;
 
 		if (is_fixup(peek_command(todo_list, 0)))
 			record_in_rewritten(base_oid, peek_command(todo_list, 0));
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 3/5] sequencer: move the code writing total_nr on the disk to a new function
       [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-09-25 20:13 ` [PATCH v1 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
@ 2019-09-25 20:13 ` Alban Gruin
  2019-09-25 20:13 ` [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-09-25 20:13 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

The total amount of commands can be used to show the progression of the
rebasing in a shell.  It is written to the disk by read_populate_todo()
when the todo list is loaded from sequencer_continue() or
pick_commits(), but not by complete_action().

This moves the part writing total_nr to a new function so it can be
called from complete_action().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 42313f8de6..ec7ea8d9e5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2342,6 +2342,16 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
 	sequencer_remove_state(&opts);
 }
 
+static void todo_list_write_total_nr(struct todo_list *todo_list)
+{
+	FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
+
+	if (f) {
+		fprintf(f, "%d\n", todo_list->total_nr);
+		fclose(f);
+	}
+}
+
 static int read_populate_todo(struct repository *r,
 			      struct todo_list *todo_list,
 			      struct replay_opts *opts)
@@ -2387,7 +2397,6 @@ static int read_populate_todo(struct repository *r,
 
 	if (is_rebase_i(opts)) {
 		struct todo_list done = TODO_LIST_INIT;
-		FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
 
 		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
 		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
@@ -2399,10 +2408,7 @@ static int read_populate_todo(struct repository *r,
 			+ count_commands(todo_list);
 		todo_list_release(&done);
 
-		if (f) {
-			fprintf(f, "%d\n", todo_list->total_nr);
-			fclose(f);
-		}
+		todo_list_write_total_nr(todo_list);
 	}
 
 	return 0;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()
       [not found] <20190925201315.19722-1-alban.gruin@gmail.com>
                   ` (2 preceding siblings ...)
  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 ` Alban Gruin
  2019-09-27 13:30   ` Phillip Wood
  2019-09-25 20:13 ` [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2019-09-25 20:13 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

get_replay_opts() did not fill `squash_onto' if possible, meaning that
this field should be read from the disk by the sequencer through
read_populate_opts().  Without this, calling `pick_commits()' directly
will result in incorrect results with `rebase --root'.

Let’s change that.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e8319d5946..2097d41edc 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	if (opts->strategy_opts)
 		parse_strategy_opts(&replay, opts->strategy_opts);
 
+	if (opts->squash_onto) {
+		oidcpy(&replay.squash_onto, opts->squash_onto);
+		replay.have_squash_onto = 1;
+	}
+
 	return replay;
 }
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
       [not found] <20190925201315.19722-1-alban.gruin@gmail.com>
                   ` (3 preceding siblings ...)
  2019-09-25 20:13 ` [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
@ 2019-09-25 20:13 ` Alban Gruin
  2019-09-27 13:26   ` Phillip Wood
                     ` (2 more replies)
  2019-09-25 20:20 ` [PATCH v1 0/5] Use complete_action's todo list to do the rebase Alban Gruin
                   ` (2 subsequent siblings)
  7 siblings, 3 replies; 50+ messages in thread
From: Alban Gruin @ 2019-09-25 20:13 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

Currently, complete_action() calls sequencer_continue() to do the
rebase.  Even though the former already has the todo list, the latter
loads it from the disk and parses it.  Calling directly pick_commits()
from complete_action() avoids this unnecessary round trip.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ec7ea8d9e5..b395dd6e11 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		return error_errno(_("could not write '%s'"), todo_file);
 	}
 
-	todo_list_release(&new_todo);
-
 	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
 		return -1;
 
 	if (require_clean_work_tree(r, "rebase", "", 1, 1))
 		return -1;
 
-	return sequencer_continue(r, opts);
+	todo_list_write_total_nr(&new_todo);
+	res = pick_commits(r, &new_todo, opts);
+	todo_list_release(&new_todo);
+
+	return res;
 }
 
 struct subject2item_entry {
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v1 0/5] Use complete_action's todo list to do the rebase
       [not found] <20190925201315.19722-1-alban.gruin@gmail.com>
                   ` (4 preceding siblings ...)
  2019-09-25 20:13 ` [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
@ 2019-09-25 20:20 ` 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
  7 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-09-25 20:20 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

[Sorry, I made a mistake and this message was not transmitted :/]

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 master (4c86140027, "Third batch").

The tip of this series is tagged as "reduce-todo-list-cont-v1" 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      | 26 ++++++++++++++++++--------
 2 files changed, 23 insertions(+), 8 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
  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  2:38   ` Junio C Hamano
  2019-10-26  7:47   ` René Scharfe
  2 siblings, 1 reply; 50+ messages in thread
From: Phillip Wood @ 2019-09-27 13:26 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano

Hi Alban

Thanks for removing some more unnecessary work reloading the the todo list.

On 25/09/2019 21:13, Alban Gruin wrote:
> Currently, complete_action() calls sequencer_continue() to do the
> rebase.  Even though the former already has the todo list, the latter
> loads it from the disk and parses it.  Calling directly pick_commits()
> from complete_action() avoids this unnecessary round trip.
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   sequencer.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index ec7ea8d9e5..b395dd6e11 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   		return error_errno(_("could not write '%s'"), todo_file);
>   	}
>   
> -	todo_list_release(&new_todo);
> -
>   	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
>   		return -1;
>   
>   	if (require_clean_work_tree(r, "rebase", "", 1, 1))
>   		return -1;
>   
> -	return sequencer_continue(r, opts);

sequencer_continue does a number of things before calling pick_commits(). It
  - calls read_and_refresh_cache() - this is unnecessary here as we've 
just called require_clean_work_tree()
  - calls read_populate_opts() - this is unnecessary as we're staring a 
new rebase so opts is fully populated
  - loads the todo list - this is unnecessary as we've just populated 
the todo list
  - commits any staged changes - this is unnecessary as we're staring a 
new rebase so there are no staged changes
  - calls record_in_rewritten() - this is unnecessary as we're starting 
a new rebase

So I agree that this patch is correct.

Thanks

Phillip

> +	todo_list_write_total_nr(&new_todo);
> +	res = pick_commits(r, &new_todo, opts);
> +	todo_list_release(&new_todo);
> +
> +	return res;
>   }
>   
>   struct subject2item_entry {
> 

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()
  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
  0 siblings, 1 reply; 50+ messages in thread
From: Phillip Wood @ 2019-09-27 13:30 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano

Hi Alban

On 25/09/2019 21:13, Alban Gruin wrote:
> get_replay_opts() did not fill `squash_onto' if possible, meaning that

I'm not sure what you mean by 'if possible' here, I think the sentance 
makes sense without that.

> this field should be read from the disk by the sequencer through
> read_populate_opts().  Without this, calling `pick_commits()' directly
> will result in incorrect results with `rebase --root'.
> 
> Let’s change that.

Good catch

Best Wishes

Phillip

> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   builtin/rebase.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index e8319d5946..2097d41edc 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>   	if (opts->strategy_opts)
>   		parse_strategy_opts(&replay, opts->strategy_opts);
>   
> +	if (opts->squash_onto) {
> +		oidcpy(&replay.squash_onto, opts->squash_onto);
> +		replay.have_squash_onto = 1;
> +	}
> +
>   	return replay;
>   }
>   
> 

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 0/5] Use complete_action’s todo list to do the rebase
       [not found] <20190925201315.19722-1-alban.gruin@gmail.com>
                   ` (5 preceding siblings ...)
  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 ` Phillip Wood
  2019-10-07  9:26 ` [PATCH v2 0/5] Use complete_action's " Alban Gruin
  7 siblings, 0 replies; 50+ messages in thread
From: Phillip Wood @ 2019-09-27 13:32 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano

Hi Alban

On 25/09/2019 21:13, Alban Gruin wrote:
> 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, tbh.)  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().

Thanks for working on this, these patches all look fine to me modulo the 
confusing wording in the commit message on patch 4

Best Wishes

Phillip

> 
> This is based on master (4c86140027, "Third batch").
> 
> The tip of this series is tagged as "reduce-todo-list-cont-v1" 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      | 26 ++++++++++++++++++--------
>   2 files changed, 23 insertions(+), 8 deletions(-)
> 

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list
  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
  0 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2019-10-02  2:10 UTC (permalink / raw)
  To: Alban Gruin; +Cc: git, Johannes Schindelin, Phillip Wood

Alban Gruin <alban.gruin@gmail.com> writes:

> `total_nr' is the total amount of items, done and toto, that are in a
> todo list.  But unlike `nr', it was not updated when an item was
> appended to the list.

s/amount/number/, as amount is specifically for something
that cannot be counted.

Perhaps a stupid language question but what is "toto"?


> This variable is mostly used by command prompts (ie. git-prompt.sh and
> the like).
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  sequencer.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index d648aaf416..575b852a5a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list)
>  static struct todo_item *append_new_todo(struct todo_list *todo_list)
>  {
>  	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
> +	todo_list->total_nr++;
>  	return todo_list->items + todo_list->nr++;
>  }

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
  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  2:38   ` Junio C Hamano
  2019-10-02 18:37     ` Alban Gruin
  2019-10-26  7:47   ` René Scharfe
  2 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2019-10-02  2:38 UTC (permalink / raw)
  To: Alban Gruin; +Cc: git, Johannes Schindelin, Phillip Wood

Alban Gruin <alban.gruin@gmail.com> writes:

> Currently, complete_action() calls sequencer_continue() to do the
> rebase.  Even though the former already has the todo list, the latter
> loads it from the disk and parses it.  Calling directly pick_commits()
> from complete_action() avoids this unnecessary round trip.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  sequencer.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

All the earlier steps in this series are necessary because the
inconsistencies caused by not doing things the code updated by the
earlier steps do (e.g. leaving number of commits recorded in
total_nr and done_nr stale) were masked by re-reading the info from
the on-disk file.  And this step exposes these inconsistencies
because the extra reading from the file no longer happens.

That is my reading of the series---did I get it right?

I wonder if that can be stated clearly in the log message of the
earlier steps.

For example, by reading 1/5 or 2/5 alone, it would be natural for
readers to wonder why the original code that did not update done_nr
correctly terminated after going through the todo list (you would
expect that when done reaches total you are finished, so if one of
these variables are not maintained correctly, your termination
condition may be borked), and then by knowing that the current code
more-or-less works correctly by not terminating too early or barfing
to say it ran out of things to do prematurely, the next thing
readers would wonder is if these patches introduce new bugs by
incrementing these variables twice (iow, "does the author of the
patch missed other places where these variables are correctly
updated?")

1/5 for example talks about the variable mostly being used by
prompts, which may be useful to reduce worries by readers about
correctness (iow, "well, if it is only showing wrong number in the
prompts and does not affect correctness otherwise, perhaps that was
why the current code that is buggy did not behave incorrectly").
But I suspect that it is not why these changes in the earlier steps
are acceptable or are right things to do.  So, perhaps replace the
"these are used only in prompts and the numbers being wrong does not
affect correctness" comments from them with:

    By forgetting to update the variable, the original code made it
    not reflect the reality, but this flaw was masked by the code
    calling (sometimes unnecessarily) read_todo_list() again to
    update the variable to its correct value before the next action
    happens.  At the end of this series, the unnecessary call will
    be removed and the inconsistency addressed by this patch would
    start to matter.

or something like that (assuming that the answer to the first
question I asked in this message is "yes", that is), or a more
concise version of it?

Thanks.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list
  2019-10-02  2:10   ` Junio C Hamano
@ 2019-10-02  8:06     ` Johannes Schindelin
  2019-10-02  8:59       ` Junio C Hamano
  0 siblings, 1 reply; 50+ messages in thread
From: Johannes Schindelin @ 2019-10-02  8:06 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Alban Gruin, git, Phillip Wood

Hi Junio,

On Wed, 2 Oct 2019, Junio C Hamano wrote:

> Alban Gruin <alban.gruin@gmail.com> writes:
>
> > `total_nr' is the total amount of items, done and toto, that are in a
> > todo list.  But unlike `nr', it was not updated when an item was
> > appended to the list.
>
> s/amount/number/, as amount is specifically for something
> that cannot be counted.
>
> Perhaps a stupid language question but what is "toto"?

"in toto" is Latin for "in total", if I remember correctly.

But in this instance, I think it is merely a typo and should have been
"todo" instead. That is what the "total_nr" is about: the number of
"done" and "todo" items, added together.

Ciao,
Dscho

>
>
> > This variable is mostly used by command prompts (ie. git-prompt.sh and
> > the like).
> >
> > Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> > ---
> >  sequencer.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index d648aaf416..575b852a5a 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list)
> >  static struct todo_item *append_new_todo(struct todo_list *todo_list)
> >  {
> >  	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
> > +	todo_list->total_nr++;
> >  	return todo_list->items + todo_list->nr++;
> >  }
>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()
  2019-09-27 13:30   ` Phillip Wood
@ 2019-10-02  8:16     ` Johannes Schindelin
  2019-10-02  9:32       ` Phillip Wood
  0 siblings, 1 reply; 50+ messages in thread
From: Johannes Schindelin @ 2019-10-02  8:16 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Alban Gruin, git, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 1923 bytes --]

Hi,

On Fri, 27 Sep 2019, Phillip Wood wrote:

> Hi Alban
>
> On 25/09/2019 21:13, Alban Gruin wrote:
> > get_replay_opts() did not fill `squash_onto' if possible, meaning that
>
> I'm not sure what you mean by 'if possible' here, I think the sentance makes
> sense without that.
>
> > this field should be read from the disk by the sequencer through
> > read_populate_opts().  Without this, calling `pick_commits()' directly
> > will result in incorrect results with `rebase --root'.

I guess I would have an easier time reading the commit message if this
paragraph read like this:

	The `get_replay_opts()` function currently does not initialize
	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 disk first.

> >
> > Let’s change that.
>
> Good catch
>
> Best Wishes
>
> Phillip
>
> > Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> > ---
> >   builtin/rebase.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index e8319d5946..2097d41edc 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct
> > rebase_options *opts)
> >    if (opts->strategy_opts)
> >     parse_strategy_opts(&replay, opts->strategy_opts);
> >   +	if (opts->squash_onto) {

I guess it does not matter much, but shouldn't this be guarded against
the case where `replay.squash_onto` was already initialized? Like, `if
(opts->squash_onto && !replay.have_squash_onto)`?

Other than that, this patch makes sense to me.

Ciao,
Dscho

> > +		oidcpy(&replay.squash_onto, opts->squash_onto);
> > +		replay.have_squash_onto = 1;
> > +	}
> > +
> >   	return replay;
> >   }
> >
> >
>
>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
  2019-09-27 13:26   ` Phillip Wood
@ 2019-10-02  8:20     ` Johannes Schindelin
  2019-10-02 18:03       ` Alban Gruin
  0 siblings, 1 reply; 50+ messages in thread
From: Johannes Schindelin @ 2019-10-02  8:20 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Alban Gruin, git, Junio C Hamano

Hi,

On Fri, 27 Sep 2019, Phillip Wood wrote:

> Hi Alban
>
> Thanks for removing some more unnecessary work reloading the the todo list.
>
> On 25/09/2019 21:13, Alban Gruin wrote:
> > Currently, complete_action() calls sequencer_continue() to do the
> > rebase.  Even though the former already has the todo list, the latter
> > loads it from the disk and parses it.  Calling directly pick_commits()
> > from complete_action() avoids this unnecessary round trip.
> > Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> > ---
> >   sequencer.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index ec7ea8d9e5..b395dd6e11 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct
> > replay_opts *opts, unsigned fla
> >    	return error_errno(_("could not write '%s'"), todo_file);
> >    }
> >   -	todo_list_release(&new_todo);
> > -
> >    if (checkout_onto(r, opts, onto_name, &oid, orig_head))
> >     return -1;
> >
> >    if (require_clean_work_tree(r, "rebase", "", 1, 1))
> >     return -1;
> >   -	return sequencer_continue(r, opts);
>
> sequencer_continue does a number of things before calling pick_commits(). It
>  - calls read_and_refresh_cache() - this is unnecessary here as we've just
> called require_clean_work_tree()
>  - calls read_populate_opts() - this is unnecessary as we're staring a new
> rebase so opts is fully populated
>  - loads the todo list - this is unnecessary as we've just populated the todo
> list
>  - commits any staged changes - this is unnecessary as we're staring a new
> rebase so there are no staged changes
>  - calls record_in_rewritten() - this is unnecessary as we're starting a new
> rebase
>
> So I agree that this patch is correct.

All true. Could this careful analysis maybe be included in the commit
message (with `s/staring/starting/`)?

Thanks,
Dscho

>
> Thanks
>
> Phillip
>
> > +	todo_list_write_total_nr(&new_todo);
> > +	res = pick_commits(r, &new_todo, opts);
> > +	todo_list_release(&new_todo);
> > +
> > +	return res;
> >   }
> >
> >   struct subject2item_entry {
> >
>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list
  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
  0 siblings, 2 replies; 50+ messages in thread
From: Junio C Hamano @ 2019-10-02  8:59 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Alban Gruin, git, Phillip Wood

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Wed, 2 Oct 2019, Junio C Hamano wrote:
>
>> Alban Gruin <alban.gruin@gmail.com> writes:
>>
>> > `total_nr' is the total amount of items, done and toto, that are in a
>> > todo list.  But unlike `nr', it was not updated when an item was
>> > appended to the list.
>>
>> s/amount/number/, as amount is specifically for something
>> that cannot be counted.
>>
>> Perhaps a stupid language question but what is "toto"?
>
> "in toto" is Latin for "in total", if I remember correctly.

And "Toto" can also be "Toyo Toki", one of the two large and well
known Japanese manufacturers of porcelain things you see in
bathrooms--oh how appropriate in this project ;-).

> But in this instance, I think it is merely a typo and should have been
> "todo" instead. That is what the "total_nr" is about: the number of
> "done" and "todo" items, added together.

If I were writing this, I would probably say "... the total number
of items, counting both done and todo,..." and with 'counting both'
I wouldn't have been so puzzled.

Thanks.

>
> Ciao,
> Dscho
>
>>
>>
>> > This variable is mostly used by command prompts (ie. git-prompt.sh and
>> > the like).
>> >
>> > Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>> > ---
>> >  sequencer.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/sequencer.c b/sequencer.c
>> > index d648aaf416..575b852a5a 100644
>> > --- a/sequencer.c
>> > +++ b/sequencer.c
>> > @@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list)
>> >  static struct todo_item *append_new_todo(struct todo_list *todo_list)
>> >  {
>> >  	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
>> > +	todo_list->total_nr++;
>> >  	return todo_list->items + todo_list->nr++;
>> >  }
>>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()
  2019-10-02  8:16     ` Johannes Schindelin
@ 2019-10-02  9:32       ` Phillip Wood
  2019-10-02 12:06         ` Johannes Schindelin
  0 siblings, 1 reply; 50+ messages in thread
From: Phillip Wood @ 2019-10-02  9:32 UTC (permalink / raw)
  To: Johannes Schindelin, Phillip Wood; +Cc: Alban Gruin, git, Junio C Hamano

Hi Dscho

On 02/10/2019 09:16, Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 27 Sep 2019, Phillip Wood wrote:
> 
>> Hi Alban
>>
>> On 25/09/2019 21:13, Alban Gruin wrote:
 >>> [...]
>>>    builtin/rebase.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>>> index e8319d5946..2097d41edc 100644
>>> --- a/builtin/rebase.c
>>> +++ b/builtin/rebase.c
>>> @@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct
>>> rebase_options *opts)
>>>     if (opts->strategy_opts)
>>>      parse_strategy_opts(&replay, opts->strategy_opts);
>>>    +	if (opts->squash_onto) {
> 
> I guess it does not matter much, but shouldn't this be guarded against
> the case where `replay.squash_onto` was already initialized? Like, `if
> (opts->squash_onto && !replay.have_squash_onto)`?

replay is uninitialized as this function takes a `struct rebase_opitons` 
and returns a new `struct replay_options` based on that.

Best Wishes

Phillip

> 
> Other than that, this patch makes sense to me.
> 
> Ciao,
> Dscho
> 
>>> +		oidcpy(&replay.squash_onto, opts->squash_onto);
>>> +		replay.have_squash_onto = 1;
>>> +	}
>>> +
>>>    	return replay;
>>>    }
>>>
>>>
>>
>>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list
  2019-10-02  8:59       ` Junio C Hamano
@ 2019-10-02  9:48         ` Johannes Schindelin
  2019-10-02 18:03         ` Alban Gruin
  1 sibling, 0 replies; 50+ messages in thread
From: Johannes Schindelin @ 2019-10-02  9:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Alban Gruin, git, Phillip Wood

Hi Junio,

On Wed, 2 Oct 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Wed, 2 Oct 2019, Junio C Hamano wrote:
> >
> >> Alban Gruin <alban.gruin@gmail.com> writes:
> >>
> >> > `total_nr' is the total amount of items, done and toto, that are in a
> >> > todo list.  But unlike `nr', it was not updated when an item was
> >> > appended to the list.
> >>
> >> s/amount/number/, as amount is specifically for something
> >> that cannot be counted.
> >>
> >> Perhaps a stupid language question but what is "toto"?
> >
> > "in toto" is Latin for "in total", if I remember correctly.
>
> And "Toto" can also be "Toyo Toki", one of the two large and well
> known Japanese manufacturers of porcelain things you see in
> bathrooms--oh how appropriate in this project ;-).

You made me laugh out loud! :-)

> > But in this instance, I think it is merely a typo and should have been
> > "todo" instead. That is what the "total_nr" is about: the number of
> > "done" and "todo" items, added together.
>
> If I were writing this, I would probably say "... the total number
> of items, counting both done and todo,..." and with 'counting both'
> I wouldn't have been so puzzled.

I also like it better the way you put it.

Ciao,
Dscho

>
> Thanks.
>
> >
> > Ciao,
> > Dscho
> >
> >>
> >>
> >> > This variable is mostly used by command prompts (ie. git-prompt.sh and
> >> > the like).
> >> >
> >> > Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> >> > ---
> >> >  sequencer.c | 1 +
> >> >  1 file changed, 1 insertion(+)
> >> >
> >> > diff --git a/sequencer.c b/sequencer.c
> >> > index d648aaf416..575b852a5a 100644
> >> > --- a/sequencer.c
> >> > +++ b/sequencer.c
> >> > @@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list)
> >> >  static struct todo_item *append_new_todo(struct todo_list *todo_list)
> >> >  {
> >> >  	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
> >> > +	todo_list->total_nr++;
> >> >  	return todo_list->items + todo_list->nr++;
> >> >  }
> >>
>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()
  2019-10-02  9:32       ` Phillip Wood
@ 2019-10-02 12:06         ` Johannes Schindelin
  0 siblings, 0 replies; 50+ messages in thread
From: Johannes Schindelin @ 2019-10-02 12:06 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Alban Gruin, git, Junio C Hamano

Hi Phillip,

On Wed, 2 Oct 2019, Phillip Wood wrote:

> On 02/10/2019 09:16, Johannes Schindelin wrote:
> >
> > On Fri, 27 Sep 2019, Phillip Wood wrote:
> >
> > > Hi Alban
> > >
> > > On 25/09/2019 21:13, Alban Gruin wrote:
> > > > [...]
> > > >    builtin/rebase.c | 5 +++++
> > > >    1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > > > index e8319d5946..2097d41edc 100644
> > > > --- a/builtin/rebase.c
> > > > +++ b/builtin/rebase.c
> > > > @@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const
> > > > struct
> > > > rebase_options *opts)
> > > >     if (opts->strategy_opts)
> > > >      parse_strategy_opts(&replay, opts->strategy_opts);
> > > >    +	if (opts->squash_onto) {
> >
> > I guess it does not matter much, but shouldn't this be guarded against
> > the case where `replay.squash_onto` was already initialized? Like, `if
> > (opts->squash_onto && !replay.have_squash_onto)`?
>
> replay is uninitialized as this function takes a `struct rebase_opitons` and
> returns a new `struct replay_options` based on that.

Ooops, you're right. The `.` in `replay.have_squash_onto` should have
been a strong hint, eh?

Thanks,
Dscho

>
> Best Wishes
>
> Phillip
>
> >
> > Other than that, this patch makes sense to me.
> >
> > Ciao,
> > Dscho
> >
> > > > +		oidcpy(&replay.squash_onto, opts->squash_onto);
> > > > +		replay.have_squash_onto = 1;
> > > > +	}
> > > > +
> > > >    	return replay;
> > > >    }
> > > >
> > > >
> > >
> > >
>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list
  2019-10-02  8:59       ` Junio C Hamano
  2019-10-02  9:48         ` Johannes Schindelin
@ 2019-10-02 18:03         ` Alban Gruin
  1 sibling, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-10-02 18:03 UTC (permalink / raw)
  To: Junio C Hamano, Johannes Schindelin; +Cc: git, Phillip Wood

Hi Junio and Johannes,

Le 02/10/2019 à 10:59, Junio C Hamano a écrit :
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> Hi Junio,
>>
>> On Wed, 2 Oct 2019, Junio C Hamano wrote:
>>
>>> Alban Gruin <alban.gruin@gmail.com> writes:
>>>
>>>> `total_nr' is the total amount of items, done and toto, that are in a
>>>> todo list.  But unlike `nr', it was not updated when an item was
>>>> appended to the list.
>>>
>>> s/amount/number/, as amount is specifically for something
>>> that cannot be counted.
>>>

Thank you for these corrections, I really appreciate it :)

>>> Perhaps a stupid language question but what is "toto"?
>>
>> "in toto" is Latin for "in total", if I remember correctly.
> 
> And "Toto" can also be "Toyo Toki", one of the two large and well
> known Japanese manufacturers of porcelain things you see in
> bathrooms--oh how appropriate in this project ;-).
> 

In French, it’s the name of a recurring character in children’s jokes.
It’s also used sometimes as a dummy variable name like foo or bar.

>> But in this instance, I think it is merely a typo and should have been
>> "todo" instead. That is what the "total_nr" is about: the number of
>> "done" and "todo" items, added together.
> 

Correct.

> If I were writing this, I would probably say "... the total number
> of items, counting both done and todo,..." and with 'counting both'
> I wouldn't have been so puzzled.
> 

I will change this.

Cheers,
Alban



> Thanks.
> 
>>
>> Ciao,
>> Dscho
>>
>>>
>>>
>>>> This variable is mostly used by command prompts (ie. git-prompt.sh and
>>>> the like).
>>>>
>>>> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>>>> ---
>>>>  sequencer.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/sequencer.c b/sequencer.c
>>>> index d648aaf416..575b852a5a 100644
>>>> --- a/sequencer.c
>>>> +++ b/sequencer.c
>>>> @@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list)
>>>>  static struct todo_item *append_new_todo(struct todo_list *todo_list)
>>>>  {
>>>>  	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
>>>> +	todo_list->total_nr++;
>>>>  	return todo_list->items + todo_list->nr++;
>>>>  }
>>>


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
  2019-10-02  8:20     ` Johannes Schindelin
@ 2019-10-02 18:03       ` Alban Gruin
  0 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-10-02 18:03 UTC (permalink / raw)
  To: Johannes Schindelin, Phillip Wood; +Cc: git, Junio C Hamano

Hi Johannes,

Le 02/10/2019 à 10:20, Johannes Schindelin a écrit :
> Hi,
> 
> On Fri, 27 Sep 2019, Phillip Wood wrote:
> 
>> Hi Alban
>>
>> Thanks for removing some more unnecessary work reloading the the todo list.
>>
>> On 25/09/2019 21:13, Alban Gruin wrote:
>>> Currently, complete_action() calls sequencer_continue() to do the
>>> rebase.  Even though the former already has the todo list, the latter
>>> loads it from the disk and parses it.  Calling directly pick_commits()
>>> from complete_action() avoids this unnecessary round trip.
>>> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>>> ---
>>>   sequencer.c | 8 +++++---
>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index ec7ea8d9e5..b395dd6e11 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct
>>> replay_opts *opts, unsigned fla
>>>    	return error_errno(_("could not write '%s'"), todo_file);
>>>    }
>>>   -	todo_list_release(&new_todo);
>>> -
>>>    if (checkout_onto(r, opts, onto_name, &oid, orig_head))
>>>     return -1;
>>>
>>>    if (require_clean_work_tree(r, "rebase", "", 1, 1))
>>>     return -1;
>>>   -	return sequencer_continue(r, opts);
>>
>> sequencer_continue does a number of things before calling pick_commits(). It
>>  - calls read_and_refresh_cache() - this is unnecessary here as we've just
>> called require_clean_work_tree()
>>  - calls read_populate_opts() - this is unnecessary as we're staring a new
>> rebase so opts is fully populated
>>  - loads the todo list - this is unnecessary as we've just populated the todo
>> list
>>  - commits any staged changes - this is unnecessary as we're staring a new
>> rebase so there are no staged changes
>>  - calls record_in_rewritten() - this is unnecessary as we're starting a new
>> rebase
>>
>> So I agree that this patch is correct.
> 
> All true. Could this careful analysis maybe be included in the commit
> message (with `s/staring/starting/`)?
> 

I will do so (same for your comment on 4/5) and resend this series as
soon as possible.

Cheers,
Alban



> Thanks,
> Dscho
> 
>>
>> Thanks
>>
>> Phillip
>>
>>> +	todo_list_write_total_nr(&new_todo);
>>> +	res = pick_commits(r, &new_todo, opts);
>>> +	todo_list_release(&new_todo);
>>> +
>>> +	return res;
>>>   }
>>>
>>>   struct subject2item_entry {
>>>
>>


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
  2019-10-02  2:38   ` Junio C Hamano
@ 2019-10-02 18:37     ` Alban Gruin
  0 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-10-02 18:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Schindelin, Phillip Wood

Hi Junio,

Le 02/10/2019 à 04:38, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> 
>> Currently, complete_action() calls sequencer_continue() to do the
>> rebase.  Even though the former already has the todo list, the latter
>> loads it from the disk and parses it.  Calling directly pick_commits()
>> from complete_action() avoids this unnecessary round trip.
>>
>> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>> ---
>>  sequencer.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> All the earlier steps in this series are necessary because the
> inconsistencies caused by not doing things the code updated by the
> earlier steps do (e.g. leaving number of commits recorded in
> total_nr and done_nr stale) were masked by re-reading the info from
> the on-disk file.  And this step exposes these inconsistencies
> because the extra reading from the file no longer happens.
> 
> That is my reading of the series---did I get it right?
> 

Yes.

> I wonder if that can be stated clearly in the log message of the
> earlier steps.
> 
> For example, by reading 1/5 or 2/5 alone, it would be natural for
> readers to wonder why the original code that did not update done_nr
> correctly terminated after going through the todo list (you would
> expect that when done reaches total you are finished, so if one of
> these variables are not maintained correctly, your termination
> condition may be borked), and then by knowing that the current code
> more-or-less works correctly by not terminating too early or barfing
> to say it ran out of things to do prematurely, the next thing
> readers would wonder is if these patches introduce new bugs by
> incrementing these variables twice (iow, "does the author of the
> patch missed other places where these variables are correctly
> updated?")
> 
> 1/5 for example talks about the variable mostly being used by
> prompts, which may be useful to reduce worries by readers about
> correctness (iow, "well, if it is only showing wrong number in the
> prompts and does not affect correctness otherwise, perhaps that was
> why the current code that is buggy did not behave incorrectly").
> But I suspect that it is not why these changes in the earlier steps
> are acceptable or are right things to do.

Interestingly, it is the only reason for these two patches.  If you skip
them both, t34??-*.sh will not fail, only t9903-bash-prompt.sh will.
This is because `total_nr' does not reflect the real number of items in
a todo list, but the number of steps, done *and* remaining.  When a
rebase is resumed, some commands may already have been run, and are no
longer part of the todo list.  So, it’s not used to determine whether or
not the rebase is done.  `nr' is used for this purpose.  `done_nr' and
`total_nr' are just used for user interaction.

>  So, perhaps replace the
> "these are used only in prompts and the numbers being wrong does not
> affect correctness" comments from them with:
> 
>     By forgetting to update the variable, the original code made it
>     not reflect the reality, but this flaw was masked by the code
>     calling (sometimes unnecessarily) read_todo_list() again to
>     update the variable to its correct value before the next action
>     happens.  At the end of this series, the unnecessary call will
>     be removed and the inconsistency addressed by this patch would
>     start to matter.
> 
> or something like that (assuming that the answer to the first
> question I asked in this message is "yes", that is), or a more
> concise version of it?
> 

I will do so.

Cheers,
Alban



> Thanks.
> 


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH v2 0/5] Use complete_action's todo list to do the rebase
       [not found] <20190925201315.19722-1-alban.gruin@gmail.com>
                   ` (6 preceding siblings ...)
  2019-09-27 13:32 ` [PATCH v1 0/5] Use complete_action’s " Phillip Wood
@ 2019-10-07  9:26 ` 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
                     ` (7 more replies)
  7 siblings, 8 replies; 50+ messages in thread
From: Alban Gruin @ 2019-10-07  9:26 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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 v1:

 - Rewording of patches 1, 2, 4 and 5 according to comments made by
   Phillip Wood, Junio C Hamano and Johannes Schindelin.

The tip of this series is tagged as "reduce-todo-list-cont-v2" 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      | 26 ++++++++++++++++++--------
 2 files changed, 23 insertions(+), 8 deletions(-)

Diff-intervalle contre v1 :
1:  d177b0de1a ! 1:  9215b191c7 sequencer: update `total_nr' when adding an item to a todo list
    @@ Metadata
      ## Commit message ##
         sequencer: update `total_nr' when adding an item to a todo list
     
    -    `total_nr' is the total amount of items, done and toto, that are in a
    -    todo list.  But unlike `nr', it was not updated when an item was
    -    appended to the list.
    +    `total_nr' is the total number of items, counting both done and todo,
    +    that are in a todo list.  But unlike `nr', it was not updated when an
    +    item was appended to the list.
     
         This variable is mostly used by command prompts (ie. git-prompt.sh and
    -    the like).
    +    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
    +    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.
     
         Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
     
2:  09fcbe159b ! 2:  7cad541092 sequencer: update `done_nr' when skipping commands in a todo list
    @@ Commit message
         or skipped, but skip_unnecessary_picks() did not update it.
     
         This variable is mostly used by command prompts (ie. git-prompt.sh and
    -    the like).
    +    the like).  As in the previous commit, this inconsistent behaviour is
    +    not a problem yet, but it would start to matter at the end of this
    +    series the same reason.
     
         Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
     
3:  26a18cd1a9 = 3:  7c9c4ddd30 sequencer: move the code writing total_nr on the disk to a new function
4:  5d74903cfe ! 4:  cd44fb4e10 rebase: fill `squash_onto' in get_replay_opts()
    @@ Metadata
      ## Commit message ##
         rebase: fill `squash_onto' in get_replay_opts()
     
    -    get_replay_opts() did not fill `squash_onto' if possible, meaning that
    -    this field should be read from the disk by the sequencer through
    -    read_populate_opts().  Without this, calling `pick_commits()' directly
    -    will result in incorrect results with `rebase --root'.
    +    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.
     
         Let’s change that.
     
5:  dc803c671f ! 5:  523fdd35a1 sequencer: directly call pick_commits() from complete_action()
    @@ Commit message
         sequencer: directly call pick_commits() from complete_action()
     
         Currently, complete_action() calls sequencer_continue() to do the
    -    rebase.  Even though the former already has the todo list, the latter
    -    loads it from the disk and parses it.  Calling directly pick_commits()
    -    from complete_action() avoids this unnecessary round trip.
    +    rebase.  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()
    +     - calls read_populate_opts() -- this is unnecessary as we're starting a
    +       new rebase, so opts is fully populated
    +     - loads the todo list -- this is unnecessary as we've just populated
    +       the todo list
    +     - 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
    +       a new rebase.
    +
    +    This changes complete_action() to directly call pick_commits() to avoid
    +    these unnecessary steps.
     
         Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
     
-- 
2.23.0


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH v2 1/5] sequencer: update `total_nr' when adding an item to a todo list
  2019-10-07  9:26 ` [PATCH v2 0/5] Use complete_action's " Alban Gruin
@ 2019-10-07  9:26   ` Alban Gruin
  2019-10-07  9:26   ` [PATCH v2 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
                     ` (6 subsequent siblings)
  7 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-10-07  9:26 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

`total_nr' is the total number of items, counting both done and todo,
that are in a todo list.  But unlike `nr', it was not updated when an
item was appended to the list.

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

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
Reworded commit.

sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index d648aaf416..575b852a5a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list)
 static struct todo_item *append_new_todo(struct todo_list *todo_list)
 {
 	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
+	todo_list->total_nr++;
 	return todo_list->items + todo_list->nr++;
 }
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 2/5] sequencer: update `done_nr' when skipping commands in a todo list
  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   ` 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
                     ` (5 subsequent siblings)
  7 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-10-07  9:26 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

In a todo list, `done_nr' is the amount of commands that were executed
or skipped, but skip_unnecessary_picks() did not update it.

This variable is mostly used by command prompts (ie. git-prompt.sh and
the like).  As in the previous commit, this inconsistent behaviour is
not a problem yet, but it would start to matter at the end of this
series the same reason.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
Reworded commit.

sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index 575b852a5a..42313f8de6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5054,6 +5054,7 @@ static int skip_unnecessary_picks(struct repository *r,
 		MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i);
 		todo_list->nr -= i;
 		todo_list->current = 0;
+		todo_list->done_nr += i;
 
 		if (is_fixup(peek_command(todo_list, 0)))
 			record_in_rewritten(base_oid, peek_command(todo_list, 0));
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 3/5] sequencer: move the code writing total_nr on the disk to a new function
  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   ` Alban Gruin
  2019-10-07  9:26   ` [PATCH v2 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
                     ` (4 subsequent siblings)
  7 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-10-07  9:26 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

The total amount of commands can be used to show the progression of the
rebasing in a shell.  It is written to the disk by read_populate_todo()
when the todo list is loaded from sequencer_continue() or
pick_commits(), but not by complete_action().

This moves the part writing total_nr to a new function so it can be
called from complete_action().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 42313f8de6..ec7ea8d9e5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2342,6 +2342,16 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
 	sequencer_remove_state(&opts);
 }
 
+static void todo_list_write_total_nr(struct todo_list *todo_list)
+{
+	FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
+
+	if (f) {
+		fprintf(f, "%d\n", todo_list->total_nr);
+		fclose(f);
+	}
+}
+
 static int read_populate_todo(struct repository *r,
 			      struct todo_list *todo_list,
 			      struct replay_opts *opts)
@@ -2387,7 +2397,6 @@ static int read_populate_todo(struct repository *r,
 
 	if (is_rebase_i(opts)) {
 		struct todo_list done = TODO_LIST_INIT;
-		FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
 
 		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
 		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
@@ -2399,10 +2408,7 @@ static int read_populate_todo(struct repository *r,
 			+ count_commands(todo_list);
 		todo_list_release(&done);
 
-		if (f) {
-			fprintf(f, "%d\n", todo_list->total_nr);
-			fclose(f);
-		}
+		todo_list_write_total_nr(todo_list);
 	}
 
 	return 0;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 4/5] rebase: fill `squash_onto' in get_replay_opts()
  2019-10-07  9:26 ` [PATCH v2 0/5] Use complete_action's " Alban Gruin
                     ` (2 preceding siblings ...)
  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   ` Alban Gruin
  2019-10-07  9:26   ` [PATCH v2 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
                     ` (3 subsequent siblings)
  7 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-10-07  9:26 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

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.

Let’s change that.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
Reworded commit.

builtin/rebase.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e8319d5946..2097d41edc 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	if (opts->strategy_opts)
 		parse_strategy_opts(&replay, opts->strategy_opts);
 
+	if (opts->squash_onto) {
+		oidcpy(&replay.squash_onto, opts->squash_onto);
+		replay.have_squash_onto = 1;
+	}
+
 	return replay;
 }
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 5/5] sequencer: directly call pick_commits() from complete_action()
  2019-10-07  9:26 ` [PATCH v2 0/5] Use complete_action's " Alban Gruin
                     ` (3 preceding siblings ...)
  2019-10-07  9:26   ` [PATCH v2 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
@ 2019-10-07  9:26   ` Alban Gruin
  2019-10-08  2:45   ` [PATCH v2 0/5] Use complete_action's todo list to do the rebase Junio C Hamano
                     ` (2 subsequent siblings)
  7 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-10-07  9:26 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin

Currently, complete_action() calls sequencer_continue() to do the
rebase.  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()
 - calls read_populate_opts() -- this is unnecessary as we're starting a
   new rebase, so opts is fully populated
 - loads the todo list -- this is unnecessary as we've just populated
   the todo list
 - 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
   a new rebase.

This changes complete_action() to directly call pick_commits() to avoid
these unnecessary steps.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
Reworded commit.

sequencer.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ec7ea8d9e5..b395dd6e11 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		return error_errno(_("could not write '%s'"), todo_file);
 	}
 
-	todo_list_release(&new_todo);
-
 	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
 		return -1;
 
 	if (require_clean_work_tree(r, "rebase", "", 1, 1))
 		return -1;
 
-	return sequencer_continue(r, opts);
+	todo_list_write_total_nr(&new_todo);
+	res = pick_commits(r, &new_todo, opts);
+	todo_list_release(&new_todo);
+
+	return res;
 }
 
 struct subject2item_entry {
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 0/5] Use complete_action's todo list to do the rebase
  2019-10-07  9:26 ` [PATCH v2 0/5] Use complete_action's " Alban Gruin
                     ` (4 preceding siblings ...)
  2019-10-07  9:26   ` [PATCH v2 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
@ 2019-10-08  2:45   ` Junio C Hamano
  2019-10-08 16:18     ` Alban Gruin
  2019-10-14 12:49   ` Johannes Schindelin
  2019-11-23 14:37   ` [PATCH v3 " Alban Gruin
  7 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2019-10-08  2:45 UTC (permalink / raw)
  To: Alban Gruin; +Cc: git, Johannes Schindelin, Phillip Wood

Alban Gruin <alban.gruin@gmail.com> writes:

> 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 v1:
>
>  - Rewording of patches 1, 2, 4 and 5 according to comments made by
>    Phillip Wood, Junio C Hamano and Johannes Schindelin.
>
> The tip of this series is tagged as "reduce-todo-list-cont-v2" 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      | 26 ++++++++++++++++++--------
>  2 files changed, 23 insertions(+), 8 deletions(-)
>
> Diff-intervalle contre v1 :
> 1:  d177b0de1a ! 1:  9215b191c7 sequencer: update `total_nr' when adding an item to a todo list
>     @@ Metadata
>       ## Commit message ##
>          sequencer: update `total_nr' when adding an item to a todo list
>      
>     -    `total_nr' is the total amount of items, done and toto, that are in a
>     -    todo list.  But unlike `nr', it was not updated when an item was
>     -    appended to the list.
>     +    `total_nr' is the total number of items, counting both done and todo,

The same s/amount/number/ needs to be done to the log message of
patches 2/5 and 3/5.  Other than that, updated log messages looked
much more understandable.  Thanks.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 0/5] Use complete_action's todo list to do the rebase
  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
  0 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-10-08 16:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Schindelin, Phillip Wood

Hi Junio,

Le 08/10/2019 à 04:45, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> 
>> 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 v1:
>>
>>  - Rewording of patches 1, 2, 4 and 5 according to comments made by
>>    Phillip Wood, Junio C Hamano and Johannes Schindelin.
>>
>> The tip of this series is tagged as "reduce-todo-list-cont-v2" 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      | 26 ++++++++++++++++++--------
>>  2 files changed, 23 insertions(+), 8 deletions(-)
>>
>> Diff-intervalle contre v1 :
>> 1:  d177b0de1a ! 1:  9215b191c7 sequencer: update `total_nr' when adding an item to a todo list
>>     @@ Metadata
>>       ## Commit message ##
>>          sequencer: update `total_nr' when adding an item to a todo list
>>      
>>     -    `total_nr' is the total amount of items, done and toto, that are in a
>>     -    todo list.  But unlike `nr', it was not updated when an item was
>>     -    appended to the list.
>>     +    `total_nr' is the total number of items, counting both done and todo,
> 
> The same s/amount/number/ needs to be done to the log message of
> patches 2/5 and 3/5.  Other than that, updated log messages looked
> much more understandable.  Thanks.
> 

Ouch, sorry about that.  Thank you for fixing them.

Cheers,
Alban


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 0/5] Use complete_action's todo list to do the rebase
  2019-10-07  9:26 ` [PATCH v2 0/5] Use complete_action's " Alban Gruin
                     ` (5 preceding siblings ...)
  2019-10-08  2:45   ` [PATCH v2 0/5] Use complete_action's todo list to do the rebase Junio C Hamano
@ 2019-10-14 12:49   ` Johannes Schindelin
  2019-11-23 14:37   ` [PATCH v3 " Alban Gruin
  7 siblings, 0 replies; 50+ messages in thread
From: Johannes Schindelin @ 2019-10-14 12:49 UTC (permalink / raw)
  To: Alban Gruin; +Cc: git, Phillip Wood, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 6540 bytes --]

Hi Alban,

On Mon, 7 Oct 2019, Alban Gruin wrote:

> 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 v1:
>
>  - Rewording of patches 1, 2, 4 and 5 according to comments made by
>    Phillip Wood, Junio C Hamano and Johannes Schindelin.
>
> The tip of this series is tagged as "reduce-todo-list-cont-v2" 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      | 26 ++++++++++++++++++--------
>  2 files changed, 23 insertions(+), 8 deletions(-)
>
> Diff-intervalle contre v1 :
> 1:  d177b0de1a ! 1:  9215b191c7 sequencer: update `total_nr' when adding an item to a todo list
>     @@ Metadata
>       ## Commit message ##
>          sequencer: update `total_nr' when adding an item to a todo list
>
>     -    `total_nr' is the total amount of items, done and toto, that are in a
>     -    todo list.  But unlike `nr', it was not updated when an item was
>     -    appended to the list.
>     +    `total_nr' is the total number of items, counting both done and todo,
>     +    that are in a todo list.  But unlike `nr', it was not updated when an
>     +    item was appended to the list.
>
>          This variable is mostly used by command prompts (ie. git-prompt.sh and
>     -    the like).
>     +    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
>     +    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.
>
>          Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>
> 2:  09fcbe159b ! 2:  7cad541092 sequencer: update `done_nr' when skipping commands in a todo list
>     @@ Commit message
>          or skipped, but skip_unnecessary_picks() did not update it.
>
>          This variable is mostly used by command prompts (ie. git-prompt.sh and
>     -    the like).
>     +    the like).  As in the previous commit, this inconsistent behaviour is
>     +    not a problem yet, but it would start to matter at the end of this
>     +    series the same reason.
>
>          Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>
> 3:  26a18cd1a9 = 3:  7c9c4ddd30 sequencer: move the code writing total_nr on the disk to a new function
> 4:  5d74903cfe ! 4:  cd44fb4e10 rebase: fill `squash_onto' in get_replay_opts()
>     @@ Metadata
>       ## Commit message ##
>          rebase: fill `squash_onto' in get_replay_opts()
>
>     -    get_replay_opts() did not fill `squash_onto' if possible, meaning that
>     -    this field should be read from the disk by the sequencer through
>     -    read_populate_opts().  Without this, calling `pick_commits()' directly
>     -    will result in incorrect results with `rebase --root'.
>     +    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.
>
>          Let’s change that.
>
> 5:  dc803c671f ! 5:  523fdd35a1 sequencer: directly call pick_commits() from complete_action()
>     @@ Commit message
>          sequencer: directly call pick_commits() from complete_action()
>
>          Currently, complete_action() calls sequencer_continue() to do the
>     -    rebase.  Even though the former already has the todo list, the latter
>     -    loads it from the disk and parses it.  Calling directly pick_commits()
>     -    from complete_action() avoids this unnecessary round trip.
>     +    rebase.  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()
>     +     - calls read_populate_opts() -- this is unnecessary as we're starting a
>     +       new rebase, so opts is fully populated
>     +     - loads the todo list -- this is unnecessary as we've just populated
>     +       the todo list
>     +     - 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
>     +       a new rebase.
>     +
>     +    This changes complete_action() to directly call pick_commits() to avoid
>     +    these unnecessary steps.
>
>          Signed-off-by: Alban Gruin <alban.gruin@gmail.com>

This range-diff looks good to me!

I just verified that b2 addresses all the concerns I offered for v1.

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
  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  2:38   ` Junio C Hamano
@ 2019-10-26  7:47   ` René Scharfe
  2019-10-26 11:27     ` Alban Gruin
  2 siblings, 1 reply; 50+ messages in thread
From: René Scharfe @ 2019-10-26  7:47 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano

Am 25.09.19 um 22:13 schrieb Alban Gruin:
> Currently, complete_action() calls sequencer_continue() to do the
> rebase.  Even though the former already has the todo list, the latter
> loads it from the disk and parses it.  Calling directly pick_commits()
> from complete_action() avoids this unnecessary round trip.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  sequencer.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index ec7ea8d9e5..b395dd6e11 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>  		return error_errno(_("could not write '%s'"), todo_file);
>  	}
>
> -	todo_list_release(&new_todo);
> -

Moving this call down leaks new_todo on error...

>  	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
>  		return -1;

... here ...

>
>  	if (require_clean_work_tree(r, "rebase", "", 1, 1))
>  		return -1;
... and here.  Perhaps set res to -1 and jump to the end?

>
> -	return sequencer_continue(r, opts);
> +	todo_list_write_total_nr(&new_todo);
> +	res = pick_commits(r, &new_todo, opts);
> +	todo_list_release(&new_todo);
> +
> +	return res;
>  }
>
>  struct subject2item_entry {
>


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
  2019-10-26  7:47   ` René Scharfe
@ 2019-10-26 11:27     ` Alban Gruin
  2019-10-28  1:39       ` Junio C Hamano
  0 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2019-10-26 11:27 UTC (permalink / raw)
  To: René Scharfe, git, Junio C Hamano; +Cc: Johannes Schindelin, Phillip Wood

Hi René,

Le 26/10/2019 à 09:47, René Scharfe a écrit :
> Am 25.09.19 um 22:13 schrieb Alban Gruin:
>> Currently, complete_action() calls sequencer_continue() to do the
>> rebase.  Even though the former already has the todo list, the latter
>> loads it from the disk and parses it.  Calling directly pick_commits()
>> from complete_action() avoids this unnecessary round trip.
>>
>> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>> ---
>>  sequencer.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index ec7ea8d9e5..b395dd6e11 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -5140,15 +5140,17 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>>  		return error_errno(_("could not write '%s'"), todo_file);
>>  	}
>>
>> -	todo_list_release(&new_todo);
>> -
> 
> Moving this call down leaks new_todo on error...
> 
>>  	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
>>  		return -1;
> 
> ... here ...
> 
>>
>>  	if (require_clean_work_tree(r, "rebase", "", 1, 1))
>>  		return -1;
> ... and here.  Perhaps set res to -1 and jump to the end?
> 
>>
>> -	return sequencer_continue(r, opts);
>> +	todo_list_write_total_nr(&new_todo);
>> +	res = pick_commits(r, &new_todo, opts);
>> +	todo_list_release(&new_todo);
>> +
>> +	return res;
>>  }
>>
>>  struct subject2item_entry {
>>
> 

Thank you for the heads up.

Junio, how do you want me to fix that?  Should I reroll the series
altogether, send a "fixup!" commit, or send a standalone patch?

Cheers,
Alban


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
  2019-10-26 11:27     ` Alban Gruin
@ 2019-10-28  1:39       ` Junio C Hamano
  2019-10-28  3:20         ` Junio C Hamano
  0 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2019-10-28  1:39 UTC (permalink / raw)
  To: Alban Gruin; +Cc: René Scharfe, git, Johannes Schindelin, Phillip Wood

Alban Gruin <alban.gruin@gmail.com> writes:

> Junio, how do you want me to fix that?  Should I reroll the series
> altogether, send a "fixup!" commit, or send a standalone patch?

Normally a topic that is not yet in 'next' or more stable tracks can
and should be rerolled (or in a rare case like changing just a
single typo on a line in the last patch in a series, can be
corrected with a "fixup!" squashed in); once the topic is in 'next'
or more stable integration branch, there needs a normal freestanding
patch that may say "earlier we did X, which is broken for such and
such reasons.  correct it this way."

But during a pre-release feature freeze period, the rules can be a
bit different ;-) Typically a topic that is not in 'master' that is
not a bugfix will never graduate from 'next' until the release
happens, and after the release, the tip of 'next' gets rebuilt from
the newly cut release, at which point a topic that wants a fresh
start (in order to avoid "oops, this was wrong, and here is a fix"
follow-up patch) can be granted one.

So I'd probably just send a "fixup!"to be queued on top of the
series to fix a leak in 'next' for now, remind the maintainer not to
merge it to 'master' before the release, and once the upcoming
release is made, send another reminder to the maintainer to squash
the "fixup!" in before rebuilding 'next', if I owned this series.

Thanks.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
  2019-10-28  1:39       ` Junio C Hamano
@ 2019-10-28  3:20         ` Junio C Hamano
  0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2019-10-28  3:20 UTC (permalink / raw)
  To: Alban Gruin; +Cc: René Scharfe, git, Johannes Schindelin, Phillip Wood

Junio C Hamano <gitster@pobox.com> writes:

> So I'd probably just send a "fixup!"to be queued on top of the
> series to fix a leak in 'next' for now, remind the maintainer not to
> merge it to 'master' before the release, and once the upcoming
> release is made, send another reminder to the maintainer to squash
> the "fixup!" in before rebuilding 'next', if I owned this series.

This is based on René's suggestion, but seeing the pre-context of
this fix, many of the error code path, after an error return from
edit_todo_list() has been handled, can follow the pattern of this
fix to jump to the cleanup label after losing their own calls to
todo_list_release().  An obvious advantage of doing so is that it
future-proofs the codepath---the todo-list structure may not stay to
be the only thing that holds resources that need cleaned up.

 sequencer.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index b395dd6e11..ec0b793fc5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5140,14 +5140,18 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		return error_errno(_("could not write '%s'"), todo_file);
 	}
 
+	res = -1;
+
 	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
-		return -1;
+		goto cleanup;
 
 	if (require_clean_work_tree(r, "rebase", "", 1, 1))
-		return -1;
+		goto cleanup;
 
 	todo_list_write_total_nr(&new_todo);
 	res = pick_commits(r, &new_todo, opts);
+
+cleanup:
 	todo_list_release(&new_todo);
 
 	return res;

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v3 0/5] Use complete_action's todo list to do the rebase
  2019-10-07  9:26 ` [PATCH v2 0/5] Use complete_action's " Alban Gruin
                     ` (6 preceding siblings ...)
  2019-10-14 12:49   ` Johannes Schindelin
@ 2019-11-23 14:37   ` Alban Gruin
  2019-11-23 14:37     ` [PATCH v3 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
                       ` (5 more replies)
  7 siblings, 6 replies; 50+ messages in thread
From: Alban Gruin @ 2019-11-23 14:37 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Jonathan Tan,
	Alban Gruin

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


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH v3 1/5] sequencer: update `total_nr' when adding an item to a todo list
  2019-11-23 14:37   ` [PATCH v3 " Alban Gruin
@ 2019-11-23 14:37     ` Alban Gruin
  2019-11-23 14:37     ` [PATCH v3 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
                       ` (4 subsequent siblings)
  5 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-11-23 14:37 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Jonathan Tan,
	Alban Gruin

`total_nr' is the total number of items, counting both done and todo,
that are in a todo list.  But unlike `nr', it was not updated when an
item was appended to the list.

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

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index d648aaf416..575b852a5a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list)
 static struct todo_item *append_new_todo(struct todo_list *todo_list)
 {
 	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
+	todo_list->total_nr++;
 	return todo_list->items + todo_list->nr++;
 }
 
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v3 2/5] sequencer: update `done_nr' when skipping commands in a todo list
  2019-11-23 14:37   ` [PATCH v3 " Alban Gruin
  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     ` 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
                       ` (3 subsequent siblings)
  5 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-11-23 14:37 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Jonathan Tan,
	Alban Gruin

In a todo list, `done_nr' is the amount of commands that were executed
or skipped, but skip_unnecessary_picks() did not update it.

This variable is mostly used by command prompts (ie. git-prompt.sh and
the like).  As in the previous commit, this inconsistent behaviour is
not a problem yet, but it would start to matter at the end of this
series the same reason.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index 575b852a5a..42313f8de6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5054,6 +5054,7 @@ static int skip_unnecessary_picks(struct repository *r,
 		MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i);
 		todo_list->nr -= i;
 		todo_list->current = 0;
+		todo_list->done_nr += i;
 
 		if (is_fixup(peek_command(todo_list, 0)))
 			record_in_rewritten(base_oid, peek_command(todo_list, 0));
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v3 3/5] sequencer: move the code writing total_nr on the disk to a new function
  2019-11-23 14:37   ` [PATCH v3 " Alban Gruin
  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     ` Alban Gruin
  2019-11-23 14:37     ` [PATCH v3 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
                       ` (2 subsequent siblings)
  5 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-11-23 14:37 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Jonathan Tan,
	Alban Gruin

The total amount of commands can be used to show the progression of the
rebasing in a shell.  It is written to the disk by read_populate_todo()
when the todo list is loaded from sequencer_continue() or
pick_commits(), but not by complete_action().

This moves the part writing total_nr to a new function so it can be
called from complete_action().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 42313f8de6..ec7ea8d9e5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2342,6 +2342,16 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
 	sequencer_remove_state(&opts);
 }
 
+static void todo_list_write_total_nr(struct todo_list *todo_list)
+{
+	FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
+
+	if (f) {
+		fprintf(f, "%d\n", todo_list->total_nr);
+		fclose(f);
+	}
+}
+
 static int read_populate_todo(struct repository *r,
 			      struct todo_list *todo_list,
 			      struct replay_opts *opts)
@@ -2387,7 +2397,6 @@ static int read_populate_todo(struct repository *r,
 
 	if (is_rebase_i(opts)) {
 		struct todo_list done = TODO_LIST_INIT;
-		FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
 
 		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
 		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
@@ -2399,10 +2408,7 @@ static int read_populate_todo(struct repository *r,
 			+ count_commands(todo_list);
 		todo_list_release(&done);
 
-		if (f) {
-			fprintf(f, "%d\n", todo_list->total_nr);
-			fclose(f);
-		}
+		todo_list_write_total_nr(todo_list);
 	}
 
 	return 0;
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v3 4/5] rebase: fill `squash_onto' in get_replay_opts()
  2019-11-23 14:37   ` [PATCH v3 " Alban Gruin
                       ` (2 preceding siblings ...)
  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     ` 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
  5 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-11-23 14:37 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Jonathan Tan,
	Alban Gruin

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.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e8319d5946..2097d41edc 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	if (opts->strategy_opts)
 		parse_strategy_opts(&replay, opts->strategy_opts);
 
+	if (opts->squash_onto) {
+		oidcpy(&replay.squash_onto, opts->squash_onto);
+		replay.have_squash_onto = 1;
+	}
+
 	return replay;
 }
 
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v3 5/5] sequencer: directly call pick_commits() from complete_action()
  2019-11-23 14:37   ` [PATCH v3 " Alban Gruin
                       ` (3 preceding siblings ...)
  2019-11-23 14:37     ` [PATCH v3 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
@ 2019-11-23 14:37     ` Alban Gruin
  2019-11-24 17:43     ` [PATCH v4 0/5] Use complete_action's todo list to do the rebase Alban Gruin
  5 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-11-23 14:37 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Jonathan Tan,
	Alban Gruin

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() in complete_action()
 - calls read_populate_opts() -- this is unnecessary as we're starting a
   new rebase, so `opts' is fully populated
 - loads the todo list -- this is unnecessary as we've just populated
   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
   a new rebase.

This changes complete_action() to directly call pick_commits() to avoid
these unnecessary steps.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ec7ea8d9e5..ec0b793fc5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5140,15 +5140,21 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		return error_errno(_("could not write '%s'"), todo_file);
 	}
 
-	todo_list_release(&new_todo);
+	res = -1;
 
 	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
-		return -1;
+		goto cleanup;
 
 	if (require_clean_work_tree(r, "rebase", "", 1, 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;
 }
 
 struct subject2item_entry {
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v4 0/5] Use complete_action's todo list to do the rebase
  2019-11-23 14:37   ` [PATCH v3 " Alban Gruin
                       ` (4 preceding siblings ...)
  2019-11-23 14:37     ` [PATCH v3 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
@ 2019-11-24 17:43     ` 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
                         ` (4 more replies)
  5 siblings, 5 replies; 50+ messages in thread
From: Alban Gruin @ 2019-11-24 17:43 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Jonathan Tan,
	Alban Gruin

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 v3:

 - s/amount/number/ on patches 2 and 3, according to a comment from
   Junio[2] that I had forgotten before I sent the v3 X-(

The tip of this series is tagged as reduce-todo-list-cont-v4 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/
[2] http://public-inbox.org/git/xmqqmuecnefe.fsf@gitster-ct.c.googlers.com/

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 v3 :
1:  11a221e82e = 1:  11a221e82e sequencer: update `total_nr' when adding an item to a todo list
2:  76a3af70b6 ! 2:  6b402a3070 sequencer: update `done_nr' when skipping commands in a todo list
    @@ Metadata
      ## Commit message ##
         sequencer: update `done_nr' when skipping commands in a todo list
     
    -    In a todo list, `done_nr' is the amount of commands that were executed
    +    In a todo list, `done_nr' is the number of commands that were executed
         or skipped, but skip_unnecessary_picks() did not update it.
     
         This variable is mostly used by command prompts (ie. git-prompt.sh and
3:  9c5bd30465 ! 3:  0171db4fba sequencer: move the code writing total_nr on the disk to a new function
    @@ Metadata
      ## Commit message ##
         sequencer: move the code writing total_nr on the disk to a new function
     
    -    The total amount of commands can be used to show the progression of the
    +    The total number of commands can be used to show the progression of the
         rebasing in a shell.  It is written to the disk by read_populate_todo()
         when the todo list is loaded from sequencer_continue() or
         pick_commits(), but not by complete_action().
4:  bc3d69a10e = 4:  88f6335c37 rebase: fill `squash_onto' in get_replay_opts()
5:  e7691db66b = 5:  53586b1bed sequencer: directly call pick_commits() from complete_action()
-- 
2.24.0


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH v4 1/5] sequencer: update `total_nr' when adding an item to a todo list
  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       ` Alban Gruin
  2019-11-24 17:43       ` [PATCH v4 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
                         ` (3 subsequent siblings)
  4 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-11-24 17:43 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Jonathan Tan,
	Alban Gruin

`total_nr' is the total number of items, counting both done and todo,
that are in a todo list.  But unlike `nr', it was not updated when an
item was appended to the list.

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

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index d648aaf416..575b852a5a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2070,6 +2070,7 @@ void todo_list_release(struct todo_list *todo_list)
 static struct todo_item *append_new_todo(struct todo_list *todo_list)
 {
 	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
+	todo_list->total_nr++;
 	return todo_list->items + todo_list->nr++;
 }
 
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v4 2/5] sequencer: update `done_nr' when skipping commands in a todo list
  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       ` 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
                         ` (2 subsequent siblings)
  4 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-11-24 17:43 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Jonathan Tan,
	Alban Gruin

In a todo list, `done_nr' is the number of commands that were executed
or skipped, but skip_unnecessary_picks() did not update it.

This variable is mostly used by command prompts (ie. git-prompt.sh and
the like).  As in the previous commit, this inconsistent behaviour is
not a problem yet, but it would start to matter at the end of this
series the same reason.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index 575b852a5a..42313f8de6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5054,6 +5054,7 @@ static int skip_unnecessary_picks(struct repository *r,
 		MOVE_ARRAY(todo_list->items, todo_list->items + i, todo_list->nr - i);
 		todo_list->nr -= i;
 		todo_list->current = 0;
+		todo_list->done_nr += i;
 
 		if (is_fixup(peek_command(todo_list, 0)))
 			record_in_rewritten(base_oid, peek_command(todo_list, 0));
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v4 3/5] sequencer: move the code writing total_nr on the disk to a new function
  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       ` Alban Gruin
  2019-11-24 17:43       ` [PATCH v4 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
  2019-11-24 17:43       ` [PATCH v4 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
  4 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-11-24 17:43 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Jonathan Tan,
	Alban Gruin

The total number of commands can be used to show the progression of the
rebasing in a shell.  It is written to the disk by read_populate_todo()
when the todo list is loaded from sequencer_continue() or
pick_commits(), but not by complete_action().

This moves the part writing total_nr to a new function so it can be
called from complete_action().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 42313f8de6..ec7ea8d9e5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2342,6 +2342,16 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
 	sequencer_remove_state(&opts);
 }
 
+static void todo_list_write_total_nr(struct todo_list *todo_list)
+{
+	FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
+
+	if (f) {
+		fprintf(f, "%d\n", todo_list->total_nr);
+		fclose(f);
+	}
+}
+
 static int read_populate_todo(struct repository *r,
 			      struct todo_list *todo_list,
 			      struct replay_opts *opts)
@@ -2387,7 +2397,6 @@ static int read_populate_todo(struct repository *r,
 
 	if (is_rebase_i(opts)) {
 		struct todo_list done = TODO_LIST_INIT;
-		FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
 
 		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
 		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
@@ -2399,10 +2408,7 @@ static int read_populate_todo(struct repository *r,
 			+ count_commands(todo_list);
 		todo_list_release(&done);
 
-		if (f) {
-			fprintf(f, "%d\n", todo_list->total_nr);
-			fclose(f);
-		}
+		todo_list_write_total_nr(todo_list);
 	}
 
 	return 0;
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v4 4/5] rebase: fill `squash_onto' in get_replay_opts()
  2019-11-24 17:43     ` [PATCH v4 0/5] Use complete_action's todo list to do the rebase Alban Gruin
                         ` (2 preceding siblings ...)
  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       ` 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
  4 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2019-11-24 17:43 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Jonathan Tan,
	Alban Gruin

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.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/rebase.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e8319d5946..2097d41edc 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	if (opts->strategy_opts)
 		parse_strategy_opts(&replay, opts->strategy_opts);
 
+	if (opts->squash_onto) {
+		oidcpy(&replay.squash_onto, opts->squash_onto);
+		replay.have_squash_onto = 1;
+	}
+
 	return replay;
 }
 
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v4 5/5] sequencer: directly call pick_commits() from complete_action()
  2019-11-24 17:43     ` [PATCH v4 0/5] Use complete_action's todo list to do the rebase Alban Gruin
                         ` (3 preceding siblings ...)
  2019-11-24 17:43       ` [PATCH v4 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
@ 2019-11-24 17:43       ` Alban Gruin
  2019-11-26 18:41         ` Jonathan Tan
  4 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2019-11-24 17:43 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Jonathan Tan,
	Alban Gruin

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() in complete_action()
 - calls read_populate_opts() -- this is unnecessary as we're starting a
   new rebase, so `opts' is fully populated
 - loads the todo list -- this is unnecessary as we've just populated
   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
   a new rebase.

This changes complete_action() to directly call pick_commits() to avoid
these unnecessary steps.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ec7ea8d9e5..ec0b793fc5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5140,15 +5140,21 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		return error_errno(_("could not write '%s'"), todo_file);
 	}
 
-	todo_list_release(&new_todo);
+	res = -1;
 
 	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
-		return -1;
+		goto cleanup;
 
 	if (require_clean_work_tree(r, "rebase", "", 1, 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;
 }
 
 struct subject2item_entry {
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* Re: [PATCH v4 4/5] rebase: fill `squash_onto' in get_replay_opts()
  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
  0 siblings, 0 replies; 50+ messages in thread
From: Jonathan Tan @ 2019-11-26 18:27 UTC (permalink / raw)
  To: alban.gruin
  Cc: git, Johannes.Schindelin, phillip.wood, gitster, jonathantanmy

Thanks for the better commit message! Just one note:

> When sequencer_continue() is called by complete_action(), `opts' has
> been filled by get_replay_opts().

I searched for "get_replay_opts" in complete_action() but couldn't find
it, and had to do some searching to realize that what you mean is that
whenever complete_action() is called by do_interactive_rebase() in
builtin/rebase.c (its only caller), the "opts" argument was filled by
get_replay_opts(). Maybe reword as:

  When do_interactive_rebase() in builtin/rebase.c calls
  complete_action(), it passes an "opts" argument generated by
  get_replay_opts(). complete_action() then passes it to
  sequencer_continue().

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

The rest is clear; thanks. I would like to make it explicit that
pick_commits() uses "squash_onto", and make the antecedents of all the
"it"s clear, so I would write it like this:

  Currently, get_replay_opts() does not initialize the "squash_onto"
  field (used by the "--root" mode); only read_populate_opts() does.
  This field is used by pick_commits(), called by sequencer_continue().
  It's not a problem yet since sequencer_continue() currently calls
  read_populate_opts() before pick_commits(), but it would lead to
  incorrect results once complete_action() is modified to call
  pick_commits() directly (bypassing sequencer_continue() and, hence,
  read_populate_opts()).

  Let's change that.

Also, I think it's better to use the regular quote ' instead of the
smart quote ’.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v4 5/5] sequencer: directly call pick_commits() from complete_action()
  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
  0 siblings, 1 reply; 50+ messages in thread
From: Jonathan Tan @ 2019-11-26 18:41 UTC (permalink / raw)
  To: alban.gruin
  Cc: git, Johannes.Schindelin, phillip.wood, gitster, jonathantanmy

> 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() in complete_action()

require_clean_work_tree() and read_and_refresh_cache() seem to be
different functions - can you explain why running the former is a good
substitute for running the latter?

>  - calls read_populate_opts() -- this is unnecessary as we're starting a
>    new rebase, so `opts' is fully populated

My comment from [1] has not been addressed. Quoting it here:

> So complete_action() (the function modified in this commit) is called
> only by do_interactive_rebase() (in builtin/rebase.c), which is only
> called by run_rebase_interactive() (in builtin/rebase.c) when command is
> ACTION_NONE, so indeed, we're starting a new rebase. But where the
> options fully populated? I see that in do_interactive_rebase(), it is
> initialized with get_replay_opts(), but that seems different from
> read_populate_opts().

[1] https://lore.kernel.org/git/20191119204146.168001-1-jonathantanmy@google.com/

>  - loads the todo list -- this is unnecessary as we've just populated
>    the todo list in complete_action()

Both functions indeed have their own todo lists that they pass to
pick_commits(), but I don't see (at least, by glancing at the code) that
both these todo lists are identical.

>  - 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
>    a new rebase.

OK - I don't know enough about the rebase mechanism to verify these, but
these seem reasonable to me.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v4 5/5] sequencer: directly call pick_commits() from complete_action()
  2019-11-26 18:41         ` Jonathan Tan
@ 2019-11-27 19:56           ` Alban Gruin
  2019-11-27 20:09             ` Alban Gruin
  0 siblings, 1 reply; 50+ messages in thread
From: Alban Gruin @ 2019-11-27 19:56 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, Johannes.Schindelin, phillip.wood, gitster

Hi Jonathan,

Le 26/11/2019 à 19:41, Jonathan Tan a écrit :
>> 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() in complete_action()
> 
> require_clean_work_tree() and read_and_refresh_cache() seem to be
> different functions - can you explain why running the former is a good
> substitute for running the latter?
> 

They both refresh the index.

require_clean_work_tree(), called when starting a new rebase, will also
check if there are any unstaged or uncommitted changes, in which case we
do not want to start a rebase.

This is not what we want when resuming a rebase (with `rebase
--continue'), because the changes might be the result of a conflict
resolution.  In this case, the last commit is amended, and the rebase is
resumed.

>>  - calls read_populate_opts() -- this is unnecessary as we're starting a
>>    new rebase, so `opts' is fully populated
> 
> My comment from [1] has not been addressed. Quoting it here:
> 
>> So complete_action() (the function modified in this commit) is called
>> only by do_interactive_rebase() (in builtin/rebase.c), which is only
>> called by run_rebase_interactive() (in builtin/rebase.c) when command is
>> ACTION_NONE, so indeed, we're starting a new rebase. But where the
>> options fully populated? I see that in do_interactive_rebase(), it is
>> initialized with get_replay_opts(), but that seems different from
>> read_populate_opts().
> 
> [1] https://lore.kernel.org/git/20191119204146.168001-1-jonathantanmy@google.com/
> 

Sorry.

For the first part of your comment, I added a comment at the beginning
of the message, although I did _not_ include an analysis on when
complete_action() is used.

get_replay_opts() converts a `struct rebase_options' (which contains the
arguments passed to `git rebase') into a `struct replay_opts' which can
be used by the sequencer, whereas read_populate_opts() loads the options
from the disk.

So, when are they written to the disk?  In do_interactive_rebase()
(builtin/rebase.c), after using get_replay_opts() to convert `opts' to
`replay', init_basic_state() is called, which calls write_basic_state(),
which write the options to the disk.

Then, until complete_action() is called, `opts' is not modified.

>>  - loads the todo list -- this is unnecessary as we've just populated
>>    the todo list in complete_action()
> 
> Both functions indeed have their own todo lists that they pass to
> pick_commits(), but I don't see (at least, by glancing at the code) that
> both these todo lists are identical.
> 

Near the end of complete_action(), the todo list is written to the disk.
 The destination is obtained with rebase_path_todo().

read_populate_todo() will read a file and parse it.  In the case of
`rebase -i', the location is obtained with rebase_path_todo(), and only
`total_nr' will be modified to contain the number of commands done and todo.

In the case of a new rebase, the done list might not be empty after
tajjimh skip_unnecessary_picks() from complete_action().  Skipped
commands are moved from the todo list to the done list.  As `total_nr'
is not changed by skip_unnecessary_picks(), it is also equal to the
number of commands remaining in the todo list and in the done list.  So,
when read_populate_todo() reads the list and the done list from the
disk, as they should not have been modified, `total_nr' should remain
the same, too.

The only thing that can change is the internal buffer (`buf'), because
skip_unnecessary_picks() don’t change it.  Since
ag/sequencer-reduce-rewriting-todo, it is no longer a textual
representation of the todo list.  Each command contains a pointer to a
location in the buffer and a length to represent its argument.

>>  - 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
>>    a new rebase.
> 
> OK - I don't know enough about the rebase mechanism to verify these, but
> these seem reasonable to me.
> 

Cheers,
Alban


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v4 5/5] sequencer: directly call pick_commits() from complete_action()
  2019-11-27 19:56           ` Alban Gruin
@ 2019-11-27 20:09             ` Alban Gruin
  0 siblings, 0 replies; 50+ messages in thread
From: Alban Gruin @ 2019-11-27 20:09 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, Johannes.Schindelin, phillip.wood, gitster

Le 27/11/2019 à 20:56, Alban Gruin a écrit :
> Near the end of complete_action(), the todo list is written to the disk.
>  The destination is obtained with rebase_path_todo().
> 
> read_populate_todo() will read a file and parse it.  In the case of
> `rebase -i', the location is obtained with rebase_path_todo(), and only
> `total_nr' will be modified to contain the number of commands done and todo.
> 
> In the case of a new rebase, the done list might not be empty after
> tajjimh skip_unnecessary_picks() from complete_action().

s/tajjimh/calling/


^ permalink raw reply	[flat|nested] 50+ messages in thread

end of thread, other threads:[~2019-11-27 20:09 UTC | newest]

Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [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   ` [PATCH v3 " Alban Gruin
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

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 NNTP newsgroup(s).