Re: [RFC/PATCH 1/2] status: give more information during rebase -i

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 3, 2015, 17:12 UTC
Message-ID: <vpqpp5cbtse.fsf@anie.imag.fr>
In-Reply-To: <1433320545-14244-1-git-send-email-guillaume.pages@ensimag.grenoble-inp.fr>

Hi,

Your code is not C90:

wt-status.c: In function ‘get_two_last_lines’:
wt-status.c:1030:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
  struct strbuf buf = STRBUF_INIT;
  ^
wt-status.c: In function ‘get_two_first_lines’:
wt-status.c:1050:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
  struct strbuf buf = STRBUF_INIT;
  ^

I have this to notice it:

$ cat config.mak CFLAGS += -Wdeclaration-after-statement -Wall -Werror #-O0 -g

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

> +void get_two_last_lines(char *filename,int * numlines, char ** lines)

Space after ,

> +{
> +	*numlines=0;

Spaces around =

Show 7 quoted lines

> +	struct strbuf buf = STRBUF_INIT;
> +	FILE *fp = fopen(git_path("%s", filename), "r");
> +	if (!fp) {
> +		strbuf_release(&buf);
> +		return;
> +	}
> +	while (strbuf_getline(&buf, fp, '\n')!=EOF){

Spaces around !=

There are many other space issues, I won't list them all.

Show 11 quoted lines

> +		(*numlines)++;
> +		lines[0]=lines[1];
> +		lines[1]=strbuf_detach(&buf, NULL);
> +	}
> +	if (!fclose(fp))
> +		strbuf_detach(&buf, NULL);
> +	else
> +		strbuf_release(&buf);
> +}
> +
> +void get_two_first_lines(char *filename,int * numlines, char ** lines)

Stick the * to the corresponding variable.

Do: int *numlines
Don't: int * numlines, int* numlines

One rationale is that when you write

int *i, j;

it reads "*i is an int, and so is j" (which is right), while

int * i, j;

may read as "both i and j are int *" (which is not right).

Show 27 quoted lines

> +{
> +	*numlines=0;
> +	struct strbuf buf = STRBUF_INIT;
> +	char *line;
> +	FILE *fp = fopen(git_path("%s", filename), "r");
> +	if (!fp) {
> +		strbuf_release(&buf);
> +		return;
> +	}
> +	while (strbuf_getline(&buf, fp, '\n')!=EOF){
> +		stripspace(&buf, 1);
> +		line = strbuf_detach(&buf, NULL);
> +		if (strcmp(line,"")==0)
> +			continue;
> +		if (*numlines<2)
> +			lines[*numlines] = line;
> +		(*numlines)++;
> +	}
> +	if (!fclose(fp))
> +		strbuf_detach(&buf, NULL);
> +	else
> +		strbuf_release(&buf);
> +}
> +
> +void show_rebase_information(struct wt_status *s,
> +				    struct wt_status_state *state,
> +				    const char *color)

static?

Show 9 quoted lines

> +{
> +	if (state->rebase_interactive_in_progress){
> +		int i, begin;
> +		int numlines =0;
> +		char* lines[2];
> +		get_two_last_lines("rebase-merge/done", &numlines, lines);
> +		if (numlines==0)
> +			status_printf_ln(s,color,"No command done.");
> +		else{

Space before {. Several instances too.

Show 16 quoted lines

> +			status_printf_ln(s,color,
> +				"Last command(s) done (%d command(s) done):",
> +				numlines);
> +			begin = numlines > 1? 0 : 1;
> +			for (i = begin; i < 2; i++){
> +				status_printf_ln(s,color,"   %s",lines[i]);
> +			}
> +			if (numlines>2 && s->hints )
> +			   status_printf_ln(s,color,
> +				"  (see more at .git/rebase-merge/done)");
> +		}
> +		numlines =0;
> +		get_two_first_lines("rebase-merge/git-rebase-todo",
> +					 &numlines, lines);
> +		if (numlines==0)
> +			status_printf_ln(s,color,

Space after ,.

Show 11 quoted lines

> +					 "No command remaining.");
> +		else{
> +
> +			status_printf_ln(s,color,
> +				"Next command(s) to do (%d remaining command(s)):",
> +				numlines);
> +			begin = numlines > 1? 0 : 1;
> +			for (i = 0; (i < 2 && i < numlines); i++){
> +				status_printf(s,color,"   %s",lines[i]);
> +			}
> +			if (s->hints )

No space before ).

> +			   status_printf_ln(s,color,
> +				"  (use git rebase --edit-todo to view and edit)");

Mark for translation -> _("..."). Many instances above.

> -	if (has_unmerged(s)) {
> +	show_rebase_information(s,state,color);
> +	if (has_unmerged(s) || state->rebase_in_progress || !stat(git_path("MERGE_MSG"), &st)) {

The show_rebase_information() line does belong to this patch, but the other change do not IHMO. It would be _much_ easier to review in its own patch with an explicit title like "status: factor two rebase-related messages together" or so.

Show 9 quoted lines

> @@ -1327,7 +1410,10 @@ void wt_status_print(struct wt_status *s)
>  		else if (!strcmp(branch_name, "HEAD")) {
>  			branch_status_color = color(WT_STATUS_NOBRANCH, s);
>  			if (state.rebase_in_progress || state.rebase_interactive_in_progress) {
> -				on_what = _("rebase in progress; onto ");
> +				if (state.rebase_interactive_in_progress)
> +					on_what = _("interactive rebase in progress; onto ");
> +				else
> +					on_what = _("rebase in progress; onto ");

I wouldn't insist on that, but splitting this one in a separate patch would make sense to me too. The patch would contain this change, and the impact on tests.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/