{"thread":{"id":"32961","subject":"[PATCH] archive: let remote clients get reachable commits","startedAt":"2013-02-21T14:24:03Z","lastAt":"2013-02-22T19:15:34Z","messageCount":7,"participants":["Sergey Segeev","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"209974","messageId":"1361456643-51851-1-git-send-email-gurugray@yandex.ru","threadId":"32961","inReplyTo":null,"subject":"[PATCH] archive: let remote clients get reachable commits","fromName":"Sergey Segeev","fromEmail":"gurugray@yandex.ru","sentAt":"2013-02-21T14:24:03Z","receivedAt":"2013-02-21T14:24:03Z","isPatch":true,"sender":{"key":"gurugray@yandex.ru","avatar":null},"body":"Some time we need to get valid commit without a ref but with proper\ntree-ish, now we can't do that.\n\nThis patch allow upload-archive's to use reachability checking\nrather than checking that is a ref. This means a remote client can\nfetch a tip of any valid sha1 or tree-ish.\n---\n archive.c | 24 +++++++++++++-----------\n t/t5000-tar-tree.sh | 2 ++\n 2 files changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 93e00bb..0a48985 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -5,6 +5,7 @@\n #include \"archive.h\"\n #include \"parse-options.h\"\n #include \"unpack-trees.h\"\n+#include \"refs.h\"\n \n static char const * const archive_usage[] = {\n \tN_(\"git archive [options] [...]\"),\n@@ -241,6 +242,13 @@ static void parse_pathspec_arg(const char **pathspec,\n \t}\n }\n \n+static int check_reachable(const char *refname, const unsigned char *sha1,\n+\t\tint flag, void *cb_data)\n+{\n+\n+\treturn in_merge_bases(cb_data, lookup_commit_reference_gently(sha1, 1));\n+}\n+\n static void parse_treeish_arg(const char **argv,\n \t\tstruct archiver_args *ar_args, const char *prefix,\n \t\tint remote)\n@@ -252,22 +260,16 @@ static void parse_treeish_arg(const char **argv,\n \tconst struct commit *commit;\n \tunsigned char sha1[20];\n \n-\t/* Remotes are only allowed to fetch actual refs */\n-\tif (remote) {\n-\t\tchar *ref = NULL;\n-\t\tconst char *colon = strchr(name, ':');\n-\t\tint refnamelen = colon ? colon - name : strlen(name);\n-\n-\t\tif (!dwim_ref(name, refnamelen, sha1, &ref))\n-\t\t\tdie(\"no such ref: %.*s\", refnamelen, name);\n-\t\tfree(ref);\n-\t}\n-\n \tif (get_sha1(name, sha1))\n \t\tdie(\"Not a valid object name\");\n \n \tcommit = lookup_commit_reference_gently(sha1, 1);\n \tif (commit) {\n+\n+\t\t/* Remotes are only allowed to fetch actual objects */\n+\t\tif (remote && !for_each_ref(check_reachable, (void *)commit))\n+\t\t\tdie(\"Not a valid object name\");\n+\n \t\tcommit_sha1 = commit->object.sha1;\n \t\tarchive_time = commit->date;\n \t} else {\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex e7c240f..fc35406 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -194,6 +194,8 @@ test_expect_success 'clients cannot access unreachable commits' '\n \tsha1=`git rev-parse HEAD` &&\n \tgit reset --hard HEAD^ &&\n \tgit archive $sha1 >remote.tar &&\n+\tgit archive --remote=. $sha1 >remote.tar &&\n+\tgit tag -d unreachable &&\n \ttest_must_fail git archive --remote=. $sha1 >remote.tar\n '\n \n-- \n1.8.1.4\n"},{"id":"209977","messageId":"20130221155208.GA19943@sigill.intra.peff.net","threadId":"32961","inReplyTo":"1361456643-51851-1-git-send-email-gurugray@yandex.ru","subject":"Re: [PATCH] archive: let remote clients get reachable commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-21T15:52:08Z","receivedAt":"2013-02-21T15:52:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 21, 2013 at 06:24:03PM +0400, Sergey Segeev wrote:\n\n> Some time we need to get valid commit without a ref but with proper\n> tree-ish, now we can't do that.\n> \n> This patch allow upload-archive's to use reachability checking\n> rather than checking that is a ref. This means a remote client can\n> fetch a tip of any valid sha1 or tree-ish.\n\nThat sounds like a good goal, but...\n\n> @@ -252,22 +260,16 @@ static void parse_treeish_arg(const char **argv,\n> \tconst struct commit *commit;\n> \tunsigned char sha1[20];\n> \n> -\t/* Remotes are only allowed to fetch actual refs */\n> -\tif (remote) {\n> -\t\tchar *ref = NULL;\n> -\t\tconst char *colon = strchr(name, ':');\n> -\t\tint refnamelen = colon ? colon - name : strlen(name);\n> -\n> -\t\tif (!dwim_ref(name, refnamelen, sha1, &ref))\n> -\t\t\tdie(\"no such ref: %.*s\", refnamelen, name);\n> -\t\tfree(ref);\n> -\t}\n\nThe point of this was to allow \"commit:path\" syntax, and check that\ncommit pointed to a ref. The natural extension would be to also check\nthat the commit part is reachable.\n\nI think it is also not sufficient to just check whether the left-hand\nside of the colon is a reachable commit. You would also want to handle\nnon-commits which are directly pointed-to by a ref or its tag (e.g.,\nthink of a tag pointing directly to a tree, like the v2.6.11 tag in the\nlinux repo).\n\nYour check...\n\n> \tcommit = lookup_commit_reference_gently(sha1, 1);\n> \tif (commit) {\n> +\n> +\t\t/* Remotes are only allowed to fetch actual objects */\n> +\t\tif (remote && !for_each_ref(check_reachable, (void *)commit))\n> +\t\t\tdie(\"Not a valid object name\");\n> +\n> \t\tcommit_sha1 = commit->object.sha1;\n> \t\tarchive_time = commit->date;\n> \t} else {\n\n...will do nothing if we do not have a commit reference (e.g., an arbitrary\nsha1, or commit:path syntax). We follow the \"else\" of this branch, and\nallow arbitrary sha1's to be fetched (like \"unreachable_sha1:subdir\").\n\n-Peff\n"},{"id":"210036","messageId":"7vehg8s295.fsf@alter.siamese.dyndns.org","threadId":"32961","inReplyTo":"995301361532360@web22h.yandex.ru","subject":"Re: [PATCH] archive: let remote clients get reachable commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-22T17:10:46Z","receivedAt":"2013-02-22T17:10:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Sergeev writes:\n\n[jc: please do not top-post]\n\n> You are right,\n> I'll rethink this patch and write some test for this cases.\n\nThanks.\n\nNote that this is harder to implement than one would naïvely think,\nif one aims for a very generic solution, without walking the whole\nhistory.\n\nI personally think that it is OK to limit the scope to expressions\nthat start from the tip of ref and expressions that start with the\nSHA-1 at the tip of ref, e.g.\n\n master~12:Documentation\n v2.6.11:arch/alpha\n 5dc01c595e6c6ec9ccda\t# tag v2.6.11\n 26791a8bcf0e6d33f43a:arch\t# tag v2.6.12\n 26791a8bcf0~12:arch\t\t# starting at 26791a8b and dig down\n\nare OK, while forbidding the following:\n\n c39ae07f393806ccf406 # tree of tag v2.6.11\n 9ee1c939d1cb936b1f98\t# commit v2.6.12^0\n 9ee1c939d1cb936b1f98:\t# tree of commit v2.6.12^0\n 9ee1c939d1cb936b1f98:arch\t# subtree of commit v2.6.12^0\n\nwhich will make it significantly easier to implement the necessary\nvalidation in a robust way.\n"},{"id":"210039","messageId":"20130222172710.GB17475@sigill.intra.peff.net","threadId":"32961","inReplyTo":"7vehg8s295.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] archive: let remote clients get reachable commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-22T17:27:10Z","receivedAt":"2013-02-22T17:27:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 22, 2013 at 09:10:46AM -0800, Junio C Hamano wrote:\n\n> I personally think that it is OK to limit the scope to expressions\n> that start from the tip of ref and expressions that start with the\n> SHA-1 at the tip of ref, e.g.\n> \n> master~12:Documentation\n> v2.6.11:arch/alpha\n> 5dc01c595e6c6ec9ccda\t# tag v2.6.11\n> 26791a8bcf0e6d33f43a:arch\t# tag v2.6.12\n> 26791a8bcf0~12:arch\t\t# starting at 26791a8b and dig down\n> \n> are OK, while forbidding the following:\n> \n> c39ae07f393806ccf406 # tree of tag v2.6.11\n> 9ee1c939d1cb936b1f98\t# commit v2.6.12^0\n> 9ee1c939d1cb936b1f98:\t# tree of commit v2.6.12^0\n> 9ee1c939d1cb936b1f98:arch\t# subtree of commit v2.6.12^0\n> \n> which will make it significantly easier to implement the necessary\n> validation in a robust way.\n\nHow are you proposing to verify master~12 in that example? Because\nduring parsing, it starts with \"master\", and we remember that? Or\nbecause you are doing a reachability traversal on all refs after we\nparse?\n\n-Peff\n"},{"id":"210042","messageId":"7vfw0odxz3.fsf@alter.siamese.dyndns.org","threadId":"32961","inReplyTo":"20130222172710.GB17475@sigill.intra.peff.net","subject":"Re: [PATCH] archive: let remote clients get reachable commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-22T18:06:56Z","receivedAt":"2013-02-22T18:06:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> How are you proposing to verify master~12 in that example? Because\n> during parsing, it starts with \"master\", and we remember that?\n\nBy not cheating (i.e. using get_sha1()), but making sure you can\nparse \"master\" and the adornment on it \"~12\" is something sane.\n\nThat is why I said \"this is harder than one would naively think, but\nlimiting will make it significantly easier\". I didn't say that it\nwould become \"trivial\", did I?\n"},{"id":"210043","messageId":"20130222182654.GA18934@sigill.intra.peff.net","threadId":"32961","inReplyTo":"7vfw0odxz3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] archive: let remote clients get reachable commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-22T18:26:54Z","receivedAt":"2013-02-22T18:26:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 22, 2013 at 10:06:56AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > How are you proposing to verify master~12 in that example? Because\n> > during parsing, it starts with \"master\", and we remember that?\n> \n> By not cheating (i.e. using get_sha1()), but making sure you can\n> parse \"master\" and the adornment on it \"~12\" is something sane.\n\nSo, like these patches:\n\n http://article.gmane.org/gmane.comp.version-control.git/188386\n\n http://article.gmane.org/gmane.comp.version-control.git/188387\n\n? They do not allow arbitrary sha1s that happen to point to branch tips,\nbut I am not sure whether that is something people care about or not.\n\n> That is why I said \"this is harder than one would naively think, but\n> limiting will make it significantly easier\". I didn't say that it\n> would become \"trivial\", did I?\n\nI'm not implying it would be trivial. It was an honest question, since\nyou did not seem to want to do the pass-more-information-out-of-get-sha1\napproach last time this came up.\n\nEven though those patches above are from me, I've come to the conclusion\nthat the best thing to do is to harmonize with upload-pack. Then you\nnever have the \"well, but I could fetch it, so why won't upload-archive\nlet me get it\" argument. Something like:\n\n 1. split name at first colon (like we already do)\n\n 2. make sure the left-hand side is reachable according to the same\n rules that upload-pack uses. Right we just say \"is it a ref\". It\n should be:\n\n 2a. if it is a commit-ish, is it reachable from a ref?\n\n 2b. otherwise, is it pointed to directly by a ref?\n\n 3. Abort if it's not reachable. Abort if it's not a tree-ish. No\n checks necessary on the right-hand side, because a path lookup in a\n tree-ish is always reachable from the tree-ish. I.e., the same rule\n we have now.\n\nI did not check if upload-pack will respect a \"want\" line for an object\naccessible only by peeling a tag. But an obvious 2c could be \"is it\naccessible by peeling the refs?\"\n\nThat leaves the only inaccessible thing as direct-sha1s of trees and\nblobs that are reachable from commits. But you also cannot ask for those\ndirectly via upload-pack, and I do not think it's worth it to do the\nmuch more expensive reachability check to verify those (OTOH, it is no\nmore expensive than the current \"counting objects\" for a clone, and we\ncould do it only as a fallback when cheaper checks do not work).\n\n-Peff\n"},{"id":"210048","messageId":"7v38wodusp.fsf@alter.siamese.dyndns.org","threadId":"32961","inReplyTo":"20130222182654.GA18934@sigill.intra.peff.net","subject":"Re: [PATCH] archive: let remote clients get reachable commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-22T19:15:34Z","receivedAt":"2013-02-22T19:15:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Feb 22, 2013 at 10:06:56AM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > How are you proposing to verify master~12 in that example? Because\n>> > during parsing, it starts with \"master\", and we remember that?\n>> \n>> By not cheating (i.e. using get_sha1()), but making sure you can\n>> parse \"master\" and the adornment on it \"~12\" is something sane.\n>\n> So, like these patches:\n>\n> http://article.gmane.org/gmane.comp.version-control.git/188386\n>\n> http://article.gmane.org/gmane.comp.version-control.git/188387\n>\n> ? They do not allow arbitrary sha1s that happen to point to branch tips,\n> but I am not sure whether that is something people care about or not.\n>\n>> That is why I said \"this is harder than one would naively think, but\n>> limiting will make it significantly easier\". I didn't say that it\n>> would become \"trivial\", did I?\n>\n> I'm not implying it would be trivial. It was an honest question, since\n> you did not seem to want to do the pass-more-information-out-of-get-sha1\n> approach last time this came up.\n\nThat does not match my recollection ($gmane/188427). In fact, I had\nthose two patches you quoted earlier in mind when I wrote the \"limit\nit and it will become easier\" response.\n\n> Even though those patches above are from me, I've come to the conclusion\n> that the best thing to do is to harmonize with upload-pack. Then you\n> never have the \"well, but I could fetch it, so why won't upload-archive\n> let me get it\" argument.\n\nThat sounds like a sensible yardstick.\n\n> 1. split name at first colon (like we already do)\n>\n> 2. make sure the left-hand side is reachable according to the same\n> rules that upload-pack uses.\n\nWell, \"upload-pack\" under the hood operates on a bare 40-hex. If\nyou mean to do \"split name at first colon, run get_sha1() on the\nLHS\" in step 1., I would agree this is a good direction to go.\n\n> Right we just say \"is it a ref\". It should be:\n\nWith s/should/could optionally/, I would agree.\n\n> That leaves the only inaccessible thing as direct-sha1s of trees and\n> blobs that are reachable from commits.\n\nYes (with s/are reachable/are only reachable/).\n"}]}