* [PATCH 0/4] Sideband the whole fetch v2 response
@ 2019-01-11 22:18 Jonathan Tan
2019-01-11 22:18 ` [PATCH 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
` (9 more replies)
0 siblings, 10 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-11 22:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
This is on ms/packet-err-check.
This is a follow-up of my earlier work to allow partial CDN offloading
of fetch v2 response packfiles, which had the issue of having to buffer
(or suspend) progress and keepalive messages because we didn't have
sideband until the packfile section started (as I wrote here [1]). These
patches expand the sideband to the whole response, eliminating that
problem.
There are 8 patches total. I'm most interested in review of the first 4
patches for inclusion into Git - I think it would be useful for servers
to be allowed to send progress messages and other notices whenever they
need to, especially if other sections are added to the response (like in
the case of CDN offloading).
The other 4 patches are from my CDN offloading work, included here to
show how this sideband-all feature could be used. But take note that
they are all WIP and that I still haven't incorporated some of the
design considerations from the earlier review [2].
[1] https://public-inbox.org/git/20181206232538.141378-1-jonathantanmy@google.com/
[2] https://public-inbox.org/git/cover.1543879256.git.jonathantanmy@google.com/
Jonathan Tan (8):
pkt-line: introduce struct packet_writer
sideband: reverse its dependency on pkt-line
{fetch,upload}-pack: sideband v2 fetch response
tests: define GIT_TEST_SIDEBAND_ALL
Documentation: order protocol v2 sections
Documentation: add Packfile URIs design doc
upload-pack: refactor reading of pack-objects out
upload-pack: send part of packfile response as uri
Documentation/technical/packfile-uri.txt | 83 ++++++++
Documentation/technical/protocol-v2.txt | 32 ++-
builtin/pack-objects.c | 48 +++++
fetch-pack.c | 22 +-
pkt-line.c | 114 ++++++++--
pkt-line.h | 34 +++
sideband.c | 161 +++++++-------
sideband.h | 15 +-
t/README | 5 +
t/lib-httpd/apache.conf | 1 +
t/t5537-fetch-shallow.sh | 3 +-
t/t5701-git-serve.sh | 2 +-
t/t5702-protocol-v2.sh | 31 ++-
upload-pack.c | 259 +++++++++++++++--------
14 files changed, 607 insertions(+), 203 deletions(-)
create mode 100644 Documentation/technical/packfile-uri.txt
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/4] pkt-line: introduce struct packet_writer
2019-01-11 22:18 [PATCH 0/4] Sideband the whole fetch v2 response Jonathan Tan
@ 2019-01-11 22:18 ` Jonathan Tan
2019-01-14 20:07 ` Junio C Hamano
2019-01-11 22:18 ` [PATCH 2/4] sideband: reverse its dependency on pkt-line Jonathan Tan
` (8 subsequent siblings)
9 siblings, 1 reply; 41+ messages in thread
From: Jonathan Tan @ 2019-01-11 22:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
It will be convenient for a future patch if writing options
(specifically, whether the written data is to be multiplexed) could be
controlled from a single place, so create struct packet_writer to serve
as that place, and modify upload-pack to use it.
Currently, it only stores the output fd, but a subsequent patch will (as
described above) introduce an option to determine if the written data is
to be multiplexed.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
pkt-line.c | 47 ++++++++++++++++++---
pkt-line.h | 14 +++++++
upload-pack.c | 112 +++++++++++++++++++++++++++-----------------------
3 files changed, 115 insertions(+), 58 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index e70ea6d88f..9d3e402d41 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -129,12 +129,14 @@ static void set_packet_header(char *buf, const int size)
#undef hex
}
-static void format_packet(struct strbuf *out, const char *fmt, va_list args)
+static void format_packet(struct strbuf *out, const char *prefix,
+ const char *fmt, va_list args)
{
size_t orig_len, n;
orig_len = out->len;
strbuf_addstr(out, "0000");
+ strbuf_addstr(out, prefix);
strbuf_vaddf(out, fmt, args);
n = out->len - orig_len;
@@ -145,13 +147,13 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)
packet_trace(out->buf + orig_len + 4, n - 4, 1);
}
-static int packet_write_fmt_1(int fd, int gently,
+static int packet_write_fmt_1(int fd, int gently, const char *prefix,
const char *fmt, va_list args)
{
static struct strbuf buf = STRBUF_INIT;
strbuf_reset(&buf);
- format_packet(&buf, fmt, args);
+ format_packet(&buf, prefix, fmt, args);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
if (!gently) {
check_pipe(errno);
@@ -168,7 +170,7 @@ void packet_write_fmt(int fd, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- packet_write_fmt_1(fd, 0, fmt, args);
+ packet_write_fmt_1(fd, 0, "", fmt, args);
va_end(args);
}
@@ -178,7 +180,7 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- status = packet_write_fmt_1(fd, 1, fmt, args);
+ status = packet_write_fmt_1(fd, 1, "", fmt, args);
va_end(args);
return status;
}
@@ -211,7 +213,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- format_packet(buf, fmt, args);
+ format_packet(buf, "", fmt, args);
va_end(args);
}
@@ -486,3 +488,36 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)
reader->line_peeked = 1;
return reader->status;
}
+
+void packet_writer_init(struct packet_writer *writer, int dest_fd)
+{
+ writer->dest_fd = dest_fd;
+}
+
+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
+{
+ va_list args;
+
+ va_start(args, fmt);
+ packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
+ va_end(args);
+}
+
+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
+{
+ va_list args;
+
+ va_start(args, fmt);
+ packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
+ va_end(args);
+}
+
+void packet_writer_delim(struct packet_writer *writer)
+{
+ packet_delim(writer->dest_fd);
+}
+
+void packet_writer_flush(struct packet_writer *writer)
+{
+ packet_flush(writer->dest_fd);
+}
diff --git a/pkt-line.h b/pkt-line.h
index d7e1dbc047..023ad2951d 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -183,4 +183,18 @@ extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);
#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
extern char packet_buffer[LARGE_PACKET_MAX];
+struct packet_writer {
+ int dest_fd;
+};
+
+void packet_writer_init(struct packet_writer *writer, int dest_fd);
+
+/* These functions die upon failure. */
+__attribute__((format (printf, 2, 3)))
+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...);
+__attribute__((format (printf, 2, 3)))
+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...);
+void packet_writer_delim(struct packet_writer *writer);
+void packet_writer_flush(struct packet_writer *writer);
+
#endif
diff --git a/upload-pack.c b/upload-pack.c
index 08b547cf01..60a26bbbfd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -613,13 +613,14 @@ static void check_non_tip(struct object_array *want_obj)
}
}
-static void send_shallow(struct commit_list *result)
+static void send_shallow(struct packet_writer *writer,
+ struct commit_list *result)
{
while (result) {
struct object *object = &result->item->object;
if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
- packet_write_fmt(1, "shallow %s",
- oid_to_hex(&object->oid));
+ packet_writer_write(writer, "shallow %s",
+ oid_to_hex(&object->oid));
register_shallow(the_repository, &object->oid);
shallow_nr++;
}
@@ -627,7 +628,8 @@ static void send_shallow(struct commit_list *result)
}
}
-static void send_unshallow(const struct object_array *shallows,
+static void send_unshallow(struct packet_writer *writer,
+ const struct object_array *shallows,
struct object_array *want_obj)
{
int i;
@@ -636,8 +638,8 @@ static void send_unshallow(const struct object_array *shallows,
struct object *object = shallows->objects[i].item;
if (object->flags & NOT_SHALLOW) {
struct commit_list *parents;
- packet_write_fmt(1, "unshallow %s",
- oid_to_hex(&object->oid));
+ packet_writer_write(writer, "unshallow %s",
+ oid_to_hex(&object->oid));
object->flags &= ~CLIENT_SHALLOW;
/*
* We want to _register_ "object" as shallow, but we
@@ -662,7 +664,7 @@ static void send_unshallow(const struct object_array *shallows,
}
}
-static void deepen(int depth, int deepen_relative,
+static void deepen(struct packet_writer *writer, int depth, int deepen_relative,
struct object_array *shallows, struct object_array *want_obj)
{
if (depth == INFINITE_DEPTH && !is_repository_shallow(the_repository)) {
@@ -680,7 +682,7 @@ static void deepen(int depth, int deepen_relative,
result = get_shallow_commits(&reachable_shallows,
depth + 1,
SHALLOW, NOT_SHALLOW);
- send_shallow(result);
+ send_shallow(writer, result);
free_commit_list(result);
object_array_clear(&reachable_shallows);
} else {
@@ -688,14 +690,15 @@ static void deepen(int depth, int deepen_relative,
result = get_shallow_commits(want_obj, depth,
SHALLOW, NOT_SHALLOW);
- send_shallow(result);
+ send_shallow(writer, result);
free_commit_list(result);
}
- send_unshallow(shallows, want_obj);
+ send_unshallow(writer, shallows, want_obj);
}
-static void deepen_by_rev_list(int ac, const char **av,
+static void deepen_by_rev_list(struct packet_writer *writer, int ac,
+ const char **av,
struct object_array *shallows,
struct object_array *want_obj)
{
@@ -703,13 +706,14 @@ static void deepen_by_rev_list(int ac, const char **av,
close_commit_graph(the_repository);
result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
- send_shallow(result);
+ send_shallow(writer, result);
free_commit_list(result);
- send_unshallow(shallows, want_obj);
+ send_unshallow(writer, shallows, want_obj);
}
/* Returns 1 if a shallow list is sent or 0 otherwise */
-static int send_shallow_list(int depth, int deepen_rev_list,
+static int send_shallow_list(struct packet_writer *writer,
+ int depth, int deepen_rev_list,
timestamp_t deepen_since,
struct string_list *deepen_not,
struct object_array *shallows,
@@ -720,7 +724,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,
if (depth > 0 && deepen_rev_list)
die("git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together");
if (depth > 0) {
- deepen(depth, deepen_relative, shallows, want_obj);
+ deepen(writer, depth, deepen_relative, shallows, want_obj);
ret = 1;
} else if (deepen_rev_list) {
struct argv_array av = ARGV_ARRAY_INIT;
@@ -741,7 +745,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,
struct object *o = want_obj->objects[i].item;
argv_array_push(&av, oid_to_hex(&o->oid));
}
- deepen_by_rev_list(av.argc, av.argv, shallows, want_obj);
+ deepen_by_rev_list(writer, av.argc, av.argv, shallows, want_obj);
argv_array_clear(&av);
ret = 1;
} else {
@@ -834,8 +838,10 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
int has_non_tip = 0;
timestamp_t deepen_since = 0;
int deepen_rev_list = 0;
+ struct packet_writer writer;
shallow_nr = 0;
+ packet_writer_init(&writer, 1);
for (;;) {
struct object *o;
const char *features;
@@ -892,9 +898,9 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
o = parse_object(the_repository, &oid_buf);
if (!o) {
- packet_write_fmt(1,
- "ERR upload-pack: not our ref %s",
- oid_to_hex(&oid_buf));
+ packet_writer_error(&writer,
+ "upload-pack: not our ref %s",
+ oid_to_hex(&oid_buf));
die("git upload-pack: not our ref %s",
oid_to_hex(&oid_buf));
}
@@ -923,7 +929,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
return;
- if (send_shallow_list(depth, deepen_rev_list, deepen_since,
+ if (send_shallow_list(&writer, depth, deepen_rev_list, deepen_since,
&deepen_not, &shallows, want_obj))
packet_flush(1);
object_array_clear(&shallows);
@@ -1102,6 +1108,8 @@ struct upload_pack_data {
int deepen_rev_list;
int deepen_relative;
+ struct packet_writer writer;
+
unsigned stateless_rpc : 1;
unsigned use_thin_pack : 1;
@@ -1125,6 +1133,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
data->haves = haves;
data->shallows = shallows;
data->deepen_not = deepen_not;
+ packet_writer_init(&data->writer, 1);
}
static void upload_pack_data_clear(struct upload_pack_data *data)
@@ -1136,7 +1145,8 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
string_list_clear(&data->deepen_not, 0);
}
-static int parse_want(const char *line, struct object_array *want_obj)
+static int parse_want(struct packet_writer *writer, const char *line,
+ struct object_array *want_obj)
{
const char *arg;
if (skip_prefix(line, "want ", &arg)) {
@@ -1149,9 +1159,9 @@ static int parse_want(const char *line, struct object_array *want_obj)
o = parse_object(the_repository, &oid);
if (!o) {
- packet_write_fmt(1,
- "ERR upload-pack: not our ref %s",
- oid_to_hex(&oid));
+ packet_writer_error(writer,
+ "upload-pack: not our ref %s",
+ oid_to_hex(&oid));
die("git upload-pack: not our ref %s",
oid_to_hex(&oid));
}
@@ -1167,7 +1177,8 @@ static int parse_want(const char *line, struct object_array *want_obj)
return 0;
}
-static int parse_want_ref(const char *line, struct string_list *wanted_refs,
+static int parse_want_ref(struct packet_writer *writer, const char *line,
+ struct string_list *wanted_refs,
struct object_array *want_obj)
{
const char *arg;
@@ -1177,7 +1188,7 @@ static int parse_want_ref(const char *line, struct string_list *wanted_refs,
struct object *o;
if (read_ref(arg, &oid)) {
- packet_write_fmt(1, "ERR unknown ref %s", arg);
+ packet_writer_error(writer, "unknown ref %s", arg);
die("unknown ref %s", arg);
}
@@ -1220,10 +1231,11 @@ static void process_args(struct packet_reader *request,
const char *p;
/* process want */
- if (parse_want(arg, want_obj))
+ if (parse_want(&data->writer, arg, want_obj))
continue;
if (allow_ref_in_want &&
- parse_want_ref(arg, &data->wanted_refs, want_obj))
+ parse_want_ref(&data->writer, arg, &data->wanted_refs,
+ want_obj))
continue;
/* process have line */
if (parse_have(arg, &data->haves))
@@ -1317,26 +1329,26 @@ static int process_haves(struct oid_array *haves, struct oid_array *common,
return 0;
}
-static int send_acks(struct oid_array *acks, struct strbuf *response,
+static int send_acks(struct packet_writer *writer, struct oid_array *acks,
const struct object_array *have_obj,
struct object_array *want_obj)
{
int i;
- packet_buf_write(response, "acknowledgments\n");
+ packet_writer_write(writer, "acknowledgments\n");
/* Send Acks */
if (!acks->nr)
- packet_buf_write(response, "NAK\n");
+ packet_writer_write(writer, "NAK\n");
for (i = 0; i < acks->nr; i++) {
- packet_buf_write(response, "ACK %s\n",
- oid_to_hex(&acks->oid[i]));
+ packet_writer_write(writer, "ACK %s\n",
+ oid_to_hex(&acks->oid[i]));
}
if (ok_to_give_up(have_obj, want_obj)) {
/* Send Ready */
- packet_buf_write(response, "ready\n");
+ packet_writer_write(writer, "ready\n");
return 1;
}
@@ -1348,25 +1360,20 @@ static int process_haves_and_send_acks(struct upload_pack_data *data,
struct object_array *want_obj)
{
struct oid_array common = OID_ARRAY_INIT;
- struct strbuf response = STRBUF_INIT;
int ret = 0;
process_haves(&data->haves, &common, have_obj);
if (data->done) {
ret = 1;
- } else if (send_acks(&common, &response, have_obj, want_obj)) {
- packet_buf_delim(&response);
+ } else if (send_acks(&data->writer, &common, have_obj, want_obj)) {
+ packet_writer_delim(&data->writer);
ret = 1;
} else {
/* Add Flush */
- packet_buf_flush(&response);
+ packet_writer_flush(&data->writer);
ret = 0;
}
- /* Send response */
- write_or_die(1, response.buf, response.len);
- strbuf_release(&response);
-
oid_array_clear(&data->haves);
oid_array_clear(&common);
return ret;
@@ -1379,15 +1386,15 @@ static void send_wanted_ref_info(struct upload_pack_data *data)
if (!data->wanted_refs.nr)
return;
- packet_write_fmt(1, "wanted-refs\n");
+ packet_writer_write(&data->writer, "wanted-refs\n");
for_each_string_list_item(item, &data->wanted_refs) {
- packet_write_fmt(1, "%s %s\n",
- oid_to_hex(item->util),
- item->string);
+ packet_writer_write(&data->writer, "%s %s\n",
+ oid_to_hex(item->util),
+ item->string);
}
- packet_delim(1);
+ packet_writer_delim(&data->writer);
}
static void send_shallow_info(struct upload_pack_data *data,
@@ -1398,14 +1405,15 @@ static void send_shallow_info(struct upload_pack_data *data,
!is_repository_shallow(the_repository))
return;
- packet_write_fmt(1, "shallow-info\n");
+ packet_writer_write(&data->writer, "shallow-info\n");
- if (!send_shallow_list(data->depth, data->deepen_rev_list,
+ if (!send_shallow_list(&data->writer, data->depth,
+ data->deepen_rev_list,
data->deepen_since, &data->deepen_not,
&data->shallows, want_obj) &&
is_repository_shallow(the_repository))
- deepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,
- want_obj);
+ deepen(&data->writer, INFINITE_DEPTH, data->deepen_relative,
+ &data->shallows, want_obj);
packet_delim(1);
}
@@ -1467,7 +1475,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
send_wanted_ref_info(&data);
send_shallow_info(&data, &want_obj);
- packet_write_fmt(1, "packfile\n");
+ packet_writer_write(&data.writer, "packfile\n");
create_pack_file(&have_obj, &want_obj);
state = FETCH_DONE;
break;
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/4] sideband: reverse its dependency on pkt-line
2019-01-11 22:18 [PATCH 0/4] Sideband the whole fetch v2 response Jonathan Tan
2019-01-11 22:18 ` [PATCH 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
@ 2019-01-11 22:18 ` Jonathan Tan
2019-01-14 20:07 ` Junio C Hamano
2019-01-11 22:18 ` [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response Jonathan Tan
` (7 subsequent siblings)
9 siblings, 1 reply; 41+ messages in thread
From: Jonathan Tan @ 2019-01-11 22:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
A subsequent patch will teach struct packet_reader a new field that, if
set, instructs it to interpret read data as multiplexed. This will
create a dependency from pkt-line to sideband.
To avoid a circular dependency, split recv_sideband() into 2 parts: the
reading loop (left in recv_sideband()) and the processing of the
contents (in diagnose_sideband()), and move the former into pkt-line.
This reverses the direction of dependency: sideband no longer depends on
pkt-line, and pkt-line now depends on sideband.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
pkt-line.c | 22 ++++++++
pkt-line.h | 16 ++++++
sideband.c | 156 +++++++++++++++++++++++++----------------------------
sideband.h | 15 +++++-
4 files changed, 125 insertions(+), 84 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index 9d3e402d41..ebdc6c2530 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -439,6 +439,28 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
return sb_out->len - orig_len;
}
+int recv_sideband(const char *me, int in_stream, int out)
+{
+ char buf[LARGE_PACKET_MAX + 1];
+ int retval = 0;
+ int len;
+
+ while (1) {
+ len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
+ retval = diagnose_sideband(me, buf, len);
+ switch (retval) {
+ case SIDEBAND_PRIMARY:
+ write_or_die(out, buf + 1, len - 1);
+ break;
+ case SIDEBAND_PROGRESS:
+ /* already written by diagnose_sideband() */
+ break;
+ default: /* flush or error */
+ return retval;
+ }
+ }
+}
+
/* Packet Reader Functions */
void packet_reader_init(struct packet_reader *reader, int fd,
char *src_buffer, size_t src_len,
diff --git a/pkt-line.h b/pkt-line.h
index 023ad2951d..a8e92a4b63 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -3,6 +3,7 @@
#include "git-compat-util.h"
#include "strbuf.h"
+#include "sideband.h"
/*
* Write a packetized stream, where each line is preceded by
@@ -120,6 +121,21 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
*/
ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
+/*
+ * Receive multiplexed output stream over git native protocol.
+ * in_stream is the input stream from the remote, which carries data
+ * in pkt_line format with band designator. Demultiplex it into out
+ * and err and return error appropriately. Band #1 carries the
+ * primary payload. Things coming over band #2 is not necessarily
+ * error; they are usually informative message on the standard error
+ * stream, aka "verbose"). A message over band #3 is a signal that
+ * the remote died unexpectedly. A flush() concludes the stream.
+ *
+ * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR
+ * or SIDEBAND_REMOTE_ERROR if an error occurred.
+ */
+int recv_sideband(const char *me, int in_stream, int out);
+
struct packet_reader {
/* source file descriptor */
int fd;
diff --git a/sideband.c b/sideband.c
index 368647acf8..842a92e975 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,7 +1,6 @@
#include "cache.h"
#include "color.h"
#include "config.h"
-#include "pkt-line.h"
#include "sideband.h"
#include "help.h"
@@ -109,103 +108,94 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
}
-/*
- * Receive multiplexed output stream over git native protocol.
- * in_stream is the input stream from the remote, which carries data
- * in pkt_line format with band designator. Demultiplex it into out
- * and err and return error appropriately. Band #1 carries the
- * primary payload. Things coming over band #2 is not necessarily
- * error; they are usually informative message on the standard error
- * stream, aka "verbose"). A message over band #3 is a signal that
- * the remote died unexpectedly. A flush() concludes the stream.
- */
-
#define DISPLAY_PREFIX "remote: "
#define ANSI_SUFFIX "\033[K"
#define DUMB_SUFFIX " "
-int recv_sideband(const char *me, int in_stream, int out)
+int diagnose_sideband(const char *me, char *buf, int len)
{
- const char *suffix;
- char buf[LARGE_PACKET_MAX + 1];
+ static const char *suffix;
struct strbuf outbuf = STRBUF_INIT;
int retval = 0;
+ const char *b, *brk;
+ int band;
+
+ if (!suffix) {
+ if (isatty(2) && !is_terminal_dumb())
+ suffix = ANSI_SUFFIX;
+ else
+ suffix = DUMB_SUFFIX;
+ }
- if (isatty(2) && !is_terminal_dumb())
- suffix = ANSI_SUFFIX;
- else
- suffix = DUMB_SUFFIX;
+ if (len == 0) {
+ retval = SIDEBAND_FLUSH;
+ goto cleanup;
+ }
+ if (len < 1) {
+ strbuf_addf(&outbuf,
+ "%s%s: protocol error: no band designator",
+ outbuf.len ? "\n" : "", me);
+ retval = SIDEBAND_PROTOCOL_ERROR;
+ goto cleanup;
+ }
+ band = buf[0] & 0xff;
+ buf[len] = '\0';
+ len--;
+ switch (band) {
+ case 3:
+ strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
+ DISPLAY_PREFIX);
+ maybe_colorize_sideband(&outbuf, buf + 1, len);
+
+ retval = SIDEBAND_REMOTE_ERROR;
+ break;
+ case 2:
+ b = buf + 1;
- while (!retval) {
- const char *b, *brk;
- int band, len;
- len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
- if (len == 0)
- break;
- if (len < 1) {
- strbuf_addf(&outbuf,
- "%s%s: protocol error: no band designator",
- outbuf.len ? "\n" : "", me);
- retval = SIDEBAND_PROTOCOL_ERROR;
- break;
- }
- band = buf[0] & 0xff;
- buf[len] = '\0';
- len--;
- switch (band) {
- case 3:
- strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
- DISPLAY_PREFIX);
- maybe_colorize_sideband(&outbuf, buf + 1, len);
-
- retval = 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 (!outbuf.len)
- strbuf_addstr(&outbuf, DISPLAY_PREFIX);
- if (linelen > 0) {
- 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;
+ /*
+ * 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 (!outbuf.len)
+ strbuf_addstr(&outbuf, DISPLAY_PREFIX);
+ if (linelen > 0) {
+ maybe_colorize_sideband(&outbuf, b, linelen);
+ strbuf_addstr(&outbuf, suffix);
}
- 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);
- break;
- default:
- strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
- outbuf.len ? "\n" : "", me, band);
- retval = SIDEBAND_PROTOCOL_ERROR;
- break;
+ strbuf_addch(&outbuf, *brk);
+ xwrite(2, outbuf.buf, outbuf.len);
+ strbuf_reset(&outbuf);
+
+ b = brk + 1;
+ }
+
+ if (*b) {
+ strbuf_addstr(&outbuf, outbuf.len ?
+ "" : DISPLAY_PREFIX);
+ maybe_colorize_sideband(&outbuf, b, strlen(b));
}
+ retval = SIDEBAND_PROGRESS;
+ break;
+ case 1:
+ retval = SIDEBAND_PRIMARY;
+ break;
+ default:
+ strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
+ outbuf.len ? "\n" : "", me, band);
+ retval = SIDEBAND_PROTOCOL_ERROR;
+ break;
}
+cleanup:
if (outbuf.len) {
strbuf_addch(&outbuf, '\n');
xwrite(2, outbuf.buf, outbuf.len);
diff --git a/sideband.h b/sideband.h
index 7a8146f161..a56cd86287 100644
--- a/sideband.h
+++ b/sideband.h
@@ -3,8 +3,21 @@
#define SIDEBAND_PROTOCOL_ERROR -2
#define SIDEBAND_REMOTE_ERROR -1
+#define SIDEBAND_FLUSH 0
+#define SIDEBAND_PRIMARY 1
+#define SIDEBAND_PROGRESS 2
+
+/*
+ * buf and len should be the result of reading a line from a remote sending
+ * multiplexed data.
+ *
+ * Determines the nature of the result and returns it. If
+ * SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS, also
+ * prints a message (or the formatted contents of the notice in the case of
+ * SIDEBAND_PROGRESS) to stderr.
+ */
+int diagnose_sideband(const char *me, char *buf, int len);
-int recv_sideband(const char *me, int in_stream, int out);
void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
#endif
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response
2019-01-11 22:18 [PATCH 0/4] Sideband the whole fetch v2 response Jonathan Tan
2019-01-11 22:18 ` [PATCH 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
2019-01-11 22:18 ` [PATCH 2/4] sideband: reverse its dependency on pkt-line Jonathan Tan
@ 2019-01-11 22:18 ` Jonathan Tan
2019-01-14 22:27 ` Junio C Hamano
2019-01-11 22:18 ` [PATCH 4/4] tests: define GIT_TEST_SIDEBAND_ALL Jonathan Tan
` (6 subsequent siblings)
9 siblings, 1 reply; 41+ messages in thread
From: Jonathan Tan @ 2019-01-11 22:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
Currently, a response to a fetch request has sideband support only while
the packfile is being sent, meaning that the server cannot send notices
until the start of the packfile.
Extend sideband support in protocol v2 fetch responses to the whole
response. upload-pack will advertise it if the
uploadpack.allowsidebandall configuration variable is set, and
fetch-pack will automatically request it if advertised.
If the sideband is to be used throughout the whole response, upload-pack
will use it to send errors instead of prefixing a PKT-LINE payload with
"ERR ".
This will be tested in a subsequent patch.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Documentation/technical/protocol-v2.txt | 10 +++++
fetch-pack.c | 12 +++++-
pkt-line.c | 51 +++++++++++++++++++------
pkt-line.h | 4 ++
sideband.c | 7 +++-
sideband.h | 2 +-
upload-pack.c | 16 ++++++++
7 files changed, 87 insertions(+), 15 deletions(-)
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 09e4e0273f..1b0633f59f 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -307,6 +307,16 @@ the 'wanted-refs' section in the server's response as explained below.
particular ref, where <ref> is the full name of a ref on the
server.
+If the 'sideband-all' feature is advertised, the following argument can be
+included in the client's request:
+
+ sideband-all
+ Instruct the server to send the whole response multiplexed, not just
+ the packfile section. All non-flush and non-delim PKT-LINE in the
+ response (not only in the packfile section) will then start with a byte
+ indicating its sideband (1, 2, or 3), and the server may send "0005\1"
+ (a PKT-LINE of sideband 1 with no payload) as a keepalive packet.
+
The response of `fetch` is broken into a number of sections separated by
delimiter packets (0001), with each section beginning with its section
header.
diff --git a/fetch-pack.c b/fetch-pack.c
index 3f9626dbf7..b89199891d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1125,7 +1125,8 @@ static int add_haves(struct fetch_negotiator *negotiator,
static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
const struct fetch_pack_args *args,
const struct ref *wants, struct oidset *common,
- int *haves_to_send, int *in_vain)
+ int *haves_to_send, int *in_vain,
+ int sideband_all)
{
int ret = 0;
struct strbuf req_buf = STRBUF_INIT;
@@ -1151,6 +1152,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
packet_buf_write(&req_buf, "include-tag");
if (prefer_ofs_delta)
packet_buf_write(&req_buf, "ofs-delta");
+ if (sideband_all)
+ packet_buf_write(&req_buf, "sideband-all");
/* Add shallow-info and deepen request */
if (server_supports_feature("fetch", "shallow", 0))
@@ -1359,6 +1362,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
packet_reader_init(&reader, fd[0], NULL, 0,
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_DIE_ON_ERR_PACKET);
+ if (server_supports_feature("fetch", "sideband-all", 0)) {
+ reader.use_sideband = 1;
+ reader.me = "fetch-pack";
+ }
while (state != FETCH_DONE) {
switch (state) {
@@ -1392,7 +1399,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
case FETCH_SEND_REQUEST:
if (send_fetch_request(&negotiator, fd[1], args, ref,
&common,
- &haves_to_send, &in_vain))
+ &haves_to_send, &in_vain,
+ reader.use_sideband))
state = FETCH_GET_PACK;
else
state = FETCH_PROCESS_ACKS;
diff --git a/pkt-line.c b/pkt-line.c
index ebdc6c2530..71b17e6b93 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -447,7 +447,7 @@ int recv_sideband(const char *me, int in_stream, int out)
while (1) {
len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
- retval = diagnose_sideband(me, buf, len);
+ retval = diagnose_sideband(me, buf, len, 0);
switch (retval) {
case SIDEBAND_PRIMARY:
write_or_die(out, buf + 1, len - 1);
@@ -483,16 +483,42 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
return reader->status;
}
- reader->status = packet_read_with_status(reader->fd,
- &reader->src_buffer,
- &reader->src_len,
- reader->buffer,
- reader->buffer_size,
- &reader->pktlen,
- reader->options);
+ while (1) {
+ int retval;
+ reader->status = packet_read_with_status(reader->fd,
+ &reader->src_buffer,
+ &reader->src_len,
+ reader->buffer,
+ reader->buffer_size,
+ &reader->pktlen,
+ reader->options);
+ if (!reader->use_sideband)
+ break;
+ retval = diagnose_sideband(reader->me, reader->buffer,
+ reader->pktlen, 1);
+ switch (retval) {
+ case SIDEBAND_PROTOCOL_ERROR:
+ case SIDEBAND_REMOTE_ERROR:
+ BUG("should have died in diagnose_sideband");
+ case SIDEBAND_FLUSH:
+ goto nonprogress;
+ case SIDEBAND_PRIMARY:
+ if (reader->pktlen != 1)
+ goto nonprogress;
+ /*
+ * Since pktlen is 1, this is a keepalive
+ * packet. Wait for the next one.
+ */
+ break;
+ default: /* SIDEBAND_PROGRESS */
+ ;
+ }
+ }
+nonprogress:
if (reader->status == PACKET_READ_NORMAL)
- reader->line = reader->buffer;
+ reader->line = reader->use_sideband ?
+ reader->buffer + 1 : reader->buffer;
else
reader->line = NULL;
@@ -514,6 +540,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)
void packet_writer_init(struct packet_writer *writer, int dest_fd)
{
writer->dest_fd = dest_fd;
+ writer->use_sideband = 0;
}
void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
@@ -521,7 +548,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
+ packet_write_fmt_1(writer->dest_fd, 0,
+ writer->use_sideband ? "\1" : "", fmt, args);
va_end(args);
}
@@ -530,7 +558,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
+ packet_write_fmt_1(writer->dest_fd, 0,
+ writer->use_sideband ? "\3" : "ERR ", fmt, args);
va_end(args);
}
diff --git a/pkt-line.h b/pkt-line.h
index a8e92a4b63..ad9a4a2cd7 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -162,6 +162,9 @@ struct packet_reader {
/* indicates if a line has been peeked */
int line_peeked;
+
+ unsigned use_sideband : 1;
+ const char *me;
};
/*
@@ -201,6 +204,7 @@ extern char packet_buffer[LARGE_PACKET_MAX];
struct packet_writer {
int dest_fd;
+ unsigned use_sideband : 1;
};
void packet_writer_init(struct packet_writer *writer, int dest_fd);
diff --git a/sideband.c b/sideband.c
index 842a92e975..cbeab380ae 100644
--- a/sideband.c
+++ b/sideband.c
@@ -113,7 +113,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
#define ANSI_SUFFIX "\033[K"
#define DUMB_SUFFIX " "
-int diagnose_sideband(const char *me, char *buf, int len)
+int diagnose_sideband(const char *me, char *buf, int len, int die_on_error)
{
static const char *suffix;
struct strbuf outbuf = STRBUF_INIT;
@@ -144,6 +144,9 @@ int diagnose_sideband(const char *me, char *buf, int len)
len--;
switch (band) {
case 3:
+ if (die_on_error)
+ die("remote error: %s", buf + 1);
+
strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
DISPLAY_PREFIX);
maybe_colorize_sideband(&outbuf, buf + 1, len);
@@ -196,6 +199,8 @@ int diagnose_sideband(const char *me, char *buf, int len)
}
cleanup:
+ if (die_on_error && retval == SIDEBAND_PROTOCOL_ERROR)
+ die("%s", outbuf.buf);
if (outbuf.len) {
strbuf_addch(&outbuf, '\n');
xwrite(2, outbuf.buf, outbuf.len);
diff --git a/sideband.h b/sideband.h
index a56cd86287..3786670694 100644
--- a/sideband.h
+++ b/sideband.h
@@ -16,7 +16,7 @@
* prints a message (or the formatted contents of the notice in the case of
* SIDEBAND_PROGRESS) to stderr.
*/
-int diagnose_sideband(const char *me, char *buf, int len);
+int diagnose_sideband(const char *me, char *buf, int len, int die_on_error);
void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
diff --git a/upload-pack.c b/upload-pack.c
index 60a26bbbfd..765b7695d2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -71,6 +71,8 @@ static int allow_filter;
static int allow_ref_in_want;
static struct list_objects_filter_options filter_options;
+static int allow_sideband_all;
+
static void reset_timeout(void)
{
alarm(timeout);
@@ -1046,6 +1048,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
allow_filter = git_config_bool(var, value);
} else if (!strcmp("uploadpack.allowrefinwant", var)) {
allow_ref_in_want = git_config_bool(var, value);
+ } else if (!strcmp("uploadpack.allowsidebandall", var)) {
+ allow_sideband_all = git_config_bool(var, value);
}
if (current_config_scope() != CONFIG_SCOPE_REPO) {
@@ -1284,6 +1288,11 @@ static void process_args(struct packet_reader *request,
continue;
}
+ if (allow_sideband_all && !strcmp(arg, "sideband-all")) {
+ data->writer.use_sideband = 1;
+ continue;
+ }
+
/* ignore unknown lines maybe? */
die("unexpected line: '%s'", arg);
}
@@ -1496,6 +1505,7 @@ int upload_pack_advertise(struct repository *r,
if (value) {
int allow_filter_value;
int allow_ref_in_want;
+ int allow_sideband_all_value;
strbuf_addstr(value, "shallow");
@@ -1510,6 +1520,12 @@ int upload_pack_advertise(struct repository *r,
&allow_ref_in_want) &&
allow_ref_in_want)
strbuf_addstr(value, " ref-in-want");
+
+ if (!repo_config_get_bool(the_repository,
+ "uploadpack.allowsidebandall",
+ &allow_sideband_all_value) &&
+ allow_sideband_all_value)
+ strbuf_addstr(value, " sideband-all");
}
return 1;
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 4/4] tests: define GIT_TEST_SIDEBAND_ALL
2019-01-11 22:18 [PATCH 0/4] Sideband the whole fetch v2 response Jonathan Tan
` (2 preceding siblings ...)
2019-01-11 22:18 ` [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response Jonathan Tan
@ 2019-01-11 22:18 ` Jonathan Tan
2019-01-11 22:18 ` [WIP 5/4] Documentation: order protocol v2 sections Jonathan Tan
` (5 subsequent siblings)
9 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-11 22:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
Define a GIT_TEST_SIDEBAND_ALL environment variable meant to be used
from tests. When set to true, this overrides uploadpack.allowsidebandall
to true, allowing the entire test suite to be run as if this
configuration is in place for all repositories.
As of this patch, all tests pass whether GIT_TEST_SIDEBAND_ALL is unset
or set to 1.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
fetch-pack.c | 3 ++-
t/README | 5 +++++
t/lib-httpd/apache.conf | 1 +
t/t5537-fetch-shallow.sh | 3 ++-
t/t5701-git-serve.sh | 2 +-
t/t5702-protocol-v2.sh | 4 ++--
upload-pack.c | 13 ++++++++-----
7 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index b89199891d..4618568fee 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1362,7 +1362,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
packet_reader_init(&reader, fd[0], NULL, 0,
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_DIE_ON_ERR_PACKET);
- if (server_supports_feature("fetch", "sideband-all", 0)) {
+ if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 1) &&
+ server_supports_feature("fetch", "sideband-all", 0)) {
reader.use_sideband = 1;
reader.me = "fetch-pack";
}
diff --git a/t/README b/t/README
index 28711cc508..b275c883b8 100644
--- a/t/README
+++ b/t/README
@@ -358,6 +358,11 @@ GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-
index to be written after every 'git repack' command, and overrides the
'core.multiPackIndex' setting to true.
+GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
+'uploadpack.allowSidebandAll' setting to true, and when false, forces
+fetch-pack to not request sideband-all (even if the server advertises
+sideband-all).
+
Naming Tests
------------
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..9a6d368247 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -78,6 +78,7 @@ PassEnv GNUPGHOME
PassEnv ASAN_OPTIONS
PassEnv GIT_TRACE
PassEnv GIT_CONFIG_NOSYSTEM
+PassEnv GIT_TEST_SIDEBAND_ALL
SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 6faf17e17a..6caf628efa 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -243,7 +243,8 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f
"$(git -C "$REPO" rev-parse HEAD)" \
"$(git -C "$REPO" rev-parse HEAD^)" \
>"$HTTPD_ROOT_PATH/one-time-sed" &&
- test_must_fail git -C client fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
+ test_must_fail env GIT_TEST_SIDEBAND_ALL=0 git -C client \
+ fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
master:a_branch &&
# Ensure that the one-time-sed script was used.
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index ae79c6bbc0..fe45bf828d 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -14,7 +14,7 @@ test_expect_success 'test capability advertisement' '
0000
EOF
- git serve --advertise-capabilities >out &&
+ GIT_TEST_SIDEBAND_ALL=0 git serve --advertise-capabilities >out &&
test-tool pkt-line unpack <out >actual &&
test_cmp expect actual
'
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..b491c62e3e 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -583,8 +583,8 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" git -C http_child \
-c protocol.version=2 \
fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err &&
- grep "fetch< acknowledgments" log &&
- ! grep "fetch< ready" log &&
+ grep "fetch< .*acknowledgments" log &&
+ ! grep "fetch< .*ready" log &&
test_i18ngrep "expected no other sections to be sent after no .ready." err
'
diff --git a/upload-pack.c b/upload-pack.c
index 765b7695d2..0c1feccaab 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1288,7 +1288,9 @@ static void process_args(struct packet_reader *request,
continue;
}
- if (allow_sideband_all && !strcmp(arg, "sideband-all")) {
+ if ((git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
+ allow_sideband_all) &&
+ !strcmp(arg, "sideband-all")) {
data->writer.use_sideband = 1;
continue;
}
@@ -1521,10 +1523,11 @@ int upload_pack_advertise(struct repository *r,
allow_ref_in_want)
strbuf_addstr(value, " ref-in-want");
- if (!repo_config_get_bool(the_repository,
- "uploadpack.allowsidebandall",
- &allow_sideband_all_value) &&
- allow_sideband_all_value)
+ if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
+ (!repo_config_get_bool(the_repository,
+ "uploadpack.allowsidebandall",
+ &allow_sideband_all_value) &&
+ allow_sideband_all_value))
strbuf_addstr(value, " sideband-all");
}
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [WIP 5/4] Documentation: order protocol v2 sections
2019-01-11 22:18 [PATCH 0/4] Sideband the whole fetch v2 response Jonathan Tan
` (3 preceding siblings ...)
2019-01-11 22:18 ` [PATCH 4/4] tests: define GIT_TEST_SIDEBAND_ALL Jonathan Tan
@ 2019-01-11 22:18 ` Jonathan Tan
2019-01-11 22:18 ` [WIP 6/4] Documentation: add Packfile URIs design doc Jonathan Tan
` (4 subsequent siblings)
9 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-11 22:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
The current C Git implementation expects Git servers to follow a
specific order of sections when transmitting protocol v2 responses, but
this is not explicit in the documentation. Make the order explicit.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Documentation/technical/protocol-v2.txt | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 1b0633f59f..c76f2311c2 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -319,11 +319,11 @@ included in the client's request:
The response of `fetch` is broken into a number of sections separated by
delimiter packets (0001), with each section beginning with its section
-header.
+header. Most sections are sent only when the packfile is sent.
- output = *section
- section = (acknowledgments | shallow-info | wanted-refs | packfile)
- (flush-pkt | delim-pkt)
+ output = acknowledgements flush-pkt |
+ [acknowledgments delim-pkt] [shallow-info delim-pkt]
+ [wanted-refs delim-pkt] packfile flush-pkt
acknowledgments = PKT-LINE("acknowledgments" LF)
(nak | *ack)
@@ -345,9 +345,10 @@ header.
*PKT-LINE(%x01-03 *%x00-ff)
acknowledgments section
- * If the client determines that it is finished with negotiations
- by sending a "done" line, the acknowledgments sections MUST be
- omitted from the server's response.
+ * If the client determines that it is finished with negotiations by
+ sending a "done" line (thus requiring the server to send a packfile),
+ the acknowledgments sections MUST be omitted from the server's
+ response.
* Always begins with the section header "acknowledgments"
@@ -398,9 +399,6 @@ header.
which the client has not indicated was shallow as a part of
its request.
- * This section is only included if a packfile section is also
- included in the response.
-
wanted-refs section
* This section is only included if the client has requested a
ref using a 'want-ref' line and if a packfile section is also
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [WIP 6/4] Documentation: add Packfile URIs design doc
2019-01-11 22:18 [PATCH 0/4] Sideband the whole fetch v2 response Jonathan Tan
` (4 preceding siblings ...)
2019-01-11 22:18 ` [WIP 5/4] Documentation: order protocol v2 sections Jonathan Tan
@ 2019-01-11 22:18 ` Jonathan Tan
2019-01-11 22:18 ` [WIP 7/4] upload-pack: refactor reading of pack-objects out Jonathan Tan
` (3 subsequent siblings)
9 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-11 22:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Documentation/technical/packfile-uri.txt | 83 ++++++++++++++++++++++++
Documentation/technical/protocol-v2.txt | 6 +-
2 files changed, 88 insertions(+), 1 deletion(-)
create mode 100644 Documentation/technical/packfile-uri.txt
diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt
new file mode 100644
index 0000000000..6535801486
--- /dev/null
+++ b/Documentation/technical/packfile-uri.txt
@@ -0,0 +1,83 @@
+Packfile URIs
+=============
+
+This feature allows servers to serve part of their packfile response as URIs.
+This allows server designs that improve scalability in bandwidth and CPU usage
+(for example, by serving some data through a CDN), and (in the future) provides
+some measure of resumability to clients.
+
+This feature is available only in protocol version 2.
+
+Protocol
+--------
+
+The server advertises `packfile-uris`.
+
+If the client replies with the following arguments:
+
+ * packfile-uris
+ * thin-pack
+ * ofs-delta
+
+when the server sends the packfile, it MAY send a `packfile-uris` section
+directly before the `packfile` section (right after `wanted-refs` if it is
+sent) containing HTTP(S) URIs. See protocol-v2.txt for the documentation of
+this section.
+
+Clients then should understand that the returned packfile could be incomplete,
+and that it needs to download all the given URIs before the fetch or clone is
+complete. Each URI should point to a Git packfile (which may be a thin pack and
+which may contain offset deltas).
+
+Server design
+-------------
+
+The server can be trivially made compatible with the proposed protocol by
+having it advertise `packfile-uris`, tolerating the client sending
+`packfile-uris`, and never sending any `packfile-uris` section. But we should
+include some sort of non-trivial implementation in the Minimum Viable Product,
+at least so that we can test the client.
+
+This is the implementation: a feature, marked experimental, that allows the
+server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>
+<uri>` entries. Whenever the list of objects to be sent is assembled, a blob
+with the given sha1 can be replaced by the given URI. This allows, for example,
+servers to delegate serving of large blobs to CDNs.
+
+Client design
+-------------
+
+While fetching, the client needs to remember the list of URIs and cannot
+declare that the fetch is complete until all URIs have been downloaded as
+packfiles.
+
+The division of work (initial fetch + additional URIs) introduces convenient
+points for resumption of an interrupted clone - such resumption can be done
+after the Minimum Viable Product (see "Future work").
+
+The client can inhibit this feature (i.e. refrain from sending the
+`packfile-urls` parameter) by passing --no-packfile-urls to `git fetch`.
+
+Future work
+-----------
+
+The protocol design allows some evolution of the server and client without any
+need for protocol changes, so only a small-scoped design is included here to
+form the MVP. For example, the following can be done:
+
+ * On the server, a long-running process that takes in entire requests and
+ outputs a list of URIs and the corresponding inclusion and exclusion sets of
+ objects. This allows, e.g., signed URIs to be used and packfiles for common
+ requests to be cached.
+ * On the client, resumption of clone. If a clone is interrupted, information
+ could be recorded in the repository's config and a "clone-resume" command
+ can resume the clone in progress. (Resumption of subsequent fetches is more
+ difficult because that must deal with the user wanting to use the repository
+ even after the fetch was interrupted.)
+
+There are some possible features that will require a change in protocol:
+
+ * Additional HTTP headers (e.g. authentication)
+ * Byte range support
+ * Different file formats referenced by URIs (e.g. raw object)
+
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index c76f2311c2..de5c63bc9b 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -323,7 +323,8 @@ header. Most sections are sent only when the packfile is sent.
output = acknowledgements flush-pkt |
[acknowledgments delim-pkt] [shallow-info delim-pkt]
- [wanted-refs delim-pkt] packfile flush-pkt
+ [wanted-refs delim-pkt] [packfile-uris delim-pkt]
+ packfile flush-pkt
acknowledgments = PKT-LINE("acknowledgments" LF)
(nak | *ack)
@@ -341,6 +342,9 @@ header. Most sections are sent only when the packfile is sent.
*PKT-LINE(wanted-ref LF)
wanted-ref = obj-id SP refname
+ packfile-uris = PKT-LINE("packfile-uris" LF) *packfile-uri
+ packfile-uri = PKT-LINE("uri" SP *%x20-ff LF)
+
packfile = PKT-LINE("packfile" LF)
*PKT-LINE(%x01-03 *%x00-ff)
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [WIP 7/4] upload-pack: refactor reading of pack-objects out
2019-01-11 22:18 [PATCH 0/4] Sideband the whole fetch v2 response Jonathan Tan
` (5 preceding siblings ...)
2019-01-11 22:18 ` [WIP 6/4] Documentation: add Packfile URIs design doc Jonathan Tan
@ 2019-01-11 22:18 ` Jonathan Tan
2019-01-11 22:18 ` [WIP 8/4] upload-pack: send part of packfile response as uri Jonathan Tan
` (2 subsequent siblings)
9 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-11 22:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
Subsequent patches will change how the output of pack-objects is
processed, so extract that processing into its own function.
Currently, at most 1 character can be buffered (in the "buffered" local
variable). One of those patches will require a larger buffer, so replace
that "buffered" local variable with a buffer array.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
upload-pack.c | 80 +++++++++++++++++++++++++++++----------------------
1 file changed, 46 insertions(+), 34 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 0c1feccaab..c87b752550 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -103,14 +103,51 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
return 0;
}
+struct output_state {
+ char buffer[8193];
+ int used;
+};
+
+static int read_pack_objects_stdout(int outfd, struct output_state *os)
+{
+ /* Data ready; we keep the last byte to ourselves
+ * in case we detect broken rev-list, so that we
+ * can leave the stream corrupted. This is
+ * unfortunate -- unpack-objects would happily
+ * accept a valid packdata with trailing garbage,
+ * so appending garbage after we pass all the
+ * pack data is not good enough to signal
+ * breakage to downstream.
+ */
+ ssize_t readsz;
+
+ readsz = xread(outfd, os->buffer + os->used,
+ sizeof(os->buffer) - os->used);
+ if (readsz < 0) {
+ return readsz;
+ }
+ os->used += readsz;
+
+ if (os->used > 1) {
+ send_client_data(1, os->buffer, os->used - 1);
+ os->buffer[0] = os->buffer[os->used - 1];
+ os->used = 1;
+ } else {
+ send_client_data(1, os->buffer, os->used);
+ os->used = 0;
+ }
+
+ return readsz;
+}
+
static void create_pack_file(const struct object_array *have_obj,
const struct object_array *want_obj)
{
struct child_process pack_objects = CHILD_PROCESS_INIT;
- char data[8193], progress[128];
+ struct output_state output_state = {0};
+ char progress[128];
char abort_msg[] = "aborting due to possible repository "
"corruption on the remote side.";
- int buffered = -1;
ssize_t sz;
int i;
FILE *pipe_fd;
@@ -237,39 +274,15 @@ static void create_pack_file(const struct object_array *have_obj,
continue;
}
if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {
- /* Data ready; we keep the last byte to ourselves
- * in case we detect broken rev-list, so that we
- * can leave the stream corrupted. This is
- * unfortunate -- unpack-objects would happily
- * accept a valid packdata with trailing garbage,
- * so appending garbage after we pass all the
- * pack data is not good enough to signal
- * breakage to downstream.
- */
- char *cp = data;
- ssize_t outsz = 0;
- if (0 <= buffered) {
- *cp++ = buffered;
- outsz++;
- }
- sz = xread(pack_objects.out, cp,
- sizeof(data) - outsz);
- if (0 < sz)
- ;
- else if (sz == 0) {
+ int result = read_pack_objects_stdout(pack_objects.out,
+ &output_state);
+
+ if (result == 0) {
close(pack_objects.out);
pack_objects.out = -1;
- }
- else
+ } else if (result < 0) {
goto fail;
- sz += outsz;
- if (1 < sz) {
- buffered = data[sz-1] & 0xFF;
- sz--;
}
- else
- buffered = -1;
- send_client_data(1, data, sz);
}
/*
@@ -294,9 +307,8 @@ static void create_pack_file(const struct object_array *have_obj,
}
/* flush the data */
- if (0 <= buffered) {
- data[0] = buffered;
- send_client_data(1, data, 1);
+ if (output_state.used > 0) {
+ send_client_data(1, output_state.buffer, output_state.used);
fprintf(stderr, "flushed.\n");
}
if (use_sideband)
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [WIP 8/4] upload-pack: send part of packfile response as uri
2019-01-11 22:18 [PATCH 0/4] Sideband the whole fetch v2 response Jonathan Tan
` (6 preceding siblings ...)
2019-01-11 22:18 ` [WIP 7/4] upload-pack: refactor reading of pack-objects out Jonathan Tan
@ 2019-01-11 22:18 ` Jonathan Tan
2019-01-15 19:40 ` [PATCH v2 0/4] Sideband the whole fetch v2 response Jonathan Tan
2019-01-16 19:28 ` [PATCH v3 " Jonathan Tan
9 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-11 22:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
This is a partial implementation of upload-pack sending part of its
packfile response as URIs.
The client is not fully implemented - it knows to ignore the
"packfile-uris" section, but because it does not actually fetch those
URIs, the returned packfile is incomplete. A test is included to show
that the appropriate URI is indeed transmitted, and that the returned
packfile is lacking exactly the expected object.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
builtin/pack-objects.c | 48 ++++++++++++++++++++++++++++++++++++
fetch-pack.c | 9 +++++++
t/t5702-protocol-v2.sh | 27 ++++++++++++++++++++
upload-pack.c | 56 ++++++++++++++++++++++++++++++++++++------
4 files changed, 133 insertions(+), 7 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 411aefd687..448c42a666 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -117,6 +117,15 @@ enum missing_action {
static enum missing_action arg_missing_action;
static show_object_fn fn_show_object;
+struct configured_exclusion {
+ struct oidmap_entry e;
+ char *uri;
+};
+static struct oidmap configured_exclusions;
+
+static int exclude_configured_blobs;
+static struct oidset excluded_by_config;
+
/*
* stats
*/
@@ -831,6 +840,23 @@ static off_t write_reused_pack(struct hashfile *f)
return reuse_packfile_offset - sizeof(struct pack_header);
}
+static void write_excluded_by_configs(void)
+{
+ struct oidset_iter iter;
+ const struct object_id *oid;
+
+ oidset_iter_init(&excluded_by_config, &iter);
+ while ((oid = oidset_iter_next(&iter))) {
+ struct configured_exclusion *ex =
+ oidmap_get(&configured_exclusions, oid);
+
+ if (!ex)
+ BUG("configured exclusion wasn't configured");
+ write_in_full(1, ex->uri, strlen(ex->uri));
+ write_in_full(1, "\n", 1);
+ }
+}
+
static const char no_split_warning[] = N_(
"disabling bitmap writing, packs are split due to pack.packSizeLimit"
);
@@ -1124,6 +1150,12 @@ static int want_object_in_pack(const struct object_id *oid,
}
}
+ if (exclude_configured_blobs &&
+ oidmap_get(&configured_exclusions, oid)) {
+ oidset_insert(&excluded_by_config, oid);
+ return 0;
+ }
+
return 1;
}
@@ -2728,6 +2760,19 @@ static int git_pack_config(const char *k, const char *v, void *cb)
pack_idx_opts.version);
return 0;
}
+ if (!strcmp(k, "uploadpack.blobpackfileuri")) {
+ struct configured_exclusion *ex = xmalloc(sizeof(*ex));
+ const char *end;
+
+ if (parse_oid_hex(v, &ex->e.oid, &end) || *end != ' ')
+ die(_("value of uploadpack.blobpackfileuri must be "
+ "of the form '<sha-1> <uri>' (got '%s')"), v);
+ if (oidmap_get(&configured_exclusions, &ex->e.oid))
+ die(_("object already configured in another "
+ "uploadpack.blobpackfileuri (got '%s')"), v);
+ ex->uri = xstrdup(end + 1);
+ oidmap_put(&configured_exclusions, ex);
+ }
return git_default_config(k, v, cb);
}
@@ -3316,6 +3361,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
N_("do not pack objects in promisor packfiles")),
OPT_BOOL(0, "delta-islands", &use_delta_islands,
N_("respect islands during delta compression")),
+ OPT_BOOL(0, "exclude-configured-blobs", &exclude_configured_blobs,
+ N_("respect uploadpack.blobpackfileuri")),
OPT_END(),
};
@@ -3489,6 +3536,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
return 0;
if (nr_result)
prepare_pack(window, depth);
+ write_excluded_by_configs();
write_pack_file();
if (progress)
fprintf_ln(stderr,
diff --git a/fetch-pack.c b/fetch-pack.c
index 4618568fee..79af87b2cf 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1429,6 +1429,15 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
receive_wanted_refs(&reader, sought, nr_sought);
/* get the pack */
+ if (process_section_header(&reader, "packfile-uris", 1)) {
+ /* skip the whole section */
+ process_section_header(&reader, "packfile-uris", 0);
+ while (packet_reader_read(&reader) == PACKET_READ_NORMAL) {
+ /* do nothing */
+ }
+ if (reader.status != PACKET_READ_DELIM)
+ die("expected DELIM");
+ }
process_section_header(&reader, "packfile", 0);
if (get_pack(args, fd, pack_lockfile))
die(_("git fetch-pack: fetch failed."));
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index b491c62e3e..ba85ee4dd9 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -588,6 +588,33 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
test_i18ngrep "expected no other sections to be sent after no .ready." err
'
+test_expect_success 'part of packfile response provided as URI' '
+ rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" http_child log &&
+
+ git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
+ echo my-blob >"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/my-blob" &&
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent/" add my-blob &&
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent/" commit -m x &&
+
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent/" hash-object my-blob >h &&
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent/" config \
+ "uploadpack.blobpackfileuri" \
+ "$(cat h) https://example.com/a-uri" &&
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent/" config \
+ "uploadpack.allowsidebandall" "true" &&
+
+ # NEEDSWORK: "git clone" fails here because it ignores the URI provided
+ # instead of fetching it.
+ test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" \
+ git -c protocol.version=2 clone \
+ "$HTTPD_URL/smart/http_parent" http_child 2>err &&
+ # Although "git clone" fails, we can still check that the server
+ # provided the URI we requested and that the error message pinpoints
+ # the object that is missing.
+ grep "clone< .*uri https://example.com/a-uri" log &&
+ test_i18ngrep "did not receive expected object $(cat h)" err
+'
+
stop_httpd
test_done
diff --git a/upload-pack.c b/upload-pack.c
index c87b752550..fe6e2300e3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -106,9 +106,12 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
struct output_state {
char buffer[8193];
int used;
+ unsigned packfile_uris_started : 1;
+ unsigned packfile_started : 1;
};
-static int read_pack_objects_stdout(int outfd, struct output_state *os)
+static int read_pack_objects_stdout(int outfd, struct output_state *os,
+ int write_packfile_line)
{
/* Data ready; we keep the last byte to ourselves
* in case we detect broken rev-list, so that we
@@ -128,6 +131,37 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os)
}
os->used += readsz;
+ while (!os->packfile_started) {
+ char *p;
+ if (os->used >= 4 && !memcmp(os->buffer, "PACK", 4)) {
+ os->packfile_started = 1;
+ if (write_packfile_line) {
+ if (os->packfile_uris_started)
+ packet_delim(1);
+ packet_write_fmt(1, "\1packfile\n");
+ }
+ break;
+ }
+ if ((p = memchr(os->buffer, '\n', os->used))) {
+ if (!os->packfile_uris_started) {
+ os->packfile_uris_started = 1;
+ if (!write_packfile_line)
+ BUG("packfile_uris requires sideband-all");
+ packet_write_fmt(1, "\1packfile-uris\n");
+ }
+ *p = '\0';
+ packet_write_fmt(1, "\1uri %s\n", os->buffer);
+
+ os->used -= p - os->buffer + 1;
+ memmove(os->buffer, p, os->used);
+ } else {
+ /*
+ * Incomplete line.
+ */
+ return readsz;
+ }
+ }
+
if (os->used > 1) {
send_client_data(1, os->buffer, os->used - 1);
os->buffer[0] = os->buffer[os->used - 1];
@@ -141,7 +175,8 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os)
}
static void create_pack_file(const struct object_array *have_obj,
- const struct object_array *want_obj)
+ const struct object_array *want_obj,
+ int write_packfile_line)
{
struct child_process pack_objects = CHILD_PROCESS_INIT;
struct output_state output_state = {0};
@@ -189,6 +224,9 @@ static void create_pack_file(const struct object_array *have_obj,
filter_options.filter_spec);
}
}
+ if (write_packfile_line)
+ argv_array_push(&pack_objects.args,
+ "--exclude-configured-blobs");
pack_objects.in = -1;
pack_objects.out = -1;
@@ -275,8 +313,8 @@ static void create_pack_file(const struct object_array *have_obj,
}
if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {
int result = read_pack_objects_stdout(pack_objects.out,
- &output_state);
-
+ &output_state,
+ write_packfile_line);
if (result == 0) {
close(pack_objects.out);
pack_objects.out = -1;
@@ -1108,7 +1146,7 @@ void upload_pack(struct upload_pack_options *options)
if (want_obj.nr) {
struct object_array have_obj = OBJECT_ARRAY_INIT;
get_common_commits(&reader, &have_obj, &want_obj);
- create_pack_file(&have_obj, &want_obj);
+ create_pack_file(&have_obj, &want_obj, 0);
}
}
@@ -1498,8 +1536,12 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
send_wanted_ref_info(&data);
send_shallow_info(&data, &want_obj);
- packet_writer_write(&data.writer, "packfile\n");
- create_pack_file(&have_obj, &want_obj);
+ if (data.writer.use_sideband) {
+ create_pack_file(&have_obj, &want_obj, 1);
+ } else {
+ packet_write_fmt(1, "packfile\n");
+ create_pack_file(&have_obj, &want_obj, 0);
+ }
state = FETCH_DONE;
break;
case FETCH_DONE:
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 1/4] pkt-line: introduce struct packet_writer
2019-01-11 22:18 ` [PATCH 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
@ 2019-01-14 20:07 ` Junio C Hamano
2019-01-15 18:18 ` Jonathan Tan
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2019-01-14 20:07 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
Jonathan Tan <jonathantanmy@google.com> writes:
The patch itself look quite noisy, but in esssense, at the lowest
level we used to have a single format_packet() that was used to
write out normal payload and an error message prefixed with "ERR ";
now the users of the function are updated to call one of the two
helper functions, packet_writer_write() or packet_writer_error().
Most of the patch noise comes from the helper functions at higher
levels getting updated to pass the packet_writer struct through the
callchain.
Which makes tons of sense.
> It will be convenient for a future patch if writing options
> (specifically, whether the written data is to be multiplexed) could be
> controlled from a single place, so create struct packet_writer to serve
> as that place, and modify upload-pack to use it.
I've singled out "ERR " in my comment above, but this only refers to
"multiplexed". Are there reasons why we want multiplexing other
than the "are we sending payload, or an error message"?
> Currently, it only stores the output fd, but a subsequent patch will (as
> described above) introduce an option to determine if the written data is
> to be multiplexed.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line
2019-01-11 22:18 ` [PATCH 2/4] sideband: reverse its dependency on pkt-line Jonathan Tan
@ 2019-01-14 20:07 ` Junio C Hamano
2019-01-14 22:11 ` Junio C Hamano
2019-01-15 18:36 ` Jonathan Tan
0 siblings, 2 replies; 41+ messages in thread
From: Junio C Hamano @ 2019-01-14 20:07 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
Jonathan Tan <jonathantanmy@google.com> writes:
> +int recv_sideband(const char *me, int in_stream, int out)
> +{
> + char buf[LARGE_PACKET_MAX + 1];
> + int retval = 0;
> + int len;
> +
> + while (1) {
> + len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
> + retval = diagnose_sideband(me, buf, len);
This name of the helper does not convey much useful information.
Let's think about it more later.
> + switch (retval) {
> + case SIDEBAND_PRIMARY:
Dedent; case/default arms align with 's' in switch in our codebase.
> + write_or_die(out, buf + 1, len - 1);
> + break;
OK, so PRIMARY return (band#1) gives us the payload unwrapped back
in "out" and we are responsible for writing it out here.
> + case SIDEBAND_PROGRESS:
> + /* already written by diagnose_sideband() */
True, that happens in "switch(band) { case 2: }" in the other helper.
> + break;
> + default: /* flush or error */
> + return retval;
Lack of corresponding comment bothers readers. In all of
REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper
stuffs the message in outbuf in "switch (band) { ... }" and writes
it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see
there is no need for us to write anything out here. Perhaps
case SIDEBAND_FLUSH:
default: /* errors: message already written */
return retval;
or something to clarify?
> +/*
> + * Receive multiplexed output stream over git native protocol.
> + * in_stream is the input stream from the remote, which carries data
> + * in pkt_line format with band designator. Demultiplex it into out
> + * and err and return error appropriately. Band #1 carries the
> + * primary payload. Things coming over band #2 is not necessarily
> + * error; they are usually informative message on the standard error
> + * stream, aka "verbose"). A message over band #3 is a signal that
> + * the remote died unexpectedly. A flush() concludes the stream.
> + *
> + * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR
> + * or SIDEBAND_REMOTE_ERROR if an error occurred.
> + */
> +int recv_sideband(const char *me, int in_stream, int out);
This is well described.
> diff --git a/sideband.c b/sideband.c
> index 368647acf8..842a92e975 100644
> --- a/sideband.c
> +++ b/sideband.c
> ...
> +int diagnose_sideband(const char *me, char *buf, int len)
> {
> + static const char *suffix;
> struct strbuf outbuf = STRBUF_INIT;
> int retval = 0;
> + const char *b, *brk;
> + int band;
> +
> + if (!suffix) {
> + if (isatty(2) && !is_terminal_dumb())
> + suffix = ANSI_SUFFIX;
> + else
> + suffix = DUMB_SUFFIX;
> + }
It may be worth considering a further clean-up for the progress
code, that consumes lines in the "switch(band)" below that are
disproportionate to what it does in this function by introducing
another helper function that is called from here. When it happens,
the above "suffix" thing should move to the helper function, too.
> + if (len == 0) {
> + retval = SIDEBAND_FLUSH;
> + goto cleanup;
> + }
> + if (len < 1) {
> + strbuf_addf(&outbuf,
> + "%s%s: protocol error: no band designator",
> + outbuf.len ? "\n" : "", me);
> + retval = SIDEBAND_PROTOCOL_ERROR;
> + goto cleanup;
> + }
> + band = buf[0] & 0xff;
> + buf[len] = '\0';
> + len--;
> + switch (band) {
> + case 3:
> +...
> + case 2:
> +...
> + case 1:
> + retval = SIDEBAND_PRIMARY;
> + break;
> + default:
> + strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
> + outbuf.len ? "\n" : "", me, band);
> + retval = SIDEBAND_PROTOCOL_ERROR;
> + break;
So, the point of this helper is to inspect a single packet-line data
and dispatch the payload of the packet based on which band it is
sent to. Perhaps call it with a name with demultiplex or dispatch
in it? "diagnose" is a bit unclear in that what trait you are
diagnosing and for what purpose.
> diff --git a/sideband.h b/sideband.h
> index 7a8146f161..a56cd86287 100644
> --- a/sideband.h
> +++ b/sideband.h
> @@ -3,8 +3,21 @@
>
> #define SIDEBAND_PROTOCOL_ERROR -2
> #define SIDEBAND_REMOTE_ERROR -1
> +#define SIDEBAND_FLUSH 0
> +#define SIDEBAND_PRIMARY 1
> +#define SIDEBAND_PROGRESS 2
> +
> +/*
> + * buf and len should be the result of reading a line from a remote sending
> + * multiplexed data.
> + *
> + * Determines the nature of the result and returns it. If
"the result" may be from the point of view of the implementor or the
"recv_sideband()" function who called packet_read(), but a casual
reader would perceive it more natural if you referred it as "a
packet read from a remote". "Inspect a packet read from the remote
and returns which sideband it is for", perhaps?
> + * SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS, also
> + * prints a message (or the formatted contents of the notice in the case of
> + * SIDEBAND_PROGRESS) to stderr.
> + */
> +int diagnose_sideband(const char *me, char *buf, int len);
>
> -int recv_sideband(const char *me, int in_stream, int out);
> void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
>
> #endif
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line
2019-01-14 20:07 ` Junio C Hamano
@ 2019-01-14 22:11 ` Junio C Hamano
2019-01-15 18:38 ` Jonathan Tan
2019-01-15 18:36 ` Jonathan Tan
1 sibling, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2019-01-14 22:11 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
Junio C Hamano <gitster@pobox.com> writes:
> Lack of corresponding comment bothers readers. In all of
> REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper
> stuffs the message in outbuf in "switch (band) { ... }" and writes
> it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see
> there is no need for us to write anything out here. Perhaps
>
> case SIDEBAND_FLUSH:
> default: /* errors: message already written */
> return retval;
>
> or something to clarify?
Forgot a footnote for *1* above. I was wondering if xwrite is what
we really want, rather than write_in_full().
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response
2019-01-11 22:18 ` [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response Jonathan Tan
@ 2019-01-14 22:27 ` Junio C Hamano
2019-01-15 19:18 ` Jonathan Tan
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2019-01-14 22:27 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
Jonathan Tan <jonathantanmy@google.com> writes:
> Currently, a response to a fetch request has sideband support only while
> the packfile is being sent, meaning that the server cannot send notices
> until the start of the packfile.
>
> Extend sideband support in protocol v2 fetch responses to the whole
> response. upload-pack will advertise it if the
> uploadpack.allowsidebandall configuration variable is set, and
> fetch-pack will automatically request it if advertised.
>
> If the sideband is to be used throughout the whole response, upload-pack
> will use it to send errors instead of prefixing a PKT-LINE payload with
> "ERR ".
Makes sense.
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 3f9626dbf7..b89199891d 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1125,7 +1125,8 @@ static int add_haves(struct fetch_negotiator *negotiator,
> static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
> const struct fetch_pack_args *args,
> const struct ref *wants, struct oidset *common,
> - int *haves_to_send, int *in_vain)
> + int *haves_to_send, int *in_vain,
> + int sideband_all)
> {
> int ret = 0;
> struct strbuf req_buf = STRBUF_INIT;
> @@ -1151,6 +1152,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
> packet_buf_write(&req_buf, "include-tag");
> if (prefer_ofs_delta)
> packet_buf_write(&req_buf, "ofs-delta");
> + if (sideband_all)
> + packet_buf_write(&req_buf, "sideband-all");
>
> /* Add shallow-info and deepen request */
> if (server_supports_feature("fetch", "shallow", 0))
> @@ -1359,6 +1362,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> packet_reader_init(&reader, fd[0], NULL, 0,
> PACKET_READ_CHOMP_NEWLINE |
> PACKET_READ_DIE_ON_ERR_PACKET);
> + if (server_supports_feature("fetch", "sideband-all", 0)) {
> + reader.use_sideband = 1;
> + reader.me = "fetch-pack";
> + }
>
> while (state != FETCH_DONE) {
> switch (state) {
> @@ -1392,7 +1399,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> case FETCH_SEND_REQUEST:
> if (send_fetch_request(&negotiator, fd[1], args, ref,
> &common,
> - &haves_to_send, &in_vain))
> + &haves_to_send, &in_vain,
> + reader.use_sideband))
> state = FETCH_GET_PACK;
> else
> state = FETCH_PROCESS_ACKS;
OK, so the "fetch" side enables sideband-all any time it knows that
the other side supports it.
It feels a bit strange at the logical level that reader.me is set
only when we are going to talk over "sideband-all". I know that at
the implementation level, reader.me is only looked at when sideband
is use, but it still feels somewhat funny. Future developers may
want to use reader.me to identify the entity that found some error
during a read, even when the sideband is not yet in use, and at that
point, uninitialized .me would be a source of an error. IOW, that
assignment smells like a timebomb waiting to go off.
> @@ -483,16 +483,42 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
> return reader->status;
> }
>
> - reader->status = packet_read_with_status(reader->fd,
> - &reader->src_buffer,
> - &reader->src_len,
> - reader->buffer,
> - reader->buffer_size,
> - &reader->pktlen,
> - reader->options);
> + while (1) {
> + int retval;
> + reader->status = packet_read_with_status(reader->fd,
> + &reader->src_buffer,
> + &reader->src_len,
> + reader->buffer,
> + reader->buffer_size,
> + &reader->pktlen,
> + reader->options);
> + if (!reader->use_sideband)
> + break;
> + retval = diagnose_sideband(reader->me, reader->buffer,
> + reader->pktlen, 1);
> + switch (retval) {
> + case SIDEBAND_PROTOCOL_ERROR:
Dedent (see previous step).
> + case SIDEBAND_REMOTE_ERROR:
> + BUG("should have died in diagnose_sideband");
> + case SIDEBAND_FLUSH:
> + goto nonprogress;
> + case SIDEBAND_PRIMARY:
> + if (reader->pktlen != 1)
> + goto nonprogress;
> + /*
> + * Since pktlen is 1, this is a keepalive
> + * packet. Wait for the next one.
> + */
What guarantees that a normal payload packet is never of length==1?
> + break;
> + default: /* SIDEBAND_PROGRESS */
> + ;
> + }
> + }
>
> +nonprogress:
It is totally unclear why this label is called nonprogress. Is it
that the primary purpose of the while loop above is to spin and eat
the keep-alive packets from the other side? If so, then it sort-of
makes sense (i.e. "we are looking for progress-meter related
packets, but now we found something else, so let's leave the loop").
It would have greatly helped reviewers to have a comment in front of
that infinite loop, perhaps like
/*
* Spin and consume the keep-alive packets
*/
while (1) {
int keepalive = 0;
get_packet();
/* decide if we got a keepalive here */
...
keepalive = ...;
if (keepalive) {
/* do the keep-alive thing here */
...
continue;
}
/* do one-off thing for non-keepalive packet if any */
...
break;
}
> if (reader->status == PACKET_READ_NORMAL)
> - reader->line = reader->buffer;
> + reader->line = reader->use_sideband ?
> + reader->buffer + 1 : reader->buffer;
Is the one byte the sideband designator?
> void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
> @@ -521,7 +548,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
> va_list args;
>
> va_start(args, fmt);
> - packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
> + packet_write_fmt_1(writer->dest_fd, 0,
> + writer->use_sideband ? "\1" : "", fmt, args);
> va_end(args);
> }
As I am superstitious, I'd prefer to see octal literals to be fully
spelled out as 3-digit sequence, i.e. "\001". Likewise for "\003".
> @@ -530,7 +558,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
> va_list args;
>
> va_start(args, fmt);
> - packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
> + packet_write_fmt_1(writer->dest_fd, 0,
> + writer->use_sideband ? "\3" : "ERR ", fmt, args);
OK, band#3 is an error message for emergency exit. It is a bit
curious that this patch does not show any line from the existing
code in the context that reacts to the ERR string, but that is
because the errors are noticed at a lot lower level when the
sideband is in use than the code that currently checks for errors?
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/4] pkt-line: introduce struct packet_writer
2019-01-14 20:07 ` Junio C Hamano
@ 2019-01-15 18:18 ` Jonathan Tan
0 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-15 18:18 UTC (permalink / raw)
To: gitster; +Cc: jonathantanmy, git
> Jonathan Tan <jonathantanmy@google.com> writes:
>
> The patch itself look quite noisy, but in esssense, at the lowest
> level we used to have a single format_packet() that was used to
> write out normal payload and an error message prefixed with "ERR ";
> now the users of the function are updated to call one of the two
> helper functions, packet_writer_write() or packet_writer_error().
> Most of the patch noise comes from the helper functions at higher
> levels getting updated to pass the packet_writer struct through the
> callchain.
>
> Which makes tons of sense.
Yes, that's right. Thanks.
> > It will be convenient for a future patch if writing options
> > (specifically, whether the written data is to be multiplexed) could be
> > controlled from a single place, so create struct packet_writer to serve
> > as that place, and modify upload-pack to use it.
>
> I've singled out "ERR " in my comment above, but this only refers to
> "multiplexed". Are there reasons why we want multiplexing other
> than the "are we sending payload, or an error message"?
The main reason is to allow progress and keepalive messages at any time,
actually. I'll add this paragraph at the start of the commit message:
A future patch will allow the client to request multiplexing of the
entire fetch response (and not only during packfile transmission), which
in turn allows the server to send progress and keepalive messages at any
time during the response.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line
2019-01-14 20:07 ` Junio C Hamano
2019-01-14 22:11 ` Junio C Hamano
@ 2019-01-15 18:36 ` Jonathan Tan
1 sibling, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-15 18:36 UTC (permalink / raw)
To: gitster; +Cc: jonathantanmy, git
> Jonathan Tan <jonathantanmy@google.com> writes:
>
> > +int recv_sideband(const char *me, int in_stream, int out)
> > +{
> > + char buf[LARGE_PACKET_MAX + 1];
> > + int retval = 0;
> > + int len;
> > +
> > + while (1) {
> > + len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
> > + retval = diagnose_sideband(me, buf, len);
>
> This name of the helper does not convey much useful information.
> Let's think about it more later.
OK - after reading the later comments, switched to demultiplex_sideband.
> > + switch (retval) {
> > + case SIDEBAND_PRIMARY:
>
> Dedent; case/default arms align with 's' in switch in our codebase.
Done.
> > + break;
> > + default: /* flush or error */
> > + return retval;
>
> Lack of corresponding comment bothers readers. In all of
> REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper
> stuffs the message in outbuf in "switch (band) { ... }" and writes
> it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see
> there is no need for us to write anything out here. Perhaps
>
> case SIDEBAND_FLUSH:
> default: /* errors: message already written */
> return retval;
>
> or something to clarify?
Done.
> > +/*
> > + * Receive multiplexed output stream over git native protocol.
> > + * in_stream is the input stream from the remote, which carries data
> > + * in pkt_line format with band designator. Demultiplex it into out
> > + * and err and return error appropriately. Band #1 carries the
> > + * primary payload. Things coming over band #2 is not necessarily
> > + * error; they are usually informative message on the standard error
> > + * stream, aka "verbose"). A message over band #3 is a signal that
> > + * the remote died unexpectedly. A flush() concludes the stream.
> > + *
> > + * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR
> > + * or SIDEBAND_REMOTE_ERROR if an error occurred.
> > + */
> > +int recv_sideband(const char *me, int in_stream, int out);
>
> This is well described.
Thanks, although the credit should be given to the original author - most of
this was moved.
> > diff --git a/sideband.c b/sideband.c
> > index 368647acf8..842a92e975 100644
> > --- a/sideband.c
> > +++ b/sideband.c
> > ...
> > +int diagnose_sideband(const char *me, char *buf, int len)
> > {
> > + static const char *suffix;
> > struct strbuf outbuf = STRBUF_INIT;
> > int retval = 0;
> > + const char *b, *brk;
> > + int band;
> > +
> > + if (!suffix) {
> > + if (isatty(2) && !is_terminal_dumb())
> > + suffix = ANSI_SUFFIX;
> > + else
> > + suffix = DUMB_SUFFIX;
> > + }
>
> It may be worth considering a further clean-up for the progress
> code, that consumes lines in the "switch(band)" below that are
> disproportionate to what it does in this function by introducing
> another helper function that is called from here. When it happens,
> the above "suffix" thing should move to the helper function, too.
That's reasonable. If it's OK, I would like to limit the scope of this
patch to splitting the existing recv_sideband() into recv_sideband() and
demultiplex_sideband() (the latter is quite close to the old
recv_sideband(), and the diff looks larger only because most of it is a
dedent).
> So, the point of this helper is to inspect a single packet-line data
> and dispatch the payload of the packet based on which band it is
> sent to. Perhaps call it with a name with demultiplex or dispatch
> in it? "diagnose" is a bit unclear in that what trait you are
> diagnosing and for what purpose.
Changed to demultiplex_sideband.
> > +/*
> > + * buf and len should be the result of reading a line from a remote sending
> > + * multiplexed data.
> > + *
> > + * Determines the nature of the result and returns it. If
>
> "the result" may be from the point of view of the implementor or the
> "recv_sideband()" function who called packet_read(), but a casual
> reader would perceive it more natural if you referred it as "a
> packet read from a remote". "Inspect a packet read from the remote
> and returns which sideband it is for", perhaps?
Done.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line
2019-01-14 22:11 ` Junio C Hamano
@ 2019-01-15 18:38 ` Jonathan Tan
0 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-15 18:38 UTC (permalink / raw)
To: gitster; +Cc: jonathantanmy, git
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Lack of corresponding comment bothers readers. In all of
> > REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper
> > stuffs the message in outbuf in "switch (band) { ... }" and writes
> > it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see
> > there is no need for us to write anything out here. Perhaps
> >
> > case SIDEBAND_FLUSH:
> > default: /* errors: message already written */
> > return retval;
> >
> > or something to clarify?
>
> Forgot a footnote for *1* above. I was wondering if xwrite is what
> we really want, rather than write_in_full().
There is an existing comment explaining this:
* The output is accumulated in a buffer and
* each line is printed to stderr using
* write(2) to ensure inter-process atomicity.
which seems reasonable to me (I know that write(2) does not guarantee to
write everything, but write_in_full() does not guarantee atomicity, as
far as I know).
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response
2019-01-14 22:27 ` Junio C Hamano
@ 2019-01-15 19:18 ` Jonathan Tan
0 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-15 19:18 UTC (permalink / raw)
To: gitster; +Cc: jonathantanmy, git
> OK, so the "fetch" side enables sideband-all any time it knows that
> the other side supports it.
>
> It feels a bit strange at the logical level that reader.me is set
> only when we are going to talk over "sideband-all". I know that at
> the implementation level, reader.me is only looked at when sideband
> is use, but it still feels somewhat funny. Future developers may
> want to use reader.me to identify the entity that found some error
> during a read, even when the sideband is not yet in use, and at that
> point, uninitialized .me would be a source of an error. IOW, that
> assignment smells like a timebomb waiting to go off.
I think the best solution here is to initialize .me to "git", like how
we do it for trace. I've done that for now.
> > + switch (retval) {
> > + case SIDEBAND_PROTOCOL_ERROR:
>
> Dedent (see previous step).
Done.
> > + case SIDEBAND_PRIMARY:
> > + if (reader->pktlen != 1)
> > + goto nonprogress;
> > + /*
> > + * Since pktlen is 1, this is a keepalive
> > + * packet. Wait for the next one.
> > + */
>
> What guarantees that a normal payload packet is never of length==1?
This is indeed assuming that we currently don't send 0004 (the
equivalent of 0005\1 without the sideband). I chose this because it is
currently what we use in create_pack_file() in upload-pack.c, but I'm OK
with alternatives (e.g. if we use 0005\2 instead).
> > + break;
> > + default: /* SIDEBAND_PROGRESS */
> > + ;
> > + }
> > + }
> >
> > +nonprogress:
>
> It is totally unclear why this label is called nonprogress. Is it
> that the primary purpose of the while loop above is to spin and eat
> the keep-alive packets from the other side? If so, then it sort-of
> makes sense (i.e. "we are looking for progress-meter related
> packets, but now we found something else, so let's leave the loop").
>
> It would have greatly helped reviewers to have a comment in front of
> that infinite loop, perhaps like
>
> /*
> * Spin and consume the keep-alive packets
> */
[snip]
Yes, it's meant to spin and consume the progress and keepalive packets.
I've added a comment at the top of the loop and renamed the jump label
to "nonprogress_received".
> > if (reader->status == PACKET_READ_NORMAL)
> > - reader->line = reader->buffer;
> > + reader->line = reader->use_sideband ?
> > + reader->buffer + 1 : reader->buffer;
>
> Is the one byte the sideband designator?
Yes. Added comment to clarify that.
> > void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
> > @@ -521,7 +548,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
> > va_list args;
> >
> > va_start(args, fmt);
> > - packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
> > + packet_write_fmt_1(writer->dest_fd, 0,
> > + writer->use_sideband ? "\1" : "", fmt, args);
> > va_end(args);
> > }
>
> As I am superstitious, I'd prefer to see octal literals to be fully
> spelled out as 3-digit sequence, i.e. "\001". Likewise for "\003".
Done.
> > @@ -530,7 +558,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
> > va_list args;
> >
> > va_start(args, fmt);
> > - packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
> > + packet_write_fmt_1(writer->dest_fd, 0,
> > + writer->use_sideband ? "\3" : "ERR ", fmt, args);
>
> OK, band#3 is an error message for emergency exit. It is a bit
> curious that this patch does not show any line from the existing
> code in the context that reacts to the ERR string, but that is
> because the errors are noticed at a lot lower level when the
> sideband is in use than the code that currently checks for errors?
Yes - the branch that this patch set is on (ms/packet-err-check) handles
this. I have taken care in this patch to ensure that the error message
printed matches what Masaya used, so that in a future patch, when we
force sideband-all (using GIT_TEST_SIDEBAND_ALL), the same message is
printed so that the existing tests still pass.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 0/4] Sideband the whole fetch v2 response
2019-01-11 22:18 [PATCH 0/4] Sideband the whole fetch v2 response Jonathan Tan
` (7 preceding siblings ...)
2019-01-11 22:18 ` [WIP 8/4] upload-pack: send part of packfile response as uri Jonathan Tan
@ 2019-01-15 19:40 ` Jonathan Tan
2019-01-15 19:40 ` [PATCH v2 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
` (5 more replies)
2019-01-16 19:28 ` [PATCH v3 " Jonathan Tan
9 siblings, 6 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-15 19:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
Like v1, this is on origin/ms/packet-err-check.
Thanks, Junio, for your reviews.
Jonathan Tan (4):
pkt-line: introduce struct packet_writer
sideband: reverse its dependency on pkt-line
{fetch,upload}-pack: sideband v2 fetch response
tests: define GIT_TEST_SIDEBAND_ALL
Documentation/technical/protocol-v2.txt | 10 ++
fetch-pack.c | 13 +-
pkt-line.c | 121 +++++++++++++++---
pkt-line.h | 34 +++++
sideband.c | 161 ++++++++++++------------
sideband.h | 14 ++-
t/README | 5 +
t/lib-httpd/apache.conf | 1 +
t/t5537-fetch-shallow.sh | 3 +-
t/t5701-git-serve.sh | 2 +-
t/t5702-protocol-v2.sh | 4 +-
upload-pack.c | 131 +++++++++++--------
12 files changed, 343 insertions(+), 156 deletions(-)
Interdiff against v1:
diff --git a/pkt-line.c b/pkt-line.c
index 71b17e6b93..69162f3990 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -447,16 +447,16 @@ int recv_sideband(const char *me, int in_stream, int out)
while (1) {
len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
- retval = diagnose_sideband(me, buf, len, 0);
+ retval = demultiplex_sideband(me, buf, len, 0);
switch (retval) {
- case SIDEBAND_PRIMARY:
- write_or_die(out, buf + 1, len - 1);
- break;
- case SIDEBAND_PROGRESS:
- /* already written by diagnose_sideband() */
- break;
- default: /* flush or error */
- return retval;
+ case SIDEBAND_PRIMARY:
+ write_or_die(out, buf + 1, len - 1);
+ break;
+ case SIDEBAND_PROGRESS:
+ /* already written by demultiplex_sideband() */
+ break;
+ default: /* errors: message already written */
+ return retval;
}
}
}
@@ -474,6 +474,7 @@ void packet_reader_init(struct packet_reader *reader, int fd,
reader->buffer = packet_buffer;
reader->buffer_size = sizeof(packet_buffer);
reader->options = options;
+ reader->me = "git";
}
enum packet_read_status packet_reader_read(struct packet_reader *reader)
@@ -483,6 +484,10 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
return reader->status;
}
+ /*
+ * Consume all progress and keepalive packets until a primary payload
+ * packet is received
+ */
while (1) {
int retval;
reader->status = packet_read_with_status(reader->fd,
@@ -494,29 +499,31 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
reader->options);
if (!reader->use_sideband)
break;
- retval = diagnose_sideband(reader->me, reader->buffer,
- reader->pktlen, 1);
+ retval = demultiplex_sideband(reader->me, reader->buffer,
+ reader->pktlen, 1);
switch (retval) {
- case SIDEBAND_PROTOCOL_ERROR:
- case SIDEBAND_REMOTE_ERROR:
- BUG("should have died in diagnose_sideband");
- case SIDEBAND_FLUSH:
- goto nonprogress;
- case SIDEBAND_PRIMARY:
- if (reader->pktlen != 1)
- goto nonprogress;
- /*
- * Since pktlen is 1, this is a keepalive
- * packet. Wait for the next one.
- */
- break;
- default: /* SIDEBAND_PROGRESS */
- ;
+ case SIDEBAND_PROTOCOL_ERROR:
+ case SIDEBAND_REMOTE_ERROR:
+ BUG("should have died in diagnose_sideband");
+ case SIDEBAND_FLUSH:
+ goto nonprogress_received;
+ case SIDEBAND_PRIMARY:
+ if (reader->pktlen != 1)
+ goto nonprogress_received;
+ /*
+ * Since the packet contains nothing but the sideband
+ * designator, this is a keepalive packet. Wait for the
+ * next one.
+ */
+ break;
+ default: /* SIDEBAND_PROGRESS */
+ ;
}
}
-nonprogress:
+nonprogress_received:
if (reader->status == PACKET_READ_NORMAL)
+ /* Skip the sideband designator if sideband is used */
reader->line = reader->use_sideband ?
reader->buffer + 1 : reader->buffer;
else
@@ -549,7 +556,7 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
va_start(args, fmt);
packet_write_fmt_1(writer->dest_fd, 0,
- writer->use_sideband ? "\1" : "", fmt, args);
+ writer->use_sideband ? "\001" : "", fmt, args);
va_end(args);
}
@@ -559,7 +566,7 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
va_start(args, fmt);
packet_write_fmt_1(writer->dest_fd, 0,
- writer->use_sideband ? "\3" : "ERR ", fmt, args);
+ writer->use_sideband ? "\003" : "ERR ", fmt, args);
va_end(args);
}
diff --git a/sideband.c b/sideband.c
index cbeab380ae..9d3051e3fe 100644
--- a/sideband.c
+++ b/sideband.c
@@ -113,7 +113,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
#define ANSI_SUFFIX "\033[K"
#define DUMB_SUFFIX " "
-int diagnose_sideband(const char *me, char *buf, int len, int die_on_error)
+int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
{
static const char *suffix;
struct strbuf outbuf = STRBUF_INIT;
diff --git a/sideband.h b/sideband.h
index 3786670694..f75c4fde2a 100644
--- a/sideband.h
+++ b/sideband.h
@@ -8,15 +8,14 @@
#define SIDEBAND_PROGRESS 2
/*
- * buf and len should be the result of reading a line from a remote sending
- * multiplexed data.
+ * Inspects a multiplexed packet read from the remote and returns which
+ * sideband it is for.
*
- * Determines the nature of the result and returns it. If
- * SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS, also
- * prints a message (or the formatted contents of the notice in the case of
- * SIDEBAND_PROGRESS) to stderr.
+ * If SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS,
+ * also prints a message (or the formatted contents of the notice in the case
+ * of SIDEBAND_PROGRESS) to stderr.
*/
-int diagnose_sideband(const char *me, char *buf, int len, int die_on_error);
+int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error);
void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v2 1/4] pkt-line: introduce struct packet_writer
2019-01-15 19:40 ` [PATCH v2 0/4] Sideband the whole fetch v2 response Jonathan Tan
@ 2019-01-15 19:40 ` Jonathan Tan
2019-01-15 19:40 ` [PATCH v2 2/4] sideband: reverse its dependency on pkt-line Jonathan Tan
` (4 subsequent siblings)
5 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-15 19:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
A future patch will allow the client to request multiplexing of the
entire fetch response (and not only during packfile transmission), which
in turn allows the server to send progress and keepalive messages at any
time during the response.
It will be convenient for a future patch if writing options
(specifically, whether the written data is to be multiplexed) could be
controlled from a single place, so create struct packet_writer to serve
as that place, and modify upload-pack to use it.
Currently, it only stores the output fd, but a subsequent patch will (as
described above) introduce an option to determine if the written data is
to be multiplexed.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
pkt-line.c | 47 ++++++++++++++++++---
pkt-line.h | 14 +++++++
upload-pack.c | 112 +++++++++++++++++++++++++++-----------------------
3 files changed, 115 insertions(+), 58 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index e70ea6d88f..9d3e402d41 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -129,12 +129,14 @@ static void set_packet_header(char *buf, const int size)
#undef hex
}
-static void format_packet(struct strbuf *out, const char *fmt, va_list args)
+static void format_packet(struct strbuf *out, const char *prefix,
+ const char *fmt, va_list args)
{
size_t orig_len, n;
orig_len = out->len;
strbuf_addstr(out, "0000");
+ strbuf_addstr(out, prefix);
strbuf_vaddf(out, fmt, args);
n = out->len - orig_len;
@@ -145,13 +147,13 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)
packet_trace(out->buf + orig_len + 4, n - 4, 1);
}
-static int packet_write_fmt_1(int fd, int gently,
+static int packet_write_fmt_1(int fd, int gently, const char *prefix,
const char *fmt, va_list args)
{
static struct strbuf buf = STRBUF_INIT;
strbuf_reset(&buf);
- format_packet(&buf, fmt, args);
+ format_packet(&buf, prefix, fmt, args);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
if (!gently) {
check_pipe(errno);
@@ -168,7 +170,7 @@ void packet_write_fmt(int fd, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- packet_write_fmt_1(fd, 0, fmt, args);
+ packet_write_fmt_1(fd, 0, "", fmt, args);
va_end(args);
}
@@ -178,7 +180,7 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- status = packet_write_fmt_1(fd, 1, fmt, args);
+ status = packet_write_fmt_1(fd, 1, "", fmt, args);
va_end(args);
return status;
}
@@ -211,7 +213,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- format_packet(buf, fmt, args);
+ format_packet(buf, "", fmt, args);
va_end(args);
}
@@ -486,3 +488,36 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)
reader->line_peeked = 1;
return reader->status;
}
+
+void packet_writer_init(struct packet_writer *writer, int dest_fd)
+{
+ writer->dest_fd = dest_fd;
+}
+
+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
+{
+ va_list args;
+
+ va_start(args, fmt);
+ packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
+ va_end(args);
+}
+
+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
+{
+ va_list args;
+
+ va_start(args, fmt);
+ packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
+ va_end(args);
+}
+
+void packet_writer_delim(struct packet_writer *writer)
+{
+ packet_delim(writer->dest_fd);
+}
+
+void packet_writer_flush(struct packet_writer *writer)
+{
+ packet_flush(writer->dest_fd);
+}
diff --git a/pkt-line.h b/pkt-line.h
index d7e1dbc047..023ad2951d 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -183,4 +183,18 @@ extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);
#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
extern char packet_buffer[LARGE_PACKET_MAX];
+struct packet_writer {
+ int dest_fd;
+};
+
+void packet_writer_init(struct packet_writer *writer, int dest_fd);
+
+/* These functions die upon failure. */
+__attribute__((format (printf, 2, 3)))
+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...);
+__attribute__((format (printf, 2, 3)))
+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...);
+void packet_writer_delim(struct packet_writer *writer);
+void packet_writer_flush(struct packet_writer *writer);
+
#endif
diff --git a/upload-pack.c b/upload-pack.c
index 08b547cf01..60a26bbbfd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -613,13 +613,14 @@ static void check_non_tip(struct object_array *want_obj)
}
}
-static void send_shallow(struct commit_list *result)
+static void send_shallow(struct packet_writer *writer,
+ struct commit_list *result)
{
while (result) {
struct object *object = &result->item->object;
if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
- packet_write_fmt(1, "shallow %s",
- oid_to_hex(&object->oid));
+ packet_writer_write(writer, "shallow %s",
+ oid_to_hex(&object->oid));
register_shallow(the_repository, &object->oid);
shallow_nr++;
}
@@ -627,7 +628,8 @@ static void send_shallow(struct commit_list *result)
}
}
-static void send_unshallow(const struct object_array *shallows,
+static void send_unshallow(struct packet_writer *writer,
+ const struct object_array *shallows,
struct object_array *want_obj)
{
int i;
@@ -636,8 +638,8 @@ static void send_unshallow(const struct object_array *shallows,
struct object *object = shallows->objects[i].item;
if (object->flags & NOT_SHALLOW) {
struct commit_list *parents;
- packet_write_fmt(1, "unshallow %s",
- oid_to_hex(&object->oid));
+ packet_writer_write(writer, "unshallow %s",
+ oid_to_hex(&object->oid));
object->flags &= ~CLIENT_SHALLOW;
/*
* We want to _register_ "object" as shallow, but we
@@ -662,7 +664,7 @@ static void send_unshallow(const struct object_array *shallows,
}
}
-static void deepen(int depth, int deepen_relative,
+static void deepen(struct packet_writer *writer, int depth, int deepen_relative,
struct object_array *shallows, struct object_array *want_obj)
{
if (depth == INFINITE_DEPTH && !is_repository_shallow(the_repository)) {
@@ -680,7 +682,7 @@ static void deepen(int depth, int deepen_relative,
result = get_shallow_commits(&reachable_shallows,
depth + 1,
SHALLOW, NOT_SHALLOW);
- send_shallow(result);
+ send_shallow(writer, result);
free_commit_list(result);
object_array_clear(&reachable_shallows);
} else {
@@ -688,14 +690,15 @@ static void deepen(int depth, int deepen_relative,
result = get_shallow_commits(want_obj, depth,
SHALLOW, NOT_SHALLOW);
- send_shallow(result);
+ send_shallow(writer, result);
free_commit_list(result);
}
- send_unshallow(shallows, want_obj);
+ send_unshallow(writer, shallows, want_obj);
}
-static void deepen_by_rev_list(int ac, const char **av,
+static void deepen_by_rev_list(struct packet_writer *writer, int ac,
+ const char **av,
struct object_array *shallows,
struct object_array *want_obj)
{
@@ -703,13 +706,14 @@ static void deepen_by_rev_list(int ac, const char **av,
close_commit_graph(the_repository);
result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
- send_shallow(result);
+ send_shallow(writer, result);
free_commit_list(result);
- send_unshallow(shallows, want_obj);
+ send_unshallow(writer, shallows, want_obj);
}
/* Returns 1 if a shallow list is sent or 0 otherwise */
-static int send_shallow_list(int depth, int deepen_rev_list,
+static int send_shallow_list(struct packet_writer *writer,
+ int depth, int deepen_rev_list,
timestamp_t deepen_since,
struct string_list *deepen_not,
struct object_array *shallows,
@@ -720,7 +724,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,
if (depth > 0 && deepen_rev_list)
die("git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together");
if (depth > 0) {
- deepen(depth, deepen_relative, shallows, want_obj);
+ deepen(writer, depth, deepen_relative, shallows, want_obj);
ret = 1;
} else if (deepen_rev_list) {
struct argv_array av = ARGV_ARRAY_INIT;
@@ -741,7 +745,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,
struct object *o = want_obj->objects[i].item;
argv_array_push(&av, oid_to_hex(&o->oid));
}
- deepen_by_rev_list(av.argc, av.argv, shallows, want_obj);
+ deepen_by_rev_list(writer, av.argc, av.argv, shallows, want_obj);
argv_array_clear(&av);
ret = 1;
} else {
@@ -834,8 +838,10 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
int has_non_tip = 0;
timestamp_t deepen_since = 0;
int deepen_rev_list = 0;
+ struct packet_writer writer;
shallow_nr = 0;
+ packet_writer_init(&writer, 1);
for (;;) {
struct object *o;
const char *features;
@@ -892,9 +898,9 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
o = parse_object(the_repository, &oid_buf);
if (!o) {
- packet_write_fmt(1,
- "ERR upload-pack: not our ref %s",
- oid_to_hex(&oid_buf));
+ packet_writer_error(&writer,
+ "upload-pack: not our ref %s",
+ oid_to_hex(&oid_buf));
die("git upload-pack: not our ref %s",
oid_to_hex(&oid_buf));
}
@@ -923,7 +929,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
return;
- if (send_shallow_list(depth, deepen_rev_list, deepen_since,
+ if (send_shallow_list(&writer, depth, deepen_rev_list, deepen_since,
&deepen_not, &shallows, want_obj))
packet_flush(1);
object_array_clear(&shallows);
@@ -1102,6 +1108,8 @@ struct upload_pack_data {
int deepen_rev_list;
int deepen_relative;
+ struct packet_writer writer;
+
unsigned stateless_rpc : 1;
unsigned use_thin_pack : 1;
@@ -1125,6 +1133,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
data->haves = haves;
data->shallows = shallows;
data->deepen_not = deepen_not;
+ packet_writer_init(&data->writer, 1);
}
static void upload_pack_data_clear(struct upload_pack_data *data)
@@ -1136,7 +1145,8 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
string_list_clear(&data->deepen_not, 0);
}
-static int parse_want(const char *line, struct object_array *want_obj)
+static int parse_want(struct packet_writer *writer, const char *line,
+ struct object_array *want_obj)
{
const char *arg;
if (skip_prefix(line, "want ", &arg)) {
@@ -1149,9 +1159,9 @@ static int parse_want(const char *line, struct object_array *want_obj)
o = parse_object(the_repository, &oid);
if (!o) {
- packet_write_fmt(1,
- "ERR upload-pack: not our ref %s",
- oid_to_hex(&oid));
+ packet_writer_error(writer,
+ "upload-pack: not our ref %s",
+ oid_to_hex(&oid));
die("git upload-pack: not our ref %s",
oid_to_hex(&oid));
}
@@ -1167,7 +1177,8 @@ static int parse_want(const char *line, struct object_array *want_obj)
return 0;
}
-static int parse_want_ref(const char *line, struct string_list *wanted_refs,
+static int parse_want_ref(struct packet_writer *writer, const char *line,
+ struct string_list *wanted_refs,
struct object_array *want_obj)
{
const char *arg;
@@ -1177,7 +1188,7 @@ static int parse_want_ref(const char *line, struct string_list *wanted_refs,
struct object *o;
if (read_ref(arg, &oid)) {
- packet_write_fmt(1, "ERR unknown ref %s", arg);
+ packet_writer_error(writer, "unknown ref %s", arg);
die("unknown ref %s", arg);
}
@@ -1220,10 +1231,11 @@ static void process_args(struct packet_reader *request,
const char *p;
/* process want */
- if (parse_want(arg, want_obj))
+ if (parse_want(&data->writer, arg, want_obj))
continue;
if (allow_ref_in_want &&
- parse_want_ref(arg, &data->wanted_refs, want_obj))
+ parse_want_ref(&data->writer, arg, &data->wanted_refs,
+ want_obj))
continue;
/* process have line */
if (parse_have(arg, &data->haves))
@@ -1317,26 +1329,26 @@ static int process_haves(struct oid_array *haves, struct oid_array *common,
return 0;
}
-static int send_acks(struct oid_array *acks, struct strbuf *response,
+static int send_acks(struct packet_writer *writer, struct oid_array *acks,
const struct object_array *have_obj,
struct object_array *want_obj)
{
int i;
- packet_buf_write(response, "acknowledgments\n");
+ packet_writer_write(writer, "acknowledgments\n");
/* Send Acks */
if (!acks->nr)
- packet_buf_write(response, "NAK\n");
+ packet_writer_write(writer, "NAK\n");
for (i = 0; i < acks->nr; i++) {
- packet_buf_write(response, "ACK %s\n",
- oid_to_hex(&acks->oid[i]));
+ packet_writer_write(writer, "ACK %s\n",
+ oid_to_hex(&acks->oid[i]));
}
if (ok_to_give_up(have_obj, want_obj)) {
/* Send Ready */
- packet_buf_write(response, "ready\n");
+ packet_writer_write(writer, "ready\n");
return 1;
}
@@ -1348,25 +1360,20 @@ static int process_haves_and_send_acks(struct upload_pack_data *data,
struct object_array *want_obj)
{
struct oid_array common = OID_ARRAY_INIT;
- struct strbuf response = STRBUF_INIT;
int ret = 0;
process_haves(&data->haves, &common, have_obj);
if (data->done) {
ret = 1;
- } else if (send_acks(&common, &response, have_obj, want_obj)) {
- packet_buf_delim(&response);
+ } else if (send_acks(&data->writer, &common, have_obj, want_obj)) {
+ packet_writer_delim(&data->writer);
ret = 1;
} else {
/* Add Flush */
- packet_buf_flush(&response);
+ packet_writer_flush(&data->writer);
ret = 0;
}
- /* Send response */
- write_or_die(1, response.buf, response.len);
- strbuf_release(&response);
-
oid_array_clear(&data->haves);
oid_array_clear(&common);
return ret;
@@ -1379,15 +1386,15 @@ static void send_wanted_ref_info(struct upload_pack_data *data)
if (!data->wanted_refs.nr)
return;
- packet_write_fmt(1, "wanted-refs\n");
+ packet_writer_write(&data->writer, "wanted-refs\n");
for_each_string_list_item(item, &data->wanted_refs) {
- packet_write_fmt(1, "%s %s\n",
- oid_to_hex(item->util),
- item->string);
+ packet_writer_write(&data->writer, "%s %s\n",
+ oid_to_hex(item->util),
+ item->string);
}
- packet_delim(1);
+ packet_writer_delim(&data->writer);
}
static void send_shallow_info(struct upload_pack_data *data,
@@ -1398,14 +1405,15 @@ static void send_shallow_info(struct upload_pack_data *data,
!is_repository_shallow(the_repository))
return;
- packet_write_fmt(1, "shallow-info\n");
+ packet_writer_write(&data->writer, "shallow-info\n");
- if (!send_shallow_list(data->depth, data->deepen_rev_list,
+ if (!send_shallow_list(&data->writer, data->depth,
+ data->deepen_rev_list,
data->deepen_since, &data->deepen_not,
&data->shallows, want_obj) &&
is_repository_shallow(the_repository))
- deepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,
- want_obj);
+ deepen(&data->writer, INFINITE_DEPTH, data->deepen_relative,
+ &data->shallows, want_obj);
packet_delim(1);
}
@@ -1467,7 +1475,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
send_wanted_ref_info(&data);
send_shallow_info(&data, &want_obj);
- packet_write_fmt(1, "packfile\n");
+ packet_writer_write(&data.writer, "packfile\n");
create_pack_file(&have_obj, &want_obj);
state = FETCH_DONE;
break;
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v2 2/4] sideband: reverse its dependency on pkt-line
2019-01-15 19:40 ` [PATCH v2 0/4] Sideband the whole fetch v2 response Jonathan Tan
2019-01-15 19:40 ` [PATCH v2 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
@ 2019-01-15 19:40 ` Jonathan Tan
2019-01-16 10:34 ` SZEDER Gábor
2019-01-15 19:40 ` [PATCH v2 3/4] {fetch,upload}-pack: sideband v2 fetch response Jonathan Tan
` (3 subsequent siblings)
5 siblings, 1 reply; 41+ messages in thread
From: Jonathan Tan @ 2019-01-15 19:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
A subsequent patch will teach struct packet_reader a new field that, if
set, instructs it to interpret read data as multiplexed. This will
create a dependency from pkt-line to sideband.
To avoid a circular dependency, split recv_sideband() into 2 parts: the
reading loop (left in recv_sideband()) and the processing of the
contents (in demultiplex_sideband()), and move the former into pkt-line.
This reverses the direction of dependency: sideband no longer depends on
pkt-line, and pkt-line now depends on sideband.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
pkt-line.c | 22 ++++++++
pkt-line.h | 16 ++++++
sideband.c | 156 +++++++++++++++++++++++++----------------------------
sideband.h | 14 ++++-
4 files changed, 124 insertions(+), 84 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index 9d3e402d41..5feb73ebec 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -439,6 +439,28 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
return sb_out->len - orig_len;
}
+int recv_sideband(const char *me, int in_stream, int out)
+{
+ char buf[LARGE_PACKET_MAX + 1];
+ int retval = 0;
+ int len;
+
+ while (1) {
+ len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
+ retval = demultiplex_sideband(me, buf, len);
+ switch (retval) {
+ case SIDEBAND_PRIMARY:
+ write_or_die(out, buf + 1, len - 1);
+ break;
+ case SIDEBAND_PROGRESS:
+ /* already written by demultiplex_sideband() */
+ break;
+ default: /* errors: message already written */
+ return retval;
+ }
+ }
+}
+
/* Packet Reader Functions */
void packet_reader_init(struct packet_reader *reader, int fd,
char *src_buffer, size_t src_len,
diff --git a/pkt-line.h b/pkt-line.h
index 023ad2951d..a8e92a4b63 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -3,6 +3,7 @@
#include "git-compat-util.h"
#include "strbuf.h"
+#include "sideband.h"
/*
* Write a packetized stream, where each line is preceded by
@@ -120,6 +121,21 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
*/
ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
+/*
+ * Receive multiplexed output stream over git native protocol.
+ * in_stream is the input stream from the remote, which carries data
+ * in pkt_line format with band designator. Demultiplex it into out
+ * and err and return error appropriately. Band #1 carries the
+ * primary payload. Things coming over band #2 is not necessarily
+ * error; they are usually informative message on the standard error
+ * stream, aka "verbose"). A message over band #3 is a signal that
+ * the remote died unexpectedly. A flush() concludes the stream.
+ *
+ * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR
+ * or SIDEBAND_REMOTE_ERROR if an error occurred.
+ */
+int recv_sideband(const char *me, int in_stream, int out);
+
struct packet_reader {
/* source file descriptor */
int fd;
diff --git a/sideband.c b/sideband.c
index 368647acf8..dce22d20b1 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,7 +1,6 @@
#include "cache.h"
#include "color.h"
#include "config.h"
-#include "pkt-line.h"
#include "sideband.h"
#include "help.h"
@@ -109,103 +108,94 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
}
-/*
- * Receive multiplexed output stream over git native protocol.
- * in_stream is the input stream from the remote, which carries data
- * in pkt_line format with band designator. Demultiplex it into out
- * and err and return error appropriately. Band #1 carries the
- * primary payload. Things coming over band #2 is not necessarily
- * error; they are usually informative message on the standard error
- * stream, aka "verbose"). A message over band #3 is a signal that
- * the remote died unexpectedly. A flush() concludes the stream.
- */
-
#define DISPLAY_PREFIX "remote: "
#define ANSI_SUFFIX "\033[K"
#define DUMB_SUFFIX " "
-int recv_sideband(const char *me, int in_stream, int out)
+int demultiplex_sideband(const char *me, char *buf, int len)
{
- const char *suffix;
- char buf[LARGE_PACKET_MAX + 1];
+ static const char *suffix;
struct strbuf outbuf = STRBUF_INIT;
int retval = 0;
+ const char *b, *brk;
+ int band;
+
+ if (!suffix) {
+ if (isatty(2) && !is_terminal_dumb())
+ suffix = ANSI_SUFFIX;
+ else
+ suffix = DUMB_SUFFIX;
+ }
- if (isatty(2) && !is_terminal_dumb())
- suffix = ANSI_SUFFIX;
- else
- suffix = DUMB_SUFFIX;
+ if (len == 0) {
+ retval = SIDEBAND_FLUSH;
+ goto cleanup;
+ }
+ if (len < 1) {
+ strbuf_addf(&outbuf,
+ "%s%s: protocol error: no band designator",
+ outbuf.len ? "\n" : "", me);
+ retval = SIDEBAND_PROTOCOL_ERROR;
+ goto cleanup;
+ }
+ band = buf[0] & 0xff;
+ buf[len] = '\0';
+ len--;
+ switch (band) {
+ case 3:
+ strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
+ DISPLAY_PREFIX);
+ maybe_colorize_sideband(&outbuf, buf + 1, len);
+
+ retval = SIDEBAND_REMOTE_ERROR;
+ break;
+ case 2:
+ b = buf + 1;
- while (!retval) {
- const char *b, *brk;
- int band, len;
- len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
- if (len == 0)
- break;
- if (len < 1) {
- strbuf_addf(&outbuf,
- "%s%s: protocol error: no band designator",
- outbuf.len ? "\n" : "", me);
- retval = SIDEBAND_PROTOCOL_ERROR;
- break;
- }
- band = buf[0] & 0xff;
- buf[len] = '\0';
- len--;
- switch (band) {
- case 3:
- strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
- DISPLAY_PREFIX);
- maybe_colorize_sideband(&outbuf, buf + 1, len);
-
- retval = 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 (!outbuf.len)
- strbuf_addstr(&outbuf, DISPLAY_PREFIX);
- if (linelen > 0) {
- 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;
+ /*
+ * 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 (!outbuf.len)
+ strbuf_addstr(&outbuf, DISPLAY_PREFIX);
+ if (linelen > 0) {
+ maybe_colorize_sideband(&outbuf, b, linelen);
+ strbuf_addstr(&outbuf, suffix);
}
- 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);
- break;
- default:
- strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
- outbuf.len ? "\n" : "", me, band);
- retval = SIDEBAND_PROTOCOL_ERROR;
- break;
+ strbuf_addch(&outbuf, *brk);
+ xwrite(2, outbuf.buf, outbuf.len);
+ strbuf_reset(&outbuf);
+
+ b = brk + 1;
+ }
+
+ if (*b) {
+ strbuf_addstr(&outbuf, outbuf.len ?
+ "" : DISPLAY_PREFIX);
+ maybe_colorize_sideband(&outbuf, b, strlen(b));
}
+ retval = SIDEBAND_PROGRESS;
+ break;
+ case 1:
+ retval = SIDEBAND_PRIMARY;
+ break;
+ default:
+ strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
+ outbuf.len ? "\n" : "", me, band);
+ retval = SIDEBAND_PROTOCOL_ERROR;
+ break;
}
+cleanup:
if (outbuf.len) {
strbuf_addch(&outbuf, '\n');
xwrite(2, outbuf.buf, outbuf.len);
diff --git a/sideband.h b/sideband.h
index 7a8146f161..7244971231 100644
--- a/sideband.h
+++ b/sideband.h
@@ -3,8 +3,20 @@
#define SIDEBAND_PROTOCOL_ERROR -2
#define SIDEBAND_REMOTE_ERROR -1
+#define SIDEBAND_FLUSH 0
+#define SIDEBAND_PRIMARY 1
+#define SIDEBAND_PROGRESS 2
+
+/*
+ * Inspects a multiplexed packet read from the remote and returns which
+ * sideband it is for.
+ *
+ * If SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS,
+ * also prints a message (or the formatted contents of the notice in the case
+ * of SIDEBAND_PROGRESS) to stderr.
+ */
+int demultiplex_sideband(const char *me, char *buf, int len);
-int recv_sideband(const char *me, int in_stream, int out);
void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
#endif
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v2 3/4] {fetch,upload}-pack: sideband v2 fetch response
2019-01-15 19:40 ` [PATCH v2 0/4] Sideband the whole fetch v2 response Jonathan Tan
2019-01-15 19:40 ` [PATCH v2 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
2019-01-15 19:40 ` [PATCH v2 2/4] sideband: reverse its dependency on pkt-line Jonathan Tan
@ 2019-01-15 19:40 ` Jonathan Tan
2019-01-15 19:40 ` [PATCH v2 4/4] tests: define GIT_TEST_SIDEBAND_ALL Jonathan Tan
` (2 subsequent siblings)
5 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-15 19:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
Currently, a response to a fetch request has sideband support only while
the packfile is being sent, meaning that the server cannot send notices
until the start of the packfile.
Extend sideband support in protocol v2 fetch responses to the whole
response. upload-pack will advertise it if the
uploadpack.allowsidebandall configuration variable is set, and
fetch-pack will automatically request it if advertised.
If the sideband is to be used throughout the whole response, upload-pack
will use it to send errors instead of prefixing a PKT-LINE payload with
"ERR ".
This will be tested in a subsequent patch.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Documentation/technical/protocol-v2.txt | 10 +++++
fetch-pack.c | 12 ++++-
pkt-line.c | 58 ++++++++++++++++++++-----
pkt-line.h | 4 ++
sideband.c | 7 ++-
sideband.h | 2 +-
upload-pack.c | 16 +++++++
7 files changed, 94 insertions(+), 15 deletions(-)
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 09e4e0273f..1b0633f59f 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -307,6 +307,16 @@ the 'wanted-refs' section in the server's response as explained below.
particular ref, where <ref> is the full name of a ref on the
server.
+If the 'sideband-all' feature is advertised, the following argument can be
+included in the client's request:
+
+ sideband-all
+ Instruct the server to send the whole response multiplexed, not just
+ the packfile section. All non-flush and non-delim PKT-LINE in the
+ response (not only in the packfile section) will then start with a byte
+ indicating its sideband (1, 2, or 3), and the server may send "0005\1"
+ (a PKT-LINE of sideband 1 with no payload) as a keepalive packet.
+
The response of `fetch` is broken into a number of sections separated by
delimiter packets (0001), with each section beginning with its section
header.
diff --git a/fetch-pack.c b/fetch-pack.c
index 3f9626dbf7..b89199891d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1125,7 +1125,8 @@ static int add_haves(struct fetch_negotiator *negotiator,
static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
const struct fetch_pack_args *args,
const struct ref *wants, struct oidset *common,
- int *haves_to_send, int *in_vain)
+ int *haves_to_send, int *in_vain,
+ int sideband_all)
{
int ret = 0;
struct strbuf req_buf = STRBUF_INIT;
@@ -1151,6 +1152,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
packet_buf_write(&req_buf, "include-tag");
if (prefer_ofs_delta)
packet_buf_write(&req_buf, "ofs-delta");
+ if (sideband_all)
+ packet_buf_write(&req_buf, "sideband-all");
/* Add shallow-info and deepen request */
if (server_supports_feature("fetch", "shallow", 0))
@@ -1359,6 +1362,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
packet_reader_init(&reader, fd[0], NULL, 0,
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_DIE_ON_ERR_PACKET);
+ if (server_supports_feature("fetch", "sideband-all", 0)) {
+ reader.use_sideband = 1;
+ reader.me = "fetch-pack";
+ }
while (state != FETCH_DONE) {
switch (state) {
@@ -1392,7 +1399,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
case FETCH_SEND_REQUEST:
if (send_fetch_request(&negotiator, fd[1], args, ref,
&common,
- &haves_to_send, &in_vain))
+ &haves_to_send, &in_vain,
+ reader.use_sideband))
state = FETCH_GET_PACK;
else
state = FETCH_PROCESS_ACKS;
diff --git a/pkt-line.c b/pkt-line.c
index 5feb73ebec..69162f3990 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -447,7 +447,7 @@ int recv_sideband(const char *me, int in_stream, int out)
while (1) {
len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
- retval = demultiplex_sideband(me, buf, len);
+ retval = demultiplex_sideband(me, buf, len, 0);
switch (retval) {
case SIDEBAND_PRIMARY:
write_or_die(out, buf + 1, len - 1);
@@ -474,6 +474,7 @@ void packet_reader_init(struct packet_reader *reader, int fd,
reader->buffer = packet_buffer;
reader->buffer_size = sizeof(packet_buffer);
reader->options = options;
+ reader->me = "git";
}
enum packet_read_status packet_reader_read(struct packet_reader *reader)
@@ -483,16 +484,48 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
return reader->status;
}
- reader->status = packet_read_with_status(reader->fd,
- &reader->src_buffer,
- &reader->src_len,
- reader->buffer,
- reader->buffer_size,
- &reader->pktlen,
- reader->options);
+ /*
+ * Consume all progress and keepalive packets until a primary payload
+ * packet is received
+ */
+ while (1) {
+ int retval;
+ reader->status = packet_read_with_status(reader->fd,
+ &reader->src_buffer,
+ &reader->src_len,
+ reader->buffer,
+ reader->buffer_size,
+ &reader->pktlen,
+ reader->options);
+ if (!reader->use_sideband)
+ break;
+ retval = demultiplex_sideband(reader->me, reader->buffer,
+ reader->pktlen, 1);
+ switch (retval) {
+ case SIDEBAND_PROTOCOL_ERROR:
+ case SIDEBAND_REMOTE_ERROR:
+ BUG("should have died in diagnose_sideband");
+ case SIDEBAND_FLUSH:
+ goto nonprogress_received;
+ case SIDEBAND_PRIMARY:
+ if (reader->pktlen != 1)
+ goto nonprogress_received;
+ /*
+ * Since the packet contains nothing but the sideband
+ * designator, this is a keepalive packet. Wait for the
+ * next one.
+ */
+ break;
+ default: /* SIDEBAND_PROGRESS */
+ ;
+ }
+ }
+nonprogress_received:
if (reader->status == PACKET_READ_NORMAL)
- reader->line = reader->buffer;
+ /* Skip the sideband designator if sideband is used */
+ reader->line = reader->use_sideband ?
+ reader->buffer + 1 : reader->buffer;
else
reader->line = NULL;
@@ -514,6 +547,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)
void packet_writer_init(struct packet_writer *writer, int dest_fd)
{
writer->dest_fd = dest_fd;
+ writer->use_sideband = 0;
}
void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
@@ -521,7 +555,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
+ packet_write_fmt_1(writer->dest_fd, 0,
+ writer->use_sideband ? "\001" : "", fmt, args);
va_end(args);
}
@@ -530,7 +565,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
+ packet_write_fmt_1(writer->dest_fd, 0,
+ writer->use_sideband ? "\003" : "ERR ", fmt, args);
va_end(args);
}
diff --git a/pkt-line.h b/pkt-line.h
index a8e92a4b63..ad9a4a2cd7 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -162,6 +162,9 @@ struct packet_reader {
/* indicates if a line has been peeked */
int line_peeked;
+
+ unsigned use_sideband : 1;
+ const char *me;
};
/*
@@ -201,6 +204,7 @@ extern char packet_buffer[LARGE_PACKET_MAX];
struct packet_writer {
int dest_fd;
+ unsigned use_sideband : 1;
};
void packet_writer_init(struct packet_writer *writer, int dest_fd);
diff --git a/sideband.c b/sideband.c
index dce22d20b1..9d3051e3fe 100644
--- a/sideband.c
+++ b/sideband.c
@@ -113,7 +113,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
#define ANSI_SUFFIX "\033[K"
#define DUMB_SUFFIX " "
-int demultiplex_sideband(const char *me, char *buf, int len)
+int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
{
static const char *suffix;
struct strbuf outbuf = STRBUF_INIT;
@@ -144,6 +144,9 @@ int demultiplex_sideband(const char *me, char *buf, int len)
len--;
switch (band) {
case 3:
+ if (die_on_error)
+ die("remote error: %s", buf + 1);
+
strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
DISPLAY_PREFIX);
maybe_colorize_sideband(&outbuf, buf + 1, len);
@@ -196,6 +199,8 @@ int demultiplex_sideband(const char *me, char *buf, int len)
}
cleanup:
+ if (die_on_error && retval == SIDEBAND_PROTOCOL_ERROR)
+ die("%s", outbuf.buf);
if (outbuf.len) {
strbuf_addch(&outbuf, '\n');
xwrite(2, outbuf.buf, outbuf.len);
diff --git a/sideband.h b/sideband.h
index 7244971231..f75c4fde2a 100644
--- a/sideband.h
+++ b/sideband.h
@@ -15,7 +15,7 @@
* also prints a message (or the formatted contents of the notice in the case
* of SIDEBAND_PROGRESS) to stderr.
*/
-int demultiplex_sideband(const char *me, char *buf, int len);
+int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error);
void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
diff --git a/upload-pack.c b/upload-pack.c
index 60a26bbbfd..765b7695d2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -71,6 +71,8 @@ static int allow_filter;
static int allow_ref_in_want;
static struct list_objects_filter_options filter_options;
+static int allow_sideband_all;
+
static void reset_timeout(void)
{
alarm(timeout);
@@ -1046,6 +1048,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
allow_filter = git_config_bool(var, value);
} else if (!strcmp("uploadpack.allowrefinwant", var)) {
allow_ref_in_want = git_config_bool(var, value);
+ } else if (!strcmp("uploadpack.allowsidebandall", var)) {
+ allow_sideband_all = git_config_bool(var, value);
}
if (current_config_scope() != CONFIG_SCOPE_REPO) {
@@ -1284,6 +1288,11 @@ static void process_args(struct packet_reader *request,
continue;
}
+ if (allow_sideband_all && !strcmp(arg, "sideband-all")) {
+ data->writer.use_sideband = 1;
+ continue;
+ }
+
/* ignore unknown lines maybe? */
die("unexpected line: '%s'", arg);
}
@@ -1496,6 +1505,7 @@ int upload_pack_advertise(struct repository *r,
if (value) {
int allow_filter_value;
int allow_ref_in_want;
+ int allow_sideband_all_value;
strbuf_addstr(value, "shallow");
@@ -1510,6 +1520,12 @@ int upload_pack_advertise(struct repository *r,
&allow_ref_in_want) &&
allow_ref_in_want)
strbuf_addstr(value, " ref-in-want");
+
+ if (!repo_config_get_bool(the_repository,
+ "uploadpack.allowsidebandall",
+ &allow_sideband_all_value) &&
+ allow_sideband_all_value)
+ strbuf_addstr(value, " sideband-all");
}
return 1;
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v2 4/4] tests: define GIT_TEST_SIDEBAND_ALL
2019-01-15 19:40 ` [PATCH v2 0/4] Sideband the whole fetch v2 response Jonathan Tan
` (2 preceding siblings ...)
2019-01-15 19:40 ` [PATCH v2 3/4] {fetch,upload}-pack: sideband v2 fetch response Jonathan Tan
@ 2019-01-15 19:40 ` Jonathan Tan
2019-01-15 19:50 ` [PATCH v2 0/4] Sideband the whole fetch v2 response Junio C Hamano
2019-01-15 21:11 ` Junio C Hamano
5 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-15 19:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
Define a GIT_TEST_SIDEBAND_ALL environment variable meant to be used
from tests. When set to true, this overrides uploadpack.allowsidebandall
to true, allowing the entire test suite to be run as if this
configuration is in place for all repositories.
As of this patch, all tests pass whether GIT_TEST_SIDEBAND_ALL is unset
or set to 1.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
fetch-pack.c | 3 ++-
t/README | 5 +++++
t/lib-httpd/apache.conf | 1 +
t/t5537-fetch-shallow.sh | 3 ++-
t/t5701-git-serve.sh | 2 +-
t/t5702-protocol-v2.sh | 4 ++--
upload-pack.c | 13 ++++++++-----
7 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index b89199891d..4618568fee 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1362,7 +1362,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
packet_reader_init(&reader, fd[0], NULL, 0,
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_DIE_ON_ERR_PACKET);
- if (server_supports_feature("fetch", "sideband-all", 0)) {
+ if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 1) &&
+ server_supports_feature("fetch", "sideband-all", 0)) {
reader.use_sideband = 1;
reader.me = "fetch-pack";
}
diff --git a/t/README b/t/README
index 28711cc508..b275c883b8 100644
--- a/t/README
+++ b/t/README
@@ -358,6 +358,11 @@ GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-
index to be written after every 'git repack' command, and overrides the
'core.multiPackIndex' setting to true.
+GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
+'uploadpack.allowSidebandAll' setting to true, and when false, forces
+fetch-pack to not request sideband-all (even if the server advertises
+sideband-all).
+
Naming Tests
------------
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..9a6d368247 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -78,6 +78,7 @@ PassEnv GNUPGHOME
PassEnv ASAN_OPTIONS
PassEnv GIT_TRACE
PassEnv GIT_CONFIG_NOSYSTEM
+PassEnv GIT_TEST_SIDEBAND_ALL
SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 6faf17e17a..6caf628efa 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -243,7 +243,8 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f
"$(git -C "$REPO" rev-parse HEAD)" \
"$(git -C "$REPO" rev-parse HEAD^)" \
>"$HTTPD_ROOT_PATH/one-time-sed" &&
- test_must_fail git -C client fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
+ test_must_fail env GIT_TEST_SIDEBAND_ALL=0 git -C client \
+ fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
master:a_branch &&
# Ensure that the one-time-sed script was used.
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index ae79c6bbc0..fe45bf828d 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -14,7 +14,7 @@ test_expect_success 'test capability advertisement' '
0000
EOF
- git serve --advertise-capabilities >out &&
+ GIT_TEST_SIDEBAND_ALL=0 git serve --advertise-capabilities >out &&
test-tool pkt-line unpack <out >actual &&
test_cmp expect actual
'
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..b491c62e3e 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -583,8 +583,8 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" git -C http_child \
-c protocol.version=2 \
fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err &&
- grep "fetch< acknowledgments" log &&
- ! grep "fetch< ready" log &&
+ grep "fetch< .*acknowledgments" log &&
+ ! grep "fetch< .*ready" log &&
test_i18ngrep "expected no other sections to be sent after no .ready." err
'
diff --git a/upload-pack.c b/upload-pack.c
index 765b7695d2..0c1feccaab 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1288,7 +1288,9 @@ static void process_args(struct packet_reader *request,
continue;
}
- if (allow_sideband_all && !strcmp(arg, "sideband-all")) {
+ if ((git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
+ allow_sideband_all) &&
+ !strcmp(arg, "sideband-all")) {
data->writer.use_sideband = 1;
continue;
}
@@ -1521,10 +1523,11 @@ int upload_pack_advertise(struct repository *r,
allow_ref_in_want)
strbuf_addstr(value, " ref-in-want");
- if (!repo_config_get_bool(the_repository,
- "uploadpack.allowsidebandall",
- &allow_sideband_all_value) &&
- allow_sideband_all_value)
+ if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
+ (!repo_config_get_bool(the_repository,
+ "uploadpack.allowsidebandall",
+ &allow_sideband_all_value) &&
+ allow_sideband_all_value))
strbuf_addstr(value, " sideband-all");
}
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v2 0/4] Sideband the whole fetch v2 response
2019-01-15 19:40 ` [PATCH v2 0/4] Sideband the whole fetch v2 response Jonathan Tan
` (3 preceding siblings ...)
2019-01-15 19:40 ` [PATCH v2 4/4] tests: define GIT_TEST_SIDEBAND_ALL Jonathan Tan
@ 2019-01-15 19:50 ` Junio C Hamano
2019-01-15 21:11 ` Junio C Hamano
5 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2019-01-15 19:50 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
Jonathan Tan <jonathantanmy@google.com> writes:
> @@ -474,6 +474,7 @@ void packet_reader_init(struct packet_reader *reader, int fd,
> reader->buffer = packet_buffer;
> reader->buffer_size = sizeof(packet_buffer);
> reader->options = options;
> + reader->me = "git";
> }
This was somewhat unexpected. I would have thought that an
interdiff would be more like
+ reader.me = "fetch-pack";
if (using sideband-all) {
reader.use_sideband = 1;
- reader.me = "fetch-pack";
}
> + case SIDEBAND_PRIMARY:
> + if (reader->pktlen != 1)
> + goto nonprogress_received;
> + /*
> + * Since the packet contains nothing but the sideband
> + * designator, this is a keepalive packet. Wait for the
> + * next one.
> + */
> + break;
> + default: /* SIDEBAND_PROGRESS */
> + ;
OK.
Will replace. Thanks.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 0/4] Sideband the whole fetch v2 response
2019-01-15 19:40 ` [PATCH v2 0/4] Sideband the whole fetch v2 response Jonathan Tan
` (4 preceding siblings ...)
2019-01-15 19:50 ` [PATCH v2 0/4] Sideband the whole fetch v2 response Junio C Hamano
@ 2019-01-15 21:11 ` Junio C Hamano
2019-01-15 22:08 ` Jonathan Tan
5 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2019-01-15 21:11 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
Jonathan Tan <jonathantanmy@google.com> writes:
> Like v1, this is on origin/ms/packet-err-check.
By the way, when merged to 'pu' as one of the earlier topic, t5409
starts to fail under --stress.
$ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'
$ make
$ cd t && sh ./t5409-col*.sh --stress
This is not new to this round; v1 exhibited the same symptom.
Thanks.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 0/4] Sideband the whole fetch v2 response
2019-01-15 21:11 ` Junio C Hamano
@ 2019-01-15 22:08 ` Jonathan Tan
2019-01-15 22:35 ` Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Jonathan Tan @ 2019-01-15 22:08 UTC (permalink / raw)
To: gitster; +Cc: jonathantanmy, git
> Jonathan Tan <jonathantanmy@google.com> writes:
>
> > Like v1, this is on origin/ms/packet-err-check.
>
> By the way, when merged to 'pu' as one of the earlier topic, t5409
> starts to fail under --stress.
>
> $ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'
> $ make
> $ cd t && sh ./t5409-col*.sh --stress
>
> This is not new to this round; v1 exhibited the same symptom.
>
> Thanks.
Thanks for checking. I don't think this branch is the cause of this
issue, though. I ran the same stress test on both:
- f3035d003e ("Merge branch 'sg/stress-test' into jch", 2019-01-14) and
- the result of merging sg/stress-test into master,
and the test fails with the same result.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 0/4] Sideband the whole fetch v2 response
2019-01-15 22:08 ` Jonathan Tan
@ 2019-01-15 22:35 ` Junio C Hamano
2019-01-15 23:02 ` Jonathan Tan
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2019-01-15 22:35 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
Jonathan Tan <jonathantanmy@google.com> writes:
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>> > Like v1, this is on origin/ms/packet-err-check.
>>
>> By the way, when merged to 'pu' as one of the earlier topic, t5409
>> starts to fail under --stress.
>>
>> $ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'
>> $ make
>> $ cd t && sh ./t5409-col*.sh --stress
>>
>> This is not new to this round; v1 exhibited the same symptom.
>>
>> Thanks.
>
> Thanks for checking. I don't think this branch is the cause of this
> issue, though. I ran the same stress test on both:
>
> - f3035d003e ("Merge branch 'sg/stress-test' into jch", 2019-01-14) and
> - the result of merging sg/stress-test into master,
>
> and the test fails with the same result.
Interesting. That is not what I am seeing (as I manually bisected
the first-parent chain between f3035d003e and the tip of pu).
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 0/4] Sideband the whole fetch v2 response
2019-01-15 22:35 ` Junio C Hamano
@ 2019-01-15 23:02 ` Jonathan Tan
2019-01-15 23:13 ` Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Jonathan Tan @ 2019-01-15 23:02 UTC (permalink / raw)
To: gitster; +Cc: jonathantanmy, git
> Jonathan Tan <jonathantanmy@google.com> writes:
>
> >> Jonathan Tan <jonathantanmy@google.com> writes:
> >>
> >> > Like v1, this is on origin/ms/packet-err-check.
> >>
> >> By the way, when merged to 'pu' as one of the earlier topic, t5409
> >> starts to fail under --stress.
> >>
> >> $ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'
> >> $ make
> >> $ cd t && sh ./t5409-col*.sh --stress
> >>
> >> This is not new to this round; v1 exhibited the same symptom.
> >>
> >> Thanks.
> >
> > Thanks for checking. I don't think this branch is the cause of this
> > issue, though. I ran the same stress test on both:
> >
> > - f3035d003e ("Merge branch 'sg/stress-test' into jch", 2019-01-14) and
> > - the result of merging sg/stress-test into master,
> >
> > and the test fails with the same result.
>
> Interesting. That is not what I am seeing (as I manually bisected
> the first-parent chain between f3035d003e and the tip of pu).
Ah...yes, you're right. I forgot to build before running the tests. I'll
take a look.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 0/4] Sideband the whole fetch v2 response
2019-01-15 23:02 ` Jonathan Tan
@ 2019-01-15 23:13 ` Junio C Hamano
2019-01-16 0:38 ` Jonathan Tan
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2019-01-15 23:13 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
Jonathan Tan <jonathantanmy@google.com> writes:
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>> >> Jonathan Tan <jonathantanmy@google.com> writes:
>> >>
>> >> > Like v1, this is on origin/ms/packet-err-check.
>> >>
>> >> By the way, when merged to 'pu' as one of the earlier topic, t5409
>> >> starts to fail under --stress.
>> >>
>> >> $ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'
>> >> $ make
>> >> $ cd t && sh ./t5409-col*.sh --stress
>> >>
>> >> This is not new to this round; v1 exhibited the same symptom.
>> >>
>> >> Thanks.
>> >
>> > Thanks for checking. I don't think this branch is the cause of this
>> > issue, though. I ran the same stress test on both:
>> >
>> > - f3035d003e ("Merge branch 'sg/stress-test' into jch", 2019-01-14) and
>> > - the result of merging sg/stress-test into master,
>> >
>> > and the test fails with the same result.
>>
>> Interesting. That is not what I am seeing (as I manually bisected
>> the first-parent chain between f3035d003e and the tip of pu).
>
> Ah...yes, you're right. I forgot to build before running the tests. I'll
> take a look.
Thanks.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 0/4] Sideband the whole fetch v2 response
2019-01-15 23:13 ` Junio C Hamano
@ 2019-01-16 0:38 ` Jonathan Tan
2019-01-16 4:12 ` Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Jonathan Tan @ 2019-01-16 0:38 UTC (permalink / raw)
To: gitster; +Cc: jonathantanmy, git
> > Ah...yes, you're right. I forgot to build before running the tests. I'll
> > take a look.
>
> Thanks.
Thanks once again for taking a look. It turns out that it's because
progress messages are sometimes split across PKT-LINEs depending on your
luck, and we need to retain the "leftover" on a \2 sideband in order to
combine it with the next one if necessary. So, for example, the
following fixup works:
diff --git a/sideband.c b/sideband.c
index c185c38637..d5da587d68 100644
--- a/sideband.c
+++ b/sideband.c
@@ -117,7 +117,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
{
static const char *suffix;
- struct strbuf outbuf = STRBUF_INIT;
+ static struct strbuf outbuf = STRBUF_INIT;
int retval = 0;
const char *b, *brk;
int band;
@@ -187,8 +187,7 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
"" : DISPLAY_PREFIX);
maybe_colorize_sideband(&outbuf, b, strlen(b));
}
- retval = SIDEBAND_PROGRESS;
- break;
+ return SIDEBAND_PROGRESS; /* skip cleanup */
case 1:
retval = SIDEBAND_PRIMARY;
break;
We could make the caller of demultiplex_sideband() store the outbuf, but
at this point, it might be better to refactor packet_reader into its own
file and have it depend on both pkt-line.h and sideband.h. If you (or
anyone else) have any ideas, let me know what you think. I'll think
further about this too.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 0/4] Sideband the whole fetch v2 response
2019-01-16 0:38 ` Jonathan Tan
@ 2019-01-16 4:12 ` Junio C Hamano
0 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2019-01-16 4:12 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
Jonathan Tan <jonathantanmy@google.com> writes:
> We could make the caller of demultiplex_sideband() store the outbuf, but
> at this point, it might be better to refactor packet_reader into its own
> file and have it depend on both pkt-line.h and sideband.h.
I do not know or too deeply care about the pkt-line / sideband
distinction, as they are logically one thing. With a packet-reader
abstraction in place, doesn't this "here is the remainder of the
packet from the previous round" belong to each reader instance?
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 2/4] sideband: reverse its dependency on pkt-line
2019-01-15 19:40 ` [PATCH v2 2/4] sideband: reverse its dependency on pkt-line Jonathan Tan
@ 2019-01-16 10:34 ` SZEDER Gábor
2019-01-16 17:43 ` Jonathan Tan
0 siblings, 1 reply; 41+ messages in thread
From: SZEDER Gábor @ 2019-01-16 10:34 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
On Tue, Jan 15, 2019 at 11:40:28AM -0800, Jonathan Tan wrote:
> A subsequent patch will teach struct packet_reader a new field that, if
> set, instructs it to interpret read data as multiplexed. This will
> create a dependency from pkt-line to sideband.
>
> To avoid a circular dependency, split recv_sideband() into 2 parts: the
> reading loop (left in recv_sideband()) and the processing of the
> contents (in demultiplex_sideband()), and move the former into pkt-line.
> This reverses the direction of dependency: sideband no longer depends on
> pkt-line, and pkt-line now depends on sideband.
In the last couple of days I noticed occasional but frequent failures
in the test 'leading space' in 't5409-colorize-remote-messas.sh' on
'pu' and on this topic. Bisect suggests that this patch is the
culprit, but of course bisecting an occasional test failure can't be
completely trusted.
The trace output of the failing test looks like this:
+ git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space
+ cat output
remote: error: error
remote: ERROR: also highlighted
remote: hint: hint
remote: hinting: not highlighted
remote: success: success
remote: warning: warning
remote: prefixerror: error
remote:
remote: error: leading space
remote:
remote: Err
To /<...>/trash directory.t5409-colorize-remote-messages.stress-4/.
* [new branch] HEAD -> leading-space
+ test_decode_color
+ awk
< ... snip enormous awk script ... >
+ grep <BOLD;RED>error<RESET>: leading space decoded
error: last command exited with $?=1
not ok 6 - leading space
Notice how that "error: leading space" lines up with the other
messages. On 'master' that line looks like this:
remote: error: leading space
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 2/4] sideband: reverse its dependency on pkt-line
2019-01-16 10:34 ` SZEDER Gábor
@ 2019-01-16 17:43 ` Jonathan Tan
2019-01-16 19:17 ` SZEDER Gábor
0 siblings, 1 reply; 41+ messages in thread
From: Jonathan Tan @ 2019-01-16 17:43 UTC (permalink / raw)
To: szeder.dev; +Cc: jonathantanmy, git
> On Tue, Jan 15, 2019 at 11:40:28AM -0800, Jonathan Tan wrote:
> > A subsequent patch will teach struct packet_reader a new field that, if
> > set, instructs it to interpret read data as multiplexed. This will
> > create a dependency from pkt-line to sideband.
> >
> > To avoid a circular dependency, split recv_sideband() into 2 parts: the
> > reading loop (left in recv_sideband()) and the processing of the
> > contents (in demultiplex_sideband()), and move the former into pkt-line.
> > This reverses the direction of dependency: sideband no longer depends on
> > pkt-line, and pkt-line now depends on sideband.
>
> In the last couple of days I noticed occasional but frequent failures
> in the test 'leading space' in 't5409-colorize-remote-messas.sh' on
> 'pu' and on this topic. Bisect suggests that this patch is the
> culprit, but of course bisecting an occasional test failure can't be
> completely trusted.
>
> The trace output of the failing test looks like this:
Thanks for checking. Junio noticed a similar thing and I have replied to
it here:
https://public-inbox.org/git/20190116003828.64889-1-jonathantanmy@google.com/
I'll try to ensure that this issue is fixed in the subsequent version.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 2/4] sideband: reverse its dependency on pkt-line
2019-01-16 17:43 ` Jonathan Tan
@ 2019-01-16 19:17 ` SZEDER Gábor
0 siblings, 0 replies; 41+ messages in thread
From: SZEDER Gábor @ 2019-01-16 19:17 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
On Wed, Jan 16, 2019 at 09:43:19AM -0800, Jonathan Tan wrote:
> > On Tue, Jan 15, 2019 at 11:40:28AM -0800, Jonathan Tan wrote:
> > In the last couple of days I noticed occasional but frequent failures
> > in the test 'leading space' in 't5409-colorize-remote-messas.sh' on
> Thanks for checking. Junio noticed a similar thing and I have replied to
> it here:
>
> https://public-inbox.org/git/20190116003828.64889-1-jonathantanmy@google.com/
>
> I'll try to ensure that this issue is fixed in the subsequent version.
Oh, indeed, that must be the same issue. After I bisected the issue I
only paid attention to the discussion of patch 2/4 in v1, and
completely ignored the replies to the cover letter.
But I'm glad that --stress makes itself useful :)
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v3 0/4] Sideband the whole fetch v2 response
2019-01-11 22:18 [PATCH 0/4] Sideband the whole fetch v2 response Jonathan Tan
` (8 preceding siblings ...)
2019-01-15 19:40 ` [PATCH v2 0/4] Sideband the whole fetch v2 response Jonathan Tan
@ 2019-01-16 19:28 ` Jonathan Tan
2019-01-16 19:28 ` [PATCH v3 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
` (4 more replies)
9 siblings, 5 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-16 19:28 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, szeder.dev
Like previous versions, this is on origin/ms/packet-err-check.
First of all, thanks to those who noticed the issue with t5409. I have
merged sg/stress-test and ran:
make DEVELOPER=1 && (cd t && sh ./t5409-col*.sh --stress)
with no issues.
demultiplex_sideband() now requires its caller to maintain a strbuf,
since progress messages can be split across packets. I have also changed
the API of demultiplex_sideband() to hopefully make things clearer.
As for keepalive packets, I think it's best if we state that the remote
should use 0005\2 instead of 0005\1, and have made the change
accordingly. This does not need to be treated specially by the client.
(I shouldn't have a problem updating my CDN offloading patches [1] to
use 0005\2 before the packfile starts and 0005\1 after the packfile
starts.)
[1] Patches 5-8 of https://public-inbox.org/git/cover.1547244620.git.jonathantanmy@google.com/
Jonathan Tan (4):
pkt-line: introduce struct packet_writer
sideband: reverse its dependency on pkt-line
{fetch,upload}-pack: sideband v2 fetch response
tests: define GIT_TEST_SIDEBAND_ALL
Documentation/technical/protocol-v2.txt | 10 ++
fetch-pack.c | 13 +-
pkt-line.c | 107 ++++++++++++--
pkt-line.h | 34 +++++
sideband.c | 178 ++++++++++++------------
sideband.h | 25 +++-
t/README | 5 +
t/lib-httpd/apache.conf | 1 +
t/t5537-fetch-shallow.sh | 3 +-
t/t5701-git-serve.sh | 2 +-
t/t5702-protocol-v2.sh | 4 +-
upload-pack.c | 131 ++++++++++-------
12 files changed, 346 insertions(+), 167 deletions(-)
Interdiff against v2:
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 1b0633f59f..39b40c0dc1 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -314,8 +314,8 @@ included in the client's request:
Instruct the server to send the whole response multiplexed, not just
the packfile section. All non-flush and non-delim PKT-LINE in the
response (not only in the packfile section) will then start with a byte
- indicating its sideband (1, 2, or 3), and the server may send "0005\1"
- (a PKT-LINE of sideband 1 with no payload) as a keepalive packet.
+ indicating its sideband (1, 2, or 3), and the server may send "0005\2"
+ (a PKT-LINE of sideband 2 with no payload) as a keepalive packet.
The response of `fetch` is broken into a number of sections separated by
delimiter packets (0001), with each section beginning with its section
diff --git a/pkt-line.c b/pkt-line.c
index 69162f3990..d4b71d3e82 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -442,21 +442,22 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
int recv_sideband(const char *me, int in_stream, int out)
{
char buf[LARGE_PACKET_MAX + 1];
- int retval = 0;
int len;
+ struct strbuf scratch = STRBUF_INIT;
+ enum sideband_type sideband_type;
while (1) {
- len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
- retval = demultiplex_sideband(me, buf, len, 0);
- switch (retval) {
+ len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX,
+ 0);
+ if (!demultiplex_sideband(me, buf, len, 0, &scratch,
+ &sideband_type))
+ continue;
+ switch (sideband_type) {
case SIDEBAND_PRIMARY:
write_or_die(out, buf + 1, len - 1);
break;
- case SIDEBAND_PROGRESS:
- /* already written by demultiplex_sideband() */
- break;
default: /* errors: message already written */
- return retval;
+ return sideband_type;
}
}
}
@@ -479,17 +480,19 @@ void packet_reader_init(struct packet_reader *reader, int fd,
enum packet_read_status packet_reader_read(struct packet_reader *reader)
{
+ struct strbuf scratch = STRBUF_INIT;
+
if (reader->line_peeked) {
reader->line_peeked = 0;
return reader->status;
}
/*
- * Consume all progress and keepalive packets until a primary payload
- * packet is received
+ * Consume all progress packets until a primary payload packet is
+ * received
*/
while (1) {
- int retval;
+ enum sideband_type sideband_type;
reader->status = packet_read_with_status(reader->fd,
&reader->src_buffer,
&reader->src_len,
@@ -499,29 +502,12 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
reader->options);
if (!reader->use_sideband)
break;
- retval = demultiplex_sideband(reader->me, reader->buffer,
- reader->pktlen, 1);
- switch (retval) {
- case SIDEBAND_PROTOCOL_ERROR:
- case SIDEBAND_REMOTE_ERROR:
- BUG("should have died in diagnose_sideband");
- case SIDEBAND_FLUSH:
- goto nonprogress_received;
- case SIDEBAND_PRIMARY:
- if (reader->pktlen != 1)
- goto nonprogress_received;
- /*
- * Since the packet contains nothing but the sideband
- * designator, this is a keepalive packet. Wait for the
- * next one.
- */
+ if (demultiplex_sideband(reader->me, reader->buffer,
+ reader->pktlen, 1, &scratch,
+ &sideband_type))
break;
- default: /* SIDEBAND_PROGRESS */
- ;
- }
}
-nonprogress_received:
if (reader->status == PACKET_READ_NORMAL)
/* Skip the sideband designator if sideband is used */
reader->line = reader->use_sideband ?
diff --git a/sideband.c b/sideband.c
index 9d3051e3fe..6a16feb262 100644
--- a/sideband.c
+++ b/sideband.c
@@ -113,11 +113,12 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
#define ANSI_SUFFIX "\033[K"
#define DUMB_SUFFIX " "
-int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
+int demultiplex_sideband(const char *me, char *buf, int len,
+ int die_on_error,
+ struct strbuf *scratch,
+ enum sideband_type *sideband_type)
{
static const char *suffix;
- struct strbuf outbuf = STRBUF_INIT;
- int retval = 0;
const char *b, *brk;
int band;
@@ -129,14 +130,14 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
}
if (len == 0) {
- retval = SIDEBAND_FLUSH;
+ *sideband_type = SIDEBAND_FLUSH;
goto cleanup;
}
if (len < 1) {
- strbuf_addf(&outbuf,
+ strbuf_addf(scratch,
"%s%s: protocol error: no band designator",
- outbuf.len ? "\n" : "", me);
- retval = SIDEBAND_PROTOCOL_ERROR;
+ scratch->len ? "\n" : "", me);
+ *sideband_type = SIDEBAND_PROTOCOL_ERROR;
goto cleanup;
}
band = buf[0] & 0xff;
@@ -146,12 +147,11 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
case 3:
if (die_on_error)
die("remote error: %s", buf + 1);
-
- strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
+ strbuf_addf(scratch, "%s%s", scratch->len ? "\n" : "",
DISPLAY_PREFIX);
- maybe_colorize_sideband(&outbuf, buf + 1, len);
+ maybe_colorize_sideband(scratch, buf + 1, len);
- retval = SIDEBAND_REMOTE_ERROR;
+ *sideband_type = SIDEBAND_REMOTE_ERROR;
break;
case 2:
b = buf + 1;
@@ -167,46 +167,45 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
while ((brk = strpbrk(b, "\n\r"))) {
int linelen = brk - b;
- if (!outbuf.len)
- strbuf_addstr(&outbuf, DISPLAY_PREFIX);
+ if (!scratch->len)
+ strbuf_addstr(scratch, DISPLAY_PREFIX);
if (linelen > 0) {
- maybe_colorize_sideband(&outbuf, b, linelen);
- strbuf_addstr(&outbuf, suffix);
+ maybe_colorize_sideband(scratch, b, linelen);
+ strbuf_addstr(scratch, suffix);
}
- strbuf_addch(&outbuf, *brk);
- xwrite(2, outbuf.buf, outbuf.len);
- strbuf_reset(&outbuf);
+ strbuf_addch(scratch, *brk);
+ xwrite(2, scratch->buf, scratch->len);
+ strbuf_reset(scratch);
b = brk + 1;
}
if (*b) {
- strbuf_addstr(&outbuf, outbuf.len ?
+ strbuf_addstr(scratch, scratch->len ?
"" : DISPLAY_PREFIX);
- maybe_colorize_sideband(&outbuf, b, strlen(b));
+ maybe_colorize_sideband(scratch, b, strlen(b));
}
- retval = SIDEBAND_PROGRESS;
- break;
+ return 0;
case 1:
- retval = SIDEBAND_PRIMARY;
+ *sideband_type = SIDEBAND_PRIMARY;
break;
default:
- strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
- outbuf.len ? "\n" : "", me, band);
- retval = SIDEBAND_PROTOCOL_ERROR;
+ strbuf_addf(scratch, "%s%s: protocol error: bad band #%d",
+ scratch->len ? "\n" : "", me, band);
+ *sideband_type = SIDEBAND_PROTOCOL_ERROR;
break;
}
cleanup:
- if (die_on_error && retval == SIDEBAND_PROTOCOL_ERROR)
- die("%s", outbuf.buf);
- if (outbuf.len) {
- strbuf_addch(&outbuf, '\n');
- xwrite(2, outbuf.buf, outbuf.len);
+ if (die_on_error && *sideband_type == SIDEBAND_PROTOCOL_ERROR)
+ die("%s", scratch->buf);
+ if (scratch->len) {
+ strbuf_addch(scratch, '\n');
+ xwrite(2, scratch->buf, scratch->len);
}
- strbuf_release(&outbuf);
- return retval;
+ strbuf_release(scratch);
+ return 1;
}
/*
diff --git a/sideband.h b/sideband.h
index f75c4fde2a..227740a58e 100644
--- a/sideband.h
+++ b/sideband.h
@@ -1,21 +1,28 @@
#ifndef SIDEBAND_H
#define SIDEBAND_H
-#define SIDEBAND_PROTOCOL_ERROR -2
-#define SIDEBAND_REMOTE_ERROR -1
-#define SIDEBAND_FLUSH 0
-#define SIDEBAND_PRIMARY 1
-#define SIDEBAND_PROGRESS 2
+enum sideband_type {
+ SIDEBAND_PROTOCOL_ERROR = -2,
+ SIDEBAND_REMOTE_ERROR = -1,
+ SIDEBAND_FLUSH = 0,
+ SIDEBAND_PRIMARY = 1
+};
/*
- * Inspects a multiplexed packet read from the remote and returns which
- * sideband it is for.
+ * Inspects a multiplexed packet read from the remote. If this packet is a
+ * progress packet and thus should not be processed by the caller, returns 0.
+ * Otherwise, returns 1, releases scratch, and sets sideband_type.
*
- * If SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS,
- * also prints a message (or the formatted contents of the notice in the case
- * of SIDEBAND_PROGRESS) to stderr.
+ * If this packet is SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or a
+ * progress packet, also prints a message to stderr.
+ *
+ * scratch must be a struct strbuf allocated by the caller. It is used to store
+ * progress messages split across multiple packets.
*/
-int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error);
+int demultiplex_sideband(const char *me, char *buf, int len,
+ int die_on_error,
+ struct strbuf *scratch,
+ enum sideband_type *sideband_type);
void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 1/4] pkt-line: introduce struct packet_writer
2019-01-16 19:28 ` [PATCH v3 " Jonathan Tan
@ 2019-01-16 19:28 ` Jonathan Tan
2019-01-16 19:28 ` [PATCH v3 2/4] sideband: reverse its dependency on pkt-line Jonathan Tan
` (3 subsequent siblings)
4 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-16 19:28 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, szeder.dev
A future patch will allow the client to request multiplexing of the
entire fetch response (and not only during packfile transmission), which
in turn allows the server to send progress and keepalive messages at any
time during the response.
It will be convenient for a future patch if writing options
(specifically, whether the written data is to be multiplexed) could be
controlled from a single place, so create struct packet_writer to serve
as that place, and modify upload-pack to use it.
Currently, it only stores the output fd, but a subsequent patch will (as
described above) introduce an option to determine if the written data is
to be multiplexed.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
pkt-line.c | 47 ++++++++++++++++++---
pkt-line.h | 14 +++++++
upload-pack.c | 112 +++++++++++++++++++++++++++-----------------------
3 files changed, 115 insertions(+), 58 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index e70ea6d88f..9d3e402d41 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -129,12 +129,14 @@ static void set_packet_header(char *buf, const int size)
#undef hex
}
-static void format_packet(struct strbuf *out, const char *fmt, va_list args)
+static void format_packet(struct strbuf *out, const char *prefix,
+ const char *fmt, va_list args)
{
size_t orig_len, n;
orig_len = out->len;
strbuf_addstr(out, "0000");
+ strbuf_addstr(out, prefix);
strbuf_vaddf(out, fmt, args);
n = out->len - orig_len;
@@ -145,13 +147,13 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)
packet_trace(out->buf + orig_len + 4, n - 4, 1);
}
-static int packet_write_fmt_1(int fd, int gently,
+static int packet_write_fmt_1(int fd, int gently, const char *prefix,
const char *fmt, va_list args)
{
static struct strbuf buf = STRBUF_INIT;
strbuf_reset(&buf);
- format_packet(&buf, fmt, args);
+ format_packet(&buf, prefix, fmt, args);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
if (!gently) {
check_pipe(errno);
@@ -168,7 +170,7 @@ void packet_write_fmt(int fd, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- packet_write_fmt_1(fd, 0, fmt, args);
+ packet_write_fmt_1(fd, 0, "", fmt, args);
va_end(args);
}
@@ -178,7 +180,7 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- status = packet_write_fmt_1(fd, 1, fmt, args);
+ status = packet_write_fmt_1(fd, 1, "", fmt, args);
va_end(args);
return status;
}
@@ -211,7 +213,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- format_packet(buf, fmt, args);
+ format_packet(buf, "", fmt, args);
va_end(args);
}
@@ -486,3 +488,36 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)
reader->line_peeked = 1;
return reader->status;
}
+
+void packet_writer_init(struct packet_writer *writer, int dest_fd)
+{
+ writer->dest_fd = dest_fd;
+}
+
+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
+{
+ va_list args;
+
+ va_start(args, fmt);
+ packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
+ va_end(args);
+}
+
+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
+{
+ va_list args;
+
+ va_start(args, fmt);
+ packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
+ va_end(args);
+}
+
+void packet_writer_delim(struct packet_writer *writer)
+{
+ packet_delim(writer->dest_fd);
+}
+
+void packet_writer_flush(struct packet_writer *writer)
+{
+ packet_flush(writer->dest_fd);
+}
diff --git a/pkt-line.h b/pkt-line.h
index d7e1dbc047..023ad2951d 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -183,4 +183,18 @@ extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);
#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
extern char packet_buffer[LARGE_PACKET_MAX];
+struct packet_writer {
+ int dest_fd;
+};
+
+void packet_writer_init(struct packet_writer *writer, int dest_fd);
+
+/* These functions die upon failure. */
+__attribute__((format (printf, 2, 3)))
+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...);
+__attribute__((format (printf, 2, 3)))
+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...);
+void packet_writer_delim(struct packet_writer *writer);
+void packet_writer_flush(struct packet_writer *writer);
+
#endif
diff --git a/upload-pack.c b/upload-pack.c
index 08b547cf01..60a26bbbfd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -613,13 +613,14 @@ static void check_non_tip(struct object_array *want_obj)
}
}
-static void send_shallow(struct commit_list *result)
+static void send_shallow(struct packet_writer *writer,
+ struct commit_list *result)
{
while (result) {
struct object *object = &result->item->object;
if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
- packet_write_fmt(1, "shallow %s",
- oid_to_hex(&object->oid));
+ packet_writer_write(writer, "shallow %s",
+ oid_to_hex(&object->oid));
register_shallow(the_repository, &object->oid);
shallow_nr++;
}
@@ -627,7 +628,8 @@ static void send_shallow(struct commit_list *result)
}
}
-static void send_unshallow(const struct object_array *shallows,
+static void send_unshallow(struct packet_writer *writer,
+ const struct object_array *shallows,
struct object_array *want_obj)
{
int i;
@@ -636,8 +638,8 @@ static void send_unshallow(const struct object_array *shallows,
struct object *object = shallows->objects[i].item;
if (object->flags & NOT_SHALLOW) {
struct commit_list *parents;
- packet_write_fmt(1, "unshallow %s",
- oid_to_hex(&object->oid));
+ packet_writer_write(writer, "unshallow %s",
+ oid_to_hex(&object->oid));
object->flags &= ~CLIENT_SHALLOW;
/*
* We want to _register_ "object" as shallow, but we
@@ -662,7 +664,7 @@ static void send_unshallow(const struct object_array *shallows,
}
}
-static void deepen(int depth, int deepen_relative,
+static void deepen(struct packet_writer *writer, int depth, int deepen_relative,
struct object_array *shallows, struct object_array *want_obj)
{
if (depth == INFINITE_DEPTH && !is_repository_shallow(the_repository)) {
@@ -680,7 +682,7 @@ static void deepen(int depth, int deepen_relative,
result = get_shallow_commits(&reachable_shallows,
depth + 1,
SHALLOW, NOT_SHALLOW);
- send_shallow(result);
+ send_shallow(writer, result);
free_commit_list(result);
object_array_clear(&reachable_shallows);
} else {
@@ -688,14 +690,15 @@ static void deepen(int depth, int deepen_relative,
result = get_shallow_commits(want_obj, depth,
SHALLOW, NOT_SHALLOW);
- send_shallow(result);
+ send_shallow(writer, result);
free_commit_list(result);
}
- send_unshallow(shallows, want_obj);
+ send_unshallow(writer, shallows, want_obj);
}
-static void deepen_by_rev_list(int ac, const char **av,
+static void deepen_by_rev_list(struct packet_writer *writer, int ac,
+ const char **av,
struct object_array *shallows,
struct object_array *want_obj)
{
@@ -703,13 +706,14 @@ static void deepen_by_rev_list(int ac, const char **av,
close_commit_graph(the_repository);
result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
- send_shallow(result);
+ send_shallow(writer, result);
free_commit_list(result);
- send_unshallow(shallows, want_obj);
+ send_unshallow(writer, shallows, want_obj);
}
/* Returns 1 if a shallow list is sent or 0 otherwise */
-static int send_shallow_list(int depth, int deepen_rev_list,
+static int send_shallow_list(struct packet_writer *writer,
+ int depth, int deepen_rev_list,
timestamp_t deepen_since,
struct string_list *deepen_not,
struct object_array *shallows,
@@ -720,7 +724,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,
if (depth > 0 && deepen_rev_list)
die("git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together");
if (depth > 0) {
- deepen(depth, deepen_relative, shallows, want_obj);
+ deepen(writer, depth, deepen_relative, shallows, want_obj);
ret = 1;
} else if (deepen_rev_list) {
struct argv_array av = ARGV_ARRAY_INIT;
@@ -741,7 +745,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,
struct object *o = want_obj->objects[i].item;
argv_array_push(&av, oid_to_hex(&o->oid));
}
- deepen_by_rev_list(av.argc, av.argv, shallows, want_obj);
+ deepen_by_rev_list(writer, av.argc, av.argv, shallows, want_obj);
argv_array_clear(&av);
ret = 1;
} else {
@@ -834,8 +838,10 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
int has_non_tip = 0;
timestamp_t deepen_since = 0;
int deepen_rev_list = 0;
+ struct packet_writer writer;
shallow_nr = 0;
+ packet_writer_init(&writer, 1);
for (;;) {
struct object *o;
const char *features;
@@ -892,9 +898,9 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
o = parse_object(the_repository, &oid_buf);
if (!o) {
- packet_write_fmt(1,
- "ERR upload-pack: not our ref %s",
- oid_to_hex(&oid_buf));
+ packet_writer_error(&writer,
+ "upload-pack: not our ref %s",
+ oid_to_hex(&oid_buf));
die("git upload-pack: not our ref %s",
oid_to_hex(&oid_buf));
}
@@ -923,7 +929,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
return;
- if (send_shallow_list(depth, deepen_rev_list, deepen_since,
+ if (send_shallow_list(&writer, depth, deepen_rev_list, deepen_since,
&deepen_not, &shallows, want_obj))
packet_flush(1);
object_array_clear(&shallows);
@@ -1102,6 +1108,8 @@ struct upload_pack_data {
int deepen_rev_list;
int deepen_relative;
+ struct packet_writer writer;
+
unsigned stateless_rpc : 1;
unsigned use_thin_pack : 1;
@@ -1125,6 +1133,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
data->haves = haves;
data->shallows = shallows;
data->deepen_not = deepen_not;
+ packet_writer_init(&data->writer, 1);
}
static void upload_pack_data_clear(struct upload_pack_data *data)
@@ -1136,7 +1145,8 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
string_list_clear(&data->deepen_not, 0);
}
-static int parse_want(const char *line, struct object_array *want_obj)
+static int parse_want(struct packet_writer *writer, const char *line,
+ struct object_array *want_obj)
{
const char *arg;
if (skip_prefix(line, "want ", &arg)) {
@@ -1149,9 +1159,9 @@ static int parse_want(const char *line, struct object_array *want_obj)
o = parse_object(the_repository, &oid);
if (!o) {
- packet_write_fmt(1,
- "ERR upload-pack: not our ref %s",
- oid_to_hex(&oid));
+ packet_writer_error(writer,
+ "upload-pack: not our ref %s",
+ oid_to_hex(&oid));
die("git upload-pack: not our ref %s",
oid_to_hex(&oid));
}
@@ -1167,7 +1177,8 @@ static int parse_want(const char *line, struct object_array *want_obj)
return 0;
}
-static int parse_want_ref(const char *line, struct string_list *wanted_refs,
+static int parse_want_ref(struct packet_writer *writer, const char *line,
+ struct string_list *wanted_refs,
struct object_array *want_obj)
{
const char *arg;
@@ -1177,7 +1188,7 @@ static int parse_want_ref(const char *line, struct string_list *wanted_refs,
struct object *o;
if (read_ref(arg, &oid)) {
- packet_write_fmt(1, "ERR unknown ref %s", arg);
+ packet_writer_error(writer, "unknown ref %s", arg);
die("unknown ref %s", arg);
}
@@ -1220,10 +1231,11 @@ static void process_args(struct packet_reader *request,
const char *p;
/* process want */
- if (parse_want(arg, want_obj))
+ if (parse_want(&data->writer, arg, want_obj))
continue;
if (allow_ref_in_want &&
- parse_want_ref(arg, &data->wanted_refs, want_obj))
+ parse_want_ref(&data->writer, arg, &data->wanted_refs,
+ want_obj))
continue;
/* process have line */
if (parse_have(arg, &data->haves))
@@ -1317,26 +1329,26 @@ static int process_haves(struct oid_array *haves, struct oid_array *common,
return 0;
}
-static int send_acks(struct oid_array *acks, struct strbuf *response,
+static int send_acks(struct packet_writer *writer, struct oid_array *acks,
const struct object_array *have_obj,
struct object_array *want_obj)
{
int i;
- packet_buf_write(response, "acknowledgments\n");
+ packet_writer_write(writer, "acknowledgments\n");
/* Send Acks */
if (!acks->nr)
- packet_buf_write(response, "NAK\n");
+ packet_writer_write(writer, "NAK\n");
for (i = 0; i < acks->nr; i++) {
- packet_buf_write(response, "ACK %s\n",
- oid_to_hex(&acks->oid[i]));
+ packet_writer_write(writer, "ACK %s\n",
+ oid_to_hex(&acks->oid[i]));
}
if (ok_to_give_up(have_obj, want_obj)) {
/* Send Ready */
- packet_buf_write(response, "ready\n");
+ packet_writer_write(writer, "ready\n");
return 1;
}
@@ -1348,25 +1360,20 @@ static int process_haves_and_send_acks(struct upload_pack_data *data,
struct object_array *want_obj)
{
struct oid_array common = OID_ARRAY_INIT;
- struct strbuf response = STRBUF_INIT;
int ret = 0;
process_haves(&data->haves, &common, have_obj);
if (data->done) {
ret = 1;
- } else if (send_acks(&common, &response, have_obj, want_obj)) {
- packet_buf_delim(&response);
+ } else if (send_acks(&data->writer, &common, have_obj, want_obj)) {
+ packet_writer_delim(&data->writer);
ret = 1;
} else {
/* Add Flush */
- packet_buf_flush(&response);
+ packet_writer_flush(&data->writer);
ret = 0;
}
- /* Send response */
- write_or_die(1, response.buf, response.len);
- strbuf_release(&response);
-
oid_array_clear(&data->haves);
oid_array_clear(&common);
return ret;
@@ -1379,15 +1386,15 @@ static void send_wanted_ref_info(struct upload_pack_data *data)
if (!data->wanted_refs.nr)
return;
- packet_write_fmt(1, "wanted-refs\n");
+ packet_writer_write(&data->writer, "wanted-refs\n");
for_each_string_list_item(item, &data->wanted_refs) {
- packet_write_fmt(1, "%s %s\n",
- oid_to_hex(item->util),
- item->string);
+ packet_writer_write(&data->writer, "%s %s\n",
+ oid_to_hex(item->util),
+ item->string);
}
- packet_delim(1);
+ packet_writer_delim(&data->writer);
}
static void send_shallow_info(struct upload_pack_data *data,
@@ -1398,14 +1405,15 @@ static void send_shallow_info(struct upload_pack_data *data,
!is_repository_shallow(the_repository))
return;
- packet_write_fmt(1, "shallow-info\n");
+ packet_writer_write(&data->writer, "shallow-info\n");
- if (!send_shallow_list(data->depth, data->deepen_rev_list,
+ if (!send_shallow_list(&data->writer, data->depth,
+ data->deepen_rev_list,
data->deepen_since, &data->deepen_not,
&data->shallows, want_obj) &&
is_repository_shallow(the_repository))
- deepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,
- want_obj);
+ deepen(&data->writer, INFINITE_DEPTH, data->deepen_relative,
+ &data->shallows, want_obj);
packet_delim(1);
}
@@ -1467,7 +1475,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
send_wanted_ref_info(&data);
send_shallow_info(&data, &want_obj);
- packet_write_fmt(1, "packfile\n");
+ packet_writer_write(&data.writer, "packfile\n");
create_pack_file(&have_obj, &want_obj);
state = FETCH_DONE;
break;
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 2/4] sideband: reverse its dependency on pkt-line
2019-01-16 19:28 ` [PATCH v3 " Jonathan Tan
2019-01-16 19:28 ` [PATCH v3 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
@ 2019-01-16 19:28 ` Jonathan Tan
2019-01-16 19:28 ` [PATCH v3 3/4] {fetch,upload}-pack: sideband v2 fetch response Jonathan Tan
` (2 subsequent siblings)
4 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-16 19:28 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, szeder.dev
A subsequent patch will teach struct packet_reader a new field that, if
set, instructs it to interpret read data as multiplexed. This will
create a dependency from pkt-line to sideband.
To avoid a circular dependency, split recv_sideband() into 2 parts: the
reading loop (left in recv_sideband()) and the processing of the
contents (in demultiplex_sideband()), and move the former into pkt-line.
This reverses the direction of dependency: sideband no longer depends on
pkt-line, and pkt-line now depends on sideband.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
pkt-line.c | 23 +++++++
pkt-line.h | 16 +++++
sideband.c | 173 +++++++++++++++++++++++++----------------------------
sideband.h | 24 +++++++-
4 files changed, 141 insertions(+), 95 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index 9d3e402d41..321ff632a5 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -439,6 +439,29 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
return sb_out->len - orig_len;
}
+int recv_sideband(const char *me, int in_stream, int out)
+{
+ char buf[LARGE_PACKET_MAX + 1];
+ int len;
+ struct strbuf scratch = STRBUF_INIT;
+ enum sideband_type sideband_type;
+
+ while (1) {
+ len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX,
+ 0);
+ if (!demultiplex_sideband(me, buf, len, &scratch,
+ &sideband_type))
+ continue;
+ switch (sideband_type) {
+ case SIDEBAND_PRIMARY:
+ write_or_die(out, buf + 1, len - 1);
+ break;
+ default: /* errors: message already written */
+ return sideband_type;
+ }
+ }
+}
+
/* Packet Reader Functions */
void packet_reader_init(struct packet_reader *reader, int fd,
char *src_buffer, size_t src_len,
diff --git a/pkt-line.h b/pkt-line.h
index 023ad2951d..a8e92a4b63 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -3,6 +3,7 @@
#include "git-compat-util.h"
#include "strbuf.h"
+#include "sideband.h"
/*
* Write a packetized stream, where each line is preceded by
@@ -120,6 +121,21 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
*/
ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
+/*
+ * Receive multiplexed output stream over git native protocol.
+ * in_stream is the input stream from the remote, which carries data
+ * in pkt_line format with band designator. Demultiplex it into out
+ * and err and return error appropriately. Band #1 carries the
+ * primary payload. Things coming over band #2 is not necessarily
+ * error; they are usually informative message on the standard error
+ * stream, aka "verbose"). A message over band #3 is a signal that
+ * the remote died unexpectedly. A flush() concludes the stream.
+ *
+ * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR
+ * or SIDEBAND_REMOTE_ERROR if an error occurred.
+ */
+int recv_sideband(const char *me, int in_stream, int out);
+
struct packet_reader {
/* source file descriptor */
int fd;
diff --git a/sideband.c b/sideband.c
index 368647acf8..e89d677626 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,7 +1,6 @@
#include "cache.h"
#include "color.h"
#include "config.h"
-#include "pkt-line.h"
#include "sideband.h"
#include "help.h"
@@ -109,109 +108,99 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
}
-/*
- * Receive multiplexed output stream over git native protocol.
- * in_stream is the input stream from the remote, which carries data
- * in pkt_line format with band designator. Demultiplex it into out
- * and err and return error appropriately. Band #1 carries the
- * primary payload. Things coming over band #2 is not necessarily
- * error; they are usually informative message on the standard error
- * stream, aka "verbose"). A message over band #3 is a signal that
- * the remote died unexpectedly. A flush() concludes the stream.
- */
-
#define DISPLAY_PREFIX "remote: "
#define ANSI_SUFFIX "\033[K"
#define DUMB_SUFFIX " "
-int recv_sideband(const char *me, int in_stream, int out)
+int demultiplex_sideband(const char *me, char *buf, int len,
+ struct strbuf *scratch,
+ enum sideband_type *sideband_type)
{
- const char *suffix;
- char buf[LARGE_PACKET_MAX + 1];
- struct strbuf outbuf = STRBUF_INIT;
- int retval = 0;
-
- if (isatty(2) && !is_terminal_dumb())
- suffix = ANSI_SUFFIX;
- else
- suffix = DUMB_SUFFIX;
-
- while (!retval) {
- const char *b, *brk;
- int band, len;
- len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
- if (len == 0)
- break;
- if (len < 1) {
- strbuf_addf(&outbuf,
- "%s%s: protocol error: no band designator",
- outbuf.len ? "\n" : "", me);
- retval = SIDEBAND_PROTOCOL_ERROR;
- break;
- }
- band = buf[0] & 0xff;
- buf[len] = '\0';
- len--;
- switch (band) {
- case 3:
- strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
- DISPLAY_PREFIX);
- maybe_colorize_sideband(&outbuf, buf + 1, len);
-
- retval = 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 (!outbuf.len)
- strbuf_addstr(&outbuf, DISPLAY_PREFIX);
- if (linelen > 0) {
- 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;
- }
+ 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;
+ }
- if (*b) {
- strbuf_addstr(&outbuf, outbuf.len ?
- "" : DISPLAY_PREFIX);
- maybe_colorize_sideband(&outbuf, b, strlen(b));
+ if (len == 0) {
+ *sideband_type = SIDEBAND_FLUSH;
+ goto cleanup;
+ }
+ if (len < 1) {
+ strbuf_addf(scratch,
+ "%s%s: protocol error: no band designator",
+ scratch->len ? "\n" : "", me);
+ *sideband_type = SIDEBAND_PROTOCOL_ERROR;
+ goto cleanup;
+ }
+ band = buf[0] & 0xff;
+ buf[len] = '\0';
+ len--;
+ switch (band) {
+ case 3:
+ 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);
}
- break;
- case 1:
- write_or_die(out, buf + 1, len);
- break;
- default:
- strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
- outbuf.len ? "\n" : "", me, band);
- retval = SIDEBAND_PROTOCOL_ERROR;
- break;
+
+ 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));
}
+ return 0;
+ case 1:
+ *sideband_type = SIDEBAND_PRIMARY;
+ break;
+ default:
+ strbuf_addf(scratch, "%s%s: protocol error: bad band #%d",
+ scratch->len ? "\n" : "", me, band);
+ *sideband_type = SIDEBAND_PROTOCOL_ERROR;
+ break;
}
- if (outbuf.len) {
- strbuf_addch(&outbuf, '\n');
- xwrite(2, outbuf.buf, outbuf.len);
+cleanup:
+ if (scratch->len) {
+ strbuf_addch(scratch, '\n');
+ xwrite(2, scratch->buf, scratch->len);
}
- strbuf_release(&outbuf);
- return retval;
+ strbuf_release(scratch);
+ return 1;
}
/*
diff --git a/sideband.h b/sideband.h
index 7a8146f161..2c4f021645 100644
--- a/sideband.h
+++ b/sideband.h
@@ -1,10 +1,28 @@
#ifndef SIDEBAND_H
#define SIDEBAND_H
-#define SIDEBAND_PROTOCOL_ERROR -2
-#define SIDEBAND_REMOTE_ERROR -1
+enum sideband_type {
+ SIDEBAND_PROTOCOL_ERROR = -2,
+ SIDEBAND_REMOTE_ERROR = -1,
+ SIDEBAND_FLUSH = 0,
+ SIDEBAND_PRIMARY = 1
+};
+
+/*
+ * Inspects a multiplexed packet read from the remote. If this packet is a
+ * progress packet and thus should not be processed by the caller, returns 0.
+ * Otherwise, returns 1, releases scratch, and sets sideband_type.
+ *
+ * If this packet is SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or a
+ * progress packet, also prints a message to stderr.
+ *
+ * scratch must be a struct strbuf allocated by the caller. It is used to store
+ * progress messages split across multiple packets.
+ */
+int demultiplex_sideband(const char *me, char *buf, int len,
+ struct strbuf *scratch,
+ enum sideband_type *sideband_type);
-int recv_sideband(const char *me, int in_stream, int out);
void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
#endif
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 3/4] {fetch,upload}-pack: sideband v2 fetch response
2019-01-16 19:28 ` [PATCH v3 " Jonathan Tan
2019-01-16 19:28 ` [PATCH v3 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
2019-01-16 19:28 ` [PATCH v3 2/4] sideband: reverse its dependency on pkt-line Jonathan Tan
@ 2019-01-16 19:28 ` Jonathan Tan
2019-01-16 19:28 ` [PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL Jonathan Tan
2019-01-17 19:43 ` [PATCH v3 0/4] Sideband the whole fetch v2 response Junio C Hamano
4 siblings, 0 replies; 41+ messages in thread
From: Jonathan Tan @ 2019-01-16 19:28 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, szeder.dev
Currently, a response to a fetch request has sideband support only while
the packfile is being sent, meaning that the server cannot send notices
until the start of the packfile.
Extend sideband support in protocol v2 fetch responses to the whole
response. upload-pack will advertise it if the
uploadpack.allowsidebandall configuration variable is set, and
fetch-pack will automatically request it if advertised.
If the sideband is to be used throughout the whole response, upload-pack
will use it to send errors instead of prefixing a PKT-LINE payload with
"ERR ".
This will be tested in a subsequent patch.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/technical/protocol-v2.txt | 10 ++++++
fetch-pack.c | 12 +++++--
pkt-line.c | 43 ++++++++++++++++++-------
pkt-line.h | 4 +++
sideband.c | 5 +++
sideband.h | 1 +
upload-pack.c | 16 +++++++++
7 files changed, 78 insertions(+), 13 deletions(-)
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 09e4e0273f..39b40c0dc1 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -307,6 +307,16 @@ the 'wanted-refs' section in the server's response as explained below.
particular ref, where <ref> is the full name of a ref on the
server.
+If the 'sideband-all' feature is advertised, the following argument can be
+included in the client's request:
+
+ sideband-all
+ Instruct the server to send the whole response multiplexed, not just
+ the packfile section. All non-flush and non-delim PKT-LINE in the
+ response (not only in the packfile section) will then start with a byte
+ indicating its sideband (1, 2, or 3), and the server may send "0005\2"
+ (a PKT-LINE of sideband 2 with no payload) as a keepalive packet.
+
The response of `fetch` is broken into a number of sections separated by
delimiter packets (0001), with each section beginning with its section
header.
diff --git a/fetch-pack.c b/fetch-pack.c
index ee0847e6ae..86e9e18901 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1090,7 +1090,8 @@ static int add_haves(struct fetch_negotiator *negotiator,
static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
const struct fetch_pack_args *args,
const struct ref *wants, struct oidset *common,
- int *haves_to_send, int *in_vain)
+ int *haves_to_send, int *in_vain,
+ int sideband_all)
{
int ret = 0;
struct strbuf req_buf = STRBUF_INIT;
@@ -1116,6 +1117,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
packet_buf_write(&req_buf, "include-tag");
if (prefer_ofs_delta)
packet_buf_write(&req_buf, "ofs-delta");
+ if (sideband_all)
+ packet_buf_write(&req_buf, "sideband-all");
/* Add shallow-info and deepen request */
if (server_supports_feature("fetch", "shallow", 0))
@@ -1324,6 +1327,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
packet_reader_init(&reader, fd[0], NULL, 0,
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_DIE_ON_ERR_PACKET);
+ if (server_supports_feature("fetch", "sideband-all", 0)) {
+ reader.use_sideband = 1;
+ reader.me = "fetch-pack";
+ }
while (state != FETCH_DONE) {
switch (state) {
@@ -1357,7 +1364,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
case FETCH_SEND_REQUEST:
if (send_fetch_request(&negotiator, fd[1], args, ref,
&common,
- &haves_to_send, &in_vain))
+ &haves_to_send, &in_vain,
+ reader.use_sideband))
state = FETCH_GET_PACK;
else
state = FETCH_PROCESS_ACKS;
diff --git a/pkt-line.c b/pkt-line.c
index 321ff632a5..d4b71d3e82 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -449,7 +449,7 @@ int recv_sideband(const char *me, int in_stream, int out)
while (1) {
len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX,
0);
- if (!demultiplex_sideband(me, buf, len, &scratch,
+ if (!demultiplex_sideband(me, buf, len, 0, &scratch,
&sideband_type))
continue;
switch (sideband_type) {
@@ -475,25 +475,43 @@ void packet_reader_init(struct packet_reader *reader, int fd,
reader->buffer = packet_buffer;
reader->buffer_size = sizeof(packet_buffer);
reader->options = options;
+ reader->me = "git";
}
enum packet_read_status packet_reader_read(struct packet_reader *reader)
{
+ struct strbuf scratch = STRBUF_INIT;
+
if (reader->line_peeked) {
reader->line_peeked = 0;
return reader->status;
}
- reader->status = packet_read_with_status(reader->fd,
- &reader->src_buffer,
- &reader->src_len,
- reader->buffer,
- reader->buffer_size,
- &reader->pktlen,
- reader->options);
+ /*
+ * Consume all progress packets until a primary payload packet is
+ * received
+ */
+ while (1) {
+ enum sideband_type sideband_type;
+ reader->status = packet_read_with_status(reader->fd,
+ &reader->src_buffer,
+ &reader->src_len,
+ reader->buffer,
+ reader->buffer_size,
+ &reader->pktlen,
+ reader->options);
+ if (!reader->use_sideband)
+ break;
+ if (demultiplex_sideband(reader->me, reader->buffer,
+ reader->pktlen, 1, &scratch,
+ &sideband_type))
+ break;
+ }
if (reader->status == PACKET_READ_NORMAL)
- reader->line = reader->buffer;
+ /* Skip the sideband designator if sideband is used */
+ reader->line = reader->use_sideband ?
+ reader->buffer + 1 : reader->buffer;
else
reader->line = NULL;
@@ -515,6 +533,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)
void packet_writer_init(struct packet_writer *writer, int dest_fd)
{
writer->dest_fd = dest_fd;
+ writer->use_sideband = 0;
}
void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
@@ -522,7 +541,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
+ packet_write_fmt_1(writer->dest_fd, 0,
+ writer->use_sideband ? "\001" : "", fmt, args);
va_end(args);
}
@@ -531,7 +551,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
va_list args;
va_start(args, fmt);
- packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
+ packet_write_fmt_1(writer->dest_fd, 0,
+ writer->use_sideband ? "\003" : "ERR ", fmt, args);
va_end(args);
}
diff --git a/pkt-line.h b/pkt-line.h
index a8e92a4b63..ad9a4a2cd7 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -162,6 +162,9 @@ struct packet_reader {
/* indicates if a line has been peeked */
int line_peeked;
+
+ unsigned use_sideband : 1;
+ const char *me;
};
/*
@@ -201,6 +204,7 @@ extern char packet_buffer[LARGE_PACKET_MAX];
struct packet_writer {
int dest_fd;
+ unsigned use_sideband : 1;
};
void packet_writer_init(struct packet_writer *writer, int dest_fd);
diff --git a/sideband.c b/sideband.c
index e89d677626..6a16feb262 100644
--- a/sideband.c
+++ b/sideband.c
@@ -114,6 +114,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
#define DUMB_SUFFIX " "
int demultiplex_sideband(const char *me, char *buf, int len,
+ int die_on_error,
struct strbuf *scratch,
enum sideband_type *sideband_type)
{
@@ -144,6 +145,8 @@ int demultiplex_sideband(const char *me, char *buf, int len,
len--;
switch (band) {
case 3:
+ if (die_on_error)
+ die("remote error: %s", buf + 1);
strbuf_addf(scratch, "%s%s", scratch->len ? "\n" : "",
DISPLAY_PREFIX);
maybe_colorize_sideband(scratch, buf + 1, len);
@@ -195,6 +198,8 @@ int demultiplex_sideband(const char *me, char *buf, int len,
}
cleanup:
+ if (die_on_error && *sideband_type == SIDEBAND_PROTOCOL_ERROR)
+ die("%s", scratch->buf);
if (scratch->len) {
strbuf_addch(scratch, '\n');
xwrite(2, scratch->buf, scratch->len);
diff --git a/sideband.h b/sideband.h
index 2c4f021645..227740a58e 100644
--- a/sideband.h
+++ b/sideband.h
@@ -20,6 +20,7 @@ enum sideband_type {
* progress messages split across multiple packets.
*/
int demultiplex_sideband(const char *me, char *buf, int len,
+ int die_on_error,
struct strbuf *scratch,
enum sideband_type *sideband_type);
diff --git a/upload-pack.c b/upload-pack.c
index 60a26bbbfd..765b7695d2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -71,6 +71,8 @@ static int allow_filter;
static int allow_ref_in_want;
static struct list_objects_filter_options filter_options;
+static int allow_sideband_all;
+
static void reset_timeout(void)
{
alarm(timeout);
@@ -1046,6 +1048,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
allow_filter = git_config_bool(var, value);
} else if (!strcmp("uploadpack.allowrefinwant", var)) {
allow_ref_in_want = git_config_bool(var, value);
+ } else if (!strcmp("uploadpack.allowsidebandall", var)) {
+ allow_sideband_all = git_config_bool(var, value);
}
if (current_config_scope() != CONFIG_SCOPE_REPO) {
@@ -1284,6 +1288,11 @@ static void process_args(struct packet_reader *request,
continue;
}
+ if (allow_sideband_all && !strcmp(arg, "sideband-all")) {
+ data->writer.use_sideband = 1;
+ continue;
+ }
+
/* ignore unknown lines maybe? */
die("unexpected line: '%s'", arg);
}
@@ -1496,6 +1505,7 @@ int upload_pack_advertise(struct repository *r,
if (value) {
int allow_filter_value;
int allow_ref_in_want;
+ int allow_sideband_all_value;
strbuf_addstr(value, "shallow");
@@ -1510,6 +1520,12 @@ int upload_pack_advertise(struct repository *r,
&allow_ref_in_want) &&
allow_ref_in_want)
strbuf_addstr(value, " ref-in-want");
+
+ if (!repo_config_get_bool(the_repository,
+ "uploadpack.allowsidebandall",
+ &allow_sideband_all_value) &&
+ allow_sideband_all_value)
+ strbuf_addstr(value, " sideband-all");
}
return 1;
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL
2019-01-16 19:28 ` [PATCH v3 " Jonathan Tan
` (2 preceding siblings ...)
2019-01-16 19:28 ` [PATCH v3 3/4] {fetch,upload}-pack: sideband v2 fetch response Jonathan Tan
@ 2019-01-16 19:28 ` Jonathan Tan
2019-01-29 23:27 ` Jonathan Nieder
2019-01-17 19:43 ` [PATCH v3 0/4] Sideband the whole fetch v2 response Junio C Hamano
4 siblings, 1 reply; 41+ messages in thread
From: Jonathan Tan @ 2019-01-16 19:28 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, szeder.dev
Define a GIT_TEST_SIDEBAND_ALL environment variable meant to be used
from tests. When set to true, this overrides uploadpack.allowsidebandall
to true, allowing the entire test suite to be run as if this
configuration is in place for all repositories.
As of this patch, all tests pass whether GIT_TEST_SIDEBAND_ALL is unset
or set to 1.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
fetch-pack.c | 3 ++-
t/README | 5 +++++
t/lib-httpd/apache.conf | 1 +
t/t5537-fetch-shallow.sh | 3 ++-
t/t5701-git-serve.sh | 2 +-
t/t5702-protocol-v2.sh | 4 ++--
upload-pack.c | 13 ++++++++-----
7 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 86e9e18901..e98294d918 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1327,7 +1327,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
packet_reader_init(&reader, fd[0], NULL, 0,
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_DIE_ON_ERR_PACKET);
- if (server_supports_feature("fetch", "sideband-all", 0)) {
+ if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 1) &&
+ server_supports_feature("fetch", "sideband-all", 0)) {
reader.use_sideband = 1;
reader.me = "fetch-pack";
}
diff --git a/t/README b/t/README
index 28711cc508..b275c883b8 100644
--- a/t/README
+++ b/t/README
@@ -358,6 +358,11 @@ GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-
index to be written after every 'git repack' command, and overrides the
'core.multiPackIndex' setting to true.
+GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
+'uploadpack.allowSidebandAll' setting to true, and when false, forces
+fetch-pack to not request sideband-all (even if the server advertises
+sideband-all).
+
Naming Tests
------------
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..9a6d368247 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -78,6 +78,7 @@ PassEnv GNUPGHOME
PassEnv ASAN_OPTIONS
PassEnv GIT_TRACE
PassEnv GIT_CONFIG_NOSYSTEM
+PassEnv GIT_TEST_SIDEBAND_ALL
SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 6faf17e17a..6caf628efa 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -243,7 +243,8 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f
"$(git -C "$REPO" rev-parse HEAD)" \
"$(git -C "$REPO" rev-parse HEAD^)" \
>"$HTTPD_ROOT_PATH/one-time-sed" &&
- test_must_fail git -C client fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
+ test_must_fail env GIT_TEST_SIDEBAND_ALL=0 git -C client \
+ fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
master:a_branch &&
# Ensure that the one-time-sed script was used.
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index ae79c6bbc0..fe45bf828d 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -14,7 +14,7 @@ test_expect_success 'test capability advertisement' '
0000
EOF
- git serve --advertise-capabilities >out &&
+ GIT_TEST_SIDEBAND_ALL=0 git serve --advertise-capabilities >out &&
test-tool pkt-line unpack <out >actual &&
test_cmp expect actual
'
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..b491c62e3e 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -583,8 +583,8 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" git -C http_child \
-c protocol.version=2 \
fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err &&
- grep "fetch< acknowledgments" log &&
- ! grep "fetch< ready" log &&
+ grep "fetch< .*acknowledgments" log &&
+ ! grep "fetch< .*ready" log &&
test_i18ngrep "expected no other sections to be sent after no .ready." err
'
diff --git a/upload-pack.c b/upload-pack.c
index 765b7695d2..0c1feccaab 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1288,7 +1288,9 @@ static void process_args(struct packet_reader *request,
continue;
}
- if (allow_sideband_all && !strcmp(arg, "sideband-all")) {
+ if ((git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
+ allow_sideband_all) &&
+ !strcmp(arg, "sideband-all")) {
data->writer.use_sideband = 1;
continue;
}
@@ -1521,10 +1523,11 @@ int upload_pack_advertise(struct repository *r,
allow_ref_in_want)
strbuf_addstr(value, " ref-in-want");
- if (!repo_config_get_bool(the_repository,
- "uploadpack.allowsidebandall",
- &allow_sideband_all_value) &&
- allow_sideband_all_value)
+ if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
+ (!repo_config_get_bool(the_repository,
+ "uploadpack.allowsidebandall",
+ &allow_sideband_all_value) &&
+ allow_sideband_all_value))
strbuf_addstr(value, " sideband-all");
}
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v3 0/4] Sideband the whole fetch v2 response
2019-01-16 19:28 ` [PATCH v3 " Jonathan Tan
` (3 preceding siblings ...)
2019-01-16 19:28 ` [PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL Jonathan Tan
@ 2019-01-17 19:43 ` Junio C Hamano
4 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2019-01-17 19:43 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, szeder.dev
Jonathan Tan <jonathantanmy@google.com> writes:
> /*
> + * Inspects a multiplexed packet read from the remote. If this packet is a
> + * progress packet and thus should not be processed by the caller, returns 0.
> + * Otherwise, returns 1, releases scratch, and sets sideband_type.
> *
> + * If this packet is SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or a
> + * progress packet, also prints a message to stderr.
> + *
> + * scratch must be a struct strbuf allocated by the caller. It is used to store
> + * progress messages split across multiple packets.
> */
> +int demultiplex_sideband(const char *me, char *buf, int len,
> + int die_on_error,
> + struct strbuf *scratch,
> + enum sideband_type *sideband_type);
OK. That sounds like a sensible way to keep the leftover across
calls.
Will queue.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL
2019-01-16 19:28 ` [PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL Jonathan Tan
@ 2019-01-29 23:27 ` Jonathan Nieder
2019-02-13 6:49 ` Jeff King
0 siblings, 1 reply; 41+ messages in thread
From: Jonathan Nieder @ 2019-01-29 23:27 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster, szeder.dev
Hi,
Jonathan Tan wrote:
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -78,6 +78,7 @@ PassEnv GNUPGHOME
> PassEnv ASAN_OPTIONS
> PassEnv GIT_TRACE
> PassEnv GIT_CONFIG_NOSYSTEM
> +PassEnv GIT_TEST_SIDEBAND_ALL
This is producing
[env:warn] [pid 248632] AH01506: PassEnv variable GIT_TEST_SIDEBAND_ALL was undefined
when tests are run with "prove".
Should we set the envvar unconditionally in lib-httpd.sh?
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL
2019-01-29 23:27 ` Jonathan Nieder
@ 2019-02-13 6:49 ` Jeff King
0 siblings, 0 replies; 41+ messages in thread
From: Jeff King @ 2019-02-13 6:49 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jonathan Tan, git, gitster, szeder.dev
On Tue, Jan 29, 2019 at 03:27:32PM -0800, Jonathan Nieder wrote:
> Jonathan Tan wrote:
>
> > --- a/t/lib-httpd/apache.conf
> > +++ b/t/lib-httpd/apache.conf
> > @@ -78,6 +78,7 @@ PassEnv GNUPGHOME
> > PassEnv ASAN_OPTIONS
> > PassEnv GIT_TRACE
> > PassEnv GIT_CONFIG_NOSYSTEM
> > +PassEnv GIT_TEST_SIDEBAND_ALL
>
> This is producing
>
> [env:warn] [pid 248632] AH01506: PassEnv variable GIT_TEST_SIDEBAND_ALL was undefined
>
> when tests are run with "prove".
>
> Should we set the envvar unconditionally in lib-httpd.sh?
Yes, that's the same solution we've had to use for GIT_VALGRIND, etc. I
don't think there's an easier way.
-Peff
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2019-02-13 6:49 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11 22:18 [PATCH 0/4] Sideband the whole fetch v2 response Jonathan Tan
2019-01-11 22:18 ` [PATCH 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
2019-01-14 20:07 ` Junio C Hamano
2019-01-15 18:18 ` Jonathan Tan
2019-01-11 22:18 ` [PATCH 2/4] sideband: reverse its dependency on pkt-line Jonathan Tan
2019-01-14 20:07 ` Junio C Hamano
2019-01-14 22:11 ` Junio C Hamano
2019-01-15 18:38 ` Jonathan Tan
2019-01-15 18:36 ` Jonathan Tan
2019-01-11 22:18 ` [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response Jonathan Tan
2019-01-14 22:27 ` Junio C Hamano
2019-01-15 19:18 ` Jonathan Tan
2019-01-11 22:18 ` [PATCH 4/4] tests: define GIT_TEST_SIDEBAND_ALL Jonathan Tan
2019-01-11 22:18 ` [WIP 5/4] Documentation: order protocol v2 sections Jonathan Tan
2019-01-11 22:18 ` [WIP 6/4] Documentation: add Packfile URIs design doc Jonathan Tan
2019-01-11 22:18 ` [WIP 7/4] upload-pack: refactor reading of pack-objects out Jonathan Tan
2019-01-11 22:18 ` [WIP 8/4] upload-pack: send part of packfile response as uri Jonathan Tan
2019-01-15 19:40 ` [PATCH v2 0/4] Sideband the whole fetch v2 response Jonathan Tan
2019-01-15 19:40 ` [PATCH v2 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
2019-01-15 19:40 ` [PATCH v2 2/4] sideband: reverse its dependency on pkt-line Jonathan Tan
2019-01-16 10:34 ` SZEDER Gábor
2019-01-16 17:43 ` Jonathan Tan
2019-01-16 19:17 ` SZEDER Gábor
2019-01-15 19:40 ` [PATCH v2 3/4] {fetch,upload}-pack: sideband v2 fetch response Jonathan Tan
2019-01-15 19:40 ` [PATCH v2 4/4] tests: define GIT_TEST_SIDEBAND_ALL Jonathan Tan
2019-01-15 19:50 ` [PATCH v2 0/4] Sideband the whole fetch v2 response Junio C Hamano
2019-01-15 21:11 ` Junio C Hamano
2019-01-15 22:08 ` Jonathan Tan
2019-01-15 22:35 ` Junio C Hamano
2019-01-15 23:02 ` Jonathan Tan
2019-01-15 23:13 ` Junio C Hamano
2019-01-16 0:38 ` Jonathan Tan
2019-01-16 4:12 ` Junio C Hamano
2019-01-16 19:28 ` [PATCH v3 " Jonathan Tan
2019-01-16 19:28 ` [PATCH v3 1/4] pkt-line: introduce struct packet_writer Jonathan Tan
2019-01-16 19:28 ` [PATCH v3 2/4] sideband: reverse its dependency on pkt-line Jonathan Tan
2019-01-16 19:28 ` [PATCH v3 3/4] {fetch,upload}-pack: sideband v2 fetch response Jonathan Tan
2019-01-16 19:28 ` [PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL Jonathan Tan
2019-01-29 23:27 ` Jonathan Nieder
2019-02-13 6:49 ` Jeff King
2019-01-17 19:43 ` [PATCH v3 0/4] Sideband the whole fetch v2 response Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).