From: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Date: Thu, 01 Sep 2016 08:33:32 GMT
Subject: Re: [PATCH 32/34] sequencer (rebase -i): show the progress
Message-ID: <1472718812.4680.20.camel@kaarsemaker.net>
In-Reply-To: <3207457a57d056c3054faf718128fb5c806cb032.1472633606.git.johannes.schindelin@gmx.de>

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.

D.