From: Matthieu Moy <Matthieu.Moy@grenoble-inp•fr>
To: Phil Hord <hordp@cisco•com>
Cc: phil.hord@gmail•com, Junio C Hamano <gitster@pobox•com>,
konglu@minatec•inpg.fr, Kong Lucien <Lucien.Kong@ensimag•imag.fr>,
git@vger•kernel.org,
Duperray Valentin <Valentin.Duperray@ensimag•imag.fr>,
Jonas Franck <Franck.Jonas@ensimag•imag.fr>,
Nguy Thomas <Thomas.Nguy@ensimag•imag.fr>,
Nguyen Huynh Khoi Nguyen
<Huynh-Khoi-Nguyen.Nguyen@ensimag•imag.fr>
Subject: Re: [PATCH] git-status: show short sequencer state
Date: Tue, 23 Oct 2012 08:05:54 +0200 [thread overview]
Message-ID: <vpqsj95soxp.fsf@grenoble-inp.fr> (raw)
In-Reply-To: <1350948569-28445-2-git-send-email-hordp@cisco.com> (Phil Hord's message of "Mon, 22 Oct 2012 19:29:29 -0400")
Phil Hord <hordp@cisco•com> 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/
next prev parent reply other threads:[~2012-10-23 6:06 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-22 23:29 [PATCH] git-status: show short sequencer state Phil Hord
2012-10-23 6:05 ` Matthieu Moy [this message]
2012-10-23 18:03 ` Phil Hord
2012-10-24 8:15 ` Matthieu Moy
-- strict thread matches above, loose matches on Subject: below --
2012-10-23 18:58 Phil Hord
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=vpqsj95soxp.fsf@grenoble-inp.fr \
--to=matthieu.moy@grenoble-inp$(echo .)fr \
--cc=Franck.Jonas@ensimag$(echo .)imag.fr \
--cc=Huynh-Khoi-Nguyen.Nguyen@ensimag$(echo .)imag.fr \
--cc=Lucien.Kong@ensimag$(echo .)imag.fr \
--cc=Thomas.Nguy@ensimag$(echo .)imag.fr \
--cc=Valentin.Duperray@ensimag$(echo .)imag.fr \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=hordp@cisco$(echo .)com \
--cc=konglu@minatec$(echo .)inpg.fr \
--cc=phil.hord@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox