# [PATCH/RFC] archive: allow archiving of reachable sha1 2 messages from 2016-07-08 to 2016-07-08. Participants: Nicolas Cornu, Junio C Hamano. Thread: https://gitlist.dev/t/42803 ## Nicolas Cornu, 2016-07-08 22:32 Subject: [PATCH/RFC] archive: allow archiving of reachable sha1 Message-ID: <20160708223250.18238-1-nicolac76@yahoo.fr> URL: https://gitlist.dev/e/20160708223250.18238-1-nicolac76%40yahoo.fr ``` Remotely specify a tree-ish by a sha1 is now valid even if uploadarchive.allowunreachable is false only if this sha1 is reachable from a branch or a tag reference. We consider those last one to be public. Signed-off-by: Nicolas Cornu --- Do you think this patch is too much "computationnally expensive"? Maybe we need an option to disable such a a feature. If we want an option I think it's better to have an option disabling this feature. This way server will accept such archiving by default. Documentation/git-upload-archive.txt | 19 ++++++------------- archive.c | 15 +++++++++++++-- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/Documentation/git-upload-archive.txt b/Documentation/git-upload-archive.txt index fba0f1c..59d9b65 100644 --- a/Documentation/git-upload-archive.txt +++ b/Documentation/git-upload-archive.txt @@ -26,25 +26,18 @@ SECURITY In order to protect the privacy of objects that have been removed from history but may not yet have been pruned, `git-upload-archive` avoids serving archives for commits and trees that are not reachable from the -repository's refs. However, because calculating object reachability is -computationally expensive, `git-upload-archive` implements a stricter -but easier-to-check set of rules: +repository's refs. `git-upload-archive` implements a stricter but +easier-to-check set of rules: 1. Clients may request a commit or tree that is pointed to directly by - a ref. E.g., `git archive --remote=origin v1.0`. + a ref or is an ancestor of a branch or tag ref. + E.g., `git archive --remote=origin v1.0`. 2. Clients may request a sub-tree within a commit or tree using the `ref:path` syntax. E.g., `git archive --remote=origin v1.0:Documentation`. - 3. Clients may _not_ use other sha1 expressions, even if the end - result is reachable. E.g., neither a relative commit like `master^` - nor a literal sha1 like `abcd1234` is allowed, even if the result - is reachable from the refs. - -Note that rule 3 disallows many cases that do not have any privacy -implications. These rules are subject to change in future versions of -git, and the server accessed by `git archive --remote` may or may not -follow these exact rules. +These rules are subject to change in future versions of git, and the server +accessed by `git archive --remote` may or may not follow these exact rules. If the config option `uploadArchive.allowUnreachable` is true, these rules are ignored, and clients may use arbitrary sha1 expressions. diff --git a/archive.c b/archive.c index 42df974..d99c195 100644 --- a/archive.c +++ b/archive.c @@ -347,6 +347,12 @@ static void parse_pathspec_arg(const char **pathspec, } } +static int is_reachable(const char *refname, const struct object_id *oid, int flags, void *cb_data) +{ + const unsigned char *sha1 = (unsigned char *)cb_data; + return in_merge_bases(lookup_commit(sha1), lookup_commit(oid->hash)); +} + static void parse_treeish_arg(const char **argv, struct archiver_args *ar_args, const char *prefix, int remote) @@ -364,8 +370,13 @@ static void parse_treeish_arg(const char **argv, const char *colon = strchrnul(name, ':'); int refnamelen = colon - name; - if (!dwim_ref(name, refnamelen, oid.hash, &ref)) - die("no such ref: %.*s", refnamelen, name); + if (!dwim_ref(name, refnamelen, oid.hash, &ref)) { + if (get_sha1(name, oid.hash)) + die("Not a valid object name"); + if (!for_each_branch_ref(&is_reachable, oid.hash) && + !for_each_tag_ref(&is_reachable, oid.hash)) + die("no such ref: %.*s", refnamelen, name); + } free(ref); } -- 2.9.0 ``` ## Junio C Hamano, 2016-07-08 22:54 Subject: Re: [PATCH/RFC] archive: allow archiving of reachable sha1 Message-ID: URL: https://gitlist.dev/e/xmqq7fcvlp0z.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160708223250.18238-1-nicolac76@yahoo.fr> ``` Nicolas Cornu writes: > Remotely specify a tree-ish by a sha1 is now valid even if > uploadarchive.allowunreachable is false only if this sha1 is reachable > from a branch or a tag reference. We consider those last one to be > public. > > Signed-off-by: Nicolas Cornu > --- > Do you think this patch is too much "computationnally expensive"? Yes. > Maybe we need an option to disable such a a feature. No, you need an option to enable it while keeping it disabled by default. > +static int is_reachable(const char *refname, const struct object_id *oid, int flags, void *cb_data) > +{ > + const unsigned char *sha1 = (unsigned char *)cb_data; > + return in_merge_bases(lookup_commit(sha1), lookup_commit(oid->hash)); > +} So this checks if the tip of a ref (i.e. lookup_commit(oid->hash)) can reach the commit in cb_data, i.e. what the caller had in oid.hash below. > static void parse_treeish_arg(const char **argv, > struct archiver_args *ar_args, const char *prefix, > int remote) > @@ -364,8 +370,13 @@ static void parse_treeish_arg(const char **argv, > const char *colon = strchrnul(name, ':'); > int refnamelen = colon - name; > > - if (!dwim_ref(name, refnamelen, oid.hash, &ref)) > - die("no such ref: %.*s", refnamelen, name); > + if (!dwim_ref(name, refnamelen, oid.hash, &ref)) { > + if (get_sha1(name, oid.hash)) > + die("Not a valid object name"); > + if (!for_each_branch_ref(&is_reachable, oid.hash) && > + !for_each_tag_ref(&is_reachable, oid.hash)) > + die("no such ref: %.*s", refnamelen, name); > + } > free(ref); > } > + if (!dwim_ref(name, refnamelen, oid.hash, &ref)) { > + if (get_sha1(name, oid.hash)) > + die("Not a valid object name"); > + if (!for_each_branch_ref(&is_reachable, oid.hash) && > + !for_each_tag_ref(&is_reachable, oid.hash)) > + die("no such ref: %.*s", refnamelen, name); Isn't this making the check unnecessarily expensive by requiring the traversal to determine the ancestry down to the same oid.hash to happen repeatedly? A repository may have quite a many tags, some very old ones, and traversing down to these old tags from the requested commit in order to prove that the commit cannot be reached by these tags (hence these tags cannot be an excuse for you to serve the commit) would be expensive. I suspect that it would be better to collect the objects at the tips in an array using for_each_*_ref(), and do the check by using a single call to in_merge_bases_many() instead, which would require only one traversal? Granted, it would still dig down to the common ancestor between the asked commit and these old tags, i.e. one near-full walk of the history, but at least you won't be making a multiple such near-full walks of the history. I also suspect that in_merge_bases_many() could be further optimized if it turns out to be necessary by teaching paint_down_to_common() an option to stop early, but that would be a second step of the optimization. > + } > free(ref); > } ```