Re: [PATCH 3/4] status: give more information during rebase -i

Guillaume Pagès  [off-list ref] writes:

git status gives more information during rebase -i, about the list of
command that are done during the rebase. It displays the two last
commands executed and the two next lines to be executed. It also gives
hints to find the whole files in .git directory.
---

Without 1/4 and 2/4 in the same thread, it is hard to guess what you
wanted to do with these two patches.  Remember, reviewers review not
just your patches but those from many others.

I would in the meantime assume these are replacement patches for the
ones in

  http://thread.gmane.org/gmane.comp.version-control.git/270743/focus=270744

"lines_to_show = 2" or "nr_lines_to_show = 2" would be easier to read.

+
+		struct strbuf buf = STRBUF_INIT;
+		struct string_list have_done = STRING_LIST_INIT_DUP;
+		struct string_list yet_to_do = STRING_LIST_INIT_DUP;
+
+		strbuf_read_file(&buf, git_path("rebase-merge/done"), 0);
+		stripspace(&buf, 1);
+		have_done.nr = string_list_split(&have_done, buf.buf, '\n', -1);
+		string_list_remove_empty_items(&have_done, 1);
+		strbuf_release(&buf);
+
+		strbuf_read_file(&buf, git_path("rebase-merge/git-rebase-todo"), 0);
+		stripspace(&buf, 1);
+		string_list_split(&yet_to_do, buf.buf, '\n', -1);
+		string_list_remove_empty_items(&yet_to_do, 1);
+		strbuf_release(&buf);
+
+		if (have_done.nr == 0)
+			status_printf_ln(s, color, _("No commands done."));

Do the users even need to be told that, I wonder?

+		else{

Style:	"else {"

+			status_printf_ln(s, color,
+				Q_("Last command done (%d command done):",
+					"Last commands done (%d commands done):",
+					have_done.nr),
+				have_done.nr);
+			begin = (have_done.nr > lines_to_show_nr) ? have_done.nr-lines_to_show_nr : 0;
+			for (i = begin; i < have_done.nr; i++) {
+				status_printf_ln(s, color, "   %s", have_done.items[i].string);
+			}

Hmm, perhaps fold lines like this (and you do not need "begin")?

			for (i = (lines_to_show_nr < have_done.nr)
                             ? have_done.nr - lines_to_show_nr : 0;
                             i < have_done.nr;
                             i++)
				status_printf_ln(...);

+			if (have_done.nr > lines_to_show_nr && s->hints)
+			   status_printf_ln(s, color,
+				_("  (see more in file %s)"), git_path("rebase-merge/done"));

That's a nice touch ;-)

+		}
+		if (yet_to_do.nr == 0)
+			status_printf_ln(s, color,
+					 _("No commands remaining."));

This I can see why we may want to say it.

+		else{
+
+			status_printf_ln(s, color,
+				Q_("Next command to do (%d remaining command):",
+					"Next commands to do (%d remaining commands):",
+					yet_to_do.nr),
+				yet_to_do.nr);
+			for (i = 0; i < lines_to_show_nr && i < yet_to_do.nr; i++) {
+				status_printf_ln(s, color, "   %s", yet_to_do.items[i].string);
+			}
+			if (s->hints)
+			   status_printf_ln(s, color,
+				_("  (use \"git rebase --edit-todo\" to view and edit)"));
+		}

Make sure you do not leak memory used by two string lists here...

+	}
+}
+
 static void show_rebase_in_progress(struct wt_status *s,
 				struct wt_status_state *state,
 				const char *color)
 {
 	struct stat st;
 
+	show_rebase_information(s, state, color);
 	if (has_unmerged(s) || state->rebase_in_progress || !stat(git_path("MERGE_MSG"), &st)) {
 		if (state->branch)
 			status_printf_ln(s, color,

"Junio C Hamano" [off-list ref] writes:

Guillaume Pagès [off-list ref] writes: 

quoted

git status gives more information during rebase -i, about the list of 
command that are done during the rebase. It displays the two last 
commands executed and the two next lines to be executed. It also gives 
hints to find the whole files in .git directory. 
--- 

Without 1/4 and 2/4 in the same thread, it is hard to guess what you 
wanted to do with these two patches. Remember, reviewers review not 
just your patches but those from many others. 

I would in the meantime assume these are replacement patches for the 
ones in 

http://thread.gmane.org/gmane.comp.version-control.git/270743/focus=270744 

You assumed correctly, I didn't wanted to flood the mailing list and I
assumed it would be linked correctly with the previous since. Looks like
I was wrong.

quoted

diff --git a/wt-status.c b/wt-status.c 
index c83eca5..7f88470 100644 
--- a/wt-status.c 
+++ b/wt-status.c 

@@ -1026,12 +1026,73 @@ static int split_commit_in_progress(struct wt_status *s) 

return split_in_progress; 
} 

+static void show_rebase_information(struct wt_status *s, 
+ struct wt_status_state *state, 
+ const char *color) 
+{ 
+ if (state->rebase_interactive_in_progress) { 
+ int i, begin; 
+ int lines_to_show_nr = 2; 

"lines_to_show = 2" or "nr_lines_to_show = 2" would be easier to read. 

Done

quoted

+ 
+ struct strbuf buf = STRBUF_INIT; 
+ struct string_list have_done = STRING_LIST_INIT_DUP; 
+ struct string_list yet_to_do = STRING_LIST_INIT_DUP; 
+ 
+ strbuf_read_file(&buf, git_path("rebase-merge/done"), 0); 
+ stripspace(&buf, 1); 
+ have_done.nr = string_list_split(&have_done, buf.buf, '\n', -1); 
+ string_list_remove_empty_items(&have_done, 1); 
+ strbuf_release(&buf); 
+ 
+ strbuf_read_file(&buf, git_path("rebase-merge/git-rebase-todo"), 0); 
+ stripspace(&buf, 1); 
+ string_list_split(&yet_to_do, buf.buf, '\n', -1); 
+ string_list_remove_empty_items(&yet_to_do, 1); 
+ strbuf_release(&buf); 
+ 
+ if (have_done.nr == 0) 
+ status_printf_ln(s, color, _("No commands done.")); 

Do the users even need to be told that, I wonder? 

I guess it removes the ambiguity of being told nothing.

quoted

+ else{ 

Style: "else {" 

Ok thanks.

quoted

+ status_printf_ln(s, color, 
+ Q_("Last command done (%d command done):", 
+ "Last commands done (%d commands done):", 
+ have_done.nr), 
+ have_done.nr); 
+ begin = (have_done.nr > lines_to_show_nr) ? have_done.nr-lines_to_show_nr : 0; 
+ for (i = begin; i < have_done.nr; i++) { 
+ status_printf_ln(s, color, " %s", have_done.items[i].string); 
+ } 

Hmm, perhaps fold lines like this (and you do not need "begin")? 

for (i = (lines_to_show_nr < have_done.nr) 
? have_done.nr - lines_to_show_nr : 0; 
i < have_done.nr; 
i++) 
status_printf_ln(...);

quoted

+ if (have_done.nr > lines_to_show_nr && s->hints) 
+ status_printf_ln(s, color, 
+ _(" (see more in file %s)"), git_path("rebase-merge/done")); 

That's a nice touch ;-) 

quoted

+ } 
+ if (yet_to_do.nr == 0) 
+ status_printf_ln(s, color, 
+ _("No commands remaining.")); 

This I can see why we may want to say it. 

quoted

+ else{ 
+ 
+ status_printf_ln(s, color, 
+ Q_("Next command to do (%d remaining command):", 
+ "Next commands to do (%d remaining commands):", 
+ yet_to_do.nr), 
+ yet_to_do.nr); 
+ for (i = 0; i < lines_to_show_nr && i < yet_to_do.nr; i++) { 
+ status_printf_ln(s, color, " %s", yet_to_do.items[i].string); 
+ } 
+ if (s->hints) 
+ status_printf_ln(s, color, 
+ _(" (use \"git rebase --edit-todo\" to view and edit)")); 
+ } 

Make sure you do not leak memory used by two string lists here... 

Done, thanks again.