{"thread":{"id":"29333","subject":"[BUG] git archive broken in 1.7.8.1","startedAt":"2012-01-10T21:18:41Z","lastAt":"2013-06-07T00:50:27Z","messageCount":30,"participants":["Albert Astals Cid","Carlos Martín Nieto","Allan Wind","Jeff King","Junio C Hamano","Ian Harvey","Michael Haggerty","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"182279","messageId":"5142795.2dTmMhVRTP@xps","threadId":"29333","inReplyTo":null,"subject":"[BUG] git archive broken in 1.7.8.1","fromName":"Albert Astals Cid","fromEmail":"aacid@kde.org","sentAt":"2012-01-10T21:18:41Z","receivedAt":"2012-01-10T21:18:41Z","isPatch":false,"sender":{"key":"aacid@kde.org","avatar":"https://gravatar.com/avatar/f75ccc222073fee7e869dee8627ed66dc18904e0cbb06314adfd8586940f97f3?d=mp&s=160"},"body":"CC me on answers since i'm not subscribed to the list\n\nHi, one of our [KDE] anongit servers was updated to 1.7.8.1 and not the syntax\n\ngit archive --remote=git://anongit.kde.org/repo.git HEAD:path\n\ndoes not seem to return a valid tar archive anymore when it did work \npreviously. In fact the man page of my version has that syntax in one of the \nexamples.\n\nIs that a regression or should have never it worked and the current behaviour \nis the correct one?\n\nAlbert\n"},{"id":"182281","messageId":"20120110213344.GI2714@centaur.lab.cmartin.tk","threadId":"29333","inReplyTo":"5142795.2dTmMhVRTP@xps","subject":"Re: [BUG] git archive broken in 1.7.8.1","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-01-10T21:33:44Z","receivedAt":"2012-01-10T21:33:44Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Tue, Jan 10, 2012 at 10:18:41PM +0100, Albert Astals Cid wrote:\n> CC me on answers since i'm not subscribed to the list\n> \n> Hi, one of our [KDE] anongit servers was updated to 1.7.8.1 and not the syntax\n> \n> git archive --remote=git://anongit.kde.org/repo.git HEAD:path\n\nThis syntax is no longer allowed due to some security tightening. Use\nthe alternate syntax\n\n git archive --remote=git://anongit.kde.org/repo.git HEAD -- path\n\n> \n> does not seem to return a valid tar archive anymore when it did work \n> previously. In fact the man page of my version has that syntax in one of the \n> examples.\n\nThat sounds like a documentation bug.\n\n cmn\n"},{"id":"182283","messageId":"1431498.0yPWNQLupF@xps","threadId":"29333","inReplyTo":"20120110213344.GI2714@centaur.lab.cmartin.tk","subject":"Re: [BUG] git archive broken in 1.7.8.1","fromName":"Albert Astals Cid","fromEmail":"aacid@kde.org","sentAt":"2012-01-10T22:05:45Z","receivedAt":"2012-01-10T22:05:45Z","isPatch":false,"sender":{"key":"aacid@kde.org","avatar":"https://gravatar.com/avatar/f75ccc222073fee7e869dee8627ed66dc18904e0cbb06314adfd8586940f97f3?d=mp&s=160"},"body":"El Dimarts, 10 de gener de 2012, a les 22:33:44, Carlos Martín Nieto va \nescriure:\n> On Tue, Jan 10, 2012 at 10:18:41PM +0100, Albert Astals Cid wrote:\n> > CC me on answers since i'm not subscribed to the list\n> > \n> > Hi, one of our [KDE] anongit servers was updated to 1.7.8.1 and not the\n> > syntax\n> > \n> > git archive --remote=git://anongit.kde.org/repo.git HEAD:path\n> \n> This syntax is no longer allowed due to some security tightening. Use\n> the alternate syntax\n> \n> git archive --remote=git://anongit.kde.org/repo.git HEAD -- path\n\nUnfortunately this producess a tarball with a different layout, e.g.\n\ngit archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD:doc/en_US\n gives me a tarball with the doc/en_US files in the root\n\ngit archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD -- doc/en_US\n gives me a tarball with the doc/en_US folders and then the files\n\nIs there a way to keep the old behaviour or do we need to update our scripts?\n\nThanks for the fast answer!\n\nAlbert\n\n> \n> > does not seem to return a valid tar archive anymore when it did work\n> > previously. In fact the man page of my version has that syntax in one of\n> > the examples.\n> \n> That sounds like a documentation bug.\n> \n> cmn\n"},{"id":"182287","messageId":"20120110225011.GJ2714@centaur.lab.cmartin.tk","threadId":"29333","inReplyTo":"1431498.0yPWNQLupF@xps","subject":"Re: [BUG] git archive broken in 1.7.8.1","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-01-10T22:50:11Z","receivedAt":"2012-01-10T22:50:11Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Tue, Jan 10, 2012 at 11:05:45PM +0100, Albert Astals Cid wrote:\n> El Dimarts, 10 de gener de 2012, a les 22:33:44, Carlos Martín Nieto va \n> escriure:\n> > On Tue, Jan 10, 2012 at 10:18:41PM +0100, Albert Astals Cid wrote:\n> > > CC me on answers since i'm not subscribed to the list\n> > > \n> > > Hi, one of our [KDE] anongit servers was updated to 1.7.8.1 and not the\n> > > syntax\n> > > \n> > > git archive --remote=git://anongit.kde.org/repo.git HEAD:path\n> > \n> > This syntax is no longer allowed due to some security tightening. Use\n> > the alternate syntax\n> > \n> > git archive --remote=git://anongit.kde.org/repo.git HEAD -- path\n> \n> Unfortunately this producess a tarball with a different layout, e.g.\n> \n> git archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD:doc/en_US\n> gives me a tarball with the doc/en_US files in the root\n> \n> git archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD -- doc/en_US\n> gives me a tarball with the doc/en_US folders and then the files\n> \n> Is there a way to keep the old behaviour or do we need to update our scripts?\n\nNot as far as I know. However, the commit that hardened the input\n(ee27ca4a781844: archive: don't let remote clients get unreachable\ncommits, 2011-11-17) does state that HEAD:doc/en_US should be valid,\nso it looks like it's actually a regression. As it's bedtime in my\ntimezone, I'm blaming Peff and I'll look into this if it hasn't been\nfixed by the time I get to the office tomorrow.\n\n> \n> Thanks for the fast answer!\n> \n> Albert\n> \n> > \n> > > does not seem to return a valid tar archive anymore when it did work\n> > > previously. In fact the man page of my version has that syntax in one of\n> > > the examples.\n> > \n> > That sounds like a documentation bug.\n\nNotice that the syntax is for the local case, not for --remote.\n\n cmn\n"},{"id":"182289","messageId":"20120110230122.GA24020@vent.lifeintegrity.localnet","threadId":"29333","inReplyTo":"1431498.0yPWNQLupF@xps","subject":"Re: [BUG] git archive broken in 1.7.8.1","fromName":"Allan Wind","fromEmail":"allan_wind@lifeintegrity.com","sentAt":"2012-01-10T23:01:22Z","receivedAt":"2012-01-10T23:01:22Z","isPatch":false,"sender":{"key":"allan_wind@lifeintegrity.com","avatar":null},"body":"On 2012-01-10 23:05:45, Albert Astals Cid wrote:\n> Unfortunately this producess a tarball with a different layout, e.g.\n> \n> git archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD:doc/en_US\n> gives me a tarball with the doc/en_US files in the root\n\nMeaning the files you have stored in git under doc/en_US are \ndumped in the root directory of the tar? That does not sound \nlike desired behavior for the feature.\n\n\n/Allan\n-- \nAllan Wind\nLife Integrity, LLC\n\n"},{"id":"182291","messageId":"20120110232132.GA29245@sigill.intra.peff.net","threadId":"29333","inReplyTo":"20120110225011.GJ2714@centaur.lab.cmartin.tk","subject":"Re: [BUG] git archive broken in 1.7.8.1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-10T23:21:32Z","receivedAt":"2012-01-10T23:21:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 10, 2012 at 11:50:11PM +0100, Carlos Martín Nieto wrote:\n\n> > Unfortunately this producess a tarball with a different layout, e.g.\n> > \n> > git archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD:doc/en_US\n> > gives me a tarball with the doc/en_US files in the root\n> > \n> > git archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD -- doc/en_US\n> > gives me a tarball with the doc/en_US folders and then the files\n> > \n> > Is there a way to keep the old behaviour or do we need to update our scripts?\n> \n> Not as far as I know. However, the commit that hardened the input\n> (ee27ca4a781844: archive: don't let remote clients get unreachable\n> commits, 2011-11-17) does state that HEAD:doc/en_US should be valid,\n> so it looks like it's actually a regression. As it's bedtime in my\n> timezone, I'm blaming Peff and I'll look into this if it hasn't been\n> fixed by the time I get to the office tomorrow.\n\nIt is definitely my fault. According to ee27ca4a, sub-trees were an\nunfortunate casualty of the tightening. However, I did have some patches\nthat moved towards allowing things like that safely (basically the rev\nparser needs to pass more context back to the caller).\n\nIt's evening here and I'm doing family stuff, but I'll take a look\neither late tonight or tomorrow morning.\n\n-Peff\n"},{"id":"182343","messageId":"1326283958-30271-1-git-send-email-cmn@elego.de","threadId":"29333","inReplyTo":"20120110232132.GA29245@sigill.intra.peff.net","subject":"[PATCH] archive: re-allow HEAD:Documentation on a remote invocation","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-01-11T12:12:38Z","receivedAt":"2012-01-11T12:12:38Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"The tightening done in (ee27ca4a: archive: don't let remote clients\nget unreachable commits, 2011-11-17) went too far and disallowed\nHEAD:Documentation as it would try to find \"HEAD:Documentation\" as a\nref.\n\nOnly DWIM the \"HEAD\" part to see if it exists as a ref. Once we're\nsure that we've been given a valid ref, we follow the normal code\npath. This still disallows attempts to access commits which are not\nbranch tips.\n\nSigned-off-by: Carlos Martín Nieto \n---\n\nAFAICT this should still be safe. Using HEAD^:Documentation or\n:Documentation still complains that HEAD^ and aren't\nrefs.\n\n archive.c | 19 +++++++++++++------\n 1 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 164bbd0..4735bfb 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -260,14 +260,21 @@ static void parse_treeish_arg(const char **argv,\n \t/* Remotes are only allowed to fetch actual refs */\n \tif (remote) {\n \t\tchar *ref = NULL;\n-\t\tif (!dwim_ref(name, strlen(name), sha1, &ref))\n-\t\t\tdie(\"no such ref: %s\", name);\n+\t\tconst char *refname, *colon = NULL;\n+\n+\t\tcolon = strchr(name, ':');\n+\t\tif (colon)\n+\t\t\trefname = xstrndup(name, colon - name);\n+\t\telse\n+\t\t\trefname = name;\n+\n+\t\tif (!dwim_ref(refname, strlen(refname), sha1, &ref))\n+\t\t\tdie(\"no such ref: %s\", refname);\n \t\tfree(ref);\n \t}\n-\telse {\n-\t\tif (get_sha1(name, sha1))\n-\t\t\tdie(\"Not a valid object name\");\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-- \n1.7.8.352.g876a6f\n"},{"id":"182348","messageId":"20120111125113.GA11984@beez.lab.cmartin.tk","threadId":"29333","inReplyTo":"20120110230122.GA24020@vent.lifeintegrity.localnet","subject":"Re: [BUG] git archive broken in 1.7.8.1","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-01-11T12:51:13Z","receivedAt":"2012-01-11T12:51:13Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Tue, Jan 10, 2012 at 06:01:22PM -0500, Allan Wind wrote:\n> On 2012-01-10 23:05:45, Albert Astals Cid wrote:\n> > Unfortunately this producess a tarball with a different layout, e.g.\n> > \n> > git archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD:doc/en_US\n> > gives me a tarball with the doc/en_US files in the root\n> \n> Meaning the files you have stored in git under doc/en_US are \n> dumped in the root directory of the tar? That does not sound \n> like desired behavior for the feature.\n\nWhich feature do you mean? Using HEAD:doc/en_US should be equivalent\nto running `tar cf - .` from inside the doc/en_US, as what you're\npassing is that tree.\n\nUsing HEAD -- doc/en_US however means that you want to pack HEAD, but\nlimit it to files that match doc/en_US, so it's working as\ndesigned. If you mean that 'HEAD -- doc/en_US' shouldn't give you a\ntar with those files at the root, then we agree.\n\n cmn\n"},{"id":"182371","messageId":"20120111193916.GA12333@sigill.intra.peff.net","threadId":"29333","inReplyTo":"1326283958-30271-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-11T19:39:16Z","receivedAt":"2012-01-11T19:39:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 11, 2012 at 01:12:38PM +0100, Carlos Martín Nieto wrote:\n\n> The tightening done in (ee27ca4a: archive: don't let remote clients\n> get unreachable commits, 2011-11-17) went too far and disallowed\n> HEAD:Documentation as it would try to find \"HEAD:Documentation\" as a\n> ref.\n> \n> Only DWIM the \"HEAD\" part to see if it exists as a ref. Once we're\n> sure that we've been given a valid ref, we follow the normal code\n> path. This still disallows attempts to access commits which are not\n> branch tips.\n\nI'd rather not do this kind of ad-hoc parsing of sha1s in the archive\ncode, and instead let the regular resolution process tell us more about\nwhat it did, so we can make a policy decision at the upper level.\n\nPatches to follow:\n\n [1/2]: get_sha1_with_context: report features used in resolution\n [2/2]: archive: loosen restrictions on remote object lookup\n\n> AFAICT this should still be safe. Using HEAD^:Documentation or\n> :Documentation still complains that HEAD^ and aren't\n> refs.\n\nMy patches enable things like HEAD^, but disallow a raw sha1. The only\nway to safely allow a raw sha1 is to check its connectivity from the\ntips, which can be somewhat expensive (you have to traverse every tree\nof every commit in the worst case).\n\n-Peff\n"},{"id":"182372","messageId":"20120111194210.GA12441@sigill.intra.peff.net","threadId":"29333","inReplyTo":"20120111193916.GA12333@sigill.intra.peff.net","subject":"[PATCH 1/2] get_sha1_with_context: report features used in resolution","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-11T19:42:10Z","receivedAt":"2012-01-11T19:42:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most callers generally treat get_sha1 as a black box, giving\nit a string from the user and expecting to get a sha1 in\nreturn. The get_sha1_with_context function gives callers\nmore information about what happened while resolving the\nobject name so they can make better decisions about how to\nuse the result. We currently use this only to provide\ninformation about the path entry used to find a blob.\n\nWe don't currently provide any information about the\nresolution rules that were used to reach the final object.\nSome callers may want these in order to enforce a policy\nthat a particular subset of the lookup rules are used (e.g.,\nwhen serving remote requests).\n\nThis patch adds a set of bit-fields that document the use of\nparticular features during an object lookup.\n\nSigned-off-by: Jeff King \n---\nThe diffstat looks a little scary, but it is mostly just the internal\nget_sha1 functions learning to pass the object_context around.\n\n cache.h | 7 +++++\n sha1_name.c | 73 +++++++++++++++++++++++++++++++++++++---------------------\n 2 files changed, 53 insertions(+), 27 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 10afd71..ac25a37 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -809,6 +809,13 @@ struct object_context {\n \tunsigned char tree[20];\n \tchar path[PATH_MAX];\n \tunsigned mode;\n+\tunsigned used_ref:1,\n+\t\t used_reflog:1,\n+\t\t used_index:1,\n+\t\t used_nth_checkout:1,\n+\t\t used_describe_name:1,\n+\t\t used_oneline:1,\n+\t\t used_raw_hex:1;\n };\n \n extern int get_sha1(const char *str, unsigned char *sha1);\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 03ffc2c..ad7c52a 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -7,7 +7,8 @@\n #include \"refs.h\"\n #include \"remote.h\"\n \n-static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *);\n+static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *,\n+\t\t\t struct object_context *);\n \n static int find_short_object_filename(int len, const char *name, unsigned char *sha1)\n {\n@@ -158,7 +159,7 @@ static int find_unique_short_object(int len, char *canonical,\n }\n \n static int get_short_sha1(const char *name, int len, unsigned char *sha1,\n-\t\t\t int quietly)\n+\t\t\t int quietly, struct object_context *oc)\n {\n \tint i, status;\n \tchar canonical[40];\n@@ -190,6 +191,8 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,\n \tstatus = find_unique_short_object(i, canonical, res, sha1);\n \tif (!quietly && (status == SHORT_NAME_AMBIGUOUS))\n \t\treturn error(\"short SHA1 %.*s is ambiguous.\", len, canonical);\n+\tif (!status)\n+\t\toc->used_raw_hex = 1;\n \treturn status;\n }\n \n@@ -204,7 +207,8 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)\n \t\treturn hex;\n \twhile (len < 40) {\n \t\tunsigned char sha1_ret[20];\n-\t\tstatus = get_short_sha1(hex, len, sha1_ret, 1);\n+\t\tstruct object_context unused;\n+\t\tstatus = get_short_sha1(hex, len, sha1_ret, 1, &unused);\n \t\tif (exists\n \t\t ? !status\n \t\t : status == SHORT_NAME_NOT_FOUND) {\n@@ -255,17 +259,21 @@ static inline int upstream_mark(const char *string, int len)\n \treturn 0;\n }\n \n-static int get_sha1_1(const char *name, int len, unsigned char *sha1);\n+static int get_sha1_1(const char *name, int len, unsigned char *sha1,\n+\t\t struct object_context *oc);\n \n-static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n+static int get_sha1_basic(const char *str, int len, unsigned char *sha1,\n+\t\t\t struct object_context *oc)\n {\n \tstatic const char *warn_msg = \"refname '%.*s' is ambiguous.\";\n \tchar *real_ref = NULL;\n \tint refs_found = 0;\n \tint at, reflog_len;\n \n-\tif (len == 40 && !get_sha1_hex(str, sha1))\n+\tif (len == 40 && !get_sha1_hex(str, sha1)) {\n+\t\toc->used_raw_hex = 1;\n \t\treturn 0;\n+\t}\n \n \t/* basic@{time or number or -number} format to query ref-log */\n \treflog_len = at = 0;\n@@ -292,7 +300,8 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\tret = interpret_branch_name(str+at, &buf);\n \t\tif (ret > 0) {\n \t\t\t/* substitute this branch name and restart */\n-\t\t\treturn get_sha1_1(buf.buf, buf.len, sha1);\n+\t\t\toc->used_nth_checkout = 1;\n+\t\t\treturn get_sha1_1(buf.buf, buf.len, sha1, oc);\n \t\t} else if (ret == 0) {\n \t\t\treturn -1;\n \t\t}\n@@ -305,6 +314,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \n \tif (!refs_found)\n \t\treturn -1;\n+\toc->used_ref = 1;\n \n \tif (warn_ambiguous_refs && refs_found > 1)\n \t\twarning(warn_msg, len, str);\n@@ -352,6 +362,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\t\t\t len, str, co_cnt);\n \t\t\t}\n \t\t}\n+\t\toc->used_reflog = 1;\n \t}\n \n \tfree(real_ref);\n@@ -359,10 +370,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n }\n \n static int get_parent(const char *name, int len,\n-\t\t unsigned char *result, int idx)\n+\t\t unsigned char *result, int idx,\n+\t\t struct object_context *oc)\n {\n \tunsigned char sha1[20];\n-\tint ret = get_sha1_1(name, len, sha1);\n+\tint ret = get_sha1_1(name, len, sha1, oc);\n \tstruct commit *commit;\n \tstruct commit_list *p;\n \n@@ -389,13 +401,14 @@ static int get_parent(const char *name, int len,\n }\n \n static int get_nth_ancestor(const char *name, int len,\n-\t\t\t unsigned char *result, int generation)\n+\t\t\t unsigned char *result, int generation,\n+\t\t\t struct object_context *oc)\n {\n \tunsigned char sha1[20];\n \tstruct commit *commit;\n \tint ret;\n \n-\tret = get_sha1_1(name, len, sha1);\n+\tret = get_sha1_1(name, len, sha1, oc);\n \tif (ret)\n \t\treturn ret;\n \tcommit = lookup_commit_reference(sha1);\n@@ -436,7 +449,8 @@ struct object *peel_to_type(const char *name, int namelen,\n \t}\n }\n \n-static int peel_onion(const char *name, int len, unsigned char *sha1)\n+static int peel_onion(const char *name, int len, unsigned char *sha1,\n+\t\t struct object_context *oc)\n {\n \tunsigned char outer[20];\n \tconst char *sp;\n@@ -476,7 +490,7 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n \telse\n \t\treturn -1;\n \n-\tif (get_sha1_1(name, sp - name - 2, outer))\n+\tif (get_sha1_1(name, sp - name - 2, outer, oc))\n \t\treturn -1;\n \n \to = parse_object(outer);\n@@ -515,14 +529,15 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n \n \t\tprefix = xstrndup(sp + 1, name + len - 1 - (sp + 1));\n \t\tcommit_list_insert((struct commit *)o, &list);\n-\t\tret = get_sha1_oneline(prefix, sha1, list);\n+\t\tret = get_sha1_oneline(prefix, sha1, list, oc);\n \t\tfree(prefix);\n \t\treturn ret;\n \t}\n \treturn 0;\n }\n \n-static int get_describe_name(const char *name, int len, unsigned char *sha1)\n+static int get_describe_name(const char *name, int len, unsigned char *sha1,\n+\t\t\t struct object_context *oc)\n {\n \tconst char *cp;\n \n@@ -535,14 +550,15 @@ static int get_describe_name(const char *name, int len, unsigned char *sha1)\n \t\t\tif (ch == 'g' && cp[-1] == '-') {\n \t\t\t\tcp++;\n \t\t\t\tlen -= cp - name;\n-\t\t\t\treturn get_short_sha1(cp, len, sha1, 1);\n+\t\t\t\treturn get_short_sha1(cp, len, sha1, 1, oc);\n \t\t\t}\n \t\t}\n \t}\n \treturn -1;\n }\n \n-static int get_sha1_1(const char *name, int len, unsigned char *sha1)\n+static int get_sha1_1(const char *name, int len, unsigned char *sha1,\n+\t\t struct object_context *oc)\n {\n \tint ret, has_suffix;\n \tconst char *cp;\n@@ -569,25 +585,25 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1)\n \t\tif (!num && len1 == len - 1)\n \t\t\tnum = 1;\n \t\tif (has_suffix == '^')\n-\t\t\treturn get_parent(name, len1, sha1, num);\n+\t\t\treturn get_parent(name, len1, sha1, num, oc);\n \t\t/* else if (has_suffix == '~') -- goes without saying */\n-\t\treturn get_nth_ancestor(name, len1, sha1, num);\n+\t\treturn get_nth_ancestor(name, len1, sha1, num, oc);\n \t}\n \n-\tret = peel_onion(name, len, sha1);\n+\tret = peel_onion(name, len, sha1, oc);\n \tif (!ret)\n \t\treturn 0;\n \n-\tret = get_sha1_basic(name, len, sha1);\n+\tret = get_sha1_basic(name, len, sha1, oc);\n \tif (!ret)\n \t\treturn 0;\n \n \t/* It could be describe output that is \"SOMETHING-gXXXX\" */\n-\tret = get_describe_name(name, len, sha1);\n+\tret = get_describe_name(name, len, sha1, oc);\n \tif (!ret)\n \t\treturn 0;\n \n-\treturn get_short_sha1(name, len, sha1, 0);\n+\treturn get_short_sha1(name, len, sha1, 0, oc);\n }\n \n /*\n@@ -619,7 +635,8 @@ static int handle_one_ref(const char *path,\n }\n \n static int get_sha1_oneline(const char *prefix, unsigned char *sha1,\n-\t\t\t struct commit_list *list)\n+\t\t\t struct commit_list *list,\n+\t\t\t struct object_context *oc)\n {\n \tstruct commit_list *backup = NULL, *l;\n \tint found = 0;\n@@ -672,6 +689,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,\n \tfor (l = backup; l; l = l->next)\n \t\tclear_commit_marks(l->item, ONELINE_SEEN);\n \tfree_commit_list(backup);\n+\toc->used_oneline = found;\n \treturn found ? 0 : -1;\n }\n \n@@ -1029,7 +1047,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \n \tmemset(oc, 0, sizeof(*oc));\n \toc->mode = S_IFINVALID;\n-\tret = get_sha1_1(name, namelen, sha1);\n+\tret = get_sha1_1(name, namelen, sha1, oc);\n \tif (!ret)\n \t\treturn ret;\n \t/* sha1:path --> object name of path in ent sha1\n@@ -1046,7 +1064,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\tif (!only_to_die && namelen > 2 && name[1] == '/') {\n \t\t\tstruct commit_list *list = NULL;\n \t\t\tfor_each_ref(handle_one_ref, &list);\n-\t\t\treturn get_sha1_oneline(name + 2, sha1, list);\n+\t\t\treturn get_sha1_oneline(name + 2, sha1, list, oc);\n \t\t}\n \t\tif (namelen < 3 ||\n \t\t name[2] != ':' ||\n@@ -1081,6 +1099,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\t\tif (ce_stage(ce) == stage) {\n \t\t\t\thashcpy(sha1, ce->sha1);\n \t\t\t\toc->mode = ce->ce_mode;\n+\t\t\t\toc->used_index = 1;\n \t\t\t\tfree(new_path);\n \t\t\t\treturn 0;\n \t\t\t}\n@@ -1107,7 +1126,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\t\tstrncpy(object_name, name, cp-name);\n \t\t\tobject_name[cp-name] = '\\0';\n \t\t}\n-\t\tif (!get_sha1_1(name, cp-name, tree_sha1)) {\n+\t\tif (!get_sha1_1(name, cp-name, tree_sha1, oc)) {\n \t\t\tconst char *filename = cp+1;\n \t\t\tchar *new_filename = NULL;\n \n-- \n1.7.9.rc0.33.gd3c17\n"},{"id":"182373","messageId":"20120111194232.GB12441@sigill.intra.peff.net","threadId":"29333","inReplyTo":"20120111193916.GA12333@sigill.intra.peff.net","subject":"[PATCH 2/2] archive: loosen restrictions on remote object lookup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-11T19:42:32Z","receivedAt":"2012-01-11T19:42:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Initially, \"git upload-archive\" would feed the tree\nspecification from the remote side directly into get_sha1,\ngiving the remote user the full power of the object name\nresolver. This was convenient, but it also meant that remote\nusers could fetch disconnected trees by their sha1s, which\nviolates the long-standing behavior of upload-pack not to\nmake such objects available.\n\nLater, commit ee27ca4 tightened this to use dwim_ref instead\nof get_sha1 for the remote case, allowing only the use of\nactual refs. Unfortunately, this broke some existing use\ncases, like fetching sub-trees with \"$ref:subdir\".\n\nThis patch loosens the restrictions to re-enable those use\ncases. It does this by using get_sha1_with_context for the\nobject lookup, and checking that only allowable features\nwere used.\n\nSigned-off-by: Jeff King \n---\n archive.c | 34 ++++++++++++++-------\n t/t5002-archive-resolution.sh | 66 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 89 insertions(+), 11 deletions(-)\n create mode 100755 t/t5002-archive-resolution.sh\n\ndiff --git a/archive.c b/archive.c\nindex 164bbd0..a031bde 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -246,6 +246,25 @@ static void parse_pathspec_arg(const char **pathspec,\n \t}\n }\n \n+static int check_object_context(int remote, const struct object_context *oc)\n+{\n+\t/* For local requests, allow anything */\n+\tif (!remote)\n+\t\treturn 1;\n+\t/*\n+\t * Otherwise, require that we accessed the object through a ref,\n+\t * but not have used any of the advanced features like looking in\n+\t * the reflog.\n+\t */\n+\treturn oc->used_ref &&\n+\t !oc->used_reflog &&\n+\t !oc->used_index &&\n+\t !oc->used_nth_checkout &&\n+\t !oc->used_describe_name &&\n+\t !oc->used_oneline &&\n+\t !oc->used_raw_hex;\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@@ -256,18 +275,11 @@ static void parse_treeish_arg(const char **argv,\n \tstruct tree *tree;\n \tconst struct commit *commit;\n \tunsigned char sha1[20];\n+\tstruct object_context oc;\n \n-\t/* Remotes are only allowed to fetch actual refs */\n-\tif (remote) {\n-\t\tchar *ref = NULL;\n-\t\tif (!dwim_ref(name, strlen(name), sha1, &ref))\n-\t\t\tdie(\"no such ref: %s\", name);\n-\t\tfree(ref);\n-\t}\n-\telse {\n-\t\tif (get_sha1(name, sha1))\n-\t\t\tdie(\"Not a valid object name\");\n-\t}\n+\tif (get_sha1_with_context(name, sha1, &oc) ||\n+\t !check_object_context(remote, &oc))\n+\t\tdie(\"Not a valid object name\");\n \n \tcommit = lookup_commit_reference_gently(sha1, 1);\n \tif (commit) {\ndiff --git a/t/t5002-archive-resolution.sh b/t/t5002-archive-resolution.sh\nnew file mode 100755\nindex 0000000..bf2b55c\n--- /dev/null\n+++ b/t/t5002-archive-resolution.sh\n@@ -0,0 +1,66 @@\n+#!/bin/sh\n+\n+test_description='test object resolution methods for local and remote archive'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo a >a &&\n+\tgit add . &&\n+\tgit commit -m one &&\n+\tsha1_one=`git rev-parse HEAD` &&\n+\tmkdir subdir &&\n+\techo b >subdir/b &&\n+\tgit add . &&\n+\tgit commit -m two &&\n+\tgit checkout -b other &&\n+\tgit checkout master\n+'\n+\n+while read desc where what expect; do\n+\tcmd=\"git archive --format=tar -o result.tar\"\n+\ttest \"$where\" = \"remote\" && cmd=\"$cmd --remote=.\"\n+\tcmd=\"$cmd $what\"\n+\n+\tif test \"$expect\" = \"deny\"; then\n+\t\ttest_expect_success \"archive $desc ($where, should deny)\" \"\n+\t\t\ttest_must_fail $cmd\n+\t\t\"\n+\telse\n+\t\ttest_expect_success \"archive $desc ($where, should work)\" '\n+\t\t\t'\"$cmd\"' &&\n+\t\t\tfor i in '\"$expect\"'; do\n+\t\t\t\techo \"$i:`basename $i`\"\n+\t\t\tdone >expect &&\n+\t\t\trm -rf result &&\n+\t\t\tmkdir result &&\n+\t\t\t(cd result &&\n+\t\t\ttar xf ../result.tar &&\n+\t\t\tfor i in `find * -type f`; do\n+\t\t\t\techo \"$i:`cat $i`\"\n+\t\t\tdone >../actual\n+\t\t\t) &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\tfi\n+done < writes:\n\n> Most callers generally treat get_sha1 as a black box, giving\n> it a string from the user and expecting to get a sha1 in\n> return. The get_sha1_with_context function gives callers\n> more information about what happened while resolving the\n> object name so they can make better decisions about how to\n> use the result. We currently use this only to provide\n> information about the path entry used to find a blob.\n>\n> We don't currently provide any information about the\n> resolution rules that were used to reach the final object.\n> Some callers may want these in order to enforce a policy\n> that a particular subset of the lookup rules are used (e.g.,\n> when serving remote requests).\n>\n> This patch adds a set of bit-fields that document the use of\n> particular features during an object lookup.\n>\n> Signed-off-by: Jeff King \n> ---\n> The diffstat looks a little scary, but it is mostly just the internal\n> get_sha1 functions learning to pass the object_context around.\n\nHmm, shouldn't this also cover peel_to_type()? That would have made it\nalso apply to the maintenance track.\n"},{"id":"182402","messageId":"7vipkh4oyn.fsf@alter.siamese.dyndns.org","threadId":"29333","inReplyTo":"1326283958-30271-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-12T02:46:56Z","receivedAt":"2012-01-12T02:46:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> The tightening done in (ee27ca4a: archive: don't let remote clients\n> get unreachable commits, 2011-11-17) went too far and disallowed\n> HEAD:Documentation as it would try to find \"HEAD:Documentation\" as a\n> ref.\n\nI do not think it went too far. Actually we discussed this exact issue\nwhen the topic was cooking, and saw no objections. The commit in question\nitself advertises this restriction.\n\nWhy are we loosening it now? I do not see a compelling reason to do so.\n\n> Only DWIM the \"HEAD\" part to see if it exists as a ref. Once we're\n> sure that we've been given a valid ref, we follow the normal code\n> path. This still disallows attempts to access commits which are not\n> branch tips.\n>\n> Signed-off-by: Carlos Martín Nieto \n> ---\n> AFAICT this should still be safe. Using HEAD^:Documentation or\n> :Documentation still complains that HEAD^ and aren't\n> refs.\n\nHaving said that, I think I agree this is a safe thing to do, _if_ we want\nto loosen it.\n"},{"id":"182403","messageId":"20120112025126.GA25365@sigill.intra.peff.net","threadId":"29333","inReplyTo":"7vmx9t4pgj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] get_sha1_with_context: report features used in resolution","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-12T02:51:26Z","receivedAt":"2012-01-12T02:51:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 11, 2012 at 06:36:12PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Most callers generally treat get_sha1 as a black box, giving\n> > it a string from the user and expecting to get a sha1 in\n> > return. The get_sha1_with_context function gives callers\n> > more information about what happened while resolving the\n> > object name so they can make better decisions about how to\n> > use the result. We currently use this only to provide\n> > information about the path entry used to find a blob.\n> >\n> > We don't currently provide any information about the\n> > resolution rules that were used to reach the final object.\n> > Some callers may want these in order to enforce a policy\n> > that a particular subset of the lookup rules are used (e.g.,\n> > when serving remote requests).\n> >\n> > This patch adds a set of bit-fields that document the use of\n> > particular features during an object lookup.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> > The diffstat looks a little scary, but it is mostly just the internal\n> > get_sha1 functions learning to pass the object_context around.\n> \n> Hmm, shouldn't this also cover peel_to_type()? That would have made it\n> also apply to the maintenance track.\n\nI don't see how peel_to_type is relevant. As far as get_sha1 is\nconcerned, the interesting thing is actually calling peel_onion. It does\nget the context passed to it in my patch, but I didn't bother marking\nthat the peel feature was used (because it wasn't relevant to the policy\nI wanted to implement in the follow-on patch).\n\nBut we could pretty easily mark the use of the peel feature, too.\n\nI'm not sure what you mean about the maintenance track, though. AFAICT,\nwe don't separately call peel_to_type, but just potentially use it as\npart of get_sha1_with_context. Am I missing something?\n\n-Peff\n"},{"id":"182405","messageId":"20120112025445.GB25365@sigill.intra.peff.net","threadId":"29333","inReplyTo":"7vipkh4oyn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-12T02:54:45Z","receivedAt":"2012-01-12T02:54:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 11, 2012 at 06:46:56PM -0800, Junio C Hamano wrote:\n\n> Carlos Martín Nieto writes:\n> \n> > The tightening done in (ee27ca4a: archive: don't let remote clients\n> > get unreachable commits, 2011-11-17) went too far and disallowed\n> > HEAD:Documentation as it would try to find \"HEAD:Documentation\" as a\n> > ref.\n> \n> I do not think it went too far. Actually we discussed this exact issue\n> when the topic was cooking, and saw no objections. The commit in question\n> itself advertises this restriction.\n\nI think you and I discussed it off list (I originally took this off-list\nbecause the original issue did have some security implications). So I\ndon't think people necessarily had a chance to object.\n\n> Why are we loosening it now? I do not see a compelling reason to do so.\n\nI see it the opposite way. People are clearly using the \"$ref:$path\"\nsyntax. So why would we restrict them from doing so? There are no\nsecurity implications (i.e., they could always just grab $ref and\nextract $path themselves). In my view, ee27ca4a was over-eager in its\nrestrictions because I wanted it to be simple and close the hole. Now we\ncan take our time adding more code to loosen it.\n\n-Peff\n"},{"id":"182406","messageId":"20120112025910.GA26038@sigill.intra.peff.net","threadId":"29333","inReplyTo":"20120112025445.GB25365@sigill.intra.peff.net","subject":"Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-12T02:59:10Z","receivedAt":"2012-01-12T02:59:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 11, 2012 at 09:54:45PM -0500, Jeff King wrote:\n\n> On Wed, Jan 11, 2012 at 06:46:56PM -0800, Junio C Hamano wrote:\n> \n> > Carlos Martín Nieto writes:\n> > \n> > > The tightening done in (ee27ca4a: archive: don't let remote clients\n> > > get unreachable commits, 2011-11-17) went too far and disallowed\n> > > HEAD:Documentation as it would try to find \"HEAD:Documentation\" as a\n> > > ref.\n> > \n> > I do not think it went too far. Actually we discussed this exact issue\n> > when the topic was cooking, and saw no objections. The commit in question\n> > itself advertises this restriction.\n> \n> I think you and I discussed it off list (I originally took this off-list\n> because the original issue did have some security implications). So I\n> don't think people necessarily had a chance to object.\n\nHere is the only on-list discussion:\n\n http://article.gmane.org/gmane.comp.version-control.git/186366\n\nQuoted below:\n\n >> * jk/maint-1.6.2-upload-archive (2011-11-21) 1 commit\n >> - archive: don't let remote clients get unreachable commits\n >> (this branch is used by jk/maint-upload-archive.)\n >>\n >> * jk/maint-upload-archive (2011-11-21) 1 commit\n >> - Merge branch 'jk/maint-1.6.2-upload-archive' into\n >> jk/maint-upload-archive\n >> (this branch uses jk/maint-1.6.2-upload-archive.)\n >>\n >> Will merge to 'next' after taking another look.\n >\n > Thanks. I also have some followup patches to re-loosen to at least\n > trees reachable from refs. Do you want to leave the tightening to\n > the maint track, and then consider the re-loosening for master?\n\n I was planning to first have the really tight version graduate to\n 'master' and ship it in 1.7.9, while possibly merging that to 1.7.8.X\n series. If we hear complaints from real users in the meantime before\n or after such releases, we could apply loosening patch on top of these\n topics and call them \"regression fix\", but I have been assuming that\n nobody would have been using this backdoor for anything that really\n matters.\n\nSo now we have heard a complaint. :)\n\n-Peff\n"},{"id":"182408","messageId":"7vehv54o6v.fsf@alter.siamese.dyndns.org","threadId":"29333","inReplyTo":"20120112025445.GB25365@sigill.intra.peff.net","subject":"Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-12T03:03:36Z","receivedAt":"2012-01-12T03:03:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I see it the opposite way. People are clearly using the \"$ref:$path\"\n> syntax. So why would we restrict them from doing so? There are no\n> security implications (i.e., they could always just grab $ref and\n> extract $path themselves). In my view, ee27ca4a was over-eager in its\n> restrictions because I wanted it to be simple and close the hole. Now we\n> can take our time adding more code to loosen it.\n\nOk, so it is more like a partial revert of whatever we did. In that case,\nI'd take CMN's patch to limit the extent of the changes, as it more\nclosely matches the spirit of the original ee27ca4 (archive: don't let\nremote clients get unreachable commits, 2011-11-17) that singled out and\ncatered to the need of \"archive\" command alone. It is already part of the\nv1.7.8.1 release, so I would prefer a change to be stupid and simple.\n"},{"id":"182412","messageId":"20120112031022.GC26363@sigill.intra.peff.net","threadId":"29333","inReplyTo":"7vehv54o6v.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-12T03:10:22Z","receivedAt":"2012-01-12T03:10:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 11, 2012 at 07:03:36PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I see it the opposite way. People are clearly using the \"$ref:$path\"\n> > syntax. So why would we restrict them from doing so? There are no\n> > security implications (i.e., they could always just grab $ref and\n> > extract $path themselves). In my view, ee27ca4a was over-eager in its\n> > restrictions because I wanted it to be simple and close the hole. Now we\n> > can take our time adding more code to loosen it.\n> \n> Ok, so it is more like a partial revert of whatever we did. In that case,\n> I'd take CMN's patch to limit the extent of the changes, as it more\n> closely matches the spirit of the original ee27ca4 (archive: don't let\n> remote clients get unreachable commits, 2011-11-17) that singled out and\n> catered to the need of \"archive\" command alone. It is already part of the\n> v1.7.8.1 release, so I would prefer a change to be stupid and simple.\n\nFor a maint release, I am OK with that. In the long term, I'd rather my\npatches go onto master (either for 1.7.9 or for later), as I think they\nare the right way to do it.\n\n-Peff\n"},{"id":"182413","messageId":"7v62gh4nex.fsf@alter.siamese.dyndns.org","threadId":"29333","inReplyTo":"20120112031022.GC26363@sigill.intra.peff.net","subject":"Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-12T03:20:22Z","receivedAt":"2012-01-12T03:20:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> For a maint release, I am OK with that. In the long term, I'd rather my\n> patches go onto master (either for 1.7.9 or for later), as I think they\n> are the right way to do it.\n\nI would prefer not to use it in 1.7.9, as CMN's patch will make its only\nimmediate user disappear. I think we are in agreement that the change to\ncarry context around is a longer term thing, and when we apply it, the\nsafety we added to \"archive\" will become simpler.\n"},{"id":"218808","messageId":"loom.20130529T133942-310@post.gmane.org","threadId":"29333","inReplyTo":"20120111194232.GB12441@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] archive: loosen restrictions on remote object lookup","fromName":"Ian Harvey","fromEmail":"iharvey@good.com","sentAt":"2013-05-29T12:05:41Z","receivedAt":"2013-05-29T12:05:41Z","isPatch":true,"sender":{"key":"iharvey@good.com","avatar":null},"body":"So, did this patch make it anywhere? We could really use it.\n\nHere's the use case. The original ee27ca4 patch broke our build system when\nthe git server was upgraded to Debian Wheezy last night. The builder fetches\nsource from the repo in two pieces using git archive, and we need to make\nsure both pieces are from the same commit. So we get a sha1 hash with git\nls-remote, and use it with git archive --remote. This, of course, breaks\nwith the 'no such ref' error.\n\nAt the very least, the documentation is wrong when it talks about passing a\ncommit ID to git archive: maintainers must surely agree that the\ndocumentation and the actual behavior ought to match.\n\nPersonally, I'd like to see this patch adopted.\n\nThanks for listening,\nIan\n"},{"id":"219462","messageId":"20130605163823.GE8664@sigill.intra.peff.net","threadId":"29333","inReplyTo":"loom.20130529T133942-310@post.gmane.org","subject":"Re: [PATCH 2/2] archive: loosen restrictions on remote object lookup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-05T16:38:23Z","receivedAt":"2013-06-05T16:38:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 29, 2013 at 12:05:41PM +0000, Ian Harvey wrote:\n\n> So, did this patch make it anywhere? We could really use it.\n> \n> Here's the use case. The original ee27ca4 patch broke our build system when\n> the git server was upgraded to Debian Wheezy last night. The builder fetches\n> source from the repo in two pieces using git archive, and we need to make\n> sure both pieces are from the same commit. So we get a sha1 hash with git\n> ls-remote, and use it with git archive --remote. This, of course, breaks\n> with the 'no such ref' error.\n\nThe patch you are responding to[1] would not help there, either. It does\nnot allow raw sha1s. The only way to do that would be:\n\n 1. Add an option to the server to allow arbitrary sha1s, even if they\n are not reachable from the ref tips. This is an easy fix, but\n requires server admins to cooperate (and they may or may not want\n to lose the \"you can only access reachable things policy\".\n\n 2. Actually do a reachability check. Doing a full object check to\n allow fetching an arbitrary tree by sha1 is probably prohibitively\n expensive[2], but we could allow the form \"[:]\", check\n that \"\" is reachable, and then allow arbitrary paths within\n it.\n\n> At the very least, the documentation is wrong when it talks about passing a\n> commit ID to git archive: maintainers must surely agree that the\n> documentation and the actual behavior ought to match.\n\nI am not sure which documentation you mean. The part about \"commit ID\"\nin the current manpage is drawing the distinction between something that\nresolves to a commit versus something that resolves to a tree. Either is\navailable both locally and remotely. I think the use of the phrase\n\"commit ID\" is questionable there, as it really means \"something that\nresolves to a commit\", not \"a sha1 commit ID\". We used to use the phrase\n\"commit-ish\" to refer to that, but I think it has fallen out of favor as\nbeing too jargon-y.\n\nThe documentation does not mention at all the restrictions placed on\nrefs using \"--remote\", and it probably should.\n\n-Peff\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/188387\n\n[2] If we had a reachability bitmap cache, calculating arbitrary object\n reachability would actually be pretty cheap. But the bitmap feature\n for core git is not yet ready for prime-time, so I think we should\n not depend on it yet.\n"},{"id":"219473","messageId":"20130605223551.GF8664@sigill.intra.peff.net","threadId":"29333","inReplyTo":"20130605163823.GE8664@sigill.intra.peff.net","subject":"[RFC/PATCH 0/4] real reachability checks for upload-archive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-05T22:35:51Z","receivedAt":"2013-06-05T22:35:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 05, 2013 at 12:38:23PM -0400, Jeff King wrote:\n\n> 2. Actually do a reachability check. Doing a full object check to\n> allow fetching an arbitrary tree by sha1 is probably prohibitively\n> expensive[2], but we could allow the form \"[:]\", check\n> that \"\" is reachable, and then allow arbitrary paths within\n> it.\n\nThinking on this more, the full reachability check is no worse than what\na clone has to do to fetch the full repository. Here's a series that\ndoes the full check. I'm not entirely happy with the performance,\nthough; details are in patch 3.\n\nI think I'd be tempted to just go the more limiting \"commit is\nreachable\" route, instead, which would solve your case (and most sane\ncases).\n\n [1/4]: clear parsed flag when we free tree buffers\n [2/4]: upload-archive: restrict remote objects with reachability check\n [3/4]: list-objects: optimize \"revs->blob_objects = 0\" case\n [4/4]: archive: ignore blob objects when checking reachability\n\n-Peff\n"},{"id":"219474","messageId":"20130605223739.GA15607@sigill.intra.peff.net","threadId":"29333","inReplyTo":"20130605223551.GF8664@sigill.intra.peff.net","subject":"[PATCH 1/4] clear parsed flag when we free tree buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-05T22:37:39Z","receivedAt":"2013-06-05T22:37:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many code paths will free a tree object's buffer and set it\nto NULL after finishing with it in order to keep memory\nusage down during a traversal. However, out of 8 sites that\ndo this, only one actually unsets the \"parsed\" flag back.\nThose sites that don't are setting a trap for later users of\nthe tree object; even after calling parse_tree, the buffer\nwill remain NULL, causing potential segfaults.\n\nIt is not known whether this is triggerable in the current\ncode. Most commands do not do an in-memory traversal\nfollowed by actually using the objects again. However, it\ndoes not hurt to be safe for future callers.\n\nIn most cases, we can abstract this out to a\n\"free_tree_buffer\" helper. However, there are two\nexceptions:\n\n 1. The fsck code relies on the parsed flag to know that we\n were able to parse the object at one point. We can\n switch this to using a flag in the \"flags\" field.\n\n 2. The index-pack code sets the buffer to NULL but does\n not free it (it is freed by a caller). We should still\n unset the parsed flag here, but we cannot use our\n helper, as we do not want to free the buffer.\n\nSigned-off-by: Jeff King \n---\nThis shouldn't have any behavior change, but I'd worry a bit that I\nmissed some case in builtin/fsck.c where the new HAS_OBJ flag would need\nset.\n\n builtin/fsck.c | 17 ++++++++---------\n builtin/index-pack.c | 1 +\n builtin/reflog.c | 3 +--\n http-push.c | 3 +--\n list-objects.c | 3 +--\n reachable.c | 3 +--\n revision.c | 3 +--\n tree.c | 8 ++++++++\n tree.h | 1 +\n walker.c | 5 +----\n 10 files changed, 24 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex bb9a2cd..579fdcc 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -16,6 +16,7 @@\n \n #define REACHABLE 0x0001\n #define SEEN 0x0002\n+#define HAS_OBJ 0x0004\n \n static int show_root;\n static int show_tags;\n@@ -101,7 +102,7 @@ static int mark_object(struct object *obj, int type, void *data)\n \tif (obj->flags & REACHABLE)\n \t\treturn 0;\n \tobj->flags |= REACHABLE;\n-\tif (!obj->parsed) {\n+\tif (!(obj->flags & HAS_OBJ)) {\n \t\tif (parent && !has_sha1_file(obj->sha1)) {\n \t\t\tprintf(\"broken link from %7s %s\\n\",\n \t\t\t\t typename(parent->type), sha1_to_hex(parent->sha1));\n@@ -127,16 +128,13 @@ static int traverse_one_object(struct object *obj)\n \tstruct tree *tree = NULL;\n \n \tif (obj->type == OBJ_TREE) {\n-\t\tobj->parsed = 0;\n \t\ttree = (struct tree *)obj;\n \t\tif (parse_tree(tree) < 0)\n \t\t\treturn 1; /* error already displayed */\n \t}\n \tresult = fsck_walk(obj, mark_object, obj);\n-\tif (tree) {\n-\t\tfree(tree->buffer);\n-\t\ttree->buffer = NULL;\n-\t}\n+\tif (tree)\n+\t\tfree_tree_buffer(tree);\n \treturn result;\n }\n \n@@ -178,7 +176,7 @@ static void check_reachable_object(struct object *obj)\n \t * except if it was in a pack-file and we didn't\n \t * do a full fsck\n \t */\n-\tif (!obj->parsed) {\n+\tif (!(obj->flags & HAS_OBJ)) {\n \t\tif (has_sha1_pack(obj->sha1))\n \t\t\treturn; /* it is in pack - forget about it */\n \t\tprintf(\"missing %s %s\\n\", typename(obj->type), sha1_to_hex(obj->sha1));\n@@ -306,8 +304,7 @@ static int fsck_obj(struct object *obj)\n \tif (obj->type == OBJ_TREE) {\n \t\tstruct tree *item = (struct tree *) obj;\n \n-\t\tfree(item->buffer);\n-\t\titem->buffer = NULL;\n+\t\tfree_tree_buffer(item);\n \t}\n \n \tif (obj->type == OBJ_COMMIT) {\n@@ -340,6 +337,7 @@ static int fsck_sha1(const unsigned char *sha1)\n \t\treturn error(\"%s: object corrupt or missing\",\n \t\t\t sha1_to_hex(sha1));\n \t}\n+\tobj->flags |= HAS_OBJ;\n \treturn fsck_obj(obj);\n }\n \n@@ -352,6 +350,7 @@ static int fsck_obj_buffer(const unsigned char *sha1, enum object_type type,\n \t\terrors_found |= ERROR_OBJECT;\n \t\treturn error(\"%s: object corrupt or missing\", sha1_to_hex(sha1));\n \t}\n+\tobj->flags = HAS_OBJ;\n \treturn fsck_obj(obj);\n }\n \ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 79dfe47..20cf284 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -765,6 +765,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tif (obj->type == OBJ_TREE) {\n \t\t\t\tstruct tree *item = (struct tree *) obj;\n \t\t\t\titem->buffer = NULL;\n+\t\t\t\tobj->parsed = 0;\n \t\t\t}\n \t\t\tif (obj->type == OBJ_COMMIT) {\n \t\t\t\tstruct commit *commit = (struct commit *) obj;\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 54184b3..ba27f7c 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -94,8 +94,7 @@ static int tree_is_complete(const unsigned char *sha1)\n \t\t\tcomplete = 0;\n \t\t}\n \t}\n-\tfree(tree->buffer);\n-\ttree->buffer = NULL;\n+\tfree_tree_buffer(tree);\n \n \tif (complete)\n \t\ttree->object.flags |= SEEN;\ndiff --git a/http-push.c b/http-push.c\nindex 395a8cf..c13b441 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1330,8 +1330,7 @@ static struct object_list **process_tree(struct tree *tree,\n \t\t\tbreak;\n \t\t}\n \n-\tfree(tree->buffer);\n-\ttree->buffer = NULL;\n+\tfree_tree_buffer(tree);\n \treturn p;\n }\n \ndiff --git a/list-objects.c b/list-objects.c\nindex 3dd4a96..c8c3463 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -123,8 +123,7 @@ static void process_tree(struct rev_info *revs,\n \t\t\t\t cb_data);\n \t}\n \tstrbuf_setlen(base, baselen);\n-\tfree(tree->buffer);\n-\ttree->buffer = NULL;\n+\tfree_tree_buffer(tree);\n }\n \n static void mark_edge_parents_uninteresting(struct commit *commit,\ndiff --git a/reachable.c b/reachable.c\nindex e7e6a1e..654a8c5 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -80,8 +80,7 @@ static void process_tree(struct tree *tree,\n \t\telse\n \t\t\tprocess_blob(lookup_blob(entry.sha1), p, &me, entry.path, cp);\n \t}\n-\tfree(tree->buffer);\n-\ttree->buffer = NULL;\n+\tfree_tree_buffer(tree);\n }\n \n static void process_tag(struct tag *tag, struct object_array *p,\ndiff --git a/revision.c b/revision.c\nindex 518cd08..eb988ee 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -135,8 +135,7 @@ void mark_tree_uninteresting(struct tree *tree)\n \t * We don't care about the tree any more\n \t * after it has been marked uninteresting.\n \t */\n-\tfree(tree->buffer);\n-\ttree->buffer = NULL;\n+\tfree_tree_buffer(tree);\n }\n \n void mark_parents_uninteresting(struct commit *commit)\ndiff --git a/tree.c b/tree.c\nindex 62fed63..1cbf60e 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -225,6 +225,14 @@ int parse_tree(struct tree *item)\n \treturn parse_tree_buffer(item, buffer, size);\n }\n \n+void free_tree_buffer(struct tree *tree)\n+{\n+\tfree(tree->buffer);\n+\ttree->buffer = NULL;\n+\ttree->size = 0;\n+\ttree->object.parsed = 0;\n+}\n+\n struct tree *parse_tree_indirect(const unsigned char *sha1)\n {\n \tstruct object *obj = parse_object(sha1);\ndiff --git a/tree.h b/tree.h\nindex 69bcb5e..601ab9c 100644\n--- a/tree.h\n+++ b/tree.h\n@@ -16,6 +16,7 @@ int parse_tree(struct tree *tree);\n int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size);\n \n int parse_tree(struct tree *tree);\n+void free_tree_buffer(struct tree *tree);\n \n /* Parses and returns the tree in the given ent, chasing tags and commits. */\n struct tree *parse_tree_indirect(const unsigned char *sha1);\ndiff --git a/walker.c b/walker.c\nindex be389dc..633596e 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -56,10 +56,7 @@ static int process_tree(struct walker *walker, struct tree *tree)\n \t\tif (!obj || process(walker, obj))\n \t\t\treturn -1;\n \t}\n-\tfree(tree->buffer);\n-\ttree->buffer = NULL;\n-\ttree->size = 0;\n-\ttree->object.parsed = 0;\n+\tfree_tree_buffer(tree);\n \treturn 0;\n }\n \n-- \n1.8.3.rc2.14.g7eee6b3\n"},{"id":"219475","messageId":"20130605223945.GB15607@sigill.intra.peff.net","threadId":"29333","inReplyTo":"20130605223551.GF8664@sigill.intra.peff.net","subject":"[PATCH 2/4] upload-archive: restrict remote objects with reachability check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-05T22:39:45Z","receivedAt":"2013-06-05T22:39:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When serving a remote request, git-upload-archive tries to\nrestrict access to unreachable objects, which matches the\nbehavior of upload-pack. However, we did so by restricting\nthe requested tree to \"[:]\", because it is fast.\nThat covers the common cases, but does not allow requesting\nitems by a specific sha1 (either a tree or a commit sha1).\n\nInstead, let's do the correct-but-slower method of actually\nwalking back from the tips to see if the requested object is\nreachable. The performance impact of this is roughly:\n\n 1. For a recent commit, the speed is about the same (we\n traverse in reverse chronological order, so we see it\n almost immediately).\n\n 2. For an older commit, even one pointed at directly by a\n ref (e.g., an old tag), we are slower, because we\n traverse from the more recent tips. We are bounded in\n this case by the time to look at all commits (i.e.,\n \"time git rev-list --all\").\n\n 3. When we see \"$ref:$path\", we typically perform much\n worse, because our traversal looks at all commits\n first, followed by all trees.\n\n 4. The worst case (which we hit for an unreachable object)\n is equivalent to \"time rev-list --objects --all\", which\n is about the same amount of time pack-objects spends\n preparing a full clone (which can be in the tens of\n seconds for a large repository).\n\nThe implementation is a fairly straightforward application\nof the traverse_commit_list function. Using the\nmark_objects_reachable function would seem more appropriate,\nbut it has no mechanism for looking for a specific object,\nwhich lets us end the traversal early in common cases.\n\nSigned-off-by: Jeff King \n---\nThe slowdown from points (2) and (3) makes me hesitate on this. We can\naddress (2) by checking if the get_sha1 lookup used a refname\nexplicitly, but I'm no sure about (3).\n\n archive.c | 70 +++++++++++++++++++++++++++------\n t/t5005-archive-resolution.sh | 91 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 150 insertions(+), 11 deletions(-)\n create mode 100755 t/t5005-archive-resolution.sh\n\ndiff --git a/archive.c b/archive.c\nindex d254fa5..4d77624 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -5,6 +5,9 @@\n #include \"archive.h\"\n #include \"parse-options.h\"\n #include \"unpack-trees.h\"\n+#include \"diff.h\"\n+#include \"revision.h\"\n+#include \"list-objects.h\"\n \n static char const * const archive_usage[] = {\n \tN_(\"git archive [options] [...]\"),\n@@ -241,6 +244,59 @@ static void parse_pathspec_arg(const char **pathspec,\n \t}\n }\n \n+struct reachable_object_data {\n+\tstruct rev_info revs;\n+\tstruct object *obj;\n+};\n+\n+static void check_object(struct object *obj, const struct name_path *path,\n+\t\t\t const char *name, void *vdata)\n+{\n+\tstruct reachable_object_data *data = vdata;\n+\t/*\n+\t * We found it; the caller will take care of marking it SEEN,\n+\t * but we can end the traversal early.\n+\t */\n+\tif (obj == data->obj) {\n+\t\tfree_commit_list(data->revs.commits);\n+\t\tdata->revs.commits = NULL;\n+\n+\t\tfree(data->revs.pending.objects);\n+\t\tdata->revs.pending.nr = 0;\n+\t\tdata->revs.pending.alloc = 0;\n+\t\tdata->revs.pending.objects = NULL;\n+\t}\n+}\n+\n+static void check_commit(struct commit *commit, void *vdata)\n+{\n+\tcheck_object(&commit->object, NULL, NULL, vdata);\n+}\n+\n+static int object_is_reachable(const unsigned char *sha1)\n+{\n+\tstatic const char *argv[] = {\n+\t\t\"rev-list\",\n+\t\t\"--objects\",\n+\t\t\"--all\",\n+\t\tNULL\n+\t};\n+\tstruct reachable_object_data data;\n+\n+\tdata.obj = parse_object(sha1);\n+\tif (!data.obj)\n+\t\treturn 0;\n+\n+\tsave_commit_buffer = 0;\n+\tinit_revisions(&data.revs, NULL);\n+\tsetup_revisions(ARRAY_SIZE(argv) - 1, argv, &data.revs, NULL);\n+\tif (prepare_revision_walk(&data.revs))\n+\t\treturn 0;\n+\n+\ttraverse_commit_list(&data.revs, check_commit, check_object, &data);\n+\treturn data.obj->flags & SEEN;\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,20 +308,12 @@ 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+\tif (remote && !object_is_reachable(sha1))\n+\t\tdie(\"Not a valid object name\");\n+\n \tcommit = lookup_commit_reference_gently(sha1, 1);\n \tif (commit) {\n \t\tcommit_sha1 = commit->object.sha1;\ndiff --git a/t/t5005-archive-resolution.sh b/t/t5005-archive-resolution.sh\nnew file mode 100755\nindex 0000000..2e7ee1e\n--- /dev/null\n+++ b/t/t5005-archive-resolution.sh\n@@ -0,0 +1,91 @@\n+#!/bin/sh\n+\n+test_description='test object resolution methods for local and remote archive'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo one >one && git add one && git commit -m one &&\n+\tsha1_referenced=`git rev-parse HEAD` &&\n+\tgit tag tagged &&\n+\techo two >two && git add two && git commit -m two &&\n+\tsha1_unreferenced=`git rev-parse HEAD` &&\n+\tgit reset --hard HEAD^ &&\n+\techo three >three && git add three && git commit -m three &&\n+\tgit tag tagged-tree HEAD^{tree} &&\n+\tgit reset --hard HEAD^ &&\n+\tmkdir subdir &&\n+\techo four >subdir/four && git add subdir && git commit -m four &&\n+\tsha1_subtree=`git rev-parse HEAD:subdir`\n+'\n+\n+# check that archiving $what from $where produces expected paths\n+check() {\n+\tdesc=$1; shift; # human-readable description\n+\twhere=$1; shift; # local|remote\n+\twhat=$1; shift; # the commit/tree id\n+\texpect=\"$*\"; # expected paths or \"deny\"\n+\n+\tcmd=\"git archive --format=tar -o result.tar\"\n+\ttest \"$where\" = \"remote\" && cmd=\"$cmd --remote=.\"\n+\tcmd=\"$cmd $what\"\n+\n+\tif test \"$expect\" = \"deny\"; then\n+\t\ttest_expect_success \"archive $desc ($where, should deny)\" \"\n+\t\t\ttest_must_fail $cmd\n+\t\t\"\n+\telse\n+\t\ttest_expect_success \"archive $desc ($where, should work)\" '\n+\t\t\t'\"$cmd\"' &&\n+\t\t\tfor i in '\"$expect\"'; do\n+\t\t\t\techo \"$i:`basename $i`\"\n+\t\t\tdone >expect &&\n+\t\t\trm -rf result &&\n+\t\t\tmkdir result &&\n+\t\t\t(cd result &&\n+\t\t\ttar xf ../result.tar &&\n+\t\t\tfor i in `find * -type f -print`; do\n+\t\t\t\techo \"$i:`cat $i`\"\n+\t\t\tdone >../actual\n+\t\t\t) &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\tfi\n+}\n+\n+check 'ref' local master one subdir/four\n+check 'ref' remote master one subdir/four\n+\n+check 'relative ref' local master^ one\n+check 'relative ref' remote master^ one\n+\n+check 'reachable sha1' local $sha1_referenced one\n+check 'reachable sha1' remote $sha1_referenced one\n+\n+check 'unreachable sha1' local $sha1_unreferenced one two\n+check 'unreachable sha1' remote $sha1_unreferenced deny\n+\n+check 'reachable reflog' local master@{0} one subdir/four\n+check 'reachable reflog' remote master@{0} one subdir/four\n+\n+check 'unreachable reflog' local master@{4} one two\n+check 'unreachable reflog' remote master@{4} deny\n+\n+check 'tree via ref^{tree}' local master^{tree} one subdir/four\n+check 'tree via ref^{tree}' remote master^{tree} one subdir/four\n+\n+check 'tree via ref:' local master: one subdir/four\n+check 'tree via ref:' remote master: one subdir/four\n+\n+check 'subtree via ref:sub' local master:subdir four\n+check 'subtree via ref:sub' remote master:subdir four\n+\n+check 'subtree via sha1' local $sha1_subtree four\n+check 'subtree via sha1' remote $sha1_subtree four\n+\n+check 'tagged commit' local tagged one\n+check 'tagged commit' remote tagged one\n+\n+check 'tagged tree' local tagged-tree one three\n+check 'tagged tree' remote tagged-tree one three\n+\n+test_done\n-- \n1.8.3.rc2.14.g7eee6b3\n"},{"id":"219476","messageId":"20130605224004.GC15607@sigill.intra.peff.net","threadId":"29333","inReplyTo":"20130605223551.GF8664@sigill.intra.peff.net","subject":"[PATCH 3/4] list-objects: optimize \"revs->blob_objects = 0\" case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-05T22:40:05Z","receivedAt":"2013-06-05T22:40:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we are traversing trees during a \"--objects\"\ntraversal, we may skip blobs if the \"blob_objects\" field of\nrev_info is not set. But we do so as the first thing in\nprocess_blob(), only after we have actually created the\n\"struct blob\" object, incurring a hash lookup. We can\noptimize out this no-op call completely.\n\nThis does not actually affect any current code, as all of\nthe current traversals always set blob_objects when looking\nat objects, anyway.\n\nSigned-off-by: Jeff King \n---\n list-objects.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex c8c3463..77e6ec5 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -116,7 +116,7 @@ static void process_tree(struct rev_info *revs,\n \t\t\tprocess_gitlink(revs, entry.sha1,\n \t\t\t\t\tshow, &me, entry.path,\n \t\t\t\t\tcb_data);\n-\t\telse\n+\t\telse if (revs->blob_objects)\n \t\t\tprocess_blob(revs,\n \t\t\t\t lookup_blob(entry.sha1),\n \t\t\t\t show, &me, entry.path,\n-- \n1.8.3.rc2.14.g7eee6b3\n"},{"id":"219477","messageId":"20130605224038.GD15607@sigill.intra.peff.net","threadId":"29333","inReplyTo":"20130605223551.GF8664@sigill.intra.peff.net","subject":"[PATCH 4/4] archive: ignore blob objects when checking reachability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-05T22:40:39Z","receivedAt":"2013-06-05T22:40:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We cannot create an archive from a blob object, so we would\nnot expect anyone to provide one to us. And if they do, we\nwill fail anyway just after the reachability check. We can\ntherefore optimize our reachability check to ignore blobs\ncompletely, and not even create a \"struct blob\" for them.\n\nDepending on the repository size and the exact place we find\nthe reachable object in the traversal, this can save 20-25%,\na we can avoid many lookups in the object hash.\n\nThe downside of this is that a blob provided to a remote\narchive process will fail with \"no such object\" rather than\n\"object is not a tree\" (we could organize the code to retain\nthe old message, but since we no longer know whether the\nblob is reachable or not, we would potentially be leaking\ninformation about the existence of unreachable objects).\n\nSigned-off-by: Jeff King \n---\n archive.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/archive.c b/archive.c\nindex 4d77624..98691cd 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -290,6 +290,7 @@ static int object_is_reachable(const unsigned char *sha1)\n \tsave_commit_buffer = 0;\n \tinit_revisions(&data.revs, NULL);\n \tsetup_revisions(ARRAY_SIZE(argv) - 1, argv, &data.revs, NULL);\n+\tdata.revs.blob_objects = 0;\n \tif (prepare_revision_walk(&data.revs))\n \t\treturn 0;\n \n-- \n1.8.3.rc2.14.g7eee6b3\n"},{"id":"219488","messageId":"51B040EC.6080707@alum.mit.edu","threadId":"29333","inReplyTo":"20130605224038.GD15607@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] archive: ignore blob objects when checking reachability","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-06T07:57:32Z","receivedAt":"2013-06-06T07:57:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/06/2013 12:40 AM, Jeff King wrote:\n> We cannot create an archive from a blob object, so we would\n> not expect anyone to provide one to us. And if they do, we\n> will fail anyway just after the reachability check. We can\n> therefore optimize our reachability check to ignore blobs\n> completely, and not even create a \"struct blob\" for them.\n> \n> Depending on the repository size and the exact place we find\n> the reachable object in the traversal, this can save 20-25%,\n> a we can avoid many lookups in the object hash.\n> \n> The downside of this is that a blob provided to a remote\n> archive process will fail with \"no such object\" rather than\n> \"object is not a tree\" (we could organize the code to retain\n> the old message, but since we no longer know whether the\n> blob is reachable or not, we would potentially be leaking\n> information about the existence of unreachable objects).\n\nCould we change the error message to \"no such tree object\" to be\nnon-committal about the reason for the failure?\n\n\nFor a moment I thought that one could get correct error messages while\nretaining the speed gain in the usual case by doing a quick object\nlookup, and then\n\n check type of object\n if object is missing:\n die(there is no such object)\n else if object is a blob:\n do reachability test including blobs\n if object is not reachable:\n die(there is no such object)\n else\n die(object is not a tree)\n else\n do reachability test excluding blobs\n etc\n\nHowever, even this would leak information about the existence of\nnonreachable objects to a client measuring time time for the response\nbecause the death due to non-reachability would take longer than death\ndue to missing object. So, if one would insist on correct error\nmessages and no information leakage, one could just skip the first\n\"object is missing\" optimization (it should be pretty rare anyway!) like so:\n\n check type of object\n if object is missing or object is a blob:\n /* Force the same delay in either case: */\n do reachability test including blobs\n if object is missing or object is not reachable:\n die(there is no such object)\n else\n die(object is not a tree)\n else\n do reachability test excluding blobs\n etc\n\nI'm not suggesting that the extra effort is worth it; I just wanted to\nmention the possibility.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"219518","messageId":"7vsj0vywrv.fsf@alter.siamese.dyndns.org","threadId":"29333","inReplyTo":"20130605223551.GF8664@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/4] real reachability checks for upload-archive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-06T17:27:48Z","receivedAt":"2013-06-06T17:27:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jun 05, 2013 at 12:38:23PM -0400, Jeff King wrote:\n>\n>> 2. Actually do a reachability check. Doing a full object check to\n>> allow fetching an arbitrary tree by sha1 is probably prohibitively\n>> expensive[2], but we could allow the form \"[:]\", check\n>> that \"\" is reachable, and then allow arbitrary paths within\n>> it.\n>\n> Thinking on this more, the full reachability check is no worse than what\n> a clone has to do to fetch the full repository. Here's a series that\n> does the full check. I'm not entirely happy with the performance,\n> though; details are in patch 3.\n\nFor some repository-servers, it may be OK to enable this by default,\nbut I suspect it would be better to have at least an opt-out server\nconfiguration.\n\n> I think I'd be tempted to just go the more limiting \"commit is\n> reachable\" route, instead, which would solve your case (and most sane\n> cases).\n\nYes, I think that is a reasonable thing to do. After all, as you\nnoted in 4/4, you cannot ask for a single blob, and not being able\nto ask for a single tree is not much different.\n\n> [1/4]: clear parsed flag when we free tree buffers\n> [2/4]: upload-archive: restrict remote objects with reachability check\n> [3/4]: list-objects: optimize \"revs->blob_objects = 0\" case\n> [4/4]: archive: ignore blob objects when checking reachability\n>\n> -Peff\n"},{"id":"219520","messageId":"7vobbjyvii.fsf@alter.siamese.dyndns.org","threadId":"29333","inReplyTo":"20130605223739.GA15607@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] clear parsed flag when we free tree buffers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-06T17:55:01Z","receivedAt":"2013-06-06T17:55:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Many code paths will free a tree object's buffer and set it\n> to NULL after finishing with it in order to keep memory\n> usage down during a traversal. However, out of 8 sites that\n> do this, only one actually unsets the \"parsed\" flag back.\n> Those sites that don't are setting a trap for later users of\n> the tree object; even after calling parse_tree, the buffer\n> will remain NULL, causing potential segfaults.\n>\n> It is not known whether this is triggerable in the current\n> code. Most commands do not do an in-memory traversal\n> followed by actually using the objects again. However, it\n> does not hurt to be safe for future callers.\n>\n> In most cases, we can abstract this out to a\n> \"free_tree_buffer\" helper. However, there are two\n> exceptions:\n>\n> 1. The fsck code relies on the parsed flag to know that we\n> were able to parse the object at one point. We can\n> switch this to using a flag in the \"flags\" field.\n>\n> 2. The index-pack code sets the buffer to NULL but does\n> not free it (it is freed by a caller). We should still\n> unset the parsed flag here, but we cannot use our\n> helper, as we do not want to free the buffer.\n>\n> Signed-off-by: Jeff King \n> ---\n> This shouldn't have any behavior change, but I'd worry a bit that I\n> missed some case in builtin/fsck.c where the new HAS_OBJ flag would need\n> set.\n\ncheck-unreachable-object?\n\nI am wondering if you can use SEEN which is already there, or at\nleast set HAS_OBJ at the same place where SEEN is set.\n\nThe overall structure of fsck is:\n\n * we scan objects we see in the object store; they are marked with\n SEEN to avoid duplicated work in fsck_obj() and this scanning\n does not have anything to do with connectivity; then\n\n * we traverse connectivity graph from the starting points (refs,\n index, etc.).\n\nand this obj->parsed check is used in the latter phase, so...\n\n\nAn unrelated tangent.\n\nI suspect that 271b8d25b25 made a copy&paste error to \"broken link\"\nmessage in builtin/fsck.c while looking at this patch, by the way.\nThe \"lookup_object() to compute the first parameter given to the\ncallback in fsck_walk() returned NULL\" case has two \"from\"; the\nlatter should be \"to\" (and I think in the longer term the function\nsignature of the callback needs to be enhanced to let us tell what\nobject name we found in \"parent\" that failed to give us an object).\n\n> builtin/fsck.c | 17 ++++++++---------\n> builtin/index-pack.c | 1 +\n> builtin/reflog.c | 3 +--\n> http-push.c | 3 +--\n> list-objects.c | 3 +--\n> reachable.c | 3 +--\n> revision.c | 3 +--\n> tree.c | 8 ++++++++\n> tree.h | 1 +\n> walker.c | 5 +----\n> 10 files changed, 24 insertions(+), 23 deletions(-)\n>\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index bb9a2cd..579fdcc 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -16,6 +16,7 @@\n> \n> #define REACHABLE 0x0001\n> #define SEEN 0x0002\n> +#define HAS_OBJ 0x0004\n> \n> static int show_root;\n> static int show_tags;\n> @@ -101,7 +102,7 @@ static int mark_object(struct object *obj, int type, void *data)\n> \tif (obj->flags & REACHABLE)\n> \t\treturn 0;\n> \tobj->flags |= REACHABLE;\n> -\tif (!obj->parsed) {\n> +\tif (!(obj->flags & HAS_OBJ)) {\n> \t\tif (parent && !has_sha1_file(obj->sha1)) {\n> \t\t\tprintf(\"broken link from %7s %s\\n\",\n> \t\t\t\t typename(parent->type), sha1_to_hex(parent->sha1));\n> @@ -127,16 +128,13 @@ static int traverse_one_object(struct object *obj)\n> \tstruct tree *tree = NULL;\n> \n> \tif (obj->type == OBJ_TREE) {\n> -\t\tobj->parsed = 0;\n> \t\ttree = (struct tree *)obj;\n> \t\tif (parse_tree(tree) < 0)\n> \t\t\treturn 1; /* error already displayed */\n> \t}\n> \tresult = fsck_walk(obj, mark_object, obj);\n> -\tif (tree) {\n> -\t\tfree(tree->buffer);\n> -\t\ttree->buffer = NULL;\n> -\t}\n> +\tif (tree)\n> +\t\tfree_tree_buffer(tree);\n> \treturn result;\n> }\n> \n> @@ -178,7 +176,7 @@ static void check_reachable_object(struct object *obj)\n> \t * except if it was in a pack-file and we didn't\n> \t * do a full fsck\n> \t */\n> -\tif (!obj->parsed) {\n> +\tif (!(obj->flags & HAS_OBJ)) {\n> \t\tif (has_sha1_pack(obj->sha1))\n> \t\t\treturn; /* it is in pack - forget about it */\n> \t\tprintf(\"missing %s %s\\n\", typename(obj->type), sha1_to_hex(obj->sha1));\n> @@ -306,8 +304,7 @@ static int fsck_obj(struct object *obj)\n> \tif (obj->type == OBJ_TREE) {\n> \t\tstruct tree *item = (struct tree *) obj;\n> \n> -\t\tfree(item->buffer);\n> -\t\titem->buffer = NULL;\n> +\t\tfree_tree_buffer(item);\n> \t}\n> \n> \tif (obj->type == OBJ_COMMIT) {\n> @@ -340,6 +337,7 @@ static int fsck_sha1(const unsigned char *sha1)\n> \t\treturn error(\"%s: object corrupt or missing\",\n> \t\t\t sha1_to_hex(sha1));\n> \t}\n> +\tobj->flags |= HAS_OBJ;\n> \treturn fsck_obj(obj);\n> }\n> \n> @@ -352,6 +350,7 @@ static int fsck_obj_buffer(const unsigned char *sha1, enum object_type type,\n> \t\terrors_found |= ERROR_OBJECT;\n> \t\treturn error(\"%s: object corrupt or missing\", sha1_to_hex(sha1));\n> \t}\n> +\tobj->flags = HAS_OBJ;\n> \treturn fsck_obj(obj);\n> }\n> \n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 79dfe47..20cf284 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -765,6 +765,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n> \t\t\tif (obj->type == OBJ_TREE) {\n> \t\t\t\tstruct tree *item = (struct tree *) obj;\n> \t\t\t\titem->buffer = NULL;\n> +\t\t\t\tobj->parsed = 0;\n> \t\t\t}\n> \t\t\tif (obj->type == OBJ_COMMIT) {\n> \t\t\t\tstruct commit *commit = (struct commit *) obj;\n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 54184b3..ba27f7c 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -94,8 +94,7 @@ static int tree_is_complete(const unsigned char *sha1)\n> \t\t\tcomplete = 0;\n> \t\t}\n> \t}\n> -\tfree(tree->buffer);\n> -\ttree->buffer = NULL;\n> +\tfree_tree_buffer(tree);\n> \n> \tif (complete)\n> \t\ttree->object.flags |= SEEN;\n> diff --git a/http-push.c b/http-push.c\n> index 395a8cf..c13b441 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -1330,8 +1330,7 @@ static struct object_list **process_tree(struct tree *tree,\n> \t\t\tbreak;\n> \t\t}\n> \n> -\tfree(tree->buffer);\n> -\ttree->buffer = NULL;\n> +\tfree_tree_buffer(tree);\n> \treturn p;\n> }\n> \n> diff --git a/list-objects.c b/list-objects.c\n> index 3dd4a96..c8c3463 100644\n> --- a/list-objects.c\n> +++ b/list-objects.c\n> @@ -123,8 +123,7 @@ static void process_tree(struct rev_info *revs,\n> \t\t\t\t cb_data);\n> \t}\n> \tstrbuf_setlen(base, baselen);\n> -\tfree(tree->buffer);\n> -\ttree->buffer = NULL;\n> +\tfree_tree_buffer(tree);\n> }\n> \n> static void mark_edge_parents_uninteresting(struct commit *commit,\n> diff --git a/reachable.c b/reachable.c\n> index e7e6a1e..654a8c5 100644\n> --- a/reachable.c\n> +++ b/reachable.c\n> @@ -80,8 +80,7 @@ static void process_tree(struct tree *tree,\n> \t\telse\n> \t\t\tprocess_blob(lookup_blob(entry.sha1), p, &me, entry.path, cp);\n> \t}\n> -\tfree(tree->buffer);\n> -\ttree->buffer = NULL;\n> +\tfree_tree_buffer(tree);\n> }\n> \n> static void process_tag(struct tag *tag, struct object_array *p,\n> diff --git a/revision.c b/revision.c\n> index 518cd08..eb988ee 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -135,8 +135,7 @@ void mark_tree_uninteresting(struct tree *tree)\n> \t * We don't care about the tree any more\n> \t * after it has been marked uninteresting.\n> \t */\n> -\tfree(tree->buffer);\n> -\ttree->buffer = NULL;\n> +\tfree_tree_buffer(tree);\n> }\n> \n> void mark_parents_uninteresting(struct commit *commit)\n> diff --git a/tree.c b/tree.c\n> index 62fed63..1cbf60e 100644\n> --- a/tree.c\n> +++ b/tree.c\n> @@ -225,6 +225,14 @@ int parse_tree(struct tree *item)\n> \treturn parse_tree_buffer(item, buffer, size);\n> }\n> \n> +void free_tree_buffer(struct tree *tree)\n> +{\n> +\tfree(tree->buffer);\n> +\ttree->buffer = NULL;\n> +\ttree->size = 0;\n> +\ttree->object.parsed = 0;\n> +}\n> +\n> struct tree *parse_tree_indirect(const unsigned char *sha1)\n> {\n> \tstruct object *obj = parse_object(sha1);\n> diff --git a/tree.h b/tree.h\n> index 69bcb5e..601ab9c 100644\n> --- a/tree.h\n> +++ b/tree.h\n> @@ -16,6 +16,7 @@ int parse_tree(struct tree *tree);\n> int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size);\n> \n> int parse_tree(struct tree *tree);\n> +void free_tree_buffer(struct tree *tree);\n> \n> /* Parses and returns the tree in the given ent, chasing tags and commits. */\n> struct tree *parse_tree_indirect(const unsigned char *sha1);\n> diff --git a/walker.c b/walker.c\n> index be389dc..633596e 100644\n> --- a/walker.c\n> +++ b/walker.c\n> @@ -56,10 +56,7 @@ static int process_tree(struct walker *walker, struct tree *tree)\n> \t\tif (!obj || process(walker, obj))\n> \t\t\treturn -1;\n> \t}\n> -\tfree(tree->buffer);\n> -\ttree->buffer = NULL;\n> -\ttree->size = 0;\n> -\ttree->object.parsed = 0;\n> +\tfree_tree_buffer(tree);\n> \treturn 0;\n> }\n"},{"id":"219574","messageId":"CAPig+cRMGBjXO3Phmv=SX7FQwZ=uCUTT9YSKiEBb6PU0ts_1uw@mail.gmail.com","threadId":"29333","inReplyTo":"20130605224038.GD15607@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] archive: ignore blob objects when checking reachability","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-06-07T00:50:27Z","receivedAt":"2013-06-07T00:50:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 5, 2013 at 6:40 PM, Jeff King wrote:\n> We cannot create an archive from a blob object, so we would\n> not expect anyone to provide one to us. And if they do, we\n> will fail anyway just after the reachability check. We can\n> therefore optimize our reachability check to ignore blobs\n> completely, and not even create a \"struct blob\" for them.\n>\n> Depending on the repository size and the exact place we find\n> the reachable object in the traversal, this can save 20-25%,\n> a we can avoid many lookups in the object hash.\n\ns/a/as/\n\n> The downside of this is that a blob provided to a remote\n> archive process will fail with \"no such object\" rather than\n> \"object is not a tree\" (we could organize the code to retain\n> the old message, but since we no longer know whether the\n> blob is reachable or not, we would potentially be leaking\n> information about the existence of unreachable objects).\n>\n> Signed-off-by: Jeff King \n"}]}