Re: [PATCH] sideband: color lines with keyword only
On Tue, Dec 4, 2018 at 12:23 AM Jonathan Nieder wrote:
> > When bf1a11f0a1 (sideband: highlight keywords in remote sideband output,
> > 2018-08-07) was introduced, it was carefully considered which strings
> > would be highlighted. However 59a255aef0 (sideband: do not read beyond
> > the end of input, 2018-08-18) brought in a regression that the original
> > did not test for. A line containing only the keyword and nothing else
> > ("SUCCESS") should still be colored.
I had intended SUCCESS on a line of its to be highlighted too, and
some earlier versions of my patch did that, but it regressed as the
patch was reworked. The SUCCESS on a line of its own is a recent
behavior of Gerrit, and is live in Gerrit 2.16.
--
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
git-clone --config remote.origin.fetch regression
(see also https://github.com/google/zoekt/issues/81) It looks like git 2.21 included a regression. The command git clone --bare --progress \ --config "remote.origin.fetch=+refs/heads/*:refs/heads/*" \ https://github.com/google/zoekt.git \ /tmp/zoekt-git2.20.git would succeed with git 2.20, but fails with fatal: multiple updates for ref 'refs/heads/master' not allowed in git 2.21, probably caused by commit 515be83. Should I call git in another way? I originally included "remote.origin.fetch=+refs/heads/*:refs/heads/*" to avoid getting Gerrit refs (refs/changes/*), but maybe I should use a different incantation? -- 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
[PATCH] RFC Highlight keywords in remote sideband output.
Supported keywords are "error", "warning", "hint" and "success".
TODO:
* make the coloring optional? What variable to use?
* doc for the coloring option.
* how to test?
Signed-off-by: Han-Wen Nienhuys
Change-Id: I090412a1288bc2caef0916447e28c2d0199da47d
---
sideband.c | 69 +++---
1 file changed, 60 insertions(+), 9 deletions(-)
diff --git a/sideband.c b/sideband.c
index 325bf0e97..c8b7cb6dd 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,53 @@
#include "cache.h"
#include "pkt-line.h"
#include "sideband.h"
+#include "color.h"
+
+/*
+ * Optionally highlight some keywords in remote output if they appear at the
+ * start of the line.
+ */
+void emit_sideband(struct strbuf *dest, const char *src, int n) {
+// NOSUBMIT - maybe use transport.color property?
+int want_color = want_color_stderr(GIT_COLOR_AUTO);
+
+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 (struct kwtable* p = keywords; p->keyword; p++) {
+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 +95,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);
+emit_sideband(&outbuf, buf+1, len);
+
retval = SIDEBAND_REMOTE_ERROR;
break;
case 2:
@@ -69,20 +118,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);
+emit_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);
+emit_sideband(&outbuf, b, strlen(b));
+}
break;
case 1:
write_or_die(out, buf + 1, len);
--
2.18.0.233.g985f88cf7e-goog
[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
[PATCH 1/2] Document git config getter return value.
---
config.h | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/config.h b/config.h
index b95bb7649..d39256eb1 100644
--- a/config.h
+++ b/config.h
@@ -178,11 +178,16 @@ struct config_set {
};
extern void git_configset_init(struct config_set *cs);
-extern int git_configset_add_file(struct config_set *cs, const char *filename);
-extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **value);
+
extern const struct string_list *git_configset_get_value_multi(struct
config_set *cs, const char *key);
extern void git_configset_clear(struct config_set *cs);
+
+/*
+ * The int return values in these functions is 1 if not found, 0 if found,
leaving
+ * the found value in the 'dest' pointer.
+ */
+extern int git_configset_add_file(struct config_set *cs, const char *filename);
+extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **dest);
extern int git_configset_get_string_const(struct config_set *cs, const char
*key, const char **dest);
extern int git_configset_get_string(struct config_set *cs, const char *key,
char **dest);
extern int git_configset_get_int(struct config_set *cs, const char *key, int
*dest);
--
2.18.0.345.g5c9ce644c3-goog
Re: [PATCH] RFC Highlight keywords in remote sideband output.
On Thu, Jul 26, 2018 at 8:50 PM Junio C Hamano wrote:
> Hold your objection a bit. I'll come back to it soon ;-)
>
> It theoretically may make more sense to color on the sender side,
> but that is true only if done at a higher layer that prepares a
> string and calls into the sideband code to send. That code must
> know what the bytes _mean_ a lot better than the code at the
> sideband layer, so we do not have to guess.
>
> Having written all the above, I think you are doing this at the
> receiving end, so this actually makes quite a lot of sense. I was
> fooled greatly by "EMIT_sideband", which in reality does NOT emit at
> all. That function is badly misnamed.
fixed.
> The function is more like "color sideband payload"; actual
> "emitting" is still done at the places the code originally "emitted"
> them to the receiving user.
>
> > Signed-off-by: Han-Wen Nienhuys
> > Change-Id: I090412a1288bc2caef0916447e28c2d0199da47d
>
> That's an unusual trailer we do not use in this project.
Yes, I know. I forgot to strip it from v2 again, though :-(
> > +void emit_sideband(struct strbuf *dest, const char *src, int n) {
>
> Open brace on its own line.
Done.
> > +// NOSUBMIT - maybe use transport.color property?
>
> Avoid // comment.
Done
> In our codebase in C, asterisk sticks to the variable not the type.
Done.
> > +} keywords[] = {
> > +{"hint", GIT_COLOR_YELLOW},
> > +{"warning", GIT_COLOR_BOLD_YELLOW},
> > +{"success", GIT_COLOR_BOLD_GREEN},
> > +{"error", GIT_COLOR_BOLD_RED},
> > +{},
>
> Drop the last sentinel element, and instead stop iteration over the
> array using (i < ARRAY_SIZE(keywords)).
Done.
> > +for (struct kwtable* p = keywords; p->keyword; p++) {
>
> Does anybody know if we already use the variable decl inside the
> "for (...)" construct like this? I know we discussed the idea of
> using it somewhere as a weather-balloon to see if people with exotic
> environment would mind, and I certainly do not mind making this
> patch serve as such a weather-baloon, but if that is what we are
> doing, I want the commit log message clearly marked as such, so that
> we can later "git log --grep=C99" to see how long ago such an
> experiment started.
I elided this. (I had expected for the compile to enforce restrictions
like these using --std=c99.)
> > * Receive multiplexed output stream over git native protocol.
> > @@ -48,8 +95,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);
> > +emit_sideband(&outbuf, buf+1, len);
> > +
>
> Let's not lose SP around "+" on both sides.
>
> Also you seem to be indenting some lines with all SP and some with
> mixture of HT and SP. We prefer to use as many 8-column HT and then
> fill the remainder with SP if needed to align with the opening
> parenthesis on line above it (imitate the way strbuf_addf() is split
> into two lines in the original in this hunk).
Fixed these, I think.
> Thanks. While there are need for mostly minor fix-ups, the logic
> seems quite sane. I think we can start without configuration and
> then "fix" it later.
I need the configuration to be able to test this, though.
> While I am OK with calling that variable "transport.", we
> should not define/explain it as "color output coming from the other
> end over the wire transport". Those who want to see messages
> emitted remotely during "git fetch" in color would want to see the
> messages generated by "git fetch" locally painted in the same color
> scheme, so it makes sense to let "git fetch" pay attention and honor
> that variable even for its own locally generated messages. The
> variable instead means "color any message, either generated locally
> or remotely, during an operation that has something to do with
> object transport", or something like that.
I used color.remote for the property, but I'm happy to colorize the
bikeshed with another color.
--
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
[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-
[PATCH 1/2] Document git config getter return value.
---
config.h | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/config.h b/config.h
index b95bb7649..d39256eb1 100644
--- a/config.h
+++ b/config.h
@@ -178,11 +178,16 @@ struct config_set {
};
extern void git_configset_init(struct config_set *cs);
-extern int git_configset_add_file(struct config_set *cs, const char *filename);
-extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **value);
+
extern const struct string_list *git_configset_get_value_multi(struct
config_set *cs, const char *key);
extern void git_configset_clear(struct config_set *cs);
+
+/*
+ * The int return values in these functions is 1 if not found, 0 if found,
leaving
+ * the found value in the 'dest' pointer.
+ */
+extern int git_configset_add_file(struct config_set *cs, const char *filename);
+extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **dest);
extern int git_configset_get_string_const(struct config_set *cs, const char
*key, const char **dest);
extern int git_configset_get_string(struct config_set *cs, const char *key,
char **dest);
extern int git_configset_get_int(struct config_set *cs, const char *key, int
*dest);
--
2.18.0.345.g5c9ce644c3-goog
[PATCH 1/1] 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 | 78 +
t/t5409-colorize-remote-messages.sh | 34 +
2 files changed, 103 insertions(+), 9 deletions(-)
create mode 100644 t/t5409-colorize-remote-messages.sh
diff --git a/sideband.c b/sideband.c
index 325bf0e97..e939cd436 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,7 +1,63 @@
#include "cache.h"
+#include "color.h"
+#include "config.h"
#include "pkt-line.h"
#include "sideband.h"
+
+/*
+ * 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 int sideband_use_color = -1;
+ int i;
+ 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);
+ }
+
+ if (!want_color_stderr(sideband_use_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.
* in_stream is the input stream from the remote, which carries data
@@ -48,8 +104,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 +127,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);
di
[PATCH 0/1] Highlight keywords in remote sideband output.
Made tests compile and pass (oops). Remove Change-Id footer. Han-Wen Nienhuys (1): Highlight keywords in remote sideband output. sideband.c | 78 + t/t5409-colorize-remote-messages.sh | 34 + 2 files changed, 103 insertions(+), 9 deletions(-) create mode 100644 t/t5409-colorize-remote-messages.sh -- 2.18.0.345.g5c9ce644c3-goog
Re: [PATCH 1/1] Highlight keywords in remote sideband output.
On Mon, Jul 30, 2018 at 11:39 PM Junio C Hamano wrote: > > + */ > > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) > > I'll make this "static" to this file while queuing. Does that mean the patch is in? -- 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
[PATCH 1/2] Document git config getter return value.
---
config.h | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/config.h b/config.h
index b95bb7649..d39256eb1 100644
--- a/config.h
+++ b/config.h
@@ -178,10 +178,16 @@ struct config_set {
};
extern void git_configset_init(struct config_set *cs);
-extern int git_configset_add_file(struct config_set *cs, const char *filename);
-extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **value);
+
extern const struct string_list *git_configset_get_value_multi(struct
config_set *cs, const char *key);
extern void git_configset_clear(struct config_set *cs);
+
+/*
+ * The int return values in the functions is 1 if not found, 0 if found,
leaving
+ * the found value in teh 'dest' pointer.
+ */
+extern int git_configset_add_file(struct config_set *cs, const char *filename);
+extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **dest);
extern int git_configset_get_string_const(struct config_set *cs, const char
*key, const char **dest);
extern int git_configset_get_string(struct config_set *cs, const char *key,
char **dest);
extern int git_configset_get_int(struct config_set *cs, const char *key, int
*dest);
--
2.18.0.345.g5c9ce644c3-goog
[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)
+{
+
[PATCH 0/2 v3] Highlight keywords in remote sideband output.
squash in Duy's patch Han-Wen Nienhuys (2): Document git config getter return value. Highlight keywords in remote sideband output. Documentation/config.txt| 9 +++ config.h| 10 ++- help.c | 1 + help.h | 1 + sideband.c | 113 +--- t/t5409-colorize-remote-messages.sh | 47 6 files changed, 170 insertions(+), 11 deletions(-) create mode 100644 t/t5409-colorize-remote-messages.sh -- 2.18.0.345.g5c9ce644c3-goog
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.
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.
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 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
[PATCH 0/2 v4] sideband: highlight keywords in remote sideband output
Address Eric Sunshine's comments. Han-Wen Nienhuys (2): config: document git config getter return value. sideband: highlight keywords in remote sideband output Documentation/config.txt| 9 +++ config.h| 10 ++- help.c | 1 + help.h | 1 + sideband.c | 119 +--- t/t5409-colorize-remote-messages.sh | 47 +++ 6 files changed, 176 insertions(+), 11 deletions(-) create mode 100644 t/t5409-colorize-remote-messages.sh -- 2.18.0.597.ga71716f1ad-goog
[PATCH 2/2] sideband: highlight keywords in remote sideband output
The colorization is controlled with the config setting "color.remote".
Supported keywords are "error", "warning", "hint" and "success". They
are highlighted if they appear at the start of the line, which is
common in error messages, eg.
ERROR: commit is missing Change-Id
The rationale for this change is that Gerrit does server-side
processing to create or update code reviews, and actionable error
messages (eg. missing Change-Id) must be communicated back to the user
during the push. User research has shown that new users have trouble
seeing these messages.
The Git push process itself prints lots of non-actionable messages
(eg. bandwidth statistics, object counters for different phases of the
process), and my hypothesis is that these obscure the actionable error
messages that our server sends back. Highlighting keywords in the
draws more attention to actionable messages.
The highlighting is done on the client rather than server side, so
servers don't have to grow capabilities to understand terminal escape
codes and terminal state. It also consistent with the current state
where Git is control of the local display (eg. prefixing messages with
"remote: ").
Finally, this solution is backwards compatible: many servers already
prefix their messages with "error", and they will benefit from this
change without requiring a server update. By contrast, a server-side
solution would likely require plumbing the TERM variable through the
git protocol, so it would require changes to both server and client.
Helped-by: Duy Nguyen
Signed-off-by: Han-Wen Nienhuys
---
Documentation/config.txt| 9 +++
help.c | 1 +
help.h | 1 +
sideband.c | 119 +---
t/t5409-colorize-remote-messages.sh | 47 +++
5 files changed, 168 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..5c72db83c 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,103 @@
#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
[PATCH 1/2] config: document git config getter return value.
---
config.h | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/config.h b/config.h
index b95bb7649..41700f40b 100644
--- a/config.h
+++ b/config.h
@@ -178,10 +178,16 @@ struct config_set {
};
extern void git_configset_init(struct config_set *cs);
-extern int git_configset_add_file(struct config_set *cs, const char *filename);
-extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **value);
+
extern const struct string_list *git_configset_get_value_multi(struct
config_set *cs, const char *key);
extern void git_configset_clear(struct config_set *cs);
+
+/*
+ * These functions return 1 if not found, and 0 if found, leaving the found
+ * value in the 'dest' pointer.
+ */
+extern int git_configset_add_file(struct config_set *cs, const char *filename);
+extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **dest);
extern int git_configset_get_string_const(struct config_set *cs, const char
*key, const char **dest);
extern int git_configset_get_string(struct config_set *cs, const char *key,
char **dest);
extern int git_configset_get_int(struct config_set *cs, const char *key, int
*dest);
--
2.18.0.597.ga71716f1ad-goog
[PATCH 1/2] config: document git config getter return value.
---
config.h | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/config.h b/config.h
index b95bb7649..41700f40b 100644
--- a/config.h
+++ b/config.h
@@ -178,10 +178,16 @@ struct config_set {
};
extern void git_configset_init(struct config_set *cs);
-extern int git_configset_add_file(struct config_set *cs, const char *filename);
-extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **value);
+
extern const struct string_list *git_configset_get_value_multi(struct
config_set *cs, const char *key);
extern void git_configset_clear(struct config_set *cs);
+
+/*
+ * These functions return 1 if not found, and 0 if found, leaving the found
+ * value in the 'dest' pointer.
+ */
+extern int git_configset_add_file(struct config_set *cs, const char *filename);
+extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **dest);
extern int git_configset_get_string_const(struct config_set *cs, const char
*key, const char **dest);
extern int git_configset_get_string(struct config_set *cs, const char *key,
char **dest);
extern int git_configset_get_int(struct config_set *cs, const char *key, int
*dest);
--
2.18.0.597.ga71716f1ad-goog
[PATCH 0/2 v4] sideband: highlight keywords in remote sideband output
Address Eric Sunshine's comments. Han-Wen Nienhuys (2): config: document git config getter return value. sideband: highlight keywords in remote sideband output Documentation/config.txt| 9 +++ config.h| 10 ++- help.c | 1 + help.h | 1 + sideband.c | 119 +--- t/t5409-colorize-remote-messages.sh | 47 +++ 6 files changed, 176 insertions(+), 11 deletions(-) create mode 100644 t/t5409-colorize-remote-messages.sh -- 2.18.0.597.ga71716f1ad-goog
[PATCH 2/2] sideband: highlight keywords in remote sideband output
The colorization is controlled with the config setting "color.remote".
Supported keywords are "error", "warning", "hint" and "success". They
are highlighted if they appear at the start of the line, which is
common in error messages, eg.
ERROR: commit is missing Change-Id
The rationale for this change is that Gerrit does server-side
processing to create or update code reviews, and actionable error
messages (eg. missing Change-Id) must be communicated back to the user
during the push. User research has shown that new users have trouble
seeing these messages.
The Git push process itself prints lots of non-actionable messages
(eg. bandwidth statistics, object counters for different phases of the
process), and my hypothesis is that these obscure the actionable error
messages that our server sends back. Highlighting keywords in the
draws more attention to actionable messages.
The highlighting is done on the client rather than server side, so
servers don't have to grow capabilities to understand terminal escape
codes and terminal state. It also consistent with the current state
where Git is control of the local display (eg. prefixing messages with
"remote: ").
Finally, this solution is backwards compatible: many servers already
prefix their messages with "error", and they will benefit from this
change without requiring a server update. By contrast, a server-side
solution would likely require plumbing the TERM variable through the
git protocol, so it would require changes to both server and client.
Helped-by: Duy Nguyen
Signed-off-by: Han-Wen Nienhuys
---
Documentation/config.txt| 9 +++
help.c | 1 +
help.h | 1 +
sideband.c | 119 +---
t/t5409-colorize-remote-messages.sh | 47 +++
5 files changed, 168 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..5c72db83c 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,103 @@
#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
Re: [PATCH 2/2] sideband: highlight keywords in remote sideband output
On Thu, Aug 2, 2018 at 8:22 PM Junio C Hamano wrote:
> >
> > Helped-by: Duy Nguyen
> > Signed-off-by: Han-Wen Nienhuys
> > ---
> > Documentation/config.txt| 9 +++
> > help.c | 1 +
> > help.h | 1 +
> > sideband.c | 119 +---
> > t/t5409-colorize-remote-messages.sh | 47 +++
> > 5 files changed, 168 insertions(+), 9 deletions(-)
> > create mode 100644 t/t5409-colorize-remote-messages.sh
>
> I'll "chmod +x" while queuing.
>
Done.
> If your "make test" did not catch this as an error, then we may need
> to fix t/Makefile, as it is supposed to run test-lint.
I've been running tests individually as
sh t5409-colorize-remote-messages.sh -v -d
> > +color.remote::
> > + A boolean to enable/disable colored remote output. If unset,
> > + then the value of `color.ui` is used (`auto` by default).
>
> Nobody tells the end-users what "colored remote output" does;
> arguably they can find it out themselves by enabling the feature and
> observing remote messages, but that is not user friendly.
expanded doc.
> > +color.remote.::
> > + Use customized color for each remote keywords. `` may be
>
> Isn't 'each' a singular, i.e. "for each remote keyword"? If so I do
> not mind dropping 's' myself while queuing.
Done.
>
> > + `hint`, `warning`, `success` or `error` which match the
> > + corresponding keyword.
>
> We need to say that keywords are painted case insensitively
> somewhere in the doc. Either do that here, or in the updated
> description of `color.remote`---I am not sure which one results in
> more readable text offhand.
Done.
> > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
> > +{
> > + int i;
>
> In a block with a dozen more more lines, it is easier to have a
> blank line between decls and the first statement, i.e. here.
Done.
> > + if (!want_color_stderr(use_sideband_colors())) {
>
> The above line is indented with SP followed by HT; don't.
Fixed. It would be great if there were a pre-commit hook that I could
install to prevent this from ever getting committed.
> > + struct kwtable* p = keywords + i;
>
> struct kwtable *p = keywords + i;
Done.
> > + int len = strlen(p->keyword);
> > +/*
> > + * Match case insensitively, so we colorize output from
> > existing
> > + * servers regardless of the case that they use for their
> > + * messages. We only highlight the word precisely, so
> > + * "successful" stays uncolored.
> > + */
>
> Indent with tabs, not a run of spaces, i.e.
Done.
> Use write_script, i.e. instead of all the above, say
Done.
> Our tests are not written to demonstrate that our code works as
> written. It is to protect our code from getting broken by others
> who may not share vision of the original author. Make sure that you
> cast what you care about in stone, e.g. include "echo ERROR: bad" or
> something in the above to ensure that future updates to the code
> will not turn the match into a case sensitive one without breaking
> the test suite.
Add some more cases.
> > + echo 1 >file &&
> > + git add file &&
> > + git commit -m 1 &&
> > + git clone . child &&
> > + cd child &&
> > + echo 2 > file &&
> > + git commit -a -m 2
>
> Don't chdir the whole testing environment like this. Depending on
> the success and failure in the middle of the above &&-chain, the
> next test will start at an unexpected place, which is bad.
>
> Instead, do something like
>
> git clone . child &&
> echo 2 >child/file &&
> git -C child commit -a -m 2
>
> or
>
Done.
> > +test_expect_success 'push' '
> > + git -c color.remote=always push -f origin HEAD:refs/heads/newbranch
> > 2>output &&
> > + test_decode_color decoded &&
> > + grep "error:" decoded &&
> > + grep "hint:" decoded &&
> > + grep "success:" decoded &&
> > + grep "warning:" decoded &&
> > + grep "prefixerror: error" decoded
>
> A comment before this test (which covers both of these two) that
> explains
Re: [PATCH 2/2] sideband: highlight keywords in remote sideband output
On Fri, Aug 3, 2018 at 5:52 AM Jonathan Nieder wrote:
>
> Hi,
>
> Han-Wen Nienhuys wrote:
>
> > The colorization is controlled with the config setting "color.remote".
> >
> > Supported keywords are "error", "warning", "hint" and "success". They
> > are highlighted if they appear at the start of the line, which is
> > common in error messages, eg.
> >
> >ERROR: commit is missing Change-Id
>
> A few questions:
>
> - should this be documented somewhere in Documentation/technical/*protocol*?
> That way, server implementors can know to take advantage of it.
The protocol docs seem to work on a much different level. Maybe there
should be a "best practices" document or similar?
> - how does this interact with (future) internationalization of server
> messages? If my server knows that the client speaks French, should
> they send "ERROR:" anyway and rely on the client to translate it?
> Or are we deferring that question to a later day?
It doesn't, and we are deferring that question.
> [...]
> > The Git push process itself prints lots of non-actionable messages
> > (eg. bandwidth statistics, object counters for different phases of the
> > process), and my hypothesis is that these obscure the actionable error
> > messages that our server sends back. Highlighting keywords in the
> > draws more attention to actionable messages.
>
> I'd be interested in ways to minimize Git's contribution to this as
> well. E.g. can we make more use of \r to make client-produced progress
> messages take less screen real estate? Should some of the servers
> involved (e.g., Gerrit) do so as well?
Yes, I'm interested in this too, but I fear it would veer into a
territory that is prone to bikeshedding.
Gerrit should definitely also send less noise.
> > Finally, this solution is backwards compatible: many servers already
> > prefix their messages with "error", and they will benefit from this
> > change without requiring a server update.
>
> Yes, this seems like a compelling reason to follow this approach.
>
> [...]
> > --- 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.
>
> What positions in a remote message are matched? If a server prints a
> message about something being discouraged because it is error-prone,
> would the "error" in error-prone turn red?
yes, if it happened to occur after a line-break.
We could decide that we will only highlight
':' rest of line
or
'\n'
that would work for the Gerrit case, and would be useful forcing
function to uniformize remote error messages.
> > +struct kwtable {
>
> nit: perhaps kwtable_entry?
done.
> > +/*
> > + * Optionally highlight some keywords in remote output if they appear at
> > the
> > + * start of the line. This should be called for a single line only.
> > + */
> > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
>
> Can this be static?
Done.
> What does 'n' represent? Can the comment say? (Or if it's the length
> of src, can it be renamed to srclen?)
Added comment.
> Super-pedantic nit: even if there are multiple special words at the
> start, this will only highlight one. :) So it could say something
> like "Optionally check if the first word on the line is a keyword and
> highlight it if so."
Super pedantic answer: if people care about it that much, they can
read the 20 lines of code below :-)
> > +{
> > + int i;
> > + if (!want_color_stderr(use_sideband_colors())) {
>
> nit: whitespace damage (you can check for this with "git show --check").
> There's a bit more elsewhere.
ran tabify on whole file.
> > + for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> > + struct kwtable* p = keywords + i;
>
> nit: * should attach to the variable, C-style.
Done.
> You can use "make style" to do some automatic formatting (though this
> is a bit experimental, so do double-check the results).
sad panda:
hanwen@han-wen:~/vc/git$ make style
git clang-format --style file --diff --exten
[PATCH v5 2/2] sideband: highlight keywords in remote sideband output
The colorization is controlled with the config setting "color.remote".
Supported keywords are "error", "warning", "hint" and "success". They
are highlighted if they appear at the start of the line, which is
common in error messages, eg.
ERROR: commit is missing Change-Id
The Git push process itself prints lots of non-actionable messages
(eg. bandwidth statistics, object counters for different phases of the
process), which obscures actionable error messages that servers may
send back. Highlighting keywords in the sideband draws more attention
to those messages.
The background for this change is that Gerrit does server-side
processing to create or update code reviews, and actionable error
messages (eg. missing Change-Id) must be communicated back to the user
during the push. User research has shown that new users have trouble
seeing these messages.
The highlighting is done on the client rather than server side, so
servers don't have to grow capabilities to understand terminal escape
codes and terminal state. It also consistent with the current state
where Git is control of the local display (eg. prefixing messages with
"remote: ").
Finally, this solution is backwards compatible: many servers already
prefix their messages with "error", and they will benefit from this
change without requiring a server update. By contrast, a server-side
solution would likely require plumbing the TERM variable through the
git protocol, so it would require changes to both server and client.
Helped-by: Duy Nguyen
Signed-off-by: Han-Wen Nienhuys
---
Documentation/config.txt| 12 +++
help.c | 1 +
help.h | 1 +
sideband.c | 126 +---
t/t5409-colorize-remote-messages.sh | 80 ++
5 files changed, 210 insertions(+), 10 deletions(-)
create mode 100755 t/t5409-colorize-remote-messages.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 63365dcf3d..33bc1a3def 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1263,6 +1263,18 @@ color.push::
color.push.error::
Use customized color for push errors.
+color.remote::
+ If set, keywords at the start of the line are highlighted. The
+ keywords are "error", "warning", "hint" and "success", and are
+ matched case-insensitively. Maybe set to `always`, `false` (or
+ `never`) or `auto` (or `true`). If unset, then the value of
+ `color.ui` is used (`auto` by default).
+
+color.remote.::
+ Use customized color for each remote keyword. `` 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 3ebf0568db..b6cafcfc0a 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 f8b15323a6..9eab6a3f89 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 325bf0e974..239be2ec85 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,108 @@
#include "cache.h"
+#include "color.h"
+#include "config.h"
#include "pkt-line.h"
#include "sideband.h"
+#include "help.h"
+
+struct keyword_entry {
+ /*
+* We use keyword as config key so it should be a single alphanumeric
word.
+*/
+ const char *keyword;
+ char color[COLOR_MAXLEN];
+};
+
+static struct keyword_entry keywords[] = {
+ { "hint&quo
[PATCH v5 0/2] sideband: highlight keywords in remote sideband output
Address Jun's & Jrn's comments. Han-Wen Nienhuys (2): config: document git config getter return value sideband: highlight keywords in remote sideband output Documentation/config.txt| 12 +++ config.h| 7 +- help.c | 1 + help.h | 1 + sideband.c | 126 +--- t/t5409-colorize-remote-messages.sh | 80 ++ 6 files changed, 216 insertions(+), 11 deletions(-) create mode 100755 t/t5409-colorize-remote-messages.sh -- 2.18.0.597.ga71716f1ad-goog
[PATCH v5 1/2] config: document git config getter return value
Signed-off-by: Han-Wen Nienhuys
---
config.h | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/config.h b/config.h
index bb2f506b27..183b31ebf4 100644
--- a/config.h
+++ b/config.h
@@ -188,9 +188,14 @@ struct config_set {
extern void git_configset_init(struct config_set *cs);
extern int git_configset_add_file(struct config_set *cs, const char *filename);
-extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **value);
extern const struct string_list *git_configset_get_value_multi(struct
config_set *cs, const char *key);
extern void git_configset_clear(struct config_set *cs);
+
+/*
+ * These functions return 1 if not found, and 0 if found, leaving the found
+ * value in the 'dest' pointer.
+ */
+extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **dest);
extern int git_configset_get_string_const(struct config_set *cs, const char
*key, const char **dest);
extern int git_configset_get_string(struct config_set *cs, const char *key,
char **dest);
extern int git_configset_get_int(struct config_set *cs, const char *key, int
*dest);
--
2.18.0.597.ga71716f1ad-goog
Re: [PATCH v5 1/2] config: document git config getter return value
On Mon, Aug 6, 2018 at 6:32 PM Junio C Hamano wrote: > patch and queue it on its own merit (not that I think the other one > is not yet good enough---I haven't even looked at it yet ;-). I discovered that emacs tabify will happily also add tabs in the middle of the line, which breaks in the case of DUMB_SUFFIX. I can reroll a v6 if needed, though. -- 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 v5 2/2] sideband: highlight keywords in remote sideband output
On Mon, Aug 6, 2018 at 7:21 PM Junio C Hamano wrote:
>
> Han-Wen Nienhuys writes:
>
> > The Git push process itself prints lots of non-actionable messages
> > (eg. bandwidth statistics, object counters for different phases of the
> > process), which obscures actionable error messages that servers may
>
> s/which obscures/which obscure/, as I think that "which" refers to
> "messages" above.
>
Done.
> > The highlighting is done on the client rather than server side, so
> > servers don't have to grow capabilities to understand terminal escape
> > codes and terminal state. It also consistent with the current state
> > where Git is control of the local display (eg. prefixing messages with
> > "remote: ").
>
> Yup.
>
> When we introduce "the receiving end asks messages to be sent with
> such and such decoration" protocol support, we would want a lot more
> than just painting messages in color, e.g. i18n, verbosity, and even
> "Hey, I am a script, send them in json".
>
> Until that happens, let's keep things simpler. No i18n messages and
> no colored output over the wire.
Ack.
>
> > +color.remote::
> > + If set, keywords at the start of the line are highlighted. The
> > + keywords are "error", "warning", "hint" and "success", and are
> > + matched case-insensitively. Maybe set to `always`, `false` (or
> > + `never`) or `auto` (or `true`). If unset, then the value of
> > + `color.ui` is used (`auto` by default).
>
> Reads much better.
>
> I am still trying to find a concise way to help readers who saw a
> line that begins with "Warnings: foo bar bla" and accept that it is
> OK the early 7 chars are not painted. "... case-insensitively and
> honoring word boundary" is the best I came up so far, but I am
> afraid that is adding more words without hinting what I want to convey
> strongly enough, so I am not going to suggest that (at least not yet).
I would suggest that the phrase "keyword" implies a tokenization, so
I'd leave as is.
> > + for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> > + strbuf_reset(&sb);
> > + strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword);
>
> This design means future enhancement to allow more keywords will
> have to be done in the form of adding more "color.remote.",
> which means a few restrictions on them are cast in stone at the
> end-user facing design level, which we need to be careful about.
>
> Side note. I do not worry about the implementation level
> limitation at all. For example, the current code will not
> allow end-users and projects to add new keywords to be
> painted, as it relies on the keywords[] static array we can
> see above. But that implementation detail does not prevent
> us from improving it later to support more code in this
> codepath that notices "color.remote.failure" in config file
> and paint a line that begins with "failure:".
>
> Because the third-level "variable" name is case insensive, matching
> of any future keyword MUST be also done case insensitively.
>
> Also, as you mentioned elsewhere in this patch, the string that can
> be in the keyword MUST begin with an alphabetic and consists only of
> alphanumeric or dash.
>
> I do not think these limitations imposed by the design decision this
> patch is making are particularly bad ones---we just need to be aware
> of and firm about them. When somebody else comes in the future and
> wants to recognize "F A I L" as a custom keyword case sensitively,
> we must be able to comfortably say "no" to it.
>
> Side note. We _could_ instead use "remotemsg..color"
> namespace, as the subsection names at the second level is a
> lot looser, but I do not think it is a good idea to use in
> this application, as they are case sensitive.
>
> The above discussion may deserve to be in the log message as a
> record to tell future ourselves why we decided to use
> color.remote..
I added a note about case insensitivity.
> > + if (git_config_get_string(sb.buf, &value))
> > + continue;
> > + if (color_parse(value, keywords[i].color))
> > + die(_("config value %s is not a color: %s"), sb.buf,
> > value);
>
> That's a bit inconsistent, isn't it? If the configuration is not
> even a string, we ignore it and continue, but if it is a string, we
>
[PATCH v6 0/1] sideband: highlight keywords in remote sideband output
address Jun's comments. Han-Wen Nienhuys (1): sideband: highlight keywords in remote sideband output Documentation/config.txt| 12 +++ help.c | 1 + help.h | 1 + sideband.c | 127 ++-- t/t5409-colorize-remote-messages.sh | 87 +++ 5 files changed, 219 insertions(+), 9 deletions(-) create mode 100755 t/t5409-colorize-remote-messages.sh -- 2.18.0.597.ga71716f1ad-goog
[PATCH v6 1/1] sideband: highlight keywords in remote sideband output
The colorization is controlled with the config setting "color.remote".
Supported keywords are "error", "warning", "hint" and "success". They
are highlighted if they appear at the start of the line, which is
common in error messages, eg.
ERROR: commit is missing Change-Id
The Git push process itself prints lots of non-actionable messages
(eg. bandwidth statistics, object counters for different phases of the
process). This obscures actionable error messages that servers may
send back. Highlighting keywords in the sideband draws more attention
to those messages.
The background for this change is that Gerrit does server-side
processing to create or update code reviews, and actionable error
messages (eg. missing Change-Id) must be communicated back to the user
during the push. User research has shown that new users have trouble
seeing these messages.
The highlighting is done on the client rather than server side, so
servers don't have to grow capabilities to understand terminal escape
codes and terminal state. It also consistent with the current state
where Git is control of the local display (eg. prefixing messages with
"remote: ").
The highlighting can be configured using color.remote.
configuration settings. Since the keys are matched case insensitively,
we match the keywords case insensitively too.
Finally, this solution is backwards compatible: many servers already
prefix their messages with "error", and they will benefit from this
change without requiring a server update. By contrast, a server-side
solution would likely require plumbing the TERM variable through the
git protocol, so it would require changes to both server and client.
Helped-by: Duy Nguyen
Signed-off-by: Han-Wen Nienhuys
---
Documentation/config.txt| 12 +++
help.c | 1 +
help.h | 1 +
sideband.c | 127 ++--
t/t5409-colorize-remote-messages.sh | 87 +++
5 files changed, 219 insertions(+), 9 deletions(-)
create mode 100755 t/t5409-colorize-remote-messages.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 63365dcf3d..33bc1a3def 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1263,6 +1263,18 @@ color.push::
color.push.error::
Use customized color for push errors.
+color.remote::
+ If set, keywords at the start of the line are highlighted. The
+ keywords are "error", "warning", "hint" and "success", and are
+ matched case-insensitively. Maybe set to `always`, `false` (or
+ `never`) or `auto` (or `true`). If unset, then the value of
+ `color.ui` is used (`auto` by default).
+
+color.remote.::
+ Use customized color for each remote keyword. `` 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 3ebf0568db..b6cafcfc0a 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 f8b15323a6..9eab6a3f89 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 325bf0e974..d882e79a5f 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,110 @@
#include "cache.h"
+#include "color.h"
+#include "config.h"
#include "pkt-line.h"
#include "sideband.h"
+#include "help.h"
+
+struct keyword_entry {
+ /*
+* We use keyword as config key so it should
Re: [PATCH v5 2/2] sideband: highlight keywords in remote sideband output
On Mon, Aug 6, 2018 at 7:21 PM Junio C Hamano wrote: > > + If set, keywords at the start of the line are highlighted. The > > + keywords are "error", "warning", "hint" and "success", and are > > + matched case-insensitively. Maybe set to `always`, `false` (or > > + `never`) or `auto` (or `true`). If unset, then the value of > > + `color.ui` is used (`auto` by default). > > Reads much better. > > I am still trying to find a concise way to help readers who saw a > line that begins with "Warnings: foo bar bla" and accept that it is > OK the early 7 chars are not painted. "... case-insensitively and > honoring word boundary" is the best I came up so far, but I am > afraid that is adding more words without hinting what I want to convey > strongly enough, so I am not going to suggest that (at least not yet). > what do you think of the idea of requiring a colon explicitly? That would avoid spurious highlighting in case a keyword happens to be wrapped onto a line start. -- 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
[PATCH v7 0/1] sideband: highlight keywords in remote sideband output
Fix nits; remove debug printf. Han-Wen Nienhuys (1): sideband: highlight keywords in remote sideband output Documentation/config.txt| 12 +++ help.c | 1 + help.h | 1 + sideband.c | 125 ++-- t/t5409-colorize-remote-messages.sh | 87 +++ 5 files changed, 217 insertions(+), 9 deletions(-) create mode 100755 t/t5409-colorize-remote-messages.sh -- 2.18.0.597.ga71716f1ad-goog
[PATCH v7 1/1] sideband: highlight keywords in remote sideband output
The colorization is controlled with the config setting "color.remote".
Supported keywords are "error", "warning", "hint" and "success". They
are highlighted if they appear at the start of the line, which is
common in error messages, eg.
ERROR: commit is missing Change-Id
The Git push process itself prints lots of non-actionable messages
(eg. bandwidth statistics, object counters for different phases of the
process). This obscures actionable error messages that servers may
send back. Highlighting keywords in the sideband draws more attention
to those messages.
The background for this change is that Gerrit does server-side
processing to create or update code reviews, and actionable error
messages (eg. missing Change-Id) must be communicated back to the user
during the push. User research has shown that new users have trouble
seeing these messages.
The highlighting is done on the client rather than server side, so
servers don't have to grow capabilities to understand terminal escape
codes and terminal state. It also consistent with the current state
where Git is control of the local display (eg. prefixing messages with
"remote: ").
The highlighting can be configured using color.remote.
configuration settings. Since the keys are matched case insensitively,
we match the keywords case insensitively too.
Finally, this solution is backwards compatible: many servers already
prefix their messages with "error", and they will benefit from this
change without requiring a server update. By contrast, a server-side
solution would likely require plumbing the TERM variable through the
git protocol, so it would require changes to both server and client.
Helped-by: Duy Nguyen
Signed-off-by: Han-Wen Nienhuys
---
Documentation/config.txt| 12 +++
help.c | 1 +
help.h | 1 +
sideband.c | 125 ++--
t/t5409-colorize-remote-messages.sh | 87 +++
5 files changed, 217 insertions(+), 9 deletions(-)
create mode 100755 t/t5409-colorize-remote-messages.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 63365dcf3d..33bc1a3def 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1263,6 +1263,18 @@ color.push::
color.push.error::
Use customized color for push errors.
+color.remote::
+ If set, keywords at the start of the line are highlighted. The
+ keywords are "error", "warning", "hint" and "success", and are
+ matched case-insensitively. Maybe set to `always`, `false` (or
+ `never`) or `auto` (or `true`). If unset, then the value of
+ `color.ui` is used (`auto` by default).
+
+color.remote.::
+ Use customized color for each remote keyword. `` 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 3ebf0568db..b6cafcfc0a 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 f8b15323a6..9eab6a3f89 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 325bf0e974..1c6bb0e25b 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,109 @@
#include "cache.h"
+#include "color.h"
+#include "config.h"
#include "pkt-line.h"
#include "sideband.h"
+#include "help.h"
+
+struct keyword_entry {
+ /*
+* We use keyword as config key so it should
Re: [PATCH v7 0/1] sideband: highlight keywords in remote sideband output
On Tue, Aug 7, 2018 at 11:01 PM Junio C Hamano wrote: > > Han-Wen Nienhuys writes: > > > Fix nits; remove debug printf. > > > > Han-Wen Nienhuys (1): > > sideband: highlight keywords in remote sideband output > > > > Documentation/config.txt| 12 +++ > > help.c | 1 + > > help.h | 1 + > > sideband.c | 125 ++-- > > t/t5409-colorize-remote-messages.sh | 87 +++ > > 5 files changed, 217 insertions(+), 9 deletions(-) > > create mode 100755 t/t5409-colorize-remote-messages.sh > > > > -- > > 2.18.0.597.ga71716f1ad-goog > > I'll squash in the following while queuing for > > > > Thanks for sticking to the topic. Thanks, LGTM. -- 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: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output
On Sat, Aug 18, 2018 at 6:16 PM Junio C Hamano wrote:
> > Actually, let's just lose the conditional. strbuf_add() would catch
> > and issue an error message when it notices that we fed negative
> > count anyway ;-).
>
> So I'll have this applied on top of the original topic to prevent a
> buggy version from escaping the lab.
>
> -- >8 --
> Subject: [PATCH] sideband: do not read beyond the end of input
>
> The caller of maybe_colorize_sideband() gives a counted buffer
> , but the callee checked src[] as if it were a NUL terminated
> buffer. If src[] had all isspace() bytes in it, we would have made
> n negative, and then
>
> (1) made number of strncasecmp() calls to see if the remaining
> bytes in src[] matched keywords, reading beyond the end of the
> array (this actually happens even if n does not go negative),
> and/or
>
> (2) called strbuf_add() with negative count, most likely triggering
> the "you want to use way too much memory" error due to unsigned
> integer overflow.
>
> Fix both issues by making sure we do not go beyond &src[n].
>
> In the longer term we may want to accept size_t as parameter for
> clarity (even though we know that a sideband message we are painting
> typically would fit on a line on a terminal and int is sufficient).
> Write it down as a NEEDSWORK comment.
>
> Helped-by: Jonathan Nieder
> Signed-off-by: Junio C Hamano
> ---
> sideband.c | 8 ++--
> t/t5409-colorize-remote-messages.sh | 14 ++
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/sideband.c b/sideband.c
> index 1c6bb0e25b..368647acf8 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -65,6 +65,8 @@ void list_config_color_sideband_slots(struct string_list
> *list, const char *pref
> * Optionally highlight one keyword in remote output if it appears at the
> start
> * of the line. This should be called for a single line only, which is
> * passed as the first N characters of the SRC array.
> + *
> + * NEEDSWORK: use "size_t n" instead for clarity.
> */
> static void maybe_colorize_sideband(struct strbuf *dest, const char *src,
> int n)
> {
> @@ -75,7 +77,7 @@ static void maybe_colorize_sideband(struct strbuf *dest,
> const char *src, int n)
> return;
> }
>
> - while (isspace(*src)) {
> + while (0 < n && isspace(*src)) {
> strbuf_addch(dest, *src);
> src++;
> n--;
> @@ -84,6 +86,9 @@ static void maybe_colorize_sideband(struct strbuf *dest,
> const char *src, int n)
> for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> struct keyword_entry *p = keywords + i;
> int len = strlen(p->keyword);
> +
> + if (n <= len)
> + continue;
I would suggest
if (n < len) continue;
..
if (!strncasecmp(p->keyword, src, len) && (n == len || !isalnum(src[len]))) {
so we colorize a single line that looks like "warning" as well
Other than that, LGTM.
--
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: Re* [PATCH v7 1/1] sideband: highlight keywords in remote sideband output
and, thanks for cleaning up after me :) -- 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
[PATCH 0/2] Two small comment fixes.
Han-Wen Nienhuys (2): Fix typo in submodule.h Document the string_list structure string-list.h | 6 ++ submodule.h | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) -- 2.14.1.821.g8fa685d3b7-goog
[PATCH 2/2] Document the string_list structure
Signed-off-by: Han-Wen Nienhuys
---
string-list.h | 6 ++
1 file changed, 6 insertions(+)
diff --git a/string-list.h b/string-list.h
index 29bfb7ae4..08b534166 100644
--- a/string-list.h
+++ b/string-list.h
@@ -8,6 +8,12 @@ struct string_list_item {
typedef int (*compare_strings_fn)(const char *, const char *);
+/* A resizable array of strings. The strings are owned if
+ * 'strdup_strings' is set. It can be used as a sorted array, and a
+ * custom comparison may be given in 'cmp'. The field 'items[i].util'
+ * may be used to implement an array of pairs. In that case, the
+ * caller is responsible for managing memory pointed to by 'util'.
+ */
struct string_list {
struct string_list_item *items;
unsigned int nr, alloc;
--
2.14.1.821.g8fa685d3b7-goog
[PATCH 1/2] Fix typo in submodule.h
Signed-off-by: Han-Wen Nienhuys --- submodule.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/submodule.h b/submodule.h index 6b52133c8..f0da0277a 100644 --- a/submodule.h +++ b/submodule.h @@ -120,7 +120,7 @@ extern int submodule_move_head(const char *path, /* * Prepare the "env_array" parameter of a "struct child_process" for executing - * a submodule by clearing any repo-specific envirionment variables, but + * a submodule by clearing any repo-specific environment variables, but * retaining any config in the environment. */ extern void prepare_submodule_repo_env(struct argv_array *out); -- 2.14.1.821.g8fa685d3b7-goog
[GSoC][PATCH v4 1/4] submodule--helper: introduce get_submodule_displaypath()
LGTM with nits.
+static char *get_submodule_displaypath(const char *path, const char *prefix)
this could do with a comment
/* the result should be freed by the caller. */
+ } else if (super_prefix) {
+ int len = strlen(super_prefix);
+ const char *format = is_dir_sep(super_prefix[len - 1]) ? "%s%s"
: "%s/%s";
what if len == 0? The handling of '/' looks like a change from the original.
[GSoC][PATCH v4 3/4] submodule: port set_name_rev() from shell to C
LGTM with nits
commit message:
"revision name, and later handles its formating and printing."
typo: formatting
+ if (!capture_command(&cp, &sb, 0) && sb.len) {
+ strbuf_strip_suffix(&sb, "\n");
+ return strbuf_detach(&sb, NULL);
+ }
you discard all output if these commands fail, so if the argument is a
not a submodule, or the other is not a sha1, it will just print
nothing without error message. Maybe that is OK, though? I don't see
documentation for these commands, so maybe this is not meant to be
usable?
[GSoC][PATCH v4 4/4] submodule: port submodule subcommand 'status' from shell to C
+ const char *const git_submodule_helper_usage[] = {
+ N_("git submodule status [--quiet] [--cached] [--recursive]
[]"),
+ NULL
the manpage over here says
git submodule [--quiet] status [--cached] [--recursive] [--] [...]
ie. multiple path arguments. Should this usage string be tweaked?
+static void print_status(struct status_cb *info, char state, const char *path,
+const struct object_id *oid, const char *displaypath)
+{
could do with a comment. What are the options for the `state` char?
+ if (state == ' ' || state == '+') {
+ struct argv_array get_rev_args = ARGV_ARRAY_INIT;
+
+ argv_array_pushl(&get_rev_args, "get-rev-name",
+path, oid_to_hex(oid), NULL);
+ get_rev_name(get_rev_args.argc, get_rev_args.argv,
+info->prefix);
since you're not really subprocessing, can't you simply have a
do_print_rev_name(char *path, char *sha) {
..
printf("\n");
}
and call that directly? Or call compute_rev_name directly. Then you
don't have to do argv setup here.
Also, the name get_rev_name() is a little unfortunate, since it
doesn't return a name, but rather prints it. Maybe the functions
implementing helper commands could be named like:
command_get_rev_name
or similar.
[PATCH 3/4] Document submodule_to_gitdir
Signed-off-by: Han-Wen Nienhuys
---
submodule.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/submodule.c b/submodule.c
index b12600fc7..b66c23f5d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1997,6 +1997,9 @@ const char *get_superproject_working_tree(void)
return ret;
}
+/* Put the gitdir for a submodule (given relative to the main repository
worktree)
+ * into `buf`, or return -1 on error.
+ */
int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
{
const struct submodule *sub;
--
2.14.1.821.g8fa685d3b7-goog
[PATCH 2/4] Clarify return value ownership of real_path and read_gitfile_gently
Signed-off-by: Han-Wen Nienhuys
---
abspath.c | 3 +++
setup.c | 3 ++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/abspath.c b/abspath.c
index 708aff8d4..792a2d533 100644
--- a/abspath.c
+++ b/abspath.c
@@ -202,6 +202,9 @@ char *strbuf_realpath(struct strbuf *resolved, const char
*path,
return retval;
}
+/* Resolve `path` into an absolute, cleaned-up path. The return value
+ * comes from a global shared buffer and should not be freed.
+ */
const char *real_path(const char *path)
{
static struct strbuf realpath = STRBUF_INIT;
diff --git a/setup.c b/setup.c
index 6d8380acd..31853724c 100644
--- a/setup.c
+++ b/setup.c
@@ -541,7 +541,8 @@ void read_gitfile_error_die(int error_code, const char
*path, const char *dir)
/*
* Try to read the location of the git directory from the .git file,
- * return path to git directory if found.
+ * return path to git directory if found. The return value comes from
+ * a shared pool and should not be freed.
*
* On failure, if return_error_code is not NULL, return_error_code
* will be set to an error code and NULL will be returned. If
--
2.14.1.821.g8fa685d3b7-goog
[PATCH 4/4] Move documentation of string_list into string-list.h
This mirrors commit bdfdaa4978dd92992737e662f25adc01d32d0774 which did
the same for strbuf.h:
* API documentation uses /** */ to set it apart from other comments.
* Function names were stripped from the comments.
* Ordering of the header was adjusted to follow the one from the text
file.
* Edited some existing comments to follow the new standard.
Signed-off-by: Han-Wen Nienhuys
---
Documentation/technical/api-string-list.txt | 209
string-list.h | 187 +
2 files changed, 159 insertions(+), 237 deletions(-)
delete mode 100644 Documentation/technical/api-string-list.txt
diff --git a/Documentation/technical/api-string-list.txt
b/Documentation/technical/api-string-list.txt
deleted file mode 100644
index c08402b12..0
--- a/Documentation/technical/api-string-list.txt
+++ /dev/null
@@ -1,209 +0,0 @@
-string-list API
-===
-
-The string_list API offers a data structure and functions to handle
-sorted and unsorted string lists. A "sorted" list is one whose
-entries are sorted by string value in `strcmp()` order.
-
-The 'string_list' struct used to be called 'path_list', but was renamed
-because it is not specific to paths.
-
-The caller:
-
-. Allocates and clears a `struct string_list` variable.
-
-. Initializes the members. You might want to set the flag `strdup_strings`
- if the strings should be strdup()ed. For example, this is necessary
- when you add something like git_path("..."), since that function returns
- a static buffer that will change with the next call to git_path().
-+
-If you need something advanced, you can manually malloc() the `items`
-member (you need this if you add things later) and you should set the
-`nr` and `alloc` members in that case, too.
-
-. Adds new items to the list, using `string_list_append`,
- `string_list_append_nodup`, `string_list_insert`,
- `string_list_split`, and/or `string_list_split_in_place`.
-
-. Can check if a string is in the list using `string_list_has_string` or
- `unsorted_string_list_has_string` and get it from the list using
- `string_list_lookup` for sorted lists.
-
-. Can sort an unsorted list using `string_list_sort`.
-
-. Can remove duplicate items from a sorted list using
- `string_list_remove_duplicates`.
-
-. Can remove individual items of an unsorted list using
- `unsorted_string_list_delete_item`.
-
-. Can remove items not matching a criterion from a sorted or unsorted
- list using `filter_string_list`, or remove empty strings using
- `string_list_remove_empty_items`.
-
-. Finally it should free the list using `string_list_clear`.
-
-Example:
-
-
-struct string_list list = STRING_LIST_INIT_NODUP;
-int i;
-
-string_list_append(&list, "foo");
-string_list_append(&list, "bar");
-for (i = 0; i < list.nr; i++)
- printf("%s\n", list.items[i].string)
-
-
-NOTE: It is more efficient to build an unsorted list and sort it
-afterwards, instead of building a sorted list (`O(n log n)` instead of
-`O(n^2)`).
-+
-However, if you use the list to check if a certain string was added
-already, you should not do that (using unsorted_string_list_has_string()),
-because the complexity would be quadratic again (but with a worse factor).
-
-Functions
--
-
-* General ones (works with sorted and unsorted lists as well)
-
-`string_list_init`::
-
- Initialize the members of the string_list, set `strdup_strings`
- member according to the value of the second parameter.
-
-`filter_string_list`::
-
- Apply a function to each item in a list, retaining only the
- items for which the function returns true. If free_util is
- true, call free() on the util members of any items that have
- to be deleted. Preserve the order of the items that are
- retained.
-
-`string_list_remove_empty_items`::
-
- Remove any empty strings from the list. If free_util is true,
- call free() on the util members of any items that have to be
- deleted. Preserve the order of the items that are retained.
-
-`print_string_list`::
-
- Dump a string_list to stdout, useful mainly for debugging purposes. It
- can take an optional header argument and it writes out the
- string-pointer pairs of the string_list, each one in its own line.
-
-`string_list_clear`::
-
- Free a string_list. The `string` pointer of the items will be freed in
- case the `strdup_strings` member of the string_list is set. The second
- parameter controls if the `util` pointer of the items should be freed
- or not.
-
-* Functions for sorted lists only
-
-`string_list_has_string`::
-
- Determine if the string_list has a given string or not.
-
-`string_list_insert`::
-
- Insert a new element to the string_list. The returned pointer can be
- handy if you want to write something to the `util` point
[PATCH 0/4] Assorted comment fixes
I followed Peff's advice for string-list.h comments. Han-Wen Nienhuys (4): Fix typo in submodule.h Clarify return value ownership of real_path and read_gitfile_gently Document submodule_to_gitdir Move documentation of string_list into string-list.h Documentation/technical/api-string-list.txt | 209 abspath.c | 3 + setup.c | 3 +- string-list.h | 187 + submodule.c | 3 + submodule.h | 2 +- 6 files changed, 168 insertions(+), 239 deletions(-) delete mode 100644 Documentation/technical/api-string-list.txt -- 2.14.1.821.g8fa685d3b7-goog
[PATCH 1/4] Fix typo in submodule.h
Signed-off-by: Han-Wen Nienhuys --- submodule.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/submodule.h b/submodule.h index 6b52133c8..f0da0277a 100644 --- a/submodule.h +++ b/submodule.h @@ -120,7 +120,7 @@ extern int submodule_move_head(const char *path, /* * Prepare the "env_array" parameter of a "struct child_process" for executing - * a submodule by clearing any repo-specific envirionment variables, but + * a submodule by clearing any repo-specific environment variables, but * retaining any config in the environment. */ extern void prepare_submodule_repo_env(struct argv_array *out); -- 2.14.1.821.g8fa685d3b7-goog
[PATCH 2/3] read_gitfile_gently: clarify return value ownership.
Signed-off-by: Han-Wen Nienhuys --- setup.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/setup.c b/setup.c index 42400fcc5..8c95841d5 100644 --- a/setup.c +++ b/setup.c @@ -541,7 +541,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir) /* * Try to read the location of the git directory from the .git file, - * return path to git directory if found. + * return path to git directory if found. The return value comes from + * a shared buffer. * * On failure, if return_error_code is not NULL, return_error_code * will be set to an error code and NULL will be returned. If -- 2.14.1.821.g8fa685d3b7-goog
[PATCH 1/3] real_path: clarify return value ownership
Signed-off-by: Han-Wen Nienhuys
---
abspath.c | 4
1 file changed, 4 insertions(+)
diff --git a/abspath.c b/abspath.c
index 708aff8d4..985798532 100644
--- a/abspath.c
+++ b/abspath.c
@@ -202,6 +202,10 @@ char *strbuf_realpath(struct strbuf *resolved, const char
*path,
return retval;
}
+/*
+ * Resolve `path` into an absolute, cleaned-up path. The return value
+ * comes from a shared buffer.
+ */
const char *real_path(const char *path)
{
static struct strbuf realpath = STRBUF_INIT;
--
2.14.1.821.g8fa685d3b7-goog
[PATCH 0/3] Comment fixes
follow more commit log conventions; verified it compiled (yay). (should I send patches that are in 'pu' again as well?) Han-Wen Nienhuys (3): real_path: clarify return value ownership read_gitfile_gently: clarify return value ownership. string-list.h: move documentation from Documentation/api/ into header Documentation/technical/api-string-list.txt | 209 abspath.c | 4 + setup.c | 3 +- string-list.h | 192 + 4 files changed, 168 insertions(+), 240 deletions(-) delete mode 100644 Documentation/technical/api-string-list.txt -- 2.14.1.821.g8fa685d3b7-goog
[PATCH 3/3] string-list.h: move documentation from Documentation/api/ into header
This mirrors commit 'bdfdaa497 ("strbuf.h: integrate api-strbuf.txt
documentation, 2015-01-16") which did the same for strbuf.h:
* API documentation uses /** */ to set it apart from other comments.
* Function names were stripped from the comments.
* Ordering of the header was adjusted to follow the one from the text
file.
* Edited some existing comments from string-list.h for consistency.
Signed-off-by: Han-Wen Nienhuys
---
Documentation/technical/api-string-list.txt | 209
string-list.h | 192 +
2 files changed, 162 insertions(+), 239 deletions(-)
delete mode 100644 Documentation/technical/api-string-list.txt
diff --git a/Documentation/technical/api-string-list.txt
b/Documentation/technical/api-string-list.txt
deleted file mode 100644
index c08402b12..0
--- a/Documentation/technical/api-string-list.txt
+++ /dev/null
@@ -1,209 +0,0 @@
-string-list API
-===
-
-The string_list API offers a data structure and functions to handle
-sorted and unsorted string lists. A "sorted" list is one whose
-entries are sorted by string value in `strcmp()` order.
-
-The 'string_list' struct used to be called 'path_list', but was renamed
-because it is not specific to paths.
-
-The caller:
-
-. Allocates and clears a `struct string_list` variable.
-
-. Initializes the members. You might want to set the flag `strdup_strings`
- if the strings should be strdup()ed. For example, this is necessary
- when you add something like git_path("..."), since that function returns
- a static buffer that will change with the next call to git_path().
-+
-If you need something advanced, you can manually malloc() the `items`
-member (you need this if you add things later) and you should set the
-`nr` and `alloc` members in that case, too.
-
-. Adds new items to the list, using `string_list_append`,
- `string_list_append_nodup`, `string_list_insert`,
- `string_list_split`, and/or `string_list_split_in_place`.
-
-. Can check if a string is in the list using `string_list_has_string` or
- `unsorted_string_list_has_string` and get it from the list using
- `string_list_lookup` for sorted lists.
-
-. Can sort an unsorted list using `string_list_sort`.
-
-. Can remove duplicate items from a sorted list using
- `string_list_remove_duplicates`.
-
-. Can remove individual items of an unsorted list using
- `unsorted_string_list_delete_item`.
-
-. Can remove items not matching a criterion from a sorted or unsorted
- list using `filter_string_list`, or remove empty strings using
- `string_list_remove_empty_items`.
-
-. Finally it should free the list using `string_list_clear`.
-
-Example:
-
-
-struct string_list list = STRING_LIST_INIT_NODUP;
-int i;
-
-string_list_append(&list, "foo");
-string_list_append(&list, "bar");
-for (i = 0; i < list.nr; i++)
- printf("%s\n", list.items[i].string)
-
-
-NOTE: It is more efficient to build an unsorted list and sort it
-afterwards, instead of building a sorted list (`O(n log n)` instead of
-`O(n^2)`).
-+
-However, if you use the list to check if a certain string was added
-already, you should not do that (using unsorted_string_list_has_string()),
-because the complexity would be quadratic again (but with a worse factor).
-
-Functions
--
-
-* General ones (works with sorted and unsorted lists as well)
-
-`string_list_init`::
-
- Initialize the members of the string_list, set `strdup_strings`
- member according to the value of the second parameter.
-
-`filter_string_list`::
-
- Apply a function to each item in a list, retaining only the
- items for which the function returns true. If free_util is
- true, call free() on the util members of any items that have
- to be deleted. Preserve the order of the items that are
- retained.
-
-`string_list_remove_empty_items`::
-
- Remove any empty strings from the list. If free_util is true,
- call free() on the util members of any items that have to be
- deleted. Preserve the order of the items that are retained.
-
-`print_string_list`::
-
- Dump a string_list to stdout, useful mainly for debugging purposes. It
- can take an optional header argument and it writes out the
- string-pointer pairs of the string_list, each one in its own line.
-
-`string_list_clear`::
-
- Free a string_list. The `string` pointer of the items will be freed in
- case the `strdup_strings` member of the string_list is set. The second
- parameter controls if the `util` pointer of the items should be freed
- or not.
-
-* Functions for sorted lists only
-
-`string_list_has_string`::
-
- Determine if the string_list has a given string or not.
-
-`string_list_insert`::
-
- Insert a new element to the string_list. The returned pointer can be
- handy
Re: [PATCH 4/4] Move documentation of string_list into string-list.h
On Tue, Sep 26, 2017 at 7:22 AM, Junio C Hamano wrote: > Junio C Hamano writes: > >> >> Thanks. I am not sure if you can safely reorder the contents of the >> header files in general, but I trust that you made sure that this >> does not introduce problems (e.g. referrals before definition). > > Alas, this time it seems my trust was grossly misplaced. Discarding > this patch and redoing the integration cycle for the day. Oops, my bad. I resent the remaining patches. They do compile now :) -- 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: grep vs git grep performance?
On Thu, Oct 26, 2017 at 5:02 PM, Joe Perches wrote: > Comparing a cache warm git grep vs command line grep > shows significant differences in cpu & wall clock. > > Any ideas how to improve this? Is git-grep multithreaded? IIRC, grep -r uses multiple threads. (Do you have a 4-core machine?) -- 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
