{"thread":{"id":"40697","subject":"[PATCH 2/4] upload-pack: strip refs before calling ref_is_hidden()","startedAt":"2015-11-01T19:34:19Z","lastAt":"2015-11-02T07:30:51Z","messageCount":10,"participants":["Lukas Fleischer","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"272697","messageId":"1446406463-22527-1-git-send-email-lfleischer@lfos.de","threadId":"40697","inReplyTo":null,"subject":"[PATCH 0/4] Improve hideRefs when used with namespaces","fromName":"Lukas Fleischer","fromEmail":"lfleischer@lfos.de","sentAt":"2015-11-01T19:34:19Z","receivedAt":"2015-11-01T19:34:19Z","isPatch":true,"sender":{"key":"lfleischer@lfos.de","avatar":"https://avatars.githubusercontent.com/u/5530842?v=4"},"body":"This is a first set of patches improving documentation and behavior of\nthe transfer.hideRefs feature as discussed in [1]. In particular,\nhideRefs is changed to generally match stripped refs by default and\nmatch full refs when prefixed with \"^\". The documentation is updated\naccordingly. Basic tests are added.\n\n[1] http://marc.info/?l=git&m=144604694223920\n\nLukas Fleischer (4):\n Document the semantics of hideRefs with namespaces\n upload-pack: strip refs before calling ref_is_hidden()\n Add support for matching full refs in hideRefs\n t5509: add basic tests for hideRefs\n\n Documentation/config.txt | 8 ++++++++\n builtin/receive-pack.c | 22 ++++++++++++++++------\n refs.c | 14 +++++++++++---\n refs.h | 2 +-\n t/t5509-fetch-push-namespaces.sh | 29 +++++++++++++++++++++++++++++\n upload-pack.c | 13 ++++++++-----\n 6 files changed, 73 insertions(+), 15 deletions(-)\n\n-- \n2.6.2\n"},{"id":"272696","messageId":"1446406463-22527-2-git-send-email-lfleischer@lfos.de","threadId":"40697","inReplyTo":"1446406463-22527-1-git-send-email-lfleischer@lfos.de","subject":"[PATCH 1/4] Document the semantics of hideRefs with namespaces","fromName":"Lukas Fleischer","fromEmail":"lfleischer@lfos.de","sentAt":"2015-11-01T19:34:20Z","receivedAt":"2015-11-01T19:34:20Z","isPatch":true,"sender":{"key":"lfleischer@lfos.de","avatar":"https://avatars.githubusercontent.com/u/5530842?v=4"},"body":"Right now, there is no clear definition of how transfer.hideRefs should\nbehave when a namespace is set. Explain that hideRefs prefixes match\nstripped names in that case. This is how hideRefs patterns are currently\nhandled in receive-pack.\n\nSigned-off-by: Lukas Fleischer \n---\n Documentation/config.txt | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1204072..3da97a1 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2684,6 +2684,13 @@ You may also include a `!` in front of the ref name to negate the entry,\n explicitly exposing it, even if an earlier entry marked it as hidden.\n If you have multiple hideRefs values, later entries override earlier ones\n (and entries in more-specific config files override less-specific ones).\n++\n+If a namespace is set, references are stripped before matching. For example, if\n+the prefix `refs/heads/master` is specified in `transfer.hideRefs` and the\n+current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master` is\n+omitted from the advertisements but `refs/heads/master` and\n+`refs/namespaces/bar/refs/heads/master` are still advertised as so-called\n+\"have\" lines.\n \n transfer.unpackLimit::\n \tWhen `fetch.unpackLimit` or `receive.unpackLimit` are\n-- \n2.6.2\n"},{"id":"272695","messageId":"1446406463-22527-3-git-send-email-lfleischer@lfos.de","threadId":"40697","inReplyTo":"1446406463-22527-1-git-send-email-lfleischer@lfos.de","subject":"[PATCH 2/4] upload-pack: strip refs before calling ref_is_hidden()","fromName":"Lukas Fleischer","fromEmail":"lfleischer@lfos.de","sentAt":"2015-11-01T19:34:21Z","receivedAt":"2015-11-01T19:34:21Z","isPatch":true,"sender":{"key":"lfleischer@lfos.de","avatar":"https://avatars.githubusercontent.com/u/5530842?v=4"},"body":"Make hideRefs handling in upload-pack consistent with the behavior\ndescribed in the documentation by stripping refs before comparing them\nwith prefixes in hideRefs.\n\nSigned-off-by: Lukas Fleischer \n---\n upload-pack.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex d0bc3ca..4ca960e 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -692,7 +692,7 @@ static int mark_our_ref(const char *refname, const struct object_id *oid)\n {\n \tstruct object *o = lookup_unknown_object(oid->hash);\n \n-\tif (ref_is_hidden(refname)) {\n+\tif (refname && ref_is_hidden(refname)) {\n \t\to->flags |= HIDDEN_REF;\n \t\treturn 1;\n \t}\n@@ -703,7 +703,7 @@ static int mark_our_ref(const char *refname, const struct object_id *oid)\n static int check_ref(const char *refname, const struct object_id *oid,\n \t\t int flag, void *cb_data)\n {\n-\tmark_our_ref(refname, oid);\n+\tmark_our_ref(strip_namespace(refname), oid);\n \treturn 0;\n }\n \n@@ -726,7 +726,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tconst char *refname_nons = strip_namespace(refname);\n \tstruct object_id peeled;\n \n-\tif (mark_our_ref(refname, oid))\n+\tif (mark_our_ref(refname_nons, oid))\n \t\treturn 0;\n \n \tif (capabilities) {\n-- \n2.6.2\n"},{"id":"272698","messageId":"1446406463-22527-4-git-send-email-lfleischer@lfos.de","threadId":"40697","inReplyTo":"1446406463-22527-1-git-send-email-lfleischer@lfos.de","subject":"[PATCH 3/4] Add support for matching full refs in hideRefs","fromName":"Lukas Fleischer","fromEmail":"lfleischer@lfos.de","sentAt":"2015-11-01T19:34:22Z","receivedAt":"2015-11-01T19:34:22Z","isPatch":true,"sender":{"key":"lfleischer@lfos.de","avatar":"https://avatars.githubusercontent.com/u/5530842?v=4"},"body":"In addition to matching stripped refs, one can now add hideRefs patterns\nthat the full (unstripped) ref is matched against. To distinguish\nbetween stripped and full matches, those new patterns must be prefixed\nwith a circumflex (^).\n\nThis commit also removes support for the undocumented and unintended\nhideRefs settings \"have\" (suppressing all \"have\" lines) and\n\"capabilities^{}\" (suppressing the capabilities line).\n\nSigned-off-by: Lukas Fleischer \n---\n Documentation/config.txt | 3 ++-\n builtin/receive-pack.c | 22 ++++++++++++++++------\n refs.c | 14 +++++++++++---\n refs.h | 2 +-\n upload-pack.c | 13 ++++++++-----\n 5 files changed, 38 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3da97a1..91ed6a5 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2690,7 +2690,8 @@ the prefix `refs/heads/master` is specified in `transfer.hideRefs` and the\n current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master` is\n omitted from the advertisements but `refs/heads/master` and\n `refs/namespaces/bar/refs/heads/master` are still advertised as so-called\n-\"have\" lines.\n+\"have\" lines. In order to match refs before stripping, add a `^` in front of\n+the ref name. If you combine `!` and `^`, `!` must be specified first.\n \n transfer.unpackLimit::\n \tWhen `fetch.unpackLimit` or `receive.unpackLimit` are\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex bcb624b..d5e58e0 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -195,9 +195,6 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \n static void show_ref(const char *path, const unsigned char *sha1)\n {\n-\tif (ref_is_hidden(path))\n-\t\treturn;\n-\n \tif (sent_capabilities) {\n \t\tpacket_write(1, \"%s %s\\n\", sha1_to_hex(sha1), path);\n \t} else {\n@@ -219,9 +216,14 @@ static void show_ref(const char *path, const unsigned char *sha1)\n \t}\n }\n \n-static int show_ref_cb(const char *path, const struct object_id *oid, int flag, void *unused)\n+static int show_ref_cb(const char *path_full, const struct object_id *oid,\n+\t\t int flag, void *unused)\n {\n-\tpath = strip_namespace(path);\n+\tconst char *path = strip_namespace(path_full);\n+\n+\tif (ref_is_hidden(path, path_full))\n+\t\treturn 1;\n+\n \t/*\n \t * Advertise refs outside our current namespace as \".have\"\n \t * refs, so that the client can use them to minimize data\n@@ -1198,7 +1200,15 @@ static void reject_updates_to_hidden(struct command *commands)\n \tstruct command *cmd;\n \n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (cmd->error_string || !ref_is_hidden(cmd->ref_name))\n+\t\tconst char *refname = cmd->ref_name;\n+\t\tstruct strbuf refname_full_buf = STRBUF_INIT;\n+\t\tconst char *refname_full;\n+\n+\t\tstrbuf_addf(&refname_full_buf, \"%s%s\", get_git_namespace(),\n+\t\t\t\trefname);\n+\t\trefname_full = strbuf_detach(&refname_full_buf, NULL);\n+\n+\t\tif (cmd->error_string || !ref_is_hidden(refname, refname_full))\n \t\t\tcontinue;\n \t\tif (is_null_sha1(cmd->new_sha1))\n \t\t\tcmd->error_string = \"deny deleting a hidden ref\";\ndiff --git a/refs.c b/refs.c\nindex 72d96ed..555c9d5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -321,7 +321,7 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti\n \treturn 0;\n }\n \n-int ref_is_hidden(const char *refname)\n+int ref_is_hidden(const char *refname, const char *refname_full)\n {\n \tint i;\n \n@@ -329,6 +329,7 @@ int ref_is_hidden(const char *refname)\n \t\treturn 0;\n \tfor (i = hide_refs->nr - 1; i >= 0; i--) {\n \t\tconst char *match = hide_refs->items[i].string;\n+\t\tconst char *subject;\n \t\tint neg = 0;\n \t\tint len;\n \n@@ -337,10 +338,17 @@ int ref_is_hidden(const char *refname)\n \t\t\tmatch++;\n \t\t}\n \n-\t\tif (!starts_with(refname, match))\n+\t\tif (*match == '^') {\n+\t\t\tsubject = refname_full;\n+\t\t\tmatch++;\n+\t\t} else {\n+\t\t\tsubject = refname;\n+\t\t}\n+\n+\t\tif (!subject || !starts_with(subject, match))\n \t\t\tcontinue;\n \t\tlen = strlen(match);\n-\t\tif (!refname[len] || refname[len] == '/')\n+\t\tif (!subject[len] || subject[len] == '/')\n \t\t\treturn !neg;\n \t}\n \treturn 0;\ndiff --git a/refs.h b/refs.h\nindex 69fa4df..fbf8e59 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -604,7 +604,7 @@ int update_ref(const char *msg, const char *refname,\n \n extern int parse_hide_refs_config(const char *var, const char *value, const char *);\n \n-extern int ref_is_hidden(const char *);\n+extern int ref_is_hidden(const char *, const char *);\n \n enum ref_type {\n \tREF_TYPE_PER_WORKTREE,\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 4ca960e..08efb1d 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -688,11 +688,12 @@ 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 struct object_id *oid)\n+static int mark_our_ref(const char *refname, const char *refname_full,\n+\t\t\tconst struct object_id *oid)\n {\n \tstruct object *o = lookup_unknown_object(oid->hash);\n \n-\tif (refname && ref_is_hidden(refname)) {\n+\tif (ref_is_hidden(refname, refname_full)) {\n \t\to->flags |= HIDDEN_REF;\n \t\treturn 1;\n \t}\n@@ -700,10 +701,12 @@ static int mark_our_ref(const char *refname, const struct object_id *oid)\n \treturn 0;\n }\n \n-static int check_ref(const char *refname, const struct object_id *oid,\n+static int check_ref(const char *refname_full, const struct object_id *oid,\n \t\t int flag, void *cb_data)\n {\n-\tmark_our_ref(strip_namespace(refname), oid);\n+\tconst char *refname = strip_namespace(refname_full);\n+\n+\tmark_our_ref(refname, refname_full, oid);\n \treturn 0;\n }\n \n@@ -726,7 +729,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tconst char *refname_nons = strip_namespace(refname);\n \tstruct object_id peeled;\n \n-\tif (mark_our_ref(refname_nons, oid))\n+\tif (mark_our_ref(refname_nons, refname, oid))\n \t\treturn 0;\n \n \tif (capabilities) {\n-- \n2.6.2\n"},{"id":"272699","messageId":"1446406463-22527-5-git-send-email-lfleischer@lfos.de","threadId":"40697","inReplyTo":"1446406463-22527-1-git-send-email-lfleischer@lfos.de","subject":"[PATCH 4/4] t5509: add basic tests for hideRefs","fromName":"Lukas Fleischer","fromEmail":"lfleischer@lfos.de","sentAt":"2015-11-01T19:34:23Z","receivedAt":"2015-11-01T19:34:23Z","isPatch":true,"sender":{"key":"lfleischer@lfos.de","avatar":"https://avatars.githubusercontent.com/u/5530842?v=4"},"body":"Test whether regular and full hideRefs patterns work as expected when\nnamespaces are used.\n\nSigned-off-by: Lukas Fleischer \n---\n t/t5509-fetch-push-namespaces.sh | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex cc0b31f..a3f1060 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -82,4 +82,33 @@ test_expect_success 'mirroring a repository using a ref namespace' '\n \t)\n '\n \n+test_expect_success \"Hide namespaced refs with transfer.hideRefs\" '\n+\tcd pushee &&\n+\ttest_config transfer.hideRefs refs/tags &&\n+\tGIT_NAMESPACE=namespace git ls-remote \"ext::git %s .\" >actual &&\n+\tprintf \"$commit1\\trefs/heads/master\\n\" >expected &&\n+\ttest_cmp expected actual &&\n+\tcd ..\n+'\n+\n+test_expect_success \"Check that transfer.hideRefs does not match unstripped refs\" '\n+\tcd pushee &&\n+\ttest_config transfer.hideRefs \"refs/namespaces/namespace/refs/tags\" &&\n+\tGIT_NAMESPACE=namespace git ls-remote \"ext::git %s .\" >actual &&\n+\tprintf \"$commit1\\trefs/heads/master\\n\" >expected &&\n+\tprintf \"$commit0\\trefs/tags/0\\n\" >>expected &&\n+\tprintf \"$commit1\\trefs/tags/1\\n\" >>expected &&\n+\ttest_cmp expected actual &&\n+\tcd ..\n+'\n+\n+test_expect_success \"Hide full refs with transfer.hideRefs\" '\n+\tcd pushee &&\n+\ttest_config transfer.hideRefs \"^refs/namespaces/namespace/refs/tags\" &&\n+\tGIT_NAMESPACE=namespace git ls-remote \"ext::git %s .\" >actual &&\n+\tprintf \"$commit1\\trefs/heads/master\\n\" >expected &&\n+\ttest_cmp expected actual &&\n+\tcd ..\n+'\n+\n test_done\n-- \n2.6.2\n"},{"id":"272705","messageId":"CAPig+cST7sATT4W4Kwp5K=DcuvoFkdgsTDA9iOk=8c_8_GE=-Q@mail.gmail.com","threadId":"40697","inReplyTo":"1446406463-22527-4-git-send-email-lfleischer@lfos.de","subject":"Re: [PATCH 3/4] Add support for matching full refs in hideRefs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-01T20:42:37Z","receivedAt":"2015-11-01T20:42:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 1, 2015 at 2:34 PM, Lukas Fleischer wrote:\n> In addition to matching stripped refs, one can now add hideRefs patterns\n> that the full (unstripped) ref is matched against. To distinguish\n> between stripped and full matches, those new patterns must be prefixed\n> with a circumflex (^).\n>\n> This commit also removes support for the undocumented and unintended\n> hideRefs settings \"have\" (suppressing all \"have\" lines) and\n> \"capabilities^{}\" (suppressing the capabilities line).\n>\n> Signed-off-by: Lukas Fleischer \n> ---\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> @@ -1198,7 +1200,15 @@ static void reject_updates_to_hidden(struct command *commands)\n> struct command *cmd;\n>\n> for (cmd = commands; cmd; cmd = cmd->next) {\n> - if (cmd->error_string || !ref_is_hidden(cmd->ref_name))\n\nWould it make sense to retain the cmd->error_string check here in\norder to avoid doing the extra refname_full construction work below?\n\n if (cmd->error_string)\n continue;\n\n> + const char *refname = cmd->ref_name;\n> + struct strbuf refname_full_buf = STRBUF_INIT;\n> + const char *refname_full;\n> +\n> + strbuf_addf(&refname_full_buf, \"%s%s\", get_git_namespace(),\n> + refname);\n> + refname_full = strbuf_detach(&refname_full_buf, NULL);\n> +\n> + if (cmd->error_string || !ref_is_hidden(refname, refname_full))\n> continue;\n\nThis is leaking refname_full each time through the loop since it never\ngets free()d. If you restructure the code like this, it might be\neasier to avoid leaks:\n\n for (cmd = ...; ... ; ...) {\n if (cmd->error_string)\n continue;\n strbuf_addf(&refname_full, \"%s%s\", ...);\n if (ref_is_hidden(...)) {\n if (is_null_sha1(...))\n cmd->error_string = \"...\";\n else\n cmd->error_string = \"...\";\n }\n strbuf_release(&refname_full);\n }\n\nAs a micro-optimization, you could also pre-populate the strbuf with\nget_git_namespace() outside the loop and remember the length. Then,\neach time through the loop, just append cmd->ref_name, do your\nprocessing, and, at the bottom of the loop, set the strbuf back to the\nremembered length. (And, you still need to free the strbuf after the\nloop.)\n\n> if (is_null_sha1(cmd->new_sha1))\n> cmd->error_string = \"deny deleting a hidden ref\";\n"},{"id":"272709","messageId":"CAPig+cR7sCqigz=--_d-bW0X=+vpbV-_N9hxriLH9fMjLJUfsA@mail.gmail.com","threadId":"40697","inReplyTo":"1446406463-22527-5-git-send-email-lfleischer@lfos.de","subject":"Re: [PATCH 4/4] t5509: add basic tests for hideRefs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-01T21:13:51Z","receivedAt":"2015-11-01T21:13:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 1, 2015 at 2:34 PM, Lukas Fleischer wrote:\n> Test whether regular and full hideRefs patterns work as expected when\n> namespaces are used.\n>\n> Signed-off-by: Lukas Fleischer \n> ---\n> diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\n> @@ -82,4 +82,33 @@ test_expect_success 'mirroring a repository using a ref namespace' '\n> )\n> '\n>\n> +test_expect_success \"Hide namespaced refs with transfer.hideRefs\" '\n\nNone of the other tests in this file capitalize the test description.\nThese new test descriptions should probably follow suit by beginning\nwith lowercase. It is also typical to use single quotes for the\ndescription rather than double.\n\n> + cd pushee &&\n> + test_config transfer.hideRefs refs/tags &&\n> + GIT_NAMESPACE=namespace git ls-remote \"ext::git %s .\" >actual &&\n> + printf \"$commit1\\trefs/heads/master\\n\" >expected &&\n> + test_cmp expected actual &&\n> + cd ..\n\nIf any of the commands above \"cd ..\" fail, then \"cd ..\" will never be\ninvoked, thus subsequent tests will fail since they won't be executed\nin the expected directory. The typical way to handle this is to place\nthe \"cd foo\" and remaining test body in a subshell, and drop \"cd ..\"\naltogether. When the subshell exits (via success or failure), the\nworking directory will be restored automatically.\n\n test_expect_success '...' '\n (\n cd pushee &&\n test_config ... &&\n ...\n )\n '\n\n> +'\n> +\n> +test_expect_success \"Check that transfer.hideRefs does not match unstripped refs\" '\n> + cd pushee &&\n> + test_config transfer.hideRefs \"refs/namespaces/namespace/refs/tags\" &&\n> + GIT_NAMESPACE=namespace git ls-remote \"ext::git %s .\" >actual &&\n> + printf \"$commit1\\trefs/heads/master\\n\" >expected &&\n> + printf \"$commit0\\trefs/tags/0\\n\" >>expected &&\n> + printf \"$commit1\\trefs/tags/1\\n\" >>expected &&\n> + test_cmp expected actual &&\n> + cd ..\n> +'\n> +\n> +test_expect_success \"Hide full refs with transfer.hideRefs\" '\n> + cd pushee &&\n> + test_config transfer.hideRefs \"^refs/namespaces/namespace/refs/tags\" &&\n> + GIT_NAMESPACE=namespace git ls-remote \"ext::git %s .\" >actual &&\n> + printf \"$commit1\\trefs/heads/master\\n\" >expected &&\n> + test_cmp expected actual &&\n> + cd ..\n> +'\n> +\n> test_done\n> --\n> 2.6.2\n"},{"id":"272719","messageId":"20151102054735.24971.50757@typhoon.lan","threadId":"40697","inReplyTo":"CAPig+cST7sATT4W4Kwp5K=DcuvoFkdgsTDA9iOk=8c_8_GE=-Q@mail.gmail.com","subject":"Re: [PATCH 3/4] Add support for matching full refs in hideRefs","fromName":"Lukas Fleischer","fromEmail":"lfleischer@lfos.de","sentAt":"2015-11-02T05:47:35Z","receivedAt":"2015-11-02T05:47:35Z","isPatch":true,"sender":{"key":"lfleischer@lfos.de","avatar":"https://avatars.githubusercontent.com/u/5530842?v=4"},"body":"On Sun, 01 Nov 2015 at 21:42:37, Eric Sunshine wrote:\n> On Sun, Nov 1, 2015 at 2:34 PM, Lukas Fleischer wrote:\n> > In addition to matching stripped refs, one can now add hideRefs patterns\n> > that the full (unstripped) ref is matched against. To distinguish\n> > between stripped and full matches, those new patterns must be prefixed\n> > with a circumflex (^).\n> >\n> > This commit also removes support for the undocumented and unintended\n> > hideRefs settings \"have\" (suppressing all \"have\" lines) and\n> > \"capabilities^{}\" (suppressing the capabilities line).\n> >\n> > Signed-off-by: Lukas Fleischer \n> > ---\n> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> > @@ -1198,7 +1200,15 @@ static void reject_updates_to_hidden(struct command *commands)\n> > struct command *cmd;\n> >\n> > for (cmd = commands; cmd; cmd = cmd->next) {\n> > - if (cmd->error_string || !ref_is_hidden(cmd->ref_name))\n> \n> Would it make sense to retain the cmd->error_string check here in\n> order to avoid doing the extra refname_full construction work below?\n> \n> if (cmd->error_string)\n> continue;\n> \n> [...]\n> This is leaking refname_full each time through the loop since it never\n> gets free()d. If you restructure the code like this, it might be\n> easier to avoid leaks:\n> \n> for (cmd = ...; ... ; ...) {\n> if (cmd->error_string)\n> continue;\n> strbuf_addf(&refname_full, \"%s%s\", ...);\n> if (ref_is_hidden(...)) {\n> if (is_null_sha1(...))\n> cmd->error_string = \"...\";\n> else\n> cmd->error_string = \"...\";\n> }\n> strbuf_release(&refname_full);\n> }\n> \n> As a micro-optimization, you could also pre-populate the strbuf with\n> get_git_namespace() outside the loop and remember the length. Then,\n> each time through the loop, just append cmd->ref_name, do your\n> processing, and, at the bottom of the loop, set the strbuf back to the\n> remembered length. (And, you still need to free the strbuf after the\n> loop.)\n> [...]\n\nSounds good to me. I changed the code to initialize the strbuf only\nonce, save the prefix length and reset using strbuf_setlen() in every\nloop iteration. Thanks!\n"},{"id":"272720","messageId":"20151102062449.24971.39537@typhoon","threadId":"40697","inReplyTo":"CAPig+cR7sCqigz=--_d-bW0X=+vpbV-_N9hxriLH9fMjLJUfsA@mail.gmail.com","subject":"Re: [PATCH 4/4] t5509: add basic tests for hideRefs","fromName":"Lukas Fleischer","fromEmail":"lfleischer@lfos.de","sentAt":"2015-11-02T06:25:19Z","receivedAt":"2015-11-02T06:25:19Z","isPatch":true,"sender":{"key":"lfleischer@lfos.de","avatar":"https://avatars.githubusercontent.com/u/5530842?v=4"},"body":"On Sun, 01 Nov 2015 at 22:13:51, Eric Sunshine wrote:\n> On Sun, Nov 1, 2015 at 2:34 PM, Lukas Fleischer wrote:\n> > Test whether regular and full hideRefs patterns work as expected when\n> > namespaces are used.\n> >\n> > Signed-off-by: Lukas Fleischer \n> > ---\n> > diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\n> > @@ -82,4 +82,33 @@ test_expect_success 'mirroring a repository using a ref namespace' '\n> > )\n> > '\n> >\n> > +test_expect_success \"Hide namespaced refs with transfer.hideRefs\" '\n> \n> None of the other tests in this file capitalize the test description.\n> These new test descriptions should probably follow suit by beginning\n> with lowercase. It is also typical to use single quotes for the\n> description rather than double.\n> \n\nGood catch. I originally put these tests somewhere else and noticed that\nadding them to t5509 is much better since we already set up the whole\nnamespace infrastructure there. Seems like I forgot to adjust the style.\nFixed that locally.\n\n> > + cd pushee &&\n> > + test_config transfer.hideRefs refs/tags &&\n> > + GIT_NAMESPACE=namespace git ls-remote \"ext::git %s .\" >actual &&\n> > + printf \"$commit1 refs/heads/master\\n\" >expected &&\n> > + test_cmp expected actual &&\n> > + cd ..\n> \n> If any of the commands above \"cd ..\" fail, then \"cd ..\" will never be\n> invoked, thus subsequent tests will fail since they won't be executed\n> in the expected directory. The typical way to handle this is to place\n> the \"cd foo\" and remaining test body in a subshell, and drop \"cd ..\"\n> altogether. When the subshell exits (via success or failure), the\n> working directory will be restored automatically.\n> \n> test_expect_success '...' '\n> (\n> cd pushee &&\n> test_config ... &&\n> ...\n> )\n> '\n> [...]\n\nI chose the `cd ..` approach because test_config does not work from a\nsubshell. However, searching the Git log for \"test_config\", I found\n1a9a23e (t7610: don't use test_config in a subshell, 2015-09-05) and\nda568b6 (t7800: don't use test_config in a subshell, 2015-09-05) which\nsuggest to use the -C switch. The test cases now look like this:\n\n test_expect_success '[...]' '\n test_config -C pushee transfer.hideRefs [...] &&\n (\n cd pushee &&\n [...]\n )\n '\n\nThanks!\n"},{"id":"272723","messageId":"CAPig+cSv1x50ZoXLRAW55v7H6K5828GhvkeptuHd-Jsr+u74qA@mail.gmail.com","threadId":"40697","inReplyTo":"20151102062449.24971.39537@typhoon","subject":"Re: [PATCH 4/4] t5509: add basic tests for hideRefs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-02T07:30:51Z","receivedAt":"2015-11-02T07:30:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 2, 2015 at 1:25 AM, Lukas Fleischer wrote:\n> On Sun, 01 Nov 2015 at 22:13:51, Eric Sunshine wrote:\n>> On Sun, Nov 1, 2015 at 2:34 PM, Lukas Fleischer wrote:\n>> > + cd pushee &&\n>> > + test_config transfer.hideRefs refs/tags &&\n>> > + GIT_NAMESPACE=namespace git ls-remote \"ext::git %s .\" >actual &&\n>> > + printf \"$commit1 refs/heads/master\\n\" >expected &&\n>> > + test_cmp expected actual &&\n>> > + cd ..\n>>\n>> If any of the commands above \"cd ..\" fail, then \"cd ..\" will never be\n>> invoked, thus subsequent tests will fail since they won't be executed\n>> in the expected directory. The typical way to handle this is to place\n>> the \"cd foo\" and remaining test body in a subshell, and drop \"cd ..\"\n>> altogether. When the subshell exits (via success or failure), the\n>> working directory will be restored automatically.\n>>\n>> test_expect_success '...' '\n>> (\n>> cd pushee &&\n>> test_config ... &&\n>> ...\n>> )\n>> '\n>> [...]\n>\n> I chose the `cd ..` approach because test_config does not work from a\n> subshell. However, searching the Git log for \"test_config\", I found\n> 1a9a23e (t7610: don't use test_config in a subshell, 2015-09-05) and\n> da568b6 (t7800: don't use test_config in a subshell, 2015-09-05) which\n> suggest to use the -C switch. The test cases now look like this:\n>\n> test_expect_success '[...]' '\n> test_config -C pushee transfer.hideRefs [...] &&\n> (\n> cd pushee &&\n> [...]\n> )\n> '\n\nYes, that can work, although for these simple cases, it might be more\nstraightforward to use the git -c option to set the config variable\njust for the duration of the one git command. For instance:\n\ntest_expect_success 'hide namespaced refs with transfer.hideRefs' '\n (\n cd pushee &&\n GIT_NAMESPACE=namespace \\\n git -c transfer.hideRefs=refs/tags \\\n ls-remote \"ext::git %s .\" >actual &&\n printf \"$commit1\\trefs/heads/master\\n\" >expected &&\n test_cmp expected actual &&\n )\n'\n\nIn fact, these test are so simple, that you don't really need the 'cd'\nat all. You could just use -C (along with -c):\n\ntest_expect_success 'hide namespaced refs with transfer.hideRefs' '\n GIT_NAMESPACE=namespace \\\n git -C pushee -c transfer.hideRefs=refs/tags \\\n ls-remote \"ext::git %s .\" >actual &&\n printf \"$commit1\\trefs/heads/master\\n\" >expected &&\n test_cmp expected actual &&\n'\n"}]}