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

From: Guillaume Pages <guillaume.pages@ensimag.grenoble-inp.fr>
Date: Jun 9, 2015, 08:57 UTC
Message-ID: <1393139524.297448.1433840258348.JavaMail.zimbra@ensimag.grenoble-inp.fr>
In-Reply-To: <xmqq7frd97g7.fsf@gitster.dls.corp.google.com>

"Junio C Hamano" <gitster@pobox.com> writes:

>Guillaume Pagès <guillaume.pages@ensimag.grenoble-inp.fr> writes:

Show 5 quoted lines

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

Show 15 quoted lines

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

Show 19 quoted lines

>> + 
>> + 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.

>> + else{

>Style: "else {"

Ok thanks.

Show 9 quoted lines

>> + 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")?

Show 5 quoted lines

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

Show 14 quoted lines

>> + 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.