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

[Junio C Hamano <gitster@pobox.com>]

Guillaume Pagès  <guillaume.pages@ensimag.grenoble-inp.fr> 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

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

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