Re: [PATCH] git-status: show short sequencer state
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Oct 23, 2012, 06:05 UTC
- Message-ID
- <vpqsj95soxp.fsf@grenoble-inp.fr>
- In-Reply-To
- <1350948569-28445-2-git-send-email-hordp@cisco.com>
Phil Hord <hordp@cisco.com> writes:
Show 6 quoted lines
> + 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 ==.
Show 5 quoted lines
> +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)
[...]
Show 5 quoted lines
> +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/