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@xxxxxx>
> > ---
> > 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