Re: [PATCH 32/34] sequencer (rebase -i): show the progress

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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

Hi Dennis,

On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:

> On wo, 2016-08-31 at 10:56 +0200, Johannes Schindelin wrote:
> > The interactive rebase keeps the user informed about its progress.
> > If the sequencer wants to do the grunt work of the interactive
> > rebase, it also needs to show that progress.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  sequencer.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/sequencer.c b/sequencer.c
> > index 89fd625..e8c6daf 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -1218,6 +1218,7 @@ struct todo_list {
> >  	struct strbuf buf;
> >  	struct todo_item *items;
> >  	int nr, alloc, current;
> > +	int done_nr, total_nr;
> >  };
> >  
> >  #define TODO_LIST_INIT { STRBUF_INIT }
> > @@ -1329,6 +1330,17 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
> >  	return res;
> >  }
> >  
> > +static int count_commands(struct todo_list *todo_list)
> > +{
> > +	int count = 0, i;
> > +
> > +	for (i = 0; i < todo_list->nr; i++)
> > +		if (todo_list->items[i].command != TODO_COMMENT)
> > +			count++;
> > +
> > +	return count;
> > +}
> > +
> >  static int read_populate_todo(struct todo_list *todo_list,
> >  			struct replay_opts *opts)
> >  {
> > @@ -1355,6 +1367,22 @@ static int read_populate_todo(struct todo_list *todo_list,
> >  	if (!todo_list->nr &&
> >  	    (!is_rebase_i(opts) || !file_exists(rebase_path_done())))
> >  		return error(_("No commits parsed."));
> > +
> > +	if (is_rebase_i(opts)) {
> > +		struct todo_list done = TODO_LIST_INIT;
> > +
> > +		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
> > +				!parse_insn_buffer(done.buf.buf, &done))
> > +			todo_list->done_nr = count_commands(&done);
> > +		else
> > +			todo_list->done_nr = 0;
> > +
> > +		todo_list->total_nr = todo_list->done_nr
> > +			+ count_commands(todo_list);
> > +
> > +		todo_list_release(&done);
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -1900,6 +1928,11 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
> >  		if (save_todo(todo_list, opts))
> >  			return -1;
> >  		if (is_rebase_i(opts)) {
> > +			if (item->command != TODO_COMMENT)
> > +				fprintf(stderr, "Rebasing (%d/%d)%s",
> > +					++(todo_list->done_nr),
> > +					todo_list->total_nr,
> > +					opts->verbose ? "\n" : "\r");
> >  			unlink(rebase_path_message());
> >  			unlink(rebase_path_author_script());
> >  			unlink(rebase_path_stopped_sha());
> 
> (picking a random commit that shows this 'symptom')
> 
> You're sprinking a lot of is_rebase_i's around sequencer.c to make sure
> there are no changes in behaviour. I wonder if the right balance has
> been struck between 'no changes in behaviour' and 'common behaviour'.
> For instance, in this case, maybe it would be a better idea for non-
> rebase uses of the sequencer to also show progress.

Actually, adding progress would make for a fine add-on patch series. I
still would like to have as faithful a conversion as possible, and
everything else can come after that.

This strategy has a couple of advantages:

- we can concentrate on correctness for now,

- I get something to show for my time with Git for Windows v2.10.0, and

- the add-on patches do not have to be done by *me* ;-)

Ciao,
Dscho