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@xxxxxxxxxxxxxxxxxxxxxxx> writes: > +void get_two_last_lines(char *filename,int * numlines, char ** lines) Space after , > +{ > + *numlines=0; Spaces around = > + 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. > + (*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). > +{ > + *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? > +{ > + 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. > + 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 ,. > + "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. > @@ -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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html