* [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
* 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 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 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 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 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
* [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
* 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 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 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 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
* [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
* 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 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 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-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 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
* 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 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 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
* [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
* [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
* 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
* [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 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
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).