2013-02-20 20:01:56 +00:00
|
|
|
#include "cache.h"
|
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.<KEYWORD>
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 <pclouds@gmail.com>
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-07 12:51:08 +00:00
|
|
|
#include "color.h"
|
|
|
|
#include "config.h"
|
2006-09-10 08:06:33 +00:00
|
|
|
#include "sideband.h"
|
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.<KEYWORD>
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 <pclouds@gmail.com>
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-07 12:51:08 +00:00
|
|
|
#include "help.h"
|
sideband: diagnose more sideband anomalies
In demultiplex_sideband(), there are two oddities when we check an
incoming packet:
- if it has zero length, then we assume it's a flush packet. This
means we fail to notice the difference between a real flush and a
true zero-length packet that's missing its sideband designator. It's
not a huge problem in practice because we'd never send a zero-length
data packet (even our keepalives are otherwise-empty sideband-1
packets).
But it would be nice to detect and report the error, since it's
likely to cause other confusion (we think the other side flushed,
but they do not).
- we try to detect packets missing their designator by checking for
"if (len < 1)". But this will never trigger for "len == 0"; we've
already detected that and left the function before then.
It _could_ detect a negative "len" parameter. But in that case, the
error message is wrong. The issue is not "no sideband" but rather
"eof while reading the packet". However, this can't actually be
triggered in practice, because neither of the two callers uses
pkt_read's GENTLE_ON_EOF flag. Which means they'd die with "the
remote end hung up unexpectedly" before we even get here.
So this truly is dead code.
We can improve these cases by passing in a pkt-line status to the
demultiplexer, and by having recv_sideband() use GENTLE_ON_EOF. This
gives us two improvements:
- we can now reliably detect flush packets, and will report a normal
packet missing its sideband designator as an error
- we'll report an eof with a more detailed "protocol error: eof while
reading sideband packet", rather than the generic "the remote end
hung up unexpectedly"
- when we see an eof, we'll flush the sideband scratch buffer, which
may provide some hints from the remote about why they hung up
(though note we already flush on newlines, so it's likely that most
such messages already made it through)
In some sense this patch goes against fbd76cd450 (sideband: reverse its
dependency on pkt-line, 2019-01-16), which caused the sideband code not
to depend on the pkt-line code. But that commit was really just trying
to deal with the circular header dependency. The two modules are
conceptually interlinked, and it was just trying to keep things
compiling. And indeed, there's a sticking point in this patch: because
pkt-line.h includes sideband.h, we can't add the reverse include we need
for the sideband code to have an "enum packet_read_status" parameter.
Nor can we forward declare it, because you can't forward declare an enum
in C. However, C does guarantee that enums fit in an int, so we can just
use that type.
One alternative would be for the callers to check themselves that they
got something sane from the pkt-line code. But besides duplicating
logic, this gets quite tricky. Any error condition requires flushing the
sideband #2 scratch buffer, which only demultiplex_sideband() knows how
to do.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-10-28 09:33:24 +00:00
|
|
|
#include "pkt-line.h"
|
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.<KEYWORD>
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 <pclouds@gmail.com>
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-07 12:51:08 +00:00
|
|
|
|
|
|
|
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", GIT_COLOR_YELLOW },
|
|
|
|
{ "warning", GIT_COLOR_BOLD_YELLOW },
|
|
|
|
{ "success", GIT_COLOR_BOLD_GREEN },
|
|
|
|
{ "error", GIT_COLOR_BOLD_RED },
|
|
|
|
};
|
|
|
|
|
|
|
|
/* Returns a color setting (GIT_COLOR_NEVER, etc). */
|
|
|
|
static int use_sideband_colors(void)
|
|
|
|
{
|
|
|
|
static int use_sideband_colors_cached = -1;
|
|
|
|
|
|
|
|
const char *key = "color.remote";
|
|
|
|
struct strbuf sb = STRBUF_INIT;
|
|
|
|
char *value;
|
|
|
|
int i;
|
|
|
|
|
|
|
|
if (use_sideband_colors_cached >= 0)
|
|
|
|
return use_sideband_colors_cached;
|
|
|
|
|
|
|
|
if (!git_config_get_string(key, &value)) {
|
|
|
|
use_sideband_colors_cached = git_config_colorbool(key, value);
|
|
|
|
} else if (!git_config_get_string("color.ui", &value)) {
|
|
|
|
use_sideband_colors_cached = git_config_colorbool("color.ui", value);
|
|
|
|
} else {
|
|
|
|
use_sideband_colors_cached = GIT_COLOR_AUTO;
|
|
|
|
}
|
|
|
|
|
|
|
|
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))
|
|
|
|
continue;
|
|
|
|
}
|
|
|
|
strbuf_release(&sb);
|
|
|
|
return use_sideband_colors_cached;
|
|
|
|
}
|
|
|
|
|
|
|
|
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 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.
|
2018-08-18 16:16:28 +00:00
|
|
|
*
|
|
|
|
* NEEDSWORK: use "size_t n" instead for clarity.
|
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.<KEYWORD>
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 <pclouds@gmail.com>
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-07 12:51:08 +00:00
|
|
|
*/
|
|
|
|
static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
|
|
|
|
{
|
|
|
|
int i;
|
|
|
|
|
|
|
|
if (!want_color_stderr(use_sideband_colors())) {
|
|
|
|
strbuf_add(dest, src, n);
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
2018-08-18 16:16:28 +00:00
|
|
|
while (0 < n && isspace(*src)) {
|
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.<KEYWORD>
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 <pclouds@gmail.com>
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-07 12:51:08 +00:00
|
|
|
strbuf_addch(dest, *src);
|
|
|
|
src++;
|
|
|
|
n--;
|
|
|
|
}
|
|
|
|
|
|
|
|
for (i = 0; i < ARRAY_SIZE(keywords); i++) {
|
|
|
|
struct keyword_entry *p = keywords + i;
|
|
|
|
int len = strlen(p->keyword);
|
2018-08-18 16:16:28 +00:00
|
|
|
|
2018-12-03 22:37:13 +00:00
|
|
|
if (n < len)
|
2018-08-18 16:16:28 +00:00
|
|
|
continue;
|
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.<KEYWORD>
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 <pclouds@gmail.com>
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-07 12:51:08 +00:00
|
|
|
/*
|
|
|
|
* 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.
|
|
|
|
*/
|
2018-12-03 22:37:13 +00:00
|
|
|
if (!strncasecmp(p->keyword, src, len) &&
|
|
|
|
(len == n || !isalnum(src[len]))) {
|
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.<KEYWORD>
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 <pclouds@gmail.com>
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-07 12:51:08 +00:00
|
|
|
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);
|
|
|
|
}
|
|
|
|
|
2006-09-10 08:06:33 +00:00
|
|
|
|
2018-04-21 11:18:42 +00:00
|
|
|
#define DISPLAY_PREFIX "remote: "
|
2008-01-08 16:24:53 +00:00
|
|
|
|
|
|
|
#define ANSI_SUFFIX "\033[K"
|
|
|
|
#define DUMB_SUFFIX " "
|
|
|
|
|
sideband: diagnose more sideband anomalies
In demultiplex_sideband(), there are two oddities when we check an
incoming packet:
- if it has zero length, then we assume it's a flush packet. This
means we fail to notice the difference between a real flush and a
true zero-length packet that's missing its sideband designator. It's
not a huge problem in practice because we'd never send a zero-length
data packet (even our keepalives are otherwise-empty sideband-1
packets).
But it would be nice to detect and report the error, since it's
likely to cause other confusion (we think the other side flushed,
but they do not).
- we try to detect packets missing their designator by checking for
"if (len < 1)". But this will never trigger for "len == 0"; we've
already detected that and left the function before then.
It _could_ detect a negative "len" parameter. But in that case, the
error message is wrong. The issue is not "no sideband" but rather
"eof while reading the packet". However, this can't actually be
triggered in practice, because neither of the two callers uses
pkt_read's GENTLE_ON_EOF flag. Which means they'd die with "the
remote end hung up unexpectedly" before we even get here.
So this truly is dead code.
We can improve these cases by passing in a pkt-line status to the
demultiplexer, and by having recv_sideband() use GENTLE_ON_EOF. This
gives us two improvements:
- we can now reliably detect flush packets, and will report a normal
packet missing its sideband designator as an error
- we'll report an eof with a more detailed "protocol error: eof while
reading sideband packet", rather than the generic "the remote end
hung up unexpectedly"
- when we see an eof, we'll flush the sideband scratch buffer, which
may provide some hints from the remote about why they hung up
(though note we already flush on newlines, so it's likely that most
such messages already made it through)
In some sense this patch goes against fbd76cd450 (sideband: reverse its
dependency on pkt-line, 2019-01-16), which caused the sideband code not
to depend on the pkt-line code. But that commit was really just trying
to deal with the circular header dependency. The two modules are
conceptually interlinked, and it was just trying to keep things
compiling. And indeed, there's a sticking point in this patch: because
pkt-line.h includes sideband.h, we can't add the reverse include we need
for the sideband code to have an "enum packet_read_status" parameter.
Nor can we forward declare it, because you can't forward declare an enum
in C. However, C does guarantee that enums fit in an int, so we can just
use that type.
One alternative would be for the callers to check themselves that they
got something sane from the pkt-line code. But besides duplicating
logic, this gets quite tricky. Any error condition requires flushing the
sideband #2 scratch buffer, which only demultiplex_sideband() knows how
to do.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-10-28 09:33:24 +00:00
|
|
|
int demultiplex_sideband(const char *me, int status,
|
|
|
|
char *buf, int len,
|
2019-01-16 19:28:14 +00:00
|
|
|
int die_on_error,
|
2019-01-16 19:28:13 +00:00
|
|
|
struct strbuf *scratch,
|
|
|
|
enum sideband_type *sideband_type)
|
2006-09-10 08:06:33 +00:00
|
|
|
{
|
2019-01-16 19:28:13 +00:00
|
|
|
static const char *suffix;
|
|
|
|
const char *b, *brk;
|
|
|
|
int band;
|
|
|
|
|
|
|
|
if (!suffix) {
|
|
|
|
if (isatty(2) && !is_terminal_dumb())
|
|
|
|
suffix = ANSI_SUFFIX;
|
|
|
|
else
|
|
|
|
suffix = DUMB_SUFFIX;
|
|
|
|
}
|
2016-06-28 04:35:26 +00:00
|
|
|
|
sideband: diagnose more sideband anomalies
In demultiplex_sideband(), there are two oddities when we check an
incoming packet:
- if it has zero length, then we assume it's a flush packet. This
means we fail to notice the difference between a real flush and a
true zero-length packet that's missing its sideband designator. It's
not a huge problem in practice because we'd never send a zero-length
data packet (even our keepalives are otherwise-empty sideband-1
packets).
But it would be nice to detect and report the error, since it's
likely to cause other confusion (we think the other side flushed,
but they do not).
- we try to detect packets missing their designator by checking for
"if (len < 1)". But this will never trigger for "len == 0"; we've
already detected that and left the function before then.
It _could_ detect a negative "len" parameter. But in that case, the
error message is wrong. The issue is not "no sideband" but rather
"eof while reading the packet". However, this can't actually be
triggered in practice, because neither of the two callers uses
pkt_read's GENTLE_ON_EOF flag. Which means they'd die with "the
remote end hung up unexpectedly" before we even get here.
So this truly is dead code.
We can improve these cases by passing in a pkt-line status to the
demultiplexer, and by having recv_sideband() use GENTLE_ON_EOF. This
gives us two improvements:
- we can now reliably detect flush packets, and will report a normal
packet missing its sideband designator as an error
- we'll report an eof with a more detailed "protocol error: eof while
reading sideband packet", rather than the generic "the remote end
hung up unexpectedly"
- when we see an eof, we'll flush the sideband scratch buffer, which
may provide some hints from the remote about why they hung up
(though note we already flush on newlines, so it's likely that most
such messages already made it through)
In some sense this patch goes against fbd76cd450 (sideband: reverse its
dependency on pkt-line, 2019-01-16), which caused the sideband code not
to depend on the pkt-line code. But that commit was really just trying
to deal with the circular header dependency. The two modules are
conceptually interlinked, and it was just trying to keep things
compiling. And indeed, there's a sticking point in this patch: because
pkt-line.h includes sideband.h, we can't add the reverse include we need
for the sideband code to have an "enum packet_read_status" parameter.
Nor can we forward declare it, because you can't forward declare an enum
in C. However, C does guarantee that enums fit in an int, so we can just
use that type.
One alternative would be for the callers to check themselves that they
got something sane from the pkt-line code. But besides duplicating
logic, this gets quite tricky. Any error condition requires flushing the
sideband #2 scratch buffer, which only demultiplex_sideband() knows how
to do.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-10-28 09:33:24 +00:00
|
|
|
if (status == PACKET_READ_EOF) {
|
2019-01-16 19:28:13 +00:00
|
|
|
strbuf_addf(scratch,
|
sideband: diagnose more sideband anomalies
In demultiplex_sideband(), there are two oddities when we check an
incoming packet:
- if it has zero length, then we assume it's a flush packet. This
means we fail to notice the difference between a real flush and a
true zero-length packet that's missing its sideband designator. It's
not a huge problem in practice because we'd never send a zero-length
data packet (even our keepalives are otherwise-empty sideband-1
packets).
But it would be nice to detect and report the error, since it's
likely to cause other confusion (we think the other side flushed,
but they do not).
- we try to detect packets missing their designator by checking for
"if (len < 1)". But this will never trigger for "len == 0"; we've
already detected that and left the function before then.
It _could_ detect a negative "len" parameter. But in that case, the
error message is wrong. The issue is not "no sideband" but rather
"eof while reading the packet". However, this can't actually be
triggered in practice, because neither of the two callers uses
pkt_read's GENTLE_ON_EOF flag. Which means they'd die with "the
remote end hung up unexpectedly" before we even get here.
So this truly is dead code.
We can improve these cases by passing in a pkt-line status to the
demultiplexer, and by having recv_sideband() use GENTLE_ON_EOF. This
gives us two improvements:
- we can now reliably detect flush packets, and will report a normal
packet missing its sideband designator as an error
- we'll report an eof with a more detailed "protocol error: eof while
reading sideband packet", rather than the generic "the remote end
hung up unexpectedly"
- when we see an eof, we'll flush the sideband scratch buffer, which
may provide some hints from the remote about why they hung up
(though note we already flush on newlines, so it's likely that most
such messages already made it through)
In some sense this patch goes against fbd76cd450 (sideband: reverse its
dependency on pkt-line, 2019-01-16), which caused the sideband code not
to depend on the pkt-line code. But that commit was really just trying
to deal with the circular header dependency. The two modules are
conceptually interlinked, and it was just trying to keep things
compiling. And indeed, there's a sticking point in this patch: because
pkt-line.h includes sideband.h, we can't add the reverse include we need
for the sideband code to have an "enum packet_read_status" parameter.
Nor can we forward declare it, because you can't forward declare an enum
in C. However, C does guarantee that enums fit in an int, so we can just
use that type.
One alternative would be for the callers to check themselves that they
got something sane from the pkt-line code. But besides duplicating
logic, this gets quite tricky. Any error condition requires flushing the
sideband #2 scratch buffer, which only demultiplex_sideband() knows how
to do.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-10-28 09:33:24 +00:00
|
|
|
"%s%s: unexpected disconnect while reading sideband packet",
|
2019-01-16 19:28:13 +00:00
|
|
|
scratch->len ? "\n" : "", me);
|
|
|
|
*sideband_type = SIDEBAND_PROTOCOL_ERROR;
|
|
|
|
goto cleanup;
|
|
|
|
}
|
sideband: diagnose more sideband anomalies
In demultiplex_sideband(), there are two oddities when we check an
incoming packet:
- if it has zero length, then we assume it's a flush packet. This
means we fail to notice the difference between a real flush and a
true zero-length packet that's missing its sideband designator. It's
not a huge problem in practice because we'd never send a zero-length
data packet (even our keepalives are otherwise-empty sideband-1
packets).
But it would be nice to detect and report the error, since it's
likely to cause other confusion (we think the other side flushed,
but they do not).
- we try to detect packets missing their designator by checking for
"if (len < 1)". But this will never trigger for "len == 0"; we've
already detected that and left the function before then.
It _could_ detect a negative "len" parameter. But in that case, the
error message is wrong. The issue is not "no sideband" but rather
"eof while reading the packet". However, this can't actually be
triggered in practice, because neither of the two callers uses
pkt_read's GENTLE_ON_EOF flag. Which means they'd die with "the
remote end hung up unexpectedly" before we even get here.
So this truly is dead code.
We can improve these cases by passing in a pkt-line status to the
demultiplexer, and by having recv_sideband() use GENTLE_ON_EOF. This
gives us two improvements:
- we can now reliably detect flush packets, and will report a normal
packet missing its sideband designator as an error
- we'll report an eof with a more detailed "protocol error: eof while
reading sideband packet", rather than the generic "the remote end
hung up unexpectedly"
- when we see an eof, we'll flush the sideband scratch buffer, which
may provide some hints from the remote about why they hung up
(though note we already flush on newlines, so it's likely that most
such messages already made it through)
In some sense this patch goes against fbd76cd450 (sideband: reverse its
dependency on pkt-line, 2019-01-16), which caused the sideband code not
to depend on the pkt-line code. But that commit was really just trying
to deal with the circular header dependency. The two modules are
conceptually interlinked, and it was just trying to keep things
compiling. And indeed, there's a sticking point in this patch: because
pkt-line.h includes sideband.h, we can't add the reverse include we need
for the sideband code to have an "enum packet_read_status" parameter.
Nor can we forward declare it, because you can't forward declare an enum
in C. However, C does guarantee that enums fit in an int, so we can just
use that type.
One alternative would be for the callers to check themselves that they
got something sane from the pkt-line code. But besides duplicating
logic, this gets quite tricky. Any error condition requires flushing the
sideband #2 scratch buffer, which only demultiplex_sideband() knows how
to do.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-10-28 09:33:24 +00:00
|
|
|
|
|
|
|
if (len < 0)
|
|
|
|
BUG("negative length on non-eof packet read");
|
|
|
|
|
|
|
|
if (len == 0) {
|
|
|
|
if (status == PACKET_READ_NORMAL) {
|
|
|
|
strbuf_addf(scratch,
|
|
|
|
"%s%s: protocol error: missing sideband designator",
|
|
|
|
scratch->len ? "\n" : "", me);
|
|
|
|
*sideband_type = SIDEBAND_PROTOCOL_ERROR;
|
|
|
|
} else {
|
|
|
|
/* covers flush, delim, etc */
|
|
|
|
*sideband_type = SIDEBAND_FLUSH;
|
|
|
|
}
|
|
|
|
goto cleanup;
|
|
|
|
}
|
|
|
|
|
2019-01-16 19:28:13 +00:00
|
|
|
band = buf[0] & 0xff;
|
|
|
|
buf[len] = '\0';
|
|
|
|
len--;
|
|
|
|
switch (band) {
|
|
|
|
case 3:
|
2019-01-16 19:28:14 +00:00
|
|
|
if (die_on_error)
|
2020-08-07 08:56:49 +00:00
|
|
|
die(_("remote error: %s"), buf + 1);
|
2019-01-16 19:28:13 +00:00
|
|
|
strbuf_addf(scratch, "%s%s", scratch->len ? "\n" : "",
|
|
|
|
DISPLAY_PREFIX);
|
|
|
|
maybe_colorize_sideband(scratch, buf + 1, len);
|
|
|
|
|
|
|
|
*sideband_type = SIDEBAND_REMOTE_ERROR;
|
|
|
|
break;
|
|
|
|
case 2:
|
|
|
|
b = buf + 1;
|
|
|
|
|
|
|
|
/*
|
|
|
|
* Append a suffix to each nonempty line to clear the
|
|
|
|
* end of the screen line.
|
|
|
|
*
|
|
|
|
* The output is accumulated in a buffer and
|
|
|
|
* each line is printed to stderr using
|
|
|
|
* write(2) to ensure inter-process atomicity.
|
|
|
|
*/
|
|
|
|
while ((brk = strpbrk(b, "\n\r"))) {
|
|
|
|
int linelen = brk - b;
|
|
|
|
|
|
|
|
if (!scratch->len)
|
|
|
|
strbuf_addstr(scratch, DISPLAY_PREFIX);
|
|
|
|
if (linelen > 0) {
|
|
|
|
maybe_colorize_sideband(scratch, b, linelen);
|
|
|
|
strbuf_addstr(scratch, suffix);
|
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.<KEYWORD>
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 <pclouds@gmail.com>
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-07 12:51:08 +00:00
|
|
|
}
|
2019-01-16 19:28:13 +00:00
|
|
|
|
|
|
|
strbuf_addch(scratch, *brk);
|
|
|
|
xwrite(2, scratch->buf, scratch->len);
|
|
|
|
strbuf_reset(scratch);
|
|
|
|
|
|
|
|
b = brk + 1;
|
|
|
|
}
|
|
|
|
|
|
|
|
if (*b) {
|
|
|
|
strbuf_addstr(scratch, scratch->len ?
|
|
|
|
"" : DISPLAY_PREFIX);
|
|
|
|
maybe_colorize_sideband(scratch, b, strlen(b));
|
2006-09-10 08:06:33 +00:00
|
|
|
}
|
2019-01-16 19:28:13 +00:00
|
|
|
return 0;
|
|
|
|
case 1:
|
|
|
|
*sideband_type = SIDEBAND_PRIMARY;
|
sideband: avoid reporting incomplete sideband messages
In 2b695ecd74d (t5500: count objects through stderr, not trace,
2020-05-06) we tried to ensure that the "Total 3" message could be
grepped in Git's output, even if it sometimes got chopped up into
multiple lines in the trace machinery.
However, the first instance where this mattered now goes through the
sideband machinery, where it is _still_ possible for messages to get
chopped up: it *is* possible for the standard error stream to be sent
byte-for-byte and hence it can be easily interrupted. Meaning: it is
possible for the single line that we're looking for to be chopped up
into multiple sideband packets, with a primary packet being delivered
between them.
This seems to happen occasionally in the `vs-test` part of our CI
builds, i.e. with binaries built using Visual C, but not when building
with GCC or clang; The symptom is that t5500.43 fails to find a line
matching `remote: Total 3` in the `log` file, which ends in something
along these lines:
remote: Tota
remote: l 3 (delta 0), reused 0 (delta 0), pack-reused 0
This should not happen, though: we have code in `demultiplex_sideband()`
_specifically_ to stitch back together lines that were delivered in
separate sideband packets.
However, this stitching was broken in a subtle way in fbd76cd450
(sideband: reverse its dependency on pkt-line, 2019-01-16): before that
change, incomplete sideband lines would not be flushed upon receiving a
primary packet, but after that patch, they would be.
The subtleness of this bug comes from the fact that it is easy to get
confused by the ambiguous meaning of the `break` keyword: after writing
the primary packet contents, the `break;` in the original version of
`recv_sideband()` does _not_ break out of the `while` loop, but instead
only ends the `switch` case:
while (!retval) {
[...]
switch (band) {
[...]
case 1:
/* Write the contents of the primary packet */
write_or_die(out, buf + 1, len);
/* Here, we do *not* break out of the loop, `retval` is unchanged */
break;
[...]
}
if (outbuf.len) {
/* Write any remaining sideband messages lacking a trailing LF */
strbuf_addch(&outbuf, '\n');
xwrite(2, outbuf.buf, outbuf.len);
}
In contrast, after fbd76cd450 (sideband: reverse its dependency on
pkt-line, 2019-01-16), the body of the `while` loop was extracted into
`demultiplex_sideband()`, crucially _including_ the logic to write
incomplete sideband messages:
switch (band) {
[...]
case 1:
*sideband_type = SIDEBAND_PRIMARY;
/* This does not break out of the loop: the loop is in the caller */
break;
[...]
}
cleanup:
[...]
/* This logic is now no longer _outside_ the loop but _inside_ */
if (scratch->len) {
strbuf_addch(scratch, '\n');
xwrite(2, scratch->buf, scratch->len);
}
The correct way to fix this is to return from `demultiplex_sideband()`
early. The caller will then write out the contents of the primary packet
and continue looping. The `scratch` buffer for incomplete sideband
messages is owned by that caller, and will continue to accumulate the
remainder(s) of those messages. The loop will only end once
`demultiplex_sideband()` returned non-zero _and_ did not indicate a
primary packet, which is the case only when we hit the `cleanup:` path,
in which we take care of flushing any unfinished sideband messages and
release the `scratch` buffer.
To ensure that this does not get broken again, we introduce a pair of
subcommands of the `pkt-line` test helper that specifically chop up the
sideband message and squeeze a primary packet into the middle.
Final note: The other test case touched by 2b695ecd74d (t5500: count
objects through stderr, not trace, 2020-05-06) is not affected by this
issue because the sideband machinery is not involved there.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-10-19 19:35:40 +00:00
|
|
|
return 1;
|
2019-01-16 19:28:13 +00:00
|
|
|
default:
|
|
|
|
strbuf_addf(scratch, "%s%s: protocol error: bad band #%d",
|
|
|
|
scratch->len ? "\n" : "", me, band);
|
|
|
|
*sideband_type = SIDEBAND_PROTOCOL_ERROR;
|
|
|
|
break;
|
2006-09-10 08:06:33 +00:00
|
|
|
}
|
2016-06-28 04:35:26 +00:00
|
|
|
|
2019-01-16 19:28:13 +00:00
|
|
|
cleanup:
|
2019-01-16 19:28:14 +00:00
|
|
|
if (die_on_error && *sideband_type == SIDEBAND_PROTOCOL_ERROR)
|
|
|
|
die("%s", scratch->buf);
|
2019-01-16 19:28:13 +00:00
|
|
|
if (scratch->len) {
|
|
|
|
strbuf_addch(scratch, '\n');
|
|
|
|
xwrite(2, scratch->buf, scratch->len);
|
2016-06-28 04:35:26 +00:00
|
|
|
}
|
2019-01-16 19:28:13 +00:00
|
|
|
strbuf_release(scratch);
|
|
|
|
return 1;
|
2006-09-10 08:06:33 +00:00
|
|
|
}
|
2006-09-10 10:20:24 +00:00
|
|
|
|
|
|
|
/*
|
|
|
|
* fd is connected to the remote side; send the sideband data
|
|
|
|
* over multiplexed packet stream.
|
|
|
|
*/
|
2016-06-14 14:49:16 +00:00
|
|
|
void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max)
|
2006-09-10 10:20:24 +00:00
|
|
|
{
|
|
|
|
const char *p = data;
|
|
|
|
|
|
|
|
while (sz) {
|
|
|
|
unsigned n;
|
|
|
|
char hdr[5];
|
|
|
|
|
|
|
|
n = sz;
|
|
|
|
if (packet_max - 5 < n)
|
|
|
|
n = packet_max - 5;
|
2009-10-31 00:47:41 +00:00
|
|
|
if (0 <= band) {
|
2015-09-24 21:06:08 +00:00
|
|
|
xsnprintf(hdr, sizeof(hdr), "%04x", n + 5);
|
2009-10-31 00:47:41 +00:00
|
|
|
hdr[4] = band;
|
2013-02-20 20:01:56 +00:00
|
|
|
write_or_die(fd, hdr, 5);
|
2009-10-31 00:47:41 +00:00
|
|
|
} else {
|
2015-09-24 21:06:08 +00:00
|
|
|
xsnprintf(hdr, sizeof(hdr), "%04x", n + 4);
|
2013-02-20 20:01:56 +00:00
|
|
|
write_or_die(fd, hdr, 4);
|
2009-10-31 00:47:41 +00:00
|
|
|
}
|
2013-02-20 20:01:56 +00:00
|
|
|
write_or_die(fd, p, n);
|
2006-09-10 10:20:24 +00:00
|
|
|
p += n;
|
|
|
|
sz -= n;
|
|
|
|
}
|
|
|
|
}
|