[RFC,5/9] sequencer: move the code writing total_nr on the disk to a new function
diff mbox series

Message ID 20190717143918.7406-6-alban.gruin@gmail.com
State New
Headers show
Series
  • rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.
Related show

Commit Message

Alban Gruin July 17, 2019, 2:39 p.m. UTC
The total amount of commands can be used to show the progression of the
rebasing in a shell.  This number 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(),
which releases its todo list before calling sequencer_continue(), which
reloads it immediatly afterwards.

To avoid to reload the todo list from the disk, sequencer_continue()
will be modified to accept a todo list, and if it is not null,
read_populate_todo() will not be called.

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

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

Comments

Junio C Hamano July 18, 2019, 8:04 p.m. UTC | #1
Alban Gruin <alban.gruin@gmail.com> writes:

> The total amount of commands can be used to show the progression of the
> rebasing in a shell.  This number 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(),
> which releases its todo list before calling sequencer_continue(), which
> reloads it immediatly afterwards.
>
> To avoid to reload the todo list from the disk, sequencer_continue()
> will be modified to accept a todo list, and if it is not null,
> read_populate_todo() will not be called.

That may be good as a plan, but readers who are reading this step
are left puzzled as the changes so far do not seem to have much to
do with that change.  Perhaps this paragraph belongs to the step
that actually makes that modification?

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

So without 3/9 and 4/9, we have been simply writing out wrong
number?  Good.

>
> 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 ec9c3d4dc5..d66b80d49f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2343,6 +2343,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)
> @@ -2388,7 +2398,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))
> @@ -2400,10 +2409,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;
Alban Gruin July 19, 2019, 6:14 p.m. UTC | #2
Hi,

Le 18/07/2019 à 22:04, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> > The total amount of commands can be used to show the progression of the
> > rebasing in a shell.  This number 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(),
> > which releases its todo list before calling sequencer_continue(), which
> > reloads it immediatly afterwards.
> > 
> > To avoid to reload the todo list from the disk, sequencer_continue()
> > will be modified to accept a todo list, and if it is not null,
> > read_populate_todo() will not be called.
> 
> That may be good as a plan, but readers who are reading this step
> are left puzzled as the changes so far do not seem to have much to
> do with that change.  Perhaps this paragraph belongs to the step
> that actually makes that modification?
> 
> > This moves the part writing total_nr to a new function so it can be
> > called by complete_action().
> 
> So without 3/9 and 4/9, we have been simply writing out wrong
> number?  Good.
> 

Not here, the numbers computed in read_populate_todo() are correct.  But they 
may be incorrect in complete_action(), before calling sequencer_continue().  
This was not a big deal as the todo list was released before and 
sequencer_continue() re-read it, but it would have became a problem with 6/9.

> > 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 ec9c3d4dc5..d66b80d49f 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2343,6 +2343,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)
> > 
> > @@ -2388,7 +2398,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))
> > 
> > @@ -2400,10 +2409,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;

Patch
diff mbox series

diff --git a/sequencer.c b/sequencer.c
index ec9c3d4dc5..d66b80d49f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2343,6 +2343,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)
@@ -2388,7 +2398,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))
@@ -2400,10 +2409,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;