From: Matthieu Moy Date: Tue, 23 Oct 2012 06:05:54 GMT Subject: Re: [PATCH] git-status: show short sequencer state Message-ID: In-Reply-To: <1350948569-28445-2-git-send-email-hordp@cisco.com> Phil Hord writes: > + merge a git-merge is in progress > + am a git-am is in progress > + rebase a git-rebase is in progress > + rebase-interactive a git-rebase--interactive is in progress > + cherry-pick a git-cherry-pick is in progress > + bisect a git-bisect is in progress Avoid using git-foo syntax in documentation, it suggests that this is valid command, which isn't true anymore. `git foo` seems the most common syntax. Also, git-rebase--interactive is not user-visible => `git rebase --interactive`. > - if (state->am_empty_patch) > + if (state->substate==WT_SUBSTATE_NOMINAL) > status_printf_ln(s, color, > _("The current patch is empty.")); This looks weird. First, spaces around == (here and below). Then, the logic is unintuitive. The "if" suggests everything is allright, and the message below is very specific. This at least deserves a comment. > if (advice_status_hints) { > - if (!state->am_empty_patch) > + if (state->substate==WT_SUBSTATE_CONFLICTED) Spaces around ==. > +static void wt_print_token(struct wt_status *s, const char *color, const char *token) > +{ > + color_fprintf(s->fp, color, "%s", token); > + fputc(s->null_termination ? '\0' : '\n', s->fp); > +} The output format seems to be meant only for machine-consumption. Is there any case when we'd want color? I'd say we can disable coloring completely for this format (normally, color=auto does the right thing, but I prefer being 100% sure I'll get no color when writing scripts) > +static void wt_shortstatus_print_sequencer(struct wt_status *s) [...] > +void wt_sequencer_print(struct wt_status *s) > +{ > + wt_shortstatus_print_sequencer(s); > +} > + Why do you need this trivial wrapper? Other than that, I like the idea (although I have no concrete use-case in mind), but I didn't actually test the patch. -- Matthieu Moy http://www-verimag.imag.fr/~moy/