# [PATCH] fetch-pack: send server options after command 3 messages from 2019-05-22 to 2019-05-28. Participants: Jonathan Tan, Jonathan Nieder, Junio C Hamano. Thread: https://gitlist.dev/t/51155 ## Jonathan Tan, 2019-05-22 20:08 Subject: [PATCH] fetch-pack: send server options after command Message-ID: <20190522200822.176870-1-jonathantanmy@google.com> URL: https://gitlist.dev/e/20190522200822.176870-1-jonathantanmy%40google.com ``` Currently, if any server options are specified during a protocol v2 fetch, server options will be sent before "command=fetch". Write server options to the request buffer in send_fetch_request() so that the components of the request are sent in the correct order. The protocol documentation states that the command must come first. The Git server implementation in serve.c (see process_request() in that file) tolerates any order of command and capability, which is perhaps why we haven't noticed this. This was noticed when testing against a JGit server implementation, which follows the documentation in this regard. Signed-off-by: Jonathan Tan --- fetch-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fetch-pack.c b/fetch-pack.c index 3f24d0c8a6..1c10f54e78 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1115,7 +1115,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, server_supports_v2("server-option", 1)) { int i; for (i = 0; i < args->server_options->nr; i++) - packet_write_fmt(fd_out, "server-option=%s", + packet_buf_write(&req_buf, "server-option=%s", args->server_options->items[i].string); } -- 2.21.0.1020.gf2820cf01a-goog ``` ## Jonathan Nieder, 2019-05-24 19:11 Subject: Re: [PATCH] fetch-pack: send server options after command Message-ID: <20190524191151.GA14606@google.com> URL: https://gitlist.dev/e/20190524191151.GA14606%40google.com In-Reply-To: <20190522200822.176870-1-jonathantanmy@google.com> ``` Jonathan Tan wrote: > Currently, if any server options are specified during a protocol v2 > fetch, server options will be sent before "command=fetch". Write server > options to the request buffer in send_fetch_request() so that the > components of the request are sent in the correct order. > > The protocol documentation states that the command must come first. The > Git server implementation in serve.c (see process_request() in that > file) tolerates any order of command and capability, which is perhaps > why we haven't noticed this. This was noticed when testing against a > JGit server implementation, which follows the documentation in this > regard. > > Signed-off-by: Jonathan Tan > --- > fetch-pack.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Oh, dear. Thanks for fixing it. Reviewed-by: Jonathan Nieder 6e98305985 (clone: send server options when using protocol v2, 2019-04-12) is part of release candidates, but it looks like we caught this in time to get the fix in before the release. Should we add an interop test for this to t/interop/? Thanks, Jonathan ``` ## Junio C Hamano, 2019-05-28 18:00 Subject: Re: [PATCH] fetch-pack: send server options after command Message-ID: URL: https://gitlist.dev/e/xmqqd0k231e1.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20190524191151.GA14606@google.com> ``` Jonathan Nieder writes: > Jonathan Tan wrote: > >> Currently, if any server options are specified during a protocol v2 >> fetch, server options will be sent before "command=fetch". Write server >> options to the request buffer in send_fetch_request() so that the >> components of the request are sent in the correct order. >> >> The protocol documentation states that the command must come first. The >> Git server implementation in serve.c (see process_request() in that >> file) tolerates any order of command and capability, which is perhaps >> why we haven't noticed this. This was noticed when testing against a >> JGit server implementation, which follows the documentation in this >> regard. >> >> Signed-off-by: Jonathan Tan >> --- >> fetch-pack.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Oh, dear. Thanks for fixing it. > > Reviewed-by: Jonathan Nieder Yeah, looks good. Thanks. > > 6e98305985 (clone: send server options when using protocol v2, > 2019-04-12) is part of release candidates, but it looks like we caught > this in time to get the fix in before the release. > > Should we add an interop test for this to t/interop/? > > Thanks, > Jonathan ```