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

Hi Dennis,

On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:

On wo, 2016-08-31 at 10:56 +0200, Johannes Schindelin wrote:

quoted

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 <redacted>
---
 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