{"thread":{"id":"42803","subject":"[PATCH/RFC] archive: allow archiving of reachable sha1","startedAt":"2016-07-08T22:36:38Z","lastAt":"2016-07-08T22:54:14Z","messageCount":2,"participants":["Nicolas Cornu","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"291066","messageId":"20160708223250.18238-1-nicolac76@yahoo.fr","threadId":"42803","inReplyTo":null,"subject":"[PATCH/RFC] archive: allow archiving of reachable sha1","fromName":"Nicolas Cornu","fromEmail":"nicolac76@gmail.com","sentAt":"2016-07-08T22:32:50Z","receivedAt":"2016-07-08T22:36:38Z","isPatch":true,"sender":{"key":"nicolac76@gmail.com","avatar":null},"body":"Remotely specify a tree-ish by a sha1 is now valid even if\nuploadarchive.allowunreachable is false only if this sha1 is reachable\nfrom a branch or a tag reference. We consider those last one to be\npublic.\n\nSigned-off-by: Nicolas Cornu <nicolac76@yahoo.fr>\n---\nDo you think this patch is too much \"computationnally expensive\"?\nMaybe we need an option to disable such a a feature.\nIf we want an option I think it's better to have an option disabling this feature.\nThis way server will accept such archiving by default.\n\n Documentation/git-upload-archive.txt | 19 ++++++-------------\n archive.c                            | 15 +++++++++++++--\n 2 files changed, 19 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git-upload-archive.txt b/Documentation/git-upload-archive.txt\nindex fba0f1c..59d9b65 100644\n--- a/Documentation/git-upload-archive.txt\n+++ b/Documentation/git-upload-archive.txt\n@@ -26,25 +26,18 @@ SECURITY\n In order to protect the privacy of objects that have been removed from\n history but may not yet have been pruned, `git-upload-archive` avoids\n serving archives for commits and trees that are not reachable from the\n-repository's refs.  However, because calculating object reachability is\n-computationally expensive, `git-upload-archive` implements a stricter\n-but easier-to-check set of rules:\n+repository's refs. `git-upload-archive` implements a stricter but\n+easier-to-check set of rules:\n \n   1. Clients may request a commit or tree that is pointed to directly by\n-     a ref. E.g., `git archive --remote=origin v1.0`.\n+     a ref or is an ancestor of a branch or tag ref.\n+     E.g., `git archive --remote=origin v1.0`.\n \n   2. Clients may request a sub-tree within a commit or tree using the\n      `ref:path` syntax. E.g., `git archive --remote=origin v1.0:Documentation`.\n \n-  3. Clients may _not_ use other sha1 expressions, even if the end\n-     result is reachable. E.g., neither a relative commit like `master^`\n-     nor a literal sha1 like `abcd1234` is allowed, even if the result\n-     is reachable from the refs.\n-\n-Note that rule 3 disallows many cases that do not have any privacy\n-implications. These rules are subject to change in future versions of\n-git, and the server accessed by `git archive --remote` may or may not\n-follow these exact rules.\n+These rules are subject to change in future versions of git, and the server\n+accessed by `git archive --remote` may or may not follow these exact rules.\n \n If the config option `uploadArchive.allowUnreachable` is true, these\n rules are ignored, and clients may use arbitrary sha1 expressions.\ndiff --git a/archive.c b/archive.c\nindex 42df974..d99c195 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -347,6 +347,12 @@ static void parse_pathspec_arg(const char **pathspec,\n \t}\n }\n \n+static int is_reachable(const char *refname, const struct object_id *oid, int flags, void *cb_data)\n+{\n+\tconst unsigned char *sha1 = (unsigned char *)cb_data;\n+\treturn in_merge_bases(lookup_commit(sha1), lookup_commit(oid->hash));\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@@ -364,8 +370,13 @@ static void parse_treeish_arg(const char **argv,\n \t\tconst char *colon = strchrnul(name, ':');\n \t\tint refnamelen = colon - name;\n \n-\t\tif (!dwim_ref(name, refnamelen, oid.hash, &ref))\n-\t\t\tdie(\"no such ref: %.*s\", refnamelen, name);\n+\t\tif (!dwim_ref(name, refnamelen, oid.hash, &ref)) {\n+\t\t\tif (get_sha1(name, oid.hash))\n+\t\t\t\tdie(\"Not a valid object name\");\n+\t\t\tif (!for_each_branch_ref(&is_reachable, oid.hash) &&\n+\t\t\t    !for_each_tag_ref(&is_reachable, oid.hash))\n+\t\t\t\tdie(\"no such ref: %.*s\", refnamelen, name);\n+\t\t}\n \t\tfree(ref);\n \t}\n \n-- \n2.9.0\n\n"},{"id":"291070","messageId":"xmqq7fcvlp0z.fsf@gitster.mtv.corp.google.com","threadId":"42803","inReplyTo":"20160708223250.18238-1-nicolac76@yahoo.fr","subject":"Re: [PATCH/RFC] archive: allow archiving of reachable sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-08T22:54:04Z","receivedAt":"2016-07-08T22:54:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Cornu <nicolac76@gmail.com> writes:\n\n> Remotely specify a tree-ish by a sha1 is now valid even if\n> uploadarchive.allowunreachable is false only if this sha1 is reachable\n> from a branch or a tag reference. We consider those last one to be\n> public.\n>\n> Signed-off-by: Nicolas Cornu <nicolac76@yahoo.fr>\n> ---\n> Do you think this patch is too much \"computationnally expensive\"?\n\nYes.\n\n> Maybe we need an option to disable such a a feature.\n\nNo, you need an option to enable it while keeping it disabled by\ndefault.\n\n> +static int is_reachable(const char *refname, const struct object_id *oid, int flags, void *cb_data)\n> +{\n> +\tconst unsigned char *sha1 = (unsigned char *)cb_data;\n> +\treturn in_merge_bases(lookup_commit(sha1), lookup_commit(oid->hash));\n> +}\n\nSo this checks if the tip of a ref (i.e. lookup_commit(oid->hash))\ncan reach the commit in cb_data, i.e. what the caller had in\noid.hash below.\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> @@ -364,8 +370,13 @@ static void parse_treeish_arg(const char **argv,\n>  \t\tconst char *colon = strchrnul(name, ':');\n>  \t\tint refnamelen = colon - name;\n>  \n> -\t\tif (!dwim_ref(name, refnamelen, oid.hash, &ref))\n> -\t\t\tdie(\"no such ref: %.*s\", refnamelen, name);\n> +\t\tif (!dwim_ref(name, refnamelen, oid.hash, &ref)) {\n> +\t\t\tif (get_sha1(name, oid.hash))\n> +\t\t\t\tdie(\"Not a valid object name\");\n> +\t\t\tif (!for_each_branch_ref(&is_reachable, oid.hash) &&\n> +\t\t\t    !for_each_tag_ref(&is_reachable, oid.hash))\n> +\t\t\t\tdie(\"no such ref: %.*s\", refnamelen, name);\n> +\t\t}\n>  \t\tfree(ref);\n>  \t}\n\n\n> +\t\tif (!dwim_ref(name, refnamelen, oid.hash, &ref)) {\n> +\t\t\tif (get_sha1(name, oid.hash))\n> +\t\t\t\tdie(\"Not a valid object name\");\n> +\t\t\tif (!for_each_branch_ref(&is_reachable, oid.hash) &&\n> +\t\t\t    !for_each_tag_ref(&is_reachable, oid.hash))\n> +\t\t\t\tdie(\"no such ref: %.*s\", refnamelen, name);\n\nIsn't this making the check unnecessarily expensive by requiring the\ntraversal to determine the ancestry down to the same oid.hash to\nhappen repeatedly?  A repository may have quite a many tags, some\nvery old ones, and traversing down to these old tags from the\nrequested commit in order to prove that the commit cannot be reached\nby these tags (hence these tags cannot be an excuse for you to\nserve the commit) would be expensive.\n\nI suspect that it would be better to collect the objects at the tips\nin an array using for_each_*_ref(), and do the check by using a\nsingle call to in_merge_bases_many() instead, which would require\nonly one traversal?  Granted, it would still dig down to the common\nancestor between the asked commit and these old tags, i.e. one\nnear-full walk of the history, but at least you won't be making a\nmultiple such near-full walks of the history.\n\nI also suspect that in_merge_bases_many() could be further optimized\nif it turns out to be necessary by teaching paint_down_to_common()\nan option to stop early, but that would be a second step of the\noptimization.\n\n> +\t\t}\n>  \t\tfree(ref);\n>  \t}\n"}]}