git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / 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; 30+ 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	[flat|nested] 30+ 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; 30+ 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	[flat|nested] 30+ 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; 30+ 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	[flat|nested] 30+ 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; 30+ 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	[flat|nested] 30+ 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
  2019-10-02  2:38   ` Junio C Hamano
  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, 2 replies; 30+ 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	[flat|nested] 30+ 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; 30+ 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] 30+ 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
  1 sibling, 1 reply; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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
  1 sibling, 1 reply; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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
                     ` (6 more replies)
  7 siblings, 7 replies; 30+ 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] 30+ 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
                     ` (5 subsequent siblings)
  6 siblings, 0 replies; 30+ 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	[flat|nested] 30+ 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
                     ` (4 subsequent siblings)
  6 siblings, 0 replies; 30+ 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	[flat|nested] 30+ 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
                     ` (3 subsequent siblings)
  6 siblings, 0 replies; 30+ 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	[flat|nested] 30+ 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
                     ` (2 subsequent siblings)
  6 siblings, 0 replies; 30+ 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	[flat|nested] 30+ 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
  2019-10-14 12:49   ` Johannes Schindelin
  6 siblings, 0 replies; 30+ 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	[flat|nested] 30+ 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
  6 siblings, 1 reply; 30+ 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] 30+ 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; 30+ 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] 30+ 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
  6 siblings, 0 replies; 30+ 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] 30+ messages in thread

end of thread, back to index

Thread overview: 30+ 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-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

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox