Re: [RFC 03/14] upload-pack: test negotiation with changing repo

[Junio C Hamano]

Jonathan Tan  writes:

>> This somehow looks like a good thing to do even in production.  Am I
>> mistaken?
>
> Yes, that's true. If this patch set stalls (for whatever reason), I'll
> spin this off into an independent patch.

... which may be needed.

As to the main goal of this topic, I think the "ask by refname (with
glob)" is very good thing to start the "client speaks first" v2
protocol, as it would allow us to reduce the size of the initial
advertisement for common cases (i.e. remote..fetch is likely
to list only refs/heads/* on the left hand side of a refspec).  And
adding this to v1 is probably a good first step to make sure the
code that is currently used by v1 protocol exchange that will be
shared with the v2 protocols are prepared to be driven by refname
without knowing the exact object name until the final round.

Re: [RFC 03/14] upload-pack: test negotiation with changing repo

[Jonathan Tan]

On 01/26/2017 02:33 PM, Junio C Hamano wrote:

Jonathan Tan  writes:


diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
new file mode 100644
index 0..060ec0300
--- /dev/null
+++ b/t/lib-httpd/one-time-sed.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+if [ -e one-time-sed ]; then
+   "$GIT_EXEC_PATH/git-http-backend" | sed "$(cat one-time-sed)"
+   rm one-time-sed
+else
+   "$GIT_EXEC_PATH/git-http-backend"
+fi


CodingGuidelines?


Thanks - done locally and will send out in the next reroll.


+inconsistency() {
+   # Simulate that the server initially reports $2 as the ref
+   # corresponding to $1, and after that, $1 as the ref corresponding to
+   # $1. This corresponds to the real-life situation where the server's
+   # repository appears to change during negotiation, for example, when
+   # different servers in a load-balancing arrangement serve (stateless)
+   # RPCs during a single negotiation.
+   printf "s/%s/%s/" \
+  $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
+  $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
+  >"$HTTPD_ROOT_PATH/one-time-sed"


I'd prefer for the printf'd result to have a final LF (i.e. not
leaving the resulting one-time-sed with a final incomplete line).
Also, do you really need the pipe to tr-d?  Doesn't the result of
$(command substitution) omit the final LF anyway?

$ printf '1 %s 2 %s 3\n' "$(echo foo)" "$(echo bar)"; echo OK
1 foo 2 bar 3
OK


Done.


diff --git a/upload-pack.c b/upload-pack.c
index b88ed8e26..0678c53d6 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -862,9 +862,13 @@ static void receive_needs(struct string_list 
*wanted_ns_refs)
} else if (skip_prefix(line, "want ", ) &&
   !get_sha1_hex(arg, sha1_buf)) {
o = parse_object(sha1_buf);
-   if (!o)
+   if (!o) {
+   packet_write_fmt(1,
+"ERR upload-pack: not our ref 
%s",
+sha1_to_hex(sha1_buf));
die("git upload-pack: not our ref %s",
sha1_to_hex(sha1_buf));
+   }


This somehow looks like a good thing to do even in production.  Am I
mistaken?


Yes, that's true. If this patch set stalls (for whatever reason), I'll 
spin this off into an independent patch.

Re: [RFC 03/14] upload-pack: test negotiation with changing repo

[Junio C Hamano]

Jonathan Tan  writes:

> diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
> new file mode 100644
> index 0..060ec0300
> --- /dev/null
> +++ b/t/lib-httpd/one-time-sed.sh
> @@ -0,0 +1,8 @@
> +#!/bin/sh
> +
> +if [ -e one-time-sed ]; then
> + "$GIT_EXEC_PATH/git-http-backend" | sed "$(cat one-time-sed)"
> + rm one-time-sed
> +else
> + "$GIT_EXEC_PATH/git-http-backend"
> +fi

CodingGuidelines?

> +inconsistency() {
> + # Simulate that the server initially reports $2 as the ref
> + # corresponding to $1, and after that, $1 as the ref corresponding to
> + # $1. This corresponds to the real-life situation where the server's
> + # repository appears to change during negotiation, for example, when
> + # different servers in a load-balancing arrangement serve (stateless)
> + # RPCs during a single negotiation.
> + printf "s/%s/%s/" \
> +$(git -C "$REPO" rev-parse $1 | tr -d "\n") \
> +$(git -C "$REPO" rev-parse $2 | tr -d "\n") \
> +>"$HTTPD_ROOT_PATH/one-time-sed"

I'd prefer for the printf'd result to have a final LF (i.e. not
leaving the resulting one-time-sed with a final incomplete line).
Also, do you really need the pipe to tr-d?  Doesn't the result of 
$(command substitution) omit the final LF anyway?

$ printf '1 %s 2 %s 3\n' "$(echo foo)" "$(echo bar)"; echo OK
1 foo 2 bar 3
OK

> diff --git a/upload-pack.c b/upload-pack.c
> index b88ed8e26..0678c53d6 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -862,9 +862,13 @@ static void receive_needs(struct string_list 
> *wanted_ns_refs)
>   } else if (skip_prefix(line, "want ", ) &&
>  !get_sha1_hex(arg, sha1_buf)) {
>   o = parse_object(sha1_buf);
> - if (!o)
> + if (!o) {
> + packet_write_fmt(1,
> +  "ERR upload-pack: not our ref 
> %s",
> +  sha1_to_hex(sha1_buf));
>   die("git upload-pack: not our ref %s",
>   sha1_to_hex(sha1_buf));
> + }

This somehow looks like a good thing to do even in production.  Am I
mistaken?

[RFC 03/14] upload-pack: test negotiation with changing repo

[Jonathan Tan]

Make upload-pack report "not our ref" errors to the client. (If not, the
client would be left waiting for a response when the server is already
dead.)

Add tests to check the behavior of upload-pack and fetch-pack when
upload-pack is served from a changing repository (for example, when
different servers in a load-balancing agreement participate in the same
stateless RPC negotiation). This forms a baseline of comparison to the
ref-in-want functionality (which will be introduced in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.

As part of this effort, a mechanism to substitute strings in an HTTP
response only on the first invocation is added.

Signed-off-by: Jonathan Tan 
---
 t/lib-httpd.sh |  1 +
 t/lib-httpd/apache.conf|  8 
 t/lib-httpd/one-time-sed.sh|  8 
 t/t5552-upload-pack-ref-in-want.sh | 91 ++
 upload-pack.c  |  6 ++-
 5 files changed, 113 insertions(+), 1 deletion(-)
 create mode 100644 t/lib-httpd/one-time-sed.sh

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465..84f8efdd4 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -132,6 +132,7 @@ prepare_httpd() {
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
install_script error.sh
+   install_script one-time-sed.sh
 
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
 
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 69174c6e3..ef218ff15 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -106,9 +106,14 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
 
+
+   SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+   SetEnv GIT_HTTP_EXPORT_ALL
+
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
+ScriptAliasMatch /one_time_sed/(.*) one-time-sed.sh/$1
 
Options FollowSymlinks
 
@@ -118,6 +123,9 @@ ScriptAlias /error/ error.sh/
 
   Options ExecCGI
 
+
+   Options ExecCGI
+
 
Options ExecCGI
 
diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
new file mode 100644
index 0..060ec0300
--- /dev/null
+++ b/t/lib-httpd/one-time-sed.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+if [ -e one-time-sed ]; then
+   "$GIT_EXEC_PATH/git-http-backend" | sed "$(cat one-time-sed)"
+   rm one-time-sed
+else
+   "$GIT_EXEC_PATH/git-http-backend"
+fi
diff --git a/t/t5552-upload-pack-ref-in-want.sh 
b/t/t5552-upload-pack-ref-in-want.sh
index 3496af641..80cf2263a 100755
--- a/t/t5552-upload-pack-ref-in-want.sh
+++ b/t/t5552-upload-pack-ref-in-want.sh
@@ -292,4 +292,95 @@ test_expect_success 'hideRefs with namespaces' '
check_output
 '
 
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for change-while-negotiating test' '
+   (
+   git init "$REPO" &&
+   cd "$REPO" &&
+   >.git/git-daemon-export-ok &&
+   test_commit m1 &&
+   git tag -d m1 &&
+
+   # Local repo with many commits (so that negotiation will take
+   # more than 1 request/response pair)
+   git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo; 
"$LOCAL_PRISTINE" &&
+   cd "$LOCAL_PRISTINE" &&
+   git checkout -b side &&
+   for i in $(seq 1 33); do test_commit s$i; done &&
+
+   # Add novel commits to upstream
+   git checkout master &&
+   cd "$REPO" &&
+   test_commit m2 &&
+   test_commit m3 &&
+   git tag -d m2 m3
+   ) &&
+   git -C "$LOCAL_PRISTINE" remote set-url origin 
"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo;
+'
+
+inconsistency() {
+   # Simulate that the server initially reports $2 as the ref
+   # corresponding to $1, and after that, $1 as the ref corresponding to
+   # $1. This corresponds to the real-life situation where the server's
+   # repository appears to change during negotiation, for example, when
+   # different servers in a load-balancing arrangement serve (stateless)
+   # RPCs during a single negotiation.
+   printf "s/%s/%s/" \
+  $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
+  $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
+  >"$HTTPD_ROOT_PATH/one-time-sed"
+}
+
+test_expect_success 'server is initially ahead - no ref in want' '
+   git -C "$REPO" config uploadpack.advertiseRefInWant false &&
+   rm -rf local &&
+   cp -r "$LOCAL_PRISTINE" local &&
+   inconsistency master 1234567890123456789012345678901234567890 &&
+   test_must_fail git -C local