{"thread":{"id":"38791","subject":"[PATCH 0/7] fix transfer.hiderefs with smart http","startedAt":"2015-03-13T04:41:01Z","lastAt":"2015-03-13T06:45:59Z","messageCount":12,"participants":["Jeff King","Duy Nguyen","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"257620","messageId":"20150313044101.GA18476@peff.net","threadId":"38791","inReplyTo":null,"subject":"[PATCH 0/7] fix transfer.hiderefs with smart http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-13T04:41:01Z","receivedAt":"2015-03-13T04:41:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I'm experimenting with using transfer.hiderefs on a server, and it's\nrather easy to cause a git client to hang when fetching from such a repo\nover smart http. Details are in the first patch.\n\nThere are 7 patches here, but the entirety of the fix is contained in\nthe first one. The rest are cleanups and test enhancements I found along\nthe way. I moved the fix to the front of the series as we probably want\nit to go to \"maint\", but the others can wait (being mostly test\nmodifications, they should not cause regressions, but they'd possibly\nwant more cooking time in case I broke the test suite for somebody).\n\nThe patches are:\n\n  [1/7]: upload-pack: fix transfer.hiderefs over smart-http\n\n    The fix.\n\n  [2/7]: upload-pack: do not check NULL return of lookup_unknown_object\n\n    A nearby cleanup.\n\n  [3/7]: t: translate SIGINT to an exit\n  [4/7]: t: redirect stderr GIT_TRACE to descriptor 4\n  [5/7]: t: pass GIT_TRACE through Apache\n\n    These all solve irritations I had when trying to debug the test.\n\n  [6/7]: t5541: move run_with_cmdline_limit to test-lib.sh\n  [7/7]: t5551: make EXPENSIVE test cheaper\n\n    I had thought at first that the problem was related to large http\n    fetches, but it turned out not to be. But I think these cleanups\n    are a good thing, as they increase the default test coverage.\n\n-Peff\n"},{"id":"257621","messageId":"20150313044212.GA18532@peff.net","threadId":"38791","inReplyTo":"20150313044101.GA18476@peff.net","subject":"[PATCH 1/7] upload-pack: fix transfer.hiderefs over smart-http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-13T04:42:12Z","receivedAt":"2015-03-13T04:42:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When upload-pack advertises the refs (either for a normal,\nnon-stateless request, or for the initial contact in a\nstateless one), we call for_each_ref with the send_ref\nfunction as its callback. send_ref, in turn, calls\nmark_our_ref, which checks whether the ref is hidden, and\nsets OUR_REF or HIDDEN_REF on the object as appropriate.  If\nit is hidden, mark_our_ref also returns \"1\" to signal\nsend_ref that the ref should not be advertised.\n\nIf we are not advertising refs, (i.e., the follow-up\ninvocation by an http client to send its \"want\" lines), we\nuse mark_our_ref directly as a callback to for_each_ref. Its\nmarking does the right thing, but when it then returns \"1\"\nto for_each_ref, the latter interprets this as an error and\nstops iterating. As a result, we skip marking all of the\nrefs that come lexicographically after it. Any \"want\" lines\nfrom the client asking for those objects will fail, as they\nwere not properly marked with OUR_REF.\n\nTo solve this, we introduce a wrapper callback around\nmark_our_ref which always returns 0 (even if the ref is\nhidden, we want to keep iterating). We also tweak the\nsignature of mark_our_ref to exclude unnecessary parameters\nthat were present only to conform to the callback interface.\nThis should make it less likely for somebody to accidentally\nuse it as a callback in the future.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5551-http-fetch-smart.sh | 11 +++++++++++\n upload-pack.c               | 14 ++++++++++----\n 2 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 6cbc12d..b970773 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -213,6 +213,17 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set\n \ttest_cmp expect_cookies.txt cookies_tail.txt\n '\n \n+test_expect_success 'transfer.hiderefs works over smart-http' '\n+\ttest_commit hidden &&\n+\ttest_commit visible &&\n+\tgit push public HEAD^:refs/heads/a HEAD:refs/heads/b &&\n+\tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n+\t\tconfig transfer.hiderefs refs/heads/a &&\n+\tgit clone --bare \"$HTTPD_URL/smart/repo.git\" hidden.git &&\n+\ttest_must_fail git -C hidden.git rev-parse --verify a &&\n+\tgit -C hidden.git rev-parse --verify b\n+'\n+\n test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n \t(\n \tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\ndiff --git a/upload-pack.c b/upload-pack.c\nindex b531a32..c8e8713 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -681,7 +681,7 @@ static void receive_needs(void)\n }\n \n /* return non-zero if the ref is hidden, otherwise 0 */\n-static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n+static int mark_our_ref(const char *refname, const unsigned char *sha1)\n {\n \tstruct object *o = lookup_unknown_object(sha1);\n \n@@ -695,6 +695,12 @@ static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag\n \treturn 0;\n }\n \n+static int check_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n+{\n+\tmark_our_ref(refname, sha1);\n+\treturn 0;\n+}\n+\n static void format_symref_info(struct strbuf *buf, struct string_list *symref)\n {\n \tstruct string_list_item *item;\n@@ -713,7 +719,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n \tconst char *refname_nons = strip_namespace(refname);\n \tunsigned char peeled[20];\n \n-\tif (mark_our_ref(refname, sha1, flag, NULL))\n+\tif (mark_our_ref(refname, sha1))\n \t\treturn 0;\n \n \tif (capabilities) {\n@@ -767,8 +773,8 @@ static void upload_pack(void)\n \t\tadvertise_shallow_grafts(1);\n \t\tpacket_flush(1);\n \t} else {\n-\t\thead_ref_namespaced(mark_our_ref, NULL);\n-\t\tfor_each_namespaced_ref(mark_our_ref, NULL);\n+\t\thead_ref_namespaced(check_ref, NULL);\n+\t\tfor_each_namespaced_ref(check_ref, NULL);\n \t}\n \tstring_list_clear(&symref, 1);\n \tif (advertise_refs)\n-- \n2.3.2.472.geadab3c\n"},{"id":"257622","messageId":"20150313044224.GB18532@peff.net","threadId":"38791","inReplyTo":"20150313044101.GA18476@peff.net","subject":"[PATCH 2/7] upload-pack: do not check NULL return of lookup_unknown_object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-13T04:42:25Z","receivedAt":"2015-03-13T04:42:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We check whether the return value of lookup_unknown_object\nis NULL, but some code paths dereference it before our\ncheck. This turns out not to be capable of causing a\nsegfault, though. The lookup_unknown_object function will\nnever return NULL, since the whole point is to allocate an\nobject struct if it does not find an existing one. So the\ncode here is not wrong, it is just confusing. Let's just\ndrop the NULL check.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n upload-pack.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c8e8713..aa84576 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -689,8 +689,6 @@ static int mark_our_ref(const char *refname, const unsigned char *sha1)\n \t\to->flags |= HIDDEN_REF;\n \t\treturn 1;\n \t}\n-\tif (!o)\n-\t\tdie(\"git upload-pack: cannot find object %s:\", sha1_to_hex(sha1));\n \to->flags |= OUR_REF;\n \treturn 0;\n }\n-- \n2.3.2.472.geadab3c\n"},{"id":"257623","messageId":"20150313044848.GC18532@peff.net","threadId":"38791","inReplyTo":"20150313044101.GA18476@peff.net","subject":"[PATCH 3/7] t: translate SIGINT to an exit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-13T04:48:48Z","receivedAt":"2015-03-13T04:48:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Right now if a test script receives SIGINT (e.g., because a\ntest was hanging and the user hit ^C), the shell exits\nimmediately. This can be annoying if the test script did any\nglobal setup, like starting apache or git-daemon, as it will\nnot have an opportunity to clean up after itself. A\nsubsequent run of the test won't be able to start its own\ndaemon, and will either fail or skip the tests.\n\nInstead, let's trap SIGINT to make sure we do a clean\nshutdown, and just chain it to a normal exit (which will\ntrigger any cleanup).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nPossibly we should catch other signals here, too, but SIGINT is the one\nthat has been biting me over the years (I would sometimes have a stray\napache process hanging around, and I only recently figured out what\ncaused it).\n\nI think \"trap ... INT\" is the most portable way to say this. I would\nhave thought numbers were more portable, but they are actually an XSI\nadd-on. Likewise, \"SIGINT\" is allowed in POSIX but not required. \"INT\"\nis part of POSIX.\n\nThis is the cleanest way I could come up with to implement this. We\ncould also just include \"INT\" on the same line as the die, but:\n\n  1. Then every script which sets a handler (like lib-httpd.sh) would\n     need to be modified to munge the INT trap, too.\n\n  2. It double-calls die, then. We die from the INT handler, which\n     triggers the EXIT handler. I guess we could clear the handler\n     inside die() if we needed to.\n\nSo I'd rather go this route, unless there turns out to be a weird\nportability problem.\n\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex c096778..f4ba3ff 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -299,6 +299,7 @@ die () {\n \n GIT_EXIT_OK=\n trap 'die' EXIT\n+trap 'exit $?' INT\n \n # The user-facing functions are loaded from a separate file so that\n # test_perf subshells can have them too\n-- \n2.3.2.472.geadab3c\n"},{"id":"257624","messageId":"20150313045056.GD18532@peff.net","threadId":"38791","inReplyTo":"20150313044101.GA18476@peff.net","subject":"[PATCH 4/7] t: redirect stderr GIT_TRACE to descriptor 4","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-13T04:50:56Z","receivedAt":"2015-03-13T04:50:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you run a test script like:\n\n  GIT_TRACE=1 ./t0061-run-command.sh\n\nyou may get test failures, because some tests capture and\ncheck the stderr output from git commands (and with\nGIT_TRACE set to 1, the trace output will be included\nthere).\n\nWhen we see GIT_TRACE set like this, we print a warning to\nthe user. However, we can do even better than that by just\npointing it to descriptor 4, which all tests leave connected\nto the test script's stderr. That's likely what the user\nintended (and any scripts that do want to see GIT_TRACE\noutput will set GIT_TRACE themselves).\n\nNot only does this avoid false negatives in the tests, but\nit means the user will actually see trace output for git\ncalls that redirect their stderr (whereas before, it was\nsometimes confusingly buried in a file).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI only today figured out the GIT_TRACE=4 trick. Perhaps everybody else\ndid long ago, and I am just slow. :)\n\nMy first inclination was to mention it in the warning message below, but\nI cannot see a downside to automatically redirecting it. Even outside of\ntest_expect_* we have the \"4>&2\" redirection, so we should hit even\nbadly written scripts which call git outside of that environment.\n\n t/test-lib.sh | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex f4ba3ff..7dd4b4d 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -152,10 +152,7 @@ unset UNZIP\n \n case $(echo $GIT_TRACE |tr \"[A-Z]\" \"[a-z]\") in\n 1|2|true)\n-\techo \"* warning: Some tests will not work if GIT_TRACE\" \\\n-\t\t\"is set as to trace on STDERR ! *\"\n-\techo \"* warning: Please set GIT_TRACE to something\" \\\n-\t\t\"other than 1, 2 or true ! *\"\n+\tGIT_TRACE=4\n \t;;\n esac\n \n-- \n2.3.2.472.geadab3c\n"},{"id":"257625","messageId":"20150313045114.GE18532@peff.net","threadId":"38791","inReplyTo":"20150313044101.GA18476@peff.net","subject":"[PATCH 5/7] t: pass GIT_TRACE through Apache","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-13T04:51:15Z","receivedAt":"2015-03-13T04:51:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Apache removes GIT_TRACE from the environment before running\ngit-http-backend. This can make it hard to debug the server\nside of an http session. Let's let it through.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/lib-httpd.sh          | 1 +\n t/lib-httpd/apache.conf | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex d154d1e..e6adf2f 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -79,6 +79,7 @@ HTTPD_DOCUMENT_ROOT_PATH=$HTTPD_ROOT_PATH/www\n # hack to suppress apache PassEnv warnings\n GIT_VALGRIND=$GIT_VALGRIND; export GIT_VALGRIND\n GIT_VALGRIND_OPTIONS=$GIT_VALGRIND_OPTIONS; export GIT_VALGRIND_OPTIONS\n+GIT_TRACE=$GIT_TRACE; export GIT_TRACE\n \n if ! test -x \"$LIB_HTTPD_PATH\"\n then\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 03a4c2e..0b81a00 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -70,6 +70,7 @@ PassEnv GIT_VALGRIND\n PassEnv GIT_VALGRIND_OPTIONS\n PassEnv GNUPGHOME\n PassEnv ASAN_OPTIONS\n+PassEnv GIT_TRACE\n \n Alias /dumb/ www/\n Alias /auth/dumb/ www/auth/dumb/\n-- \n2.3.2.472.geadab3c\n"},{"id":"257626","messageId":"20150313045307.GF18532@peff.net","threadId":"38791","inReplyTo":"20150313044101.GA18476@peff.net","subject":"[PATCH 6/7] t5541: move run_with_cmdline_limit to test-lib.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-13T04:53:07Z","receivedAt":"2015-03-13T04:53:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use this to test http pushing with a restricted\ncommandline. Other scripts (like t5551, which does http\nfetching) will want to use it, too.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAs we discussed a while ago, this is the exact same code that\nrun_with_limited_stack uses in t7004. However, I think they are\nconceptually two different things, and I could imagine a platform where\nthey have distinct implementations. So I did not refactor t7004 to make\nuse of this.\n\n t/t5541-http-push-smart.sh | 6 ------\n t/test-lib.sh              | 6 ++++++\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex d2c681e..1ecb588 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -324,12 +324,6 @@ test_expect_success 'push into half-auth-complete requires password' '\n \ttest_cmp expect actual\n '\n \n-run_with_limited_cmdline () {\n-\t(ulimit -s 128 && \"$@\")\n-}\n-\n-test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'\n-\n test_expect_success CMDLINE_LIMIT 'push 2000 tags over http' '\n \tsha1=$(git rev-parse HEAD) &&\n \ttest_seq 2000 |\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 7dd4b4d..9914d3e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1062,3 +1062,9 @@ test_lazy_prereq UNZIP '\n \t\"$GIT_UNZIP\" -v\n \ttest $? -ne 127\n '\n+\n+run_with_limited_cmdline () {\n+\t(ulimit -s 128 && \"$@\")\n+}\n+\n+test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'\n-- \n2.3.2.472.geadab3c\n"},{"id":"257627","messageId":"20150313045705.GA18807@peff.net","threadId":"38791","inReplyTo":"20150313044101.GA18476@peff.net","subject":"[PATCH 7/7] t5551: make EXPENSIVE test cheaper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-13T04:57:05Z","receivedAt":"2015-03-13T04:57:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We create 50,000 tags to check that we don't overflow the\ncommand-line of fetch-pack. But by using run_with_cmdline_limit,\nwe can get the same effect with a much smaller number of\ntags. This makes the test fast enough that we can drop the\nEXPENSIVE prereq, which means people will actually run it.\n\nIt was not documented to do so, but this test was also the\nonly test of a clone-over-http that requires multiple POSTs\nduring the conversation. We can continue to test that by\ndropping http.postbuffer to its minimum size, and checking\nthat we get two POSTs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is still a slightly expensive test, but it's on par with what the\npush test does, so hopefully it's OK. And it bothers me that we didn't\nexercise the multiple-POST code at all in the test suite, as I assume\nbasically nobody runs the tests with \"--long\".\n\nI don't think we can shrink it any more. We enforce a hard minimum of\nLARGE_PACKET_MAX on http.postbuffer, and that assumption runs through\nthe code. Lots of things break if you try to circumvent it, and I didn't\nbother digging further.\n\n t/t5551-http-fetch-smart.sh | 19 ++++++++++++++-----\n 1 file changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex b970773..df47851 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -224,10 +224,10 @@ test_expect_success 'transfer.hiderefs works over smart-http' '\n \tgit -C hidden.git rev-parse --verify b\n '\n \n-test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n+test_expect_success 'create 2,000 tags in the repo' '\n \t(\n \tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n-\tfor i in `test_seq 50000`\n+\tfor i in $(test_seq 2000)\n \tdo\n \t\techo \"commit refs/heads/too-many-refs\"\n \t\techo \"mark :$i\"\n@@ -248,13 +248,22 @@ test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n \t)\n '\n \n-test_expect_success EXPENSIVE 'clone the 50,000 tag repo to check OS command line overflow' '\n-\tgit clone $HTTPD_URL/smart/repo.git too-many-refs &&\n+test_expect_success CMDLINE_LIMIT \\\n+\t'clone the 2,000 tag repo to check OS command line overflow' '\n+\trun_with_limited_cmdline git clone $HTTPD_URL/smart/repo.git too-many-refs &&\n \t(\n \t\tcd too-many-refs &&\n-\t\ttest $(git for-each-ref refs/tags | wc -l) = 50000\n+\t\tgit for-each-ref refs/tags >actual &&\n+\t\ttest_line_count = 2000 actual\n \t)\n '\n \n+test_expect_success 'large fetch-pack requests can be split across POSTs' '\n+\tGIT_CURL_VERBOSE=1 git -c http.postbuffer=65536 \\\n+\t\tclone --bare \"$HTTPD_URL/smart/repo.git\" split.git 2>err &&\n+\tgrep \"^> POST\" err >posts &&\n+\ttest_line_count = 2 posts\n+'\n+\n stop_httpd\n test_done\n-- \n2.3.2.472.geadab3c\n"},{"id":"257628","messageId":"20150313045952.GA18853@peff.net","threadId":"38791","inReplyTo":"20150313044101.GA18476@peff.net","subject":"Re: [PATCH 0/7] fix transfer.hiderefs with smart http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-13T04:59:52Z","receivedAt":"2015-03-13T04:59:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 13, 2015 at 12:41:01AM -0400, Jeff King wrote:\n\n> I'm experimenting with using transfer.hiderefs on a server, and it's\n> rather easy to cause a git client to hang when fetching from such a repo\n> over smart http. Details are in the first patch.\n\nA note on this hang. What happens is that upload-pack sees a bogus\n\"want\" line and calls die(). The client then sits there forever, but I'm\nnot exactly sure what it's waiting for.\n\nThis series fixes the bug that caused us to erroneously call die() in\nupload-pack, so the hang is \"fixed\". But there are other reasons to call\ndie(); it would probably be a good thing if the client side noticed the\nproblem and aborted.\n\n-Peff\n"},{"id":"257629","messageId":"CACsJy8B+dWoq+ide2_-zJuLqFzg2WM3XuyL2B77PrR=PdrBfeQ@mail.gmail.com","threadId":"38791","inReplyTo":"20150313045952.GA18853@peff.net","subject":"Re: [PATCH 0/7] fix transfer.hiderefs with smart http","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-13T05:21:11Z","receivedAt":"2015-03-13T05:21:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 13, 2015 at 11:59 AM, Jeff King <peff@peff.net> wrote:\n> On Fri, Mar 13, 2015 at 12:41:01AM -0400, Jeff King wrote:\n>\n>> I'm experimenting with using transfer.hiderefs on a server, and it's\n>> rather easy to cause a git client to hang when fetching from such a repo\n>> over smart http. Details are in the first patch.\n>\n> A note on this hang. What happens is that upload-pack sees a bogus\n> \"want\" line and calls die(). The client then sits there forever, but I'm\n> not exactly sure what it's waiting for.\n>\n> This series fixes the bug that caused us to erroneously call die() in\n> upload-pack, so the hang is \"fixed\". But there are other reasons to call\n> die(); it would probably be a good thing if the client side noticed the\n> problem and aborted.\n\nMaybe we could install a custom die handler that also sends \"ERR\" line\nto the client before dying. Even with old clients where ERR lines are\nnot recognized, they would see that as a sign of error and abort. The\nonly thing to be careful is not sending ERR while we're in the middle\nof sending some pkt-line, and that only happens when die() is called\ninside packet_write() and we can catch that easily. This is for\nupload-pack only as the client side can also use packet_buf_write(), a\nbit harder to know if some pkt-line is being sent.\n-- \nDuy\n"},{"id":"257633","messageId":"xmqqioe5zacq.fsf@gitster.dls.corp.google.com","threadId":"38791","inReplyTo":"20150313044212.GA18532@peff.net","subject":"Re: [PATCH 1/7] upload-pack: fix transfer.hiderefs over smart-http","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-13T06:21:09Z","receivedAt":"2015-03-13T06:21:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> When upload-pack advertises the refs (either for a normal,\n> non-stateless request, or for the initial contact in a\n> stateless one), we call for_each_ref with the send_ref\n> function as its callback. send_ref, in turn, calls\n> mark_our_ref, which checks whether the ref is hidden, and\n> sets OUR_REF or HIDDEN_REF on the object as appropriate.  If\n> it is hidden, mark_our_ref also returns \"1\" to signal\n> send_ref that the ref should not be advertised.\n>\n> If we are not advertising refs, (i.e., the follow-up\n> invocation by an http client to send its \"want\" lines), we\n> use mark_our_ref directly as a callback to for_each_ref. Its\n> marking does the right thing, but when it then returns \"1\"\n> to for_each_ref, the latter interprets this as an error and\n> stops iterating. As a result, we skip marking all of the\n> refs that come lexicographically after it. Any \"want\" lines\n> from the client asking for those objects will fail, as they\n> were not properly marked with OUR_REF.\n\nNicely described in a way that the reason of the breakage and the\nfix is obvious to those who already know what the codepath is\nsupposed to be doing.\n\n> To solve this, we introduce a wrapper callback around\n> mark_our_ref which always returns 0 (even if the ref is\n> hidden, we want to keep iterating). We also tweak the\n> signature of mark_our_ref to exclude unnecessary parameters\n> that were present only to conform to the callback interface.\n> This should make it less likely for somebody to accidentally\n> use it as a callback in the future.\n\nI especially love this kind of future-proofing ;-).\n\nThanks, will queue.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t5551-http-fetch-smart.sh | 11 +++++++++++\n>  upload-pack.c               | 14 ++++++++++----\n>  2 files changed, 21 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index 6cbc12d..b970773 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -213,6 +213,17 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set\n>  \ttest_cmp expect_cookies.txt cookies_tail.txt\n>  '\n>  \n> +test_expect_success 'transfer.hiderefs works over smart-http' '\n> +\ttest_commit hidden &&\n> +\ttest_commit visible &&\n> +\tgit push public HEAD^:refs/heads/a HEAD:refs/heads/b &&\n> +\tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n> +\t\tconfig transfer.hiderefs refs/heads/a &&\n> +\tgit clone --bare \"$HTTPD_URL/smart/repo.git\" hidden.git &&\n> +\ttest_must_fail git -C hidden.git rev-parse --verify a &&\n> +\tgit -C hidden.git rev-parse --verify b\n> +'\n> +\n>  test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n>  \t(\n>  \tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> diff --git a/upload-pack.c b/upload-pack.c\n> index b531a32..c8e8713 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -681,7 +681,7 @@ static void receive_needs(void)\n>  }\n>  \n>  /* return non-zero if the ref is hidden, otherwise 0 */\n> -static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n> +static int mark_our_ref(const char *refname, const unsigned char *sha1)\n>  {\n>  \tstruct object *o = lookup_unknown_object(sha1);\n>  \n> @@ -695,6 +695,12 @@ static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag\n>  \treturn 0;\n>  }\n>  \n> +static int check_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n> +{\n> +\tmark_our_ref(refname, sha1);\n> +\treturn 0;\n> +}\n> +\n>  static void format_symref_info(struct strbuf *buf, struct string_list *symref)\n>  {\n>  \tstruct string_list_item *item;\n> @@ -713,7 +719,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n>  \tconst char *refname_nons = strip_namespace(refname);\n>  \tunsigned char peeled[20];\n>  \n> -\tif (mark_our_ref(refname, sha1, flag, NULL))\n> +\tif (mark_our_ref(refname, sha1))\n>  \t\treturn 0;\n>  \n>  \tif (capabilities) {\n> @@ -767,8 +773,8 @@ static void upload_pack(void)\n>  \t\tadvertise_shallow_grafts(1);\n>  \t\tpacket_flush(1);\n>  \t} else {\n> -\t\thead_ref_namespaced(mark_our_ref, NULL);\n> -\t\tfor_each_namespaced_ref(mark_our_ref, NULL);\n> +\t\thead_ref_namespaced(check_ref, NULL);\n> +\t\tfor_each_namespaced_ref(check_ref, NULL);\n>  \t}\n>  \tstring_list_clear(&symref, 1);\n>  \tif (advertise_refs)\n"},{"id":"257635","messageId":"CAPig+cQg8yhSAVi5w1w967iyTxYEgXBnSYw7wFoV0GqTk9KP+A@mail.gmail.com","threadId":"38791","inReplyTo":"20150313045307.GF18532@peff.net","subject":"Re: [PATCH 6/7] t5541: move run_with_cmdline_limit to test-lib.sh","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-13T06:45:59Z","receivedAt":"2015-03-13T06:45:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 13, 2015 at 12:53 AM, Jeff King <peff@peff.net> wrote:\n> We use this to test http pushing with a restricted\n> commandline. Other scripts (like t5551, which does http\n> fetching) will want to use it, too.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> As we discussed a while ago, this is the exact same code that\n> run_with_limited_stack uses in t7004. However, I think they are\n> conceptually two different things, and I could imagine a platform where\n> they have distinct implementations. So I did not refactor t7004 to make\n> use of this.\n\nPerhaps this commentary should be moved to the commit message so that\nthe next person who notices that run_with_limited_stack() is the same\nwill understand why it was left alone.\n\n>  t/t5541-http-push-smart.sh | 6 ------\n>  t/test-lib.sh              | 6 ++++++\n>  2 files changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\n> index d2c681e..1ecb588 100755\n> --- a/t/t5541-http-push-smart.sh\n> +++ b/t/t5541-http-push-smart.sh\n> @@ -324,12 +324,6 @@ test_expect_success 'push into half-auth-complete requires password' '\n>         test_cmp expect actual\n>  '\n>\n> -run_with_limited_cmdline () {\n> -       (ulimit -s 128 && \"$@\")\n> -}\n> -\n> -test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'\n> -\n>  test_expect_success CMDLINE_LIMIT 'push 2000 tags over http' '\n>         sha1=$(git rev-parse HEAD) &&\n>         test_seq 2000 |\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 7dd4b4d..9914d3e 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1062,3 +1062,9 @@ test_lazy_prereq UNZIP '\n>         \"$GIT_UNZIP\" -v\n>         test $? -ne 127\n>  '\n> +\n> +run_with_limited_cmdline () {\n> +       (ulimit -s 128 && \"$@\")\n> +}\n> +\n> +test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'\n> --\n> 2.3.2.472.geadab3c\n"}]}