Re: [PATCH 2/2] Highlight keywords in remote sideband output.
On Thu, Aug 2, 2018 at 10:37 AM Eric Sunshine wrote: > > On Thu, Aug 2, 2018 at 7:46 AM Han-Wen Nienhuys wrote: > > Sure. My doubt is that it's hard to tell what the state of my patch is > > at any given time. > > Understandable. Junio sends out a periodic "What's cooking" email > summarizing the state of patches sent to the list. The most recent one > is here [1]. Please contrast that with [2], specifically: The discussion thread in the list archive is the authoritative record of the discussion; treat "What's cooking" as my personal note, nothing more. [2] https://public-inbox.org/git/xmqqsh3x69ap@gitster-ct.c.googlers.com/
Re: [PATCH 2/2] Highlight keywords in remote sideband output.
On Thu, Aug 2, 2018 at 7:46 AM Han-Wen Nienhuys wrote: > Sure. My doubt is that it's hard to tell what the state of my patch is > at any given time. Understandable. Junio sends out a periodic "What's cooking" email summarizing the state of patches sent to the list. The most recent one is here [1]. A patch series may be annotated with "will merge to next" (meaning he thinks it's stable and probably ready to expose to a wider audience), "waiting for review" (meaning not enough people have shown interest to look it over), "waiting for a re-roll" (meaning he expects a new version). Other annotations are possible. If a patch series isn't annotated, it means he hasn't made any decision about it yet. Also, if your patch series is not in "What's cooking", it may mean that Junio just hasn't gotten around to your submission yet, and there is a good chance it will be in the following "What's cooking". If, however, a couple weeks or more elapse and your submission doesn't show up, then it may have gotten lost in the noise, in which case it's a good idea to send a "ping" as a reminder. [1]: https://public-inbox.org/git/xmqqd0vbt14e@gitster-ct.c.googlers.com/
Re: [PATCH 2/2] Highlight keywords in remote sideband output.
On Thu, Aug 2, 2018 at 12:24 PM Eric Sunshine wrote: > > On Wed, Aug 1, 2018 at 2:17 PM Junio C Hamano wrote: > > Han-Wen Nienhuys writes: > > > Sorry for being dense, but do you want me to send an updated patch or > > > not based on your and Eric's comments or not? > > > > It would help to see the comments responded with either "such a > > change is not needed for such and such reasons", "it may make sense > > but let's leave it to a follow-up patch later," etc., or with a > > "here is an updated patch, taking all the comments to the previous > > version into account---note that I rejected that particular comment > > because of such and such reasons". > > Right. The way to know whether or not an updated patch is warranted is > to respond to review comments, saying that you agree or disagree with > various points raised (and why), and by answering the (genuine) > questions raised during review. The outcome of the dialogue with > reviewers will make it clear if an updated patch is necessary. (It's > also a courtesy to respond to review comments since reviewing is > time-consuming business and it's good to let reviewers know that the > time spent reviewing was not in vain.) Sure. My doubt is that it's hard to tell what the state of my patch is at any given time. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Re: [PATCH 2/2] Highlight keywords in remote sideband output.
On Tue, Jul 31, 2018 at 10:21 PM Eric Sunshine wrote:
>
> On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys wrote:
> > Highlight keywords in remote sideband output.
>
> Prefix with the module you're touching, don't capitalize, and drop the
> period. Perhaps:
Done.
> sideband: highlight keywords in remote sideband output
>
> > The highlighting is done on the client-side. Supported keywords are
> > "error", "warning", "hint" and "success".
> >
> > The colorization is controlled with the config setting "color.remote".
>
> What's the motivation for this change? The commit message should say
> something about that and give an explanation of why this is done
> client-side rather than server-side.
Added
> > Co-authored-by: Duy Nguyen
>
> Helped-by: is more typical.
Done.
> > Signed-off-by: Han-Wen Nienhuys
> > ---
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > @@ -1229,6 +1229,15 @@ color.push::
> > +color.remote::
> > + A boolean to enable/disable colored remote output. If unset,
> > + then the value of `color.ui` is used (`auto` by default).
>
> If this is "boolean", what does "auto" mean? Perhaps update the
> description to better match other color-related options.
All other doc entries say "boolean" here too. I'm happy to fix
phrasing of this file in a follow-on change, but let's keep it like
this for consistency.
> > diff --git a/sideband.c b/sideband.c
> > @@ -1,6 +1,97 @@
> > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
> > +{
> > + int i;
> > +
> > + load_sideband_colors();
> > + if (!want_color_stderr(sideband_use_color)) {
> > + strbuf_add(dest, src, n);
> > + return;
> > + }
>
> Can load_sideband_colors() be moved below the !want_color_stderr()
> conditional?
Reorganized this a bit.
> > +
> > + while (isspace(*src)) {
> > + strbuf_addch(dest, *src);
> > + src++;
> > + n--;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> > + struct kwtable* p = keywords + i;
> > + int len = strlen(p->keyword);
>
> Would it make sense to precompute each keyword length so you don't
> have to recompute them repeatedly, or is that premature optimization?
That is premature optimization. The next line does a strncasecmp on
the same data so the cost (loading the keywords into CPU cache) is the
same, while precomputing the length makes the code more error prone.
> > + if (!strncasecmp(p->keyword, src, len) &&
> > !isalnum(src[len])) {
>
> So, the strncasecmp() is checking if one of the recognized keywords is
> at the 'src' position, and the !isalnum() ensures that you won't pick
> up something of which the keyword is merely a prefix. For instance,
> you won't mistakenly highlight "successful". It also works correctly
> when 'len' happens to reference the end-of-string NUL. Okay.
added comment.
> > + strbuf_addstr(dest, p->color);
> > + strbuf_add(dest, src, len);
> > + strbuf_addstr(dest, GIT_COLOR_RESET);
> > + n -= len;
> > + src += len;
> > + break;
> > + }
>
> So, despite the explanation in the commit message, this function isn't
> _generally_ highlighting keywords in the sideband. Instead, it is
> highlighting a keyword only if it finds it at the start of string
> (ignoring whitespace). Perhaps the commit message could be more clear
> about that.
updated message.
> A natural follow-on question is whether strings are fed to this
> function one line at a time or if the incoming string may have
> embedded newlines (in which case, you might need to find a prefix
> following a newline, as well?).
added comment.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Re: [PATCH 2/2] Highlight keywords in remote sideband output.
On Wed, Aug 1, 2018 at 2:17 PM Junio C Hamano wrote:
> Han-Wen Nienhuys writes:
> > Sorry for being dense, but do you want me to send an updated patch or
> > not based on your and Eric's comments or not?
>
> It would help to see the comments responded with either "such a
> change is not needed for such and such reasons", "it may make sense
> but let's leave it to a follow-up patch later," etc., or with a
> "here is an updated patch, taking all the comments to the previous
> version into account---note that I rejected that particular comment
> because of such and such reasons".
Right. The way to know whether or not an updated patch is warranted is
to respond to review comments, saying that you agree or disagree with
various points raised (and why), and by answering the (genuine)
questions raised during review. The outcome of the dialogue with
reviewers will make it clear if an updated patch is necessary. (It's
also a courtesy to respond to review comments since reviewing is
time-consuming business and it's good to let reviewers know that the
time spent reviewing was not in vain.)
Regarding my question about whether load_sideband_colors() can be
moved below the '!want_color_stderr(sideband_use_color)' conditional,
after studying the code further, I see that it can't be, because
load_sideband_colors() is responsible for setting
'sideband_use_color'. The fact that this code confused or left
questions in the mind of a reviewer may indicate that responsibilities
are not partitioned in the best manner, and that the code could be
structured to be more clear.
For instance, it might make sense to rip all the 'sideband_use_color'
gunk out of load_sideband_colors() and move it to
maybe_colorize_sideband(), perhaps like this:
void maybe_colorize_sideband(...)
{
/* one-time initialization */
if (sideband_use_color < 0) {
if (!git_config_get_string(key, &value))
sideband_use_color = git_config_colorbool(key, value);
if (sideband_use_color > 0)
load_sideband_colors();
}
if (!want_color_stderr(sideband_use_color)) {
strbuf_add(dest, src, n);
return;
}
...as before...
}
You may or may not agree with the above suggestion; it's good to let
the reviewer know how you feel about it.
Your follow-on comment about how Gerrit has for years used "ERROR" is
exactly the sort of information which should be in the commit messages
since it saves reviewers (and future readers, months or years down the
road) from head-scratching, wondering why the code was written the way
it was (strcasecmp() vs. strcmp(), for instance).
The more pertinent information you can say up front in the commit
message, the less likely reviewers will be confused or wonder why you
made the choices you did. My question about whether
maybe_colorize_sideband() is fed lines one-by-one or whether its input
may contain embedded newlines is a good example of how a more complete
commit message could help. As the author of the patch, you have been
working in this code and likely know the answer, but reviewers won't
necessarily have this information at hand. If the commit message says
up front that this function processes lines one-by-one, then the
reviewer feels reassured that the patch author understands the
implications of the change (as opposed to the patch author perhaps not
having thought of the possibility of embedded newlines). So, it's a
genuine question (I still don't know the answer.)
Re: [PATCH 2/2] Highlight keywords in remote sideband output.
On Wed, Aug 1, 2018 at 8:17 PM Junio C Hamano wrote: > >> Hmm, do we actually say things like "Error: blah"? I am not sure if > >> I like this strncasecmp all that much. > > > > this is for the remote end, so what we (git-core) says isn't all that > > relevant. > > It is very relevant, I would think. Because the coloring is > controlled at the client end with this implementation, third-party > remote implementations have strong incentive to follow what our > remote end says and not to deviate. Preventing them from being > different just to be different does help the users, no? But the ship has already sailed: Gerrit has been saying "ERROR" instead of "error" for many years. In the case of Gerrit, the upper case message is a (poor) way to make the error message stand out from the sea of progress messages that "git push" prints on the terminal, without requiring a newer version of git-core. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Re: [PATCH 2/2] Highlight keywords in remote sideband output.
Han-Wen Nienhuys writes: > On Wed, Aug 1, 2018 at 5:41 PM Junio C Hamano wrote: > >> Hmm, do we actually say things like "Error: blah"? I am not sure if >> I like this strncasecmp all that much. > > this is for the remote end, so what we (git-core) says isn't all that > relevant. It is very relevant, I would think. Because the coloring is controlled at the client end with this implementation, third-party remote implementations have strong incentive to follow what our remote end says and not to deviate. Preventing them from being different just to be different does help the users, no? >> > So, despite the explanation in the commit message, this function isn't >> > _generally_ highlighting keywords in the sideband. Instead, it is >> > highlighting a keyword only if it finds it at the start of string >> > (ignoring whitespace). Perhaps the commit message could be more clear >> > about that. >> >> Sounds good. >> >> I didn't comment on other parts of your review posed as questions >> (that require some digging and thinking), but I think they are all >> worthwhile thing to think about. > > Sorry for being dense, but do you want me to send an updated patch or > not based on your and Eric's comments or not? It would help to see the comments responded with either "such a change is not needed for such and such reasons", "it may make sense but let's leave it to a follow-up patch later," etc., or with a "here is an updated patch, taking all the comments to the previous version into account---note that I rejected that particular comment because of such and such reasons".
Re: [PATCH 2/2] Highlight keywords in remote sideband output.
On Wed, Aug 1, 2018 at 5:41 PM Junio C Hamano wrote: > Hmm, do we actually say things like "Error: blah"? I am not sure if > I like this strncasecmp all that much. this is for the remote end, so what we (git-core) says isn't all that relevant. The reason I put this here is that Gerrit has some messages that say "ERROR: .. " > >> + strbuf_addstr(dest, p->color); > >> + strbuf_add(dest, src, len); > >> + strbuf_addstr(dest, GIT_COLOR_RESET); > >> + n -= len; > >> + src += len; > >> + break; > >> + } > > > > So, despite the explanation in the commit message, this function isn't > > _generally_ highlighting keywords in the sideband. Instead, it is > > highlighting a keyword only if it finds it at the start of string > > (ignoring whitespace). Perhaps the commit message could be more clear > > about that. > > Sounds good. > > I didn't comment on other parts of your review posed as questions > (that require some digging and thinking), but I think they are all > worthwhile thing to think about. Sorry for being dense, but do you want me to send an updated patch or not based on your and Eric's comments or not? thanks, -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Re: [PATCH 2/2] Highlight keywords in remote sideband output.
Eric Sunshine writes:
> On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys wrote:
>> Highlight keywords in remote sideband output.
>
> Prefix with the module you're touching, don't capitalize, and drop the
> period. Perhaps:
>
> sideband: highlight keywords in remote sideband output
Yup (I locally did something similar when queued it).
>> The highlighting is done on the client-side. Supported keywords are
>> "error", "warning", "hint" and "success".
>>
>> The colorization is controlled with the config setting "color.remote".
>
> What's the motivation for this change? The commit message should say
> something about that and give an explanation of why this is done
> client-side rather than server-side.
Good suggestion.
>
>> Co-authored-by: Duy Nguyen
>
> Helped-by: is more typical.
>
>> Signed-off-by: Han-Wen Nienhuys
>> ---
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> @@ -1229,6 +1229,15 @@ color.push::
>> +color.remote::
>> + A boolean to enable/disable colored remote output. If unset,
>> + then the value of `color.ui` is used (`auto` by default).
>
> If this is "boolean", what does "auto" mean? Perhaps update the
> description to better match other color-related options.
Existing `color.branch` is already loose in the same way, but others
like color.{diff,grep,interactive} read better. No, past mistakes
by others is not a good excuse to make things worse, but I'd say it
also is OK to clean this up, together with `color.branch`, after this
change on top.
>> + if (!strncasecmp(p->keyword, src, len) &&
>> !isalnum(src[len])) {
>
> So, the strncasecmp() is checking if one of the recognized keywords is
> at the 'src' position, and the !isalnum() ensures that you won't pick
> up something of which the keyword is merely a prefix. For instance,
> you won't mistakenly highlight "successful". It also works correctly
> when 'len' happens to reference the end-of-string NUL. Okay.
Hmm, do we actually say things like "Error: blah"? I am not sure if
I like this strncasecmp all that much.
>> + strbuf_addstr(dest, p->color);
>> + strbuf_add(dest, src, len);
>> + strbuf_addstr(dest, GIT_COLOR_RESET);
>> + n -= len;
>> + src += len;
>> + break;
>> + }
>
> So, despite the explanation in the commit message, this function isn't
> _generally_ highlighting keywords in the sideband. Instead, it is
> highlighting a keyword only if it finds it at the start of string
> (ignoring whitespace). Perhaps the commit message could be more clear
> about that.
Sounds good.
I didn't comment on other parts of your review posed as questions
(that require some digging and thinking), but I think they are all
worthwhile thing to think about.
Thanks.
Re: [PATCH 2/2] Highlight keywords in remote sideband output.
On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys wrote:
> Highlight keywords in remote sideband output.
Prefix with the module you're touching, don't capitalize, and drop the
period. Perhaps:
sideband: highlight keywords in remote sideband output
> The highlighting is done on the client-side. Supported keywords are
> "error", "warning", "hint" and "success".
>
> The colorization is controlled with the config setting "color.remote".
What's the motivation for this change? The commit message should say
something about that and give an explanation of why this is done
client-side rather than server-side.
> Co-authored-by: Duy Nguyen
Helped-by: is more typical.
> Signed-off-by: Han-Wen Nienhuys
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1229,6 +1229,15 @@ color.push::
> +color.remote::
> + A boolean to enable/disable colored remote output. If unset,
> + then the value of `color.ui` is used (`auto` by default).
If this is "boolean", what does "auto" mean? Perhaps update the
description to better match other color-related options.
> diff --git a/sideband.c b/sideband.c
> @@ -1,6 +1,97 @@
> +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
> +{
> + int i;
> +
> + load_sideband_colors();
> + if (!want_color_stderr(sideband_use_color)) {
> + strbuf_add(dest, src, n);
> + return;
> + }
Can load_sideband_colors() be moved below the !want_color_stderr() conditional?
> +
> + while (isspace(*src)) {
> + strbuf_addch(dest, *src);
> + src++;
> + n--;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> + struct kwtable* p = keywords + i;
> + int len = strlen(p->keyword);
Would it make sense to precompute each keyword length so you don't
have to recompute them repeatedly, or is that premature optimization?
> + if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len]))
> {
So, the strncasecmp() is checking if one of the recognized keywords is
at the 'src' position, and the !isalnum() ensures that you won't pick
up something of which the keyword is merely a prefix. For instance,
you won't mistakenly highlight "successful". It also works correctly
when 'len' happens to reference the end-of-string NUL. Okay.
> + strbuf_addstr(dest, p->color);
> + strbuf_add(dest, src, len);
> + strbuf_addstr(dest, GIT_COLOR_RESET);
> + n -= len;
> + src += len;
> + break;
> + }
So, despite the explanation in the commit message, this function isn't
_generally_ highlighting keywords in the sideband. Instead, it is
highlighting a keyword only if it finds it at the start of string
(ignoring whitespace). Perhaps the commit message could be more clear
about that.
A natural follow-on question is whether strings are fed to this
function one line at a time or if the incoming string may have
embedded newlines (in which case, you might need to find a prefix
following a newline, as well?).
> + }
> +
> + strbuf_add(dest, src, n);
> +}
Re: [PATCH 2/2] Highlight keywords in remote sideband output.
Han-Wen Nienhuys writes:
> The highlighting is done on the client-side. Supported keywords are
> "error", "warning", "hint" and "success".
>
> The colorization is controlled with the config setting "color.remote".
>
> Co-authored-by: Duy Nguyen
> Signed-off-by: Han-Wen Nienhuys
Thanks. I'll squash the following in while queuing, though.
* maybe_colorize_sideband() does not have outside caller; make it
static to avoid missing-prototype error that breaks compilation.
* correct space-before-tab whitespace style violation.
* use write_script.
* a test script must be executable to avoid triggering test-lint.
* avoid overlong lines in the test.
* no SP between redirection operator and its target.
Other than that, the result looks good to me. So that others can
eyeball the result once more, I'll keep it in 'pu' for a few days,
and if nothing else comes up, hopefully the topic can be merged to
'next' after that.
diff --git a/sideband.c b/sideband.c
index 0d67583ec5..be4635446c 100644
--- a/sideband.c
+++ b/sideband.c
@@ -60,12 +60,12 @@ void list_config_color_sideband_slots(struct string_list
*list, const char *pref
* Optionally highlight some keywords in remote output if they appear at the
* start of the line.
*/
-void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int
n)
{
int i;
load_sideband_colors();
- if (!want_color_stderr(sideband_use_color)) {
+ if (!want_color_stderr(sideband_use_color)) {
strbuf_add(dest, src, n);
return;
}
diff --git a/t/t5409-colorize-remote-messages.sh
b/t/t5409-colorize-remote-messages.sh
old mode 100644
new mode 100755
index 4e1bd421ff..4547ec95b8
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -6,27 +6,27 @@ test_description='remote messages are colorized on the client'
test_expect_success 'setup' '
mkdir .git/hooks &&
- cat << EOF > .git/hooks/update &&
-#!/bin/sh
-echo error: error
-echo hint: hint
-echo success: success
-echo warning: warning
-echo prefixerror: error
-exit 0
-EOF
- chmod +x .git/hooks/update &&
+ write_script .git/hooks/update <<-\EOF &&
+ echo error: error
+ echo hint: hint
+ echo success: success
+ echo warning: warning
+ echo prefixerror: error
+ exit 0
+ EOF
+
echo 1 >file &&
git add file &&
git commit -m 1 &&
git clone . child &&
cd child &&
- echo 2 > file &&
+ echo 2 >file &&
git commit -a -m 2
'
test_expect_success 'push' '
- git -c color.remote=always push -f origin HEAD:refs/heads/newbranch
2>output &&
+ git -c color.remote=always \
+ push -f origin HEAD:refs/heads/newbranch 2>output &&
test_decode_color decoded &&
grep "error:" decoded &&
grep "hint:" decoded &&
@@ -36,7 +36,8 @@ test_expect_success 'push' '
'
test_expect_success 'push with customized color' '
- git -c color.remote=always -c color.remote.error=white push -f origin
HEAD:refs/heads/newbranch2 2>output &&
+ git -c color.remote=always -c color.remote.error=white \
+ push -f origin HEAD:refs/heads/newbranch2 2>output &&
test_decode_color decoded &&
grep "error:" decoded &&
grep "hint:" decoded &&
[PATCH 2/2] Highlight keywords in remote sideband output.
The highlighting is done on the client-side. Supported keywords are
"error", "warning", "hint" and "success".
The colorization is controlled with the config setting "color.remote".
Co-authored-by: Duy Nguyen
Signed-off-by: Han-Wen Nienhuys
---
Documentation/config.txt| 9 +++
help.c | 1 +
help.h | 1 +
sideband.c | 113 +---
t/t5409-colorize-remote-messages.sh | 47
5 files changed, 162 insertions(+), 9 deletions(-)
create mode 100644 t/t5409-colorize-remote-messages.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 43b2de7b5..0783323be 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1229,6 +1229,15 @@ color.push::
color.push.error::
Use customized color for push errors.
+color.remote::
+ A boolean to enable/disable colored remote output. If unset,
+ then the value of `color.ui` is used (`auto` by default).
+
+color.remote.::
+ Use customized color for each remote keywords. `` may be
+ `hint`, `warning`, `success` or `error` which match the
+ corresponding keyword.
+
color.showBranch::
A boolean to enable/disable color in the output of
linkgit:git-show-branch[1]. May be set to `always`,
diff --git a/help.c b/help.c
index 3ebf0568d..b6cafcfc0 100644
--- a/help.c
+++ b/help.c
@@ -425,6 +425,7 @@ void list_config_help(int for_human)
{ "color.diff", "", list_config_color_diff_slots },
{ "color.grep", "", list_config_color_grep_slots },
{ "color.interactive", "",
list_config_color_interactive_slots },
+ { "color.remote", "", list_config_color_sideband_slots },
{ "color.status", "", list_config_color_status_slots },
{ "fsck", "", list_config_fsck_msg_ids },
{ "receive.fsck", "", list_config_fsck_msg_ids },
diff --git a/help.h b/help.h
index f8b15323a..9eab6a3f8 100644
--- a/help.h
+++ b/help.h
@@ -83,6 +83,7 @@ void list_config_color_diff_slots(struct string_list *list,
const char *prefix);
void list_config_color_grep_slots(struct string_list *list, const char
*prefix);
void list_config_color_interactive_slots(struct string_list *list, const char
*prefix);
void list_config_color_status_slots(struct string_list *list, const char
*prefix);
+void list_config_color_sideband_slots(struct string_list *list, const char
*prefix);
void list_config_fsck_msg_ids(struct string_list *list, const char *prefix);
#endif /* HELP_H */
diff --git a/sideband.c b/sideband.c
index 325bf0e97..0d67583ec 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,97 @@
#include "cache.h"
+#include "color.h"
+#include "config.h"
#include "pkt-line.h"
#include "sideband.h"
+#include "help.h"
+
+struct kwtable {
+ /*
+* We use keyword as config key so it can't contain funny chars like
+* spaces. When we do that, we need a separate field for slot name in
+* load_sideband_colors().
+*/
+ const char *keyword;
+ char color[COLOR_MAXLEN];
+};
+
+static struct kwtable keywords[] = {
+ { "hint", GIT_COLOR_YELLOW },
+ { "warning",GIT_COLOR_BOLD_YELLOW },
+ { "success",GIT_COLOR_BOLD_GREEN },
+ { "error", GIT_COLOR_BOLD_RED },
+};
+
+static int sideband_use_color = -1;
+
+static void load_sideband_colors(void)
+{
+ const char *key = "color.remote";
+ struct strbuf sb = STRBUF_INIT;
+ char *value;
+ int i;
+
+ if (sideband_use_color >= 0)
+ return;
+
+ if (!git_config_get_string(key, &value))
+ sideband_use_color = git_config_colorbool(key, value);
+
+ for (i = 0; i < ARRAY_SIZE(keywords); i++) {
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword);
+ if (git_config_get_string(sb.buf, &value))
+ continue;
+ if (color_parse(value, keywords[i].color))
+ die(_("expecting a color: %s"), value);
+ }
+ strbuf_release(&sb);
+}
+
+void list_config_color_sideband_slots(struct string_list *list, const char
*prefix)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(keywords); i++)
+ list_config_item(list, prefix, keywords[i].keyword);
+}
+
+/*
+ * Optionally highlight some keywords in remote output if they appear at the
+ * start of the line.
+ */
+void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+{
+ int i;
+
+ load_sideband_colors();
+ if (!want_color_stderr(sideband_use_color)) {
+ strbuf_add(dest, src, n);
+ return;
+ }
+
+ while (isspace(*src)) {
+ strbuf_addch(dest, *src);
+ src++;
+ n--;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(keywords); i++) {
+
[PATCH 2/2] Highlight keywords in remote sideband output.
The highlighting is done on the client-side. Supported keywords are
"error", "warning", "hint" and "success".
The colorization is controlled with the config setting "color.remote".
Signed-off-by: Han-Wen Nienhuys
---
sideband.c | 79 +
t/t5409-colorize-remote-messages.sh | 34 +
2 files changed, 104 insertions(+), 9 deletions(-)
create mode 100644 t/t5409-colorize-remote-messages.sh
diff --git a/sideband.c b/sideband.c
index 325bf0e97..15213a7c1 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,63 @@
#include "cache.h"
#include "pkt-line.h"
#include "sideband.h"
+#include "color.h"
+
+static int sideband_use_color = -1;
+
+/*
+ * Optionally highlight some keywords in remote output if they appear at the
+ * start of the line.
+ */
+void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+{
+ if (sideband_use_color < 0) {
+ const char *key = "color.remote";
+ char *value = NULL;
+ if (!git_config_get_string(key, &value))
+ sideband_use_color = git_config_colorbool(key, value);
+ }
+
+ int want_color = want_color_stderr(sideband_use_color);
+ int i;
+
+ if (!want_color) {
+ strbuf_add(dest, src, n);
+ return;
+ }
+
+ struct kwtable {
+ const char *keyword;
+ const char *color;
+ } keywords[] = {
+ {"hint", GIT_COLOR_YELLOW},
+ {"warning", GIT_COLOR_BOLD_YELLOW},
+ {"success", GIT_COLOR_BOLD_GREEN},
+ {"error", GIT_COLOR_BOLD_RED},
+ };
+
+ while (isspace(*src)) {
+ strbuf_addch(dest, *src);
+ src++;
+ n--;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(keywords); i++) {
+ struct kwtable* p = keywords + i;
+ int len = strlen(p->keyword);
+ if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {
+ strbuf_addstr(dest, p->color);
+ strbuf_add(dest, src, len);
+ strbuf_addstr(dest, GIT_COLOR_RESET);
+ n -= len;
+ src += len;
+ break;
+ }
+ }
+
+ strbuf_add(dest, src, n);
+}
+
/*
* Receive multiplexed output stream over git native protocol.
@@ -48,8 +105,10 @@ int recv_sideband(const char *me, int in_stream, int out)
len--;
switch (band) {
case 3:
- strbuf_addf(&outbuf, "%s%s%s", outbuf.len ? "\n" : "",
- DISPLAY_PREFIX, buf + 1);
+ strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
+ DISPLAY_PREFIX);
+ maybe_colorize_sideband(&outbuf, buf + 1, len);
+
retval = SIDEBAND_REMOTE_ERROR;
break;
case 2:
@@ -69,20 +128,22 @@ int recv_sideband(const char *me, int in_stream, int out)
if (!outbuf.len)
strbuf_addstr(&outbuf, DISPLAY_PREFIX);
if (linelen > 0) {
- strbuf_addf(&outbuf, "%.*s%s%c",
- linelen, b, suffix, *brk);
- } else {
- strbuf_addch(&outbuf, *brk);
+ maybe_colorize_sideband(&outbuf, b,
linelen);
+ strbuf_addstr(&outbuf, suffix);
}
+
+ strbuf_addch(&outbuf, *brk);
xwrite(2, outbuf.buf, outbuf.len);
strbuf_reset(&outbuf);
b = brk + 1;
}
- if (*b)
- strbuf_addf(&outbuf, "%s%s", outbuf.len ?
- "" : DISPLAY_PREFIX, b);
+ if (*b) {
+ strbuf_addstr(&outbuf, outbuf.len ?
+ "" : DISPLAY_PREFIX);
+ maybe_colorize_sideband(&outbuf, b, strlen(b));
+ }
break;
case 1:
write_or_die(out, buf + 1, len);
diff --git a/t/t5409-colorize-remote-messages.sh
b/t/t5409-colorize-remote-messages.sh
new file mode 100644
index 0..1620cffbe
--- /dev/null
+++ b/t/t5409-colorize-remote-messages.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='remote messages are colorized on the client'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ mkdir .git/hooks &&
+cat << EOF > .git/hooks/update
[PATCH 2/2] Highlight keywords in remote sideband output.
The highlighting is done on the client-side. Supported keywords are
"error", "warning", "hint" and "success".
The colorization is controlled with the config setting "color.remote".
Signed-off-by: Han-Wen Nienhuys
Change-Id: I090412a1288bc2caef0916447e28c2d0199da47d
---
sideband.c | 79 +
t/t5409-colorize-remote-messages.sh | 34 +
2 files changed, 104 insertions(+), 9 deletions(-)
create mode 100644 t/t5409-colorize-remote-messages.sh
diff --git a/sideband.c b/sideband.c
index 325bf0e97..15213a7c1 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,63 @@
#include "cache.h"
#include "pkt-line.h"
#include "sideband.h"
+#include "color.h"
+
+static int sideband_use_color = -1;
+
+/*
+ * Optionally highlight some keywords in remote output if they appear at the
+ * start of the line.
+ */
+void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+{
+ if (sideband_use_color < 0) {
+ const char *key = "color.remote";
+ char *value = NULL;
+ if (!git_config_get_string(key, &value))
+ sideband_use_color = git_config_colorbool(key, value);
+ }
+
+ int want_color = want_color_stderr(sideband_use_color);
+ int i;
+
+ if (!want_color) {
+ strbuf_add(dest, src, n);
+ return;
+ }
+
+ struct kwtable {
+ const char *keyword;
+ const char *color;
+ } keywords[] = {
+ {"hint", GIT_COLOR_YELLOW},
+ {"warning", GIT_COLOR_BOLD_YELLOW},
+ {"success", GIT_COLOR_BOLD_GREEN},
+ {"error", GIT_COLOR_BOLD_RED},
+ };
+
+ while (isspace(*src)) {
+ strbuf_addch(dest, *src);
+ src++;
+ n--;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(keywords); i++) {
+ struct kwtable* p = keywords + i;
+ int len = strlen(p->keyword);
+ if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {
+ strbuf_addstr(dest, p->color);
+ strbuf_add(dest, src, len);
+ strbuf_addstr(dest, GIT_COLOR_RESET);
+ n -= len;
+ src += len;
+ break;
+ }
+ }
+
+ strbuf_add(dest, src, n);
+}
+
/*
* Receive multiplexed output stream over git native protocol.
@@ -48,8 +105,10 @@ int recv_sideband(const char *me, int in_stream, int out)
len--;
switch (band) {
case 3:
- strbuf_addf(&outbuf, "%s%s%s", outbuf.len ? "\n" : "",
- DISPLAY_PREFIX, buf + 1);
+ strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
+ DISPLAY_PREFIX);
+ maybe_colorize_sideband(&outbuf, buf + 1, len);
+
retval = SIDEBAND_REMOTE_ERROR;
break;
case 2:
@@ -69,20 +128,22 @@ int recv_sideband(const char *me, int in_stream, int out)
if (!outbuf.len)
strbuf_addstr(&outbuf, DISPLAY_PREFIX);
if (linelen > 0) {
- strbuf_addf(&outbuf, "%.*s%s%c",
- linelen, b, suffix, *brk);
- } else {
- strbuf_addch(&outbuf, *brk);
+ maybe_colorize_sideband(&outbuf, b,
linelen);
+ strbuf_addstr(&outbuf, suffix);
}
+
+ strbuf_addch(&outbuf, *brk);
xwrite(2, outbuf.buf, outbuf.len);
strbuf_reset(&outbuf);
b = brk + 1;
}
- if (*b)
- strbuf_addf(&outbuf, "%s%s", outbuf.len ?
- "" : DISPLAY_PREFIX, b);
+ if (*b) {
+ strbuf_addstr(&outbuf, outbuf.len ?
+ "" : DISPLAY_PREFIX);
+ maybe_colorize_sideband(&outbuf, b, strlen(b));
+ }
break;
case 1:
write_or_die(out, buf + 1, len);
diff --git a/t/t5409-colorize-remote-messages.sh
b/t/t5409-colorize-remote-messages.sh
new file mode 100644
index 0..1620cffbe
--- /dev/null
+++ b/t/t5409-colorize-remote-messages.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='remote messages are colorized on the client'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ mkd
