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

[Johannes Schindelin]

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 
> > ---
> >  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(, rebase_path_done(), 0) > 0 &&
> > +   !parse_insn_buffer(done.buf.buf, ))
> > +   todo_list->done_nr = count_commands();
> > +   else
> > +   todo_list->done_nr = 0;
> > +
> > +   todo_list->total_nr = todo_list->done_nr
> > +   + count_commands(todo_list);
> > +
> > +   todo_list_release();
> > +   }
> > +
> >     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

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

[Dennis Kaarsemaker]

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 
> ---
>  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(, rebase_path_done(), 0) > 0 &&
> + !parse_insn_buffer(done.buf.buf, ))
> + todo_list->done_nr = count_commands();
> + else
> + todo_list->done_nr = 0;
> +
> + todo_list->total_nr = todo_list->done_nr
> + + count_commands(todo_list);
> +
> + todo_list_release();
> + }
> +
>   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.

D.