Re: [RFC PATCH 5/9] sequencer: move the code writing total_nr on the disk to a new function

From: Alban Gruin <alban.gruin@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [RFC PATCH 5/9] sequencer: move the code writing total_nr on the disk to a new function
Date: Fri, 19 Jul 2019 20:14:02 +0200	[thread overview]
Message-ID: <6145932.27YfOFK3Rs@andromeda> (raw)
In-Reply-To: <xmqqk1cfxg6d.fsf@gitster-ct.c.googlers.com>

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;