Re: [PATCH] archive: let remote clients get reachable commits
- From
Jeff King <peff@peff.net>
- Date
- Feb 21, 2013, 15:52 UTC
- Message-ID
- <20130221155208.GA19943@sigill.intra.peff.net>
- In-Reply-To
- <1361456643-51851-1-git-send-email-gurugray@yandex.ru>
On Thu, Feb 21, 2013 at 06:24:03PM +0400, Sergey Segeev wrote:
Show 6 quoted lines
> Some time we need to get valid commit without a ref but with proper > tree-ish, now we can't do that. > > This patch allow upload-archive's to use reachability checking > rather than checking that is a ref. This means a remote client can > fetch a tip of any valid sha1 or tree-ish.
That sounds like a good goal, but...
Show 14 quoted lines
> @@ -252,22 +260,16 @@ static void parse_treeish_arg(const char **argv,
> const struct commit *commit;
> unsigned char sha1[20];
>
> - /* Remotes are only allowed to fetch actual refs */
> - if (remote) {
> - char *ref = NULL;
> - const char *colon = strchr(name, ':');
> - int refnamelen = colon ? colon - name : strlen(name);
> -
> - if (!dwim_ref(name, refnamelen, sha1, &ref))
> - die("no such ref: %.*s", refnamelen, name);
> - free(ref);
> - }The point of this was to allow "commit:path" syntax, and check that commit pointed to a ref. The natural extension would be to also check that the commit part is reachable.
I think it is also not sufficient to just check whether the left-hand side of the colon is a reachable commit. You would also want to handle non-commits which are directly pointed-to by a ref or its tag (e.g., think of a tag pointing directly to a tree, like the v2.6.11 tag in the linux repo).
Your check...
Show 10 quoted lines
> commit = lookup_commit_reference_gently(sha1, 1);
> if (commit) {
> +
> + /* Remotes are only allowed to fetch actual objects */
> + if (remote && !for_each_ref(check_reachable, (void *)commit))
> + die("Not a valid object name");
> +
> commit_sha1 = commit->object.sha1;
> archive_time = commit->date;
> } else {...will do nothing if we do not have a commit reference (e.g., an arbitrary sha1, or commit:path syntax). We follow the "else" of this branch, and allow arbitrary sha1's to be fetched (like "unreachable_sha1:subdir").
-Peff