git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
JTJonathan Tan <jonathantanmy@google.com>
Date
Jan 25, 2017, 22:02 UTC
Message-ID
<afe5d7d3f876893fdad318665805df1e056717c6.1485381677.git.jonathantanmy@google.com>
In-Reply-To
<cover.1485381677.git.jonathantanmy@google.com>

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 <jonathantanmy@google.com>
---
 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
 </LocationMatch>
+<LocationMatch /one_time_sed/>
+	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+	SetEnv GIT_HTTP_EXPORT_ALL
+</LocationMatch>
 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
 <Directory ${GIT_EXEC_PATH}>
 	Options FollowSymlinks
 </Directory>
@@ -118,6 +123,9 @@ ScriptAlias /error/ error.sh/
 <Files error.sh>
   Options ExecCGI
 </Files>
+<Files one-time-sed.sh>
+	Options ExecCGI
+</Files>
 <Files ${GIT_EXEC_PATH}/git-http-backend>
 	Options ExecCGI
 </Files>
diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
new file mode 100644
index 000000000..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 fetch 2> err &&
+	grep "ERR upload-pack: not our ref" err
+'
+
+test_expect_failure 'server is initially ahead - ref in want' '
+	git -C "$REPO" config uploadpack.advertiseRefInWant true &&
+	rm -rf local &&
+	cp -r "$LOCAL_PRISTINE" local &&
+	inconsistency master 1234567890123456789012345678901234567890 &&
+	git -C local fetch &&
+
+	git -C "$REPO" rev-parse --verify master > expected &&
+	git -C local rev-parse --verify refs/remotes/origin/master > actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'server is initially behind - no ref in want' '
+	git -C "$REPO" config uploadpack.advertiseRefInWant false &&
+	rm -rf local &&
+	cp -r "$LOCAL_PRISTINE" local &&
+	inconsistency master "master^" &&
+	git -C local fetch &&
+
+	git -C "$REPO" rev-parse --verify "master^" > expected &&
+	git -C local rev-parse --verify refs/remotes/origin/master > actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure 'server is initially behind - ref in want' '
+	git -C "$REPO" config uploadpack.advertiseRefInWant true &&
+	rm -rf local &&
+	cp -r "$LOCAL_PRISTINE" local &&
+	inconsistency master "master^" &&
+	git -C local fetch &&
+
+	git -C "$REPO" rev-parse --verify "master" > expected &&
+	git -C local rev-parse --verify refs/remotes/origin/master > actual &&
+	test_cmp expected actual
+'
+
+stop_httpd
+
 test_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 ", &arg) &&
 			   !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));
+			}
 			if (!(o->flags & WANTED)) {
 				o->flags |= WANTED;
 				if (!((allow_unadvertised_object_request & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1

-- 
2.11.0.483.g087da7b7c-goog
Previous: Jonathan TanNext: Junio C Hamano
Message 12 of 30 in “[RFC 00/14] Allow fetch-pack to send ref names (globs allowed)”
  1. Jonathan TanJan 25, 2017
  2. 02/14 upload-pack: allow ref name and glob requestsJonathan Tan, Jan 25, 2017
  3. Junio C HamanoJan 26, 2017
  4. Jonathan TanJan 27, 2017
  5. Junio C HamanoJan 27, 2017
  6. 01/14 upload-pack: move parsing of "want" lineJonathan Tan, Jan 25, 2017
  7. 12/14 fetch-pack: do not printf after closing stdoutJonathan Tan, Jan 25, 2017
  8. Stefan BellerJan 26, 2017
  9. Jonathan TanJan 26, 2017
  10. 13/14 fetch: send want-ref and receive fetched refsJonathan Tan, Jan 25, 2017
  11. 04/14 fetch: refactor the population of hashesJonathan Tan, Jan 25, 2017
  12. 03/14 upload-pack: test negotiation with changing repoJonathan Tan, Jan 25, 2017
  13. Junio C HamanoJan 26, 2017
  14. Jonathan TanJan 27, 2017
  15. Junio C HamanoFeb 22, 2017
  16. upload-pack: report "not our ref" to clientJonathan Tan, Feb 23, 2017
  17. Junio C HamanoFeb 23, 2017
  18. 11/14 fetch-pack: support want-refJonathan Tan, Jan 25, 2017
  19. 07/14 fetch-pack: put shallow info in out paramJonathan Tan, Jan 25, 2017
  20. 14/14 DONT USE advertise_ref_in_want=1Jonathan Tan, Jan 25, 2017
  21. 05/14 fetch: refactor fetch_refs into two functionsJonathan Tan, Jan 25, 2017
  22. 06/14 fetch: refactor to make function args narrowerJonathan Tan, Jan 25, 2017
  23. 09/14 transport: put ref oid in out paramJonathan Tan, Jan 25, 2017
  24. 08/14 fetch-pack: check returned refs for matchesJonathan Tan, Jan 25, 2017
  25. 10/14 fetch-pack: support partial names and globsJonathan Tan, Jan 25, 2017
  26. Stefan BellerJan 26, 2017
  27. Jeff KingJan 26, 2017
  28. Jonathan TanJan 27, 2017
  29. Jonathan TanFeb 7, 2017
  30. Junio C HamanoFeb 9, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.