{"thread":{"id":"14936","subject":"RFC: Allow missing objects during packing","startedAt":"2008-08-11T18:28:39Z","lastAt":"2008-08-12T18:43:45Z","messageCount":12,"participants":["Shawn O. Pearce","Junio C Hamano","Nicolas Pitre"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"86801","messageId":"20080811182839.GJ26363@spearce.org","threadId":"14936","inReplyTo":null,"subject":"RFC: Allow missing objects during packing","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-11T18:28:39Z","receivedAt":"2008-08-11T18:28:39Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"So I have this horribly bizarre case that I just feel dirty telling\nyou about. But I have to deal with it. So here goes...\n\nI want to publish a fork of an open source project, but I'm not\nallowed to publish the base history of the project. For political\nreasons I can only publish patches/deltas, even though the project\nis licensed under the LGPL. Don't ask. Seriously. In 6 months\neveryone will probably forget about this and I can just publish\nthe whole thing. But not right now.\n\nWith this silly little patch to builtin-pack-objects I can make\nit work by publishing what amounts to a shallow clone. The fork\nwas created by doing something like this:\n\n\t$ git clone --bare git://upstream.org/proj.git upstream.git\n\t$ cd fork\n\t$ git remote add -f upstream ../upstream.git\n\t$ echo ../upstream.git/objects >.git/objects/info/alternates\n\t$ git repack -a -d -f -l\n\n\t# now fork/.git has only its \"delta\"\n\t# add the base trees, but not blobs\n\t#\n\t$ jgit --git-dir=../upstream.git tree-copy upstream/master .\n\n\t# remove the shared odb\n\t#\n $ rm .git/objects/info/alternates\n\nUsers who want to clone from fork.git have to do this:\n\n\t$ git clone -o upstream git://upstream.org/proj.git proj\n\t$ cd proj\n\t$ git remote add -f origin git://forky.org/fork.git\n\tremote: Counting objects: 5, done.\n\tremote: error: unable to find d30dcb7f07b6bf1a6cc5da4f3c1d0e6fc690dc45\n\tremote: git: unable to get type of object d30dremote: cb7f07b6bf1a6cc5da4f3c1d0e6fc690dc45: No such file or directory\n\tremote: Compressing objects: 100% (3/3), done.\n\tremote: Total 3 (delta 1), reused 0 (delta 0)\n\tUnpacking objects: 100% (3/3), done.\n\nBasically this little patch just makes pack-objects issue the\nwarning, but then skip over the object and move on.\n\nLater during deltification the selected base object has a type which\ndoes not match any other object, so it never gets considered, and\nthere is no error caused by it not being present. We just cannot\ngenerate a delta during the fetch and the client winds up getting\na little larger download.\n\nThoughts? Should I work this up into a real patch and try to get\nit into the tree? It seems pretty harmless to allow an object we\naren't going to transmit but that we want to use as a delta base\nin a thin pack to be missing. At worst we just get a little bit\nmore data transfer.\n\n--8<--\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 2dadec1..0f29f14 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1097,7 +1097,7 @@ static void check_object(struct object_entry *entry)\n \n \tentry->type = sha1_object_info(entry->idx.sha1, &entry->size);\n \tif (entry->type < 0)\n-\t\tdie(\"unable to get type of object %s\",\n+\t\twarn(\"unable to get type of object %s\",\n \t\t sha1_to_hex(entry->idx.sha1));\n }\n\n\nAnd this is the Java program to build up the base tree, as otherwise\nthe thin pack creation code crashes out trying to build a list of\nwhat the client already has/knows. Since this is only the base\ncommit and its trees (no blobs, and no history beyond the base\ncommit) I should be able to get away with publishing it as part of\nthe fork.\n\n--8<--\nimport org.kohsuke.args4j.Argument;\nimport org.spearce.jgit.lib.Constants;\nimport org.spearce.jgit.lib.ObjectId;\nimport org.spearce.jgit.lib.PackWriter;\nimport org.spearce.jgit.lib.Repository;\nimport org.spearce.jgit.lib.TextProgressMonitor;\nimport org.spearce.jgit.pgm.Command;\nimport org.spearce.jgit.pgm.TextBuiltin;\nimport org.spearce.jgit.revwalk.RevCommit;\nimport org.spearce.jgit.revwalk.RevObject;\nimport org.spearce.jgit.treewalk.TreeWalk;\n\nimport java.io.BufferedOutputStream;\nimport java.io.File;\nimport java.io.FileOutputStream;\nimport java.util.LinkedList;\nimport java.util.List;\n\n@Command(name = \"tree-copy\", usage = \"Copy a base commit and its trees\")\npublic class TreeCopy extends TextBuiltin {\n @Argument(index = 0, required = true, metaVar = \"commit\", usage = \"base commit\")\n private RevCommit baseCommit;\n\n @Argument(index = 1, required = true, metaVar = \"dir\", usage = \"destination repository\")\n private File destGitDir;\n\n @Override\n protected void run() throws Exception {\n final List toCopy = new LinkedList();\n\n toCopy.add(baseCommit);\n toCopy.add(baseCommit.getTree());\n\n final TreeWalk tw = new TreeWalk(db);\n tw.reset();\n tw.addTree(baseCommit.getTree());\n\n while (tw.next()) {\n switch (tw.getFileMode(0).getObjectType()) {\n case Constants.OBJ_TREE:\n toCopy.add(argWalk.lookupTree(tw.getObjectId(0)));\n tw.enterSubtree();\n break;\n default:\n break;\n }\n }\n\n final Repository destdb = new Repository(destGitDir);\n final PackWriter packer = new PackWriter(db, new TextProgressMonitor());\n packer.preparePack(toCopy.iterator());\n final String packName = \"pack-\" + packer.computeName();\n final File packDir = new File(destdb.getDirectory(), \"objects/pack\");\n final File packPath = new File(packDir, packName + \".pack\");\n final File idxPath = new File(packDir, packName + \".idx\");\n\n if (packPath.exists() && idxPath.exists())\n throw die(packPath.getName() + \" already exists.\");\n\n {\n final BufferedOutputStream os =\n new BufferedOutputStream(new FileOutputStream(packPath));\n packer.writePack(os);\n os.close();\n packPath.setReadOnly();\n }\n {\n final BufferedOutputStream os =\n new BufferedOutputStream(new FileOutputStream(idxPath));\n packer.writeIndex(os);\n os.close();\n idxPath.setReadOnly();\n }\n }\n}\n \n-- \nShawn.\n"},{"id":"86846","messageId":"7vk5enuqfg.fsf@gitster.siamese.dyndns.org","threadId":"14936","inReplyTo":"20080811182839.GJ26363@spearce.org","subject":"Re: RFC: Allow missing objects during packing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-11T22:39:47Z","receivedAt":"2008-08-11T22:39:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> ... It seems pretty harmless to allow an object we\n> aren't going to transmit but that we want to use as a delta base\n> in a thin pack to be missing. At worst we just get a little bit\n> more data transfer.\n\nIf the check is only about a thin delta base that is not going to be\ntransmit, I'd agree. But I do not see how you are distinguishing that\ncase and the case where an object you are actually sending is missing (in\nwhich case we would want to error out, wouldn't we?)\n"},{"id":"86848","messageId":"20080811224404.GQ26363@spearce.org","threadId":"14936","inReplyTo":"7vk5enuqfg.fsf@gitster.siamese.dyndns.org","subject":"Re: RFC: Allow missing objects during packing","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-11T22:44:04Z","receivedAt":"2008-08-11T22:44:04Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > ... It seems pretty harmless to allow an object we\n> > aren't going to transmit but that we want to use as a delta base\n> > in a thin pack to be missing. At worst we just get a little bit\n> > more data transfer.\n> \n> If the check is only about a thin delta base that is not going to be\n> transmit, I'd agree. But I do not see how you are distinguishing that\n> case and the case where an object you are actually sending is missing (in\n> which case we would want to error out, wouldn't we?)\n\nArrgh. Good catch. My patch is flawed in that it does not correctly\nfail if we really needed the missing object in this output pack.\n\nI don't think that would be hard to fix. I'll respin something\nshortly.\n\n-- \nShawn.\n"},{"id":"86889","messageId":"20080812012859.GT26363@spearce.org","threadId":"14936","inReplyTo":"20080811224404.GQ26363@spearce.org","subject":"Re: RFC: Allow missing objects during packing","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-12T01:28:59Z","receivedAt":"2008-08-12T01:28:59Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"> Junio C Hamano wrote:\n> > If the check is only about a thin delta base that is not going to be\n> > transmit, I'd agree. But I do not see how you are distinguishing that\n> > case and the case where an object you are actually sending is missing (in\n> > which case we would want to error out, wouldn't we?)\n\nTurns out to be pretty simple I think. We just delay the\nerror handling for ->type < 0 until write_object(). If we\nget this far we know we wanted to include the object but\nwe really don't have it. Up until that point its fine\nfor us to get objects which are missing, we'll just wind\nup with a suboptimal pack.\n\nWe also don't even need to report the error from sha1_object_info\nas it already issues an error message (see sha1_loose_object_info).\n\n--8<--\npack-objects: Allow missing base objects when creating thin packs\n\nIf we are building a thin pack and one of the base objects we would\nconsider for deltification is missing its OK, the other side already\nhas that base object. We may be able to get a delta from another\nobject, or we can simply send the new object whole (no delta).\n\nThis allows a shallow clone which may have only commits and trees\n(but only partial blobs) to generate a pack for a fetch client,\nso the shallow clone only needs to contain objects that are not\nin the common base.\n\nSigned-off-by: Shawn O. Pearce \n---\n builtin-pack-objects.c | 5 ++---\n 1 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 2dadec1..187cb19 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -243,6 +243,8 @@ static unsigned long write_object(struct sha1file *f,\n \t\tcrc32_begin(f);\n \n \ttype = entry->type;\n+\tif (type < 0)\n+\t\tdie(\"unable to read object %s\", sha1_to_hex(entry->idx.sha1));\n \n \t/* write limit if limited packsize and not first object */\n \tlimit = pack_size_limit && nr_written ?\n@@ -1096,9 +1098,6 @@ static void check_object(struct object_entry *entry)\n \t}\n \n \tentry->type = sha1_object_info(entry->idx.sha1, &entry->size);\n-\tif (entry->type < 0)\n-\t\tdie(\"unable to get type of object %s\",\n-\t\t sha1_to_hex(entry->idx.sha1));\n }\n \n static int pack_offset_sort(const void *_a, const void *_b)\n-- \n1.6.0.rc2.22.g71b99\n\n-- \nShawn.\n"},{"id":"86892","messageId":"7vbpzzt26q.fsf@gitster.siamese.dyndns.org","threadId":"14936","inReplyTo":"20080812012859.GT26363@spearce.org","subject":"Re: RFC: Allow missing objects during packing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-12T02:08:45Z","receivedAt":"2008-08-12T02:08:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> If we are building a thin pack and one of the base objects we would\n> consider for deltification is missing its OK, the other side already\n> has that base object. We may be able to get a delta from another\n> object, or we can simply send the new object whole (no delta).\n>\n> This allows a shallow clone which may have only commits and trees\n> (but only partial blobs) to generate a pack for a fetch client,\n> so the shallow clone only needs to contain objects that are not\n> in the common base.\n\nI'd queue this to 'pu' (or 'next' if you want, but that amounts to about\nthe same thing as we will be rebuilding 'next' soon after 1.6.0 ships),\nbut this would probably be better demonstrated with a test script that\ndemonstrates in what workflow this would be useful.\n"},{"id":"86898","messageId":"alpine.LFD.1.10.0808120023250.22892@xanadu.home","threadId":"14936","inReplyTo":"20080812012859.GT26363@spearce.org","subject":"Re: RFC: Allow missing objects during packing","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-12T04:44:11Z","receivedAt":"2008-08-12T04:44:11Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 11 Aug 2008, Shawn O. Pearce wrote:\n\n> > Junio C Hamano wrote:\n> > > If the check is only about a thin delta base that is not going to be\n> > > transmit, I'd agree. But I do not see how you are distinguishing that\n> > > case and the case where an object you are actually sending is missing (in\n> > > which case we would want to error out, wouldn't we?)\n> \n> Turns out to be pretty simple I think. We just delay the\n> error handling for ->type < 0 until write_object(). If we\n> get this far we know we wanted to include the object but\n> we really don't have it. Up until that point its fine\n> for us to get objects which are missing, we'll just wind\n> up with a suboptimal pack.\n\nIf you're going to die anyway due to an object with unknown type, better \ndo so _before_ going through the delta search phase and leaving a \npartial pack behind. IOW, the type check can be performed in \nprepare_pack() instead of write_object() like:\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 2dadec1..01ab49c 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1722,8 +1733,12 @@ static void prepare_pack(int window, int depth)\n \t\tif (entry->no_try_delta)\n \t\t\tcontinue;\n \n-\t\tif (!entry->preferred_base)\n+\t\tif (!entry->preferred_base) {\n \t\t\tnr_deltas++;\n+\t\t\tif (entry->type < 0)\n+\t\t\t\tdie(\"unable to get type of object %s\",\n+\t\t\t\t sha1_to_hex(entry->idx.sha1));\n+\t\t}\n \n \t\tdelta_list[n++] = entry;\n \t}\n\nAlso a comment in check_object() mentioning where the return value of \nsha1_object_info() is verified would be in order.\n\nAnd I also agree with Junio about a test script for this so the usage is \nfully demonstrated, and to ensure it keeps on working as intended \n(most people will simply never exercise this otherwise).\n\n\nNicolas\n"},{"id":"86945","messageId":"20080812164149.GB31092@spearce.org","threadId":"14936","inReplyTo":"alpine.LFD.1.10.0808120023250.22892@xanadu.home","subject":"[PATCH] pack-objects: Allow missing base objects when creating thin packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-12T16:41:49Z","receivedAt":"2008-08-12T16:41:49Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If we are building a thin pack and one of the base objects we would\nconsider for deltification is missing its OK, the other side already\nhas that base object. We may be able to get a delta from another\nobject, or we can simply send the new object whole (no delta).\n\nThis change allows a shallow clone to store only the objects which\nare unique to it, as well as the boundary commit and its trees, but\navoids storing the boundary blobs. This special form of a shallow\nclone is able to represent just the difference between two trees.\n\nPack objects change suggested by Nicolas Pitre.\n\nSigned-off-by: Shawn O. Pearce \n---\n\n Nicolas Pitre wrote:\n > On Mon, 11 Aug 2008, Shawn O. Pearce wrote:\n > > > Junio C Hamano wrote:\n > > > > If the check is only about a thin delta base that is not going to be\n > > > > transmit, I'd agree.\n > \n > If you're going to die anyway due to an object with unknown type, better \n > do so _before_ going through the delta search phase and leaving a \n > partial pack behind.\n\n Now with a test! :-)\n \n builtin-pack-objects.c | 15 ++++++--\n t/t5306-pack-nobase.sh | 80 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 91 insertions(+), 4 deletions(-)\n create mode 100755 t/t5306-pack-nobase.sh\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 2dadec1..a90302d 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1096,9 +1096,12 @@ static void check_object(struct object_entry *entry)\n \t}\n \n \tentry->type = sha1_object_info(entry->idx.sha1, &entry->size);\n-\tif (entry->type < 0)\n-\t\tdie(\"unable to get type of object %s\",\n-\t\t sha1_to_hex(entry->idx.sha1));\n+\t/*\n+ \t * The error condition is checked in prepare_pack(). This is\n+ \t * to permit a missing preferred base object to be ignored\n+ \t * as a preferred base. Doing so can result in a larger\n+ \t * pack file, but the transfer will still take place.\n+ \t */\n }\n \n static int pack_offset_sort(const void *_a, const void *_b)\n@@ -1722,8 +1725,12 @@ static void prepare_pack(int window, int depth)\n \t\tif (entry->no_try_delta)\n \t\t\tcontinue;\n \n-\t\tif (!entry->preferred_base)\n+\t\tif (!entry->preferred_base) {\n \t\t\tnr_deltas++;\n+\t\t\tif (entry->type < 0)\n+\t\t\t\tdie(\"unable to get type of object %s\",\n+\t\t\t\t sha1_to_hex(entry->idx.sha1));\n+\t\t}\n \n \t\tdelta_list[n++] = entry;\n \t}\ndiff --git a/t/t5306-pack-nobase.sh b/t/t5306-pack-nobase.sh\nnew file mode 100755\nindex 0000000..503e9d4\n--- /dev/null\n+++ b/t/t5306-pack-nobase.sh\n@@ -0,0 +1,80 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2008 Google Inc.\n+#\n+\n+test_description='git-pack-object with missing base\n+\n+'\n+. ./test-lib.sh\n+\n+# Create A-B chain\n+#\n+test_expect_success \\\n+ 'setup base' \\\n+ 'for a in a b c d e f g h i; do echo $a >>text; done &&\n+ echo side >side &&\n+ git update-index --add text side &&\n+ A=$(echo A | git commit-tree $(git write-tree)) &&\n+\n+ echo m >>text &&\n+ git update-index text &&\n+ B=$(echo B | git commit-tree $(git write-tree) -p $A) &&\n+ git update-ref HEAD $B\n+ '\n+\n+# Create repository with C whose parent is B.\n+# Repository contains C, C^{tree}, C:text, B, B^{tree}.\n+# Repository is missing B:text (best delta base for C:text).\n+# Repository is missing A (parent of B).\n+# Repository is missing A:side.\n+#\n+test_expect_success \\\n+ 'setup patch_clone' \\\n+ 'base_objects=$(pwd)/.git/objects &&\n+ (mkdir patch_clone &&\n+ cd patch_clone &&\n+ git init &&\n+ echo \"$base_objects\" >.git/objects/info/alternates &&\n+ echo q >>text &&\n+ git read-tree $B &&\n+ git update-index text &&\n+ git update-ref HEAD $(echo C | git commit-tree $(git write-tree) -p $B) &&\n+ rm .git/objects/info/alternates &&\n+\n+ git --git-dir=../.git cat-file commit $B |\n+ git hash-object -t commit -w --stdin &&\n+\n+ git --git-dir=../.git cat-file tree \"$B^{tree}\" |\n+ git hash-object -t tree -w --stdin\n+ ) &&\n+ C=$(git --git-dir=patch_clone/.git rev-parse HEAD)\n+ '\n+\n+# Clone patch_clone indirectly by cloning base and fetching.\n+#\n+test_expect_success \\\n+ 'indirectly clone patch_clone' \\\n+ '(mkdir user_clone &&\n+ cd user_clone &&\n+ git init &&\n+ git pull ../.git &&\n+ test $(git rev-parse HEAD) = $B\n+\n+ git pull ../patch_clone/.git &&\n+ test $(git rev-parse HEAD) = $C\n+ )\n+ '\n+\n+# Cloning the patch_clone directly should fail.\n+#\n+test_expect_success \\\n+ 'clone of patch_clone is incomplete' \\\n+ '(mkdir user_direct &&\n+ cd user_direct &&\n+ git init &&\n+ test_must_fail git fetch ../patch_clone/.git\n+ )\n+ '\n+\n+test_done\n-- \n1.6.0.rc2.22.g71b99\n\n\n-- \nShawn.\n"},{"id":"86955","messageId":"alpine.LFD.1.10.0808121402440.22892@xanadu.home","threadId":"14936","inReplyTo":"20080812164149.GB31092@spearce.org","subject":"Re: [PATCH] pack-objects: Allow missing base objects when creating thin packs","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-12T18:12:30Z","receivedAt":"2008-08-12T18:12:30Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 12 Aug 2008, Shawn O. Pearce wrote:\n\n> Now with a test! :-)\n> \n[...]\n> +# Clone patch_clone indirectly by cloning base and fetching.\n> +#\n> +test_expect_success \\\n> + 'indirectly clone patch_clone' \\\n> + '(mkdir user_clone &&\n> + cd user_clone &&\n> + git init &&\n> + git pull ../.git &&\n> + test $(git rev-parse HEAD) = $B\n> +\n> + git pull ../patch_clone/.git &&\n> + test $(git rev-parse HEAD) = $C\n> + )\n> + '\n\nWhat if the first test command fails? Won't its result be ignored?\n\n\nNicolas\n"},{"id":"86957","messageId":"20080812181843.GD31092@spearce.org","threadId":"14936","inReplyTo":"alpine.LFD.1.10.0808121402440.22892@xanadu.home","subject":"Re: [PATCH] pack-objects: Allow missing base objects when creating thin packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-12T18:18:43Z","receivedAt":"2008-08-12T18:18:43Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> On Tue, 12 Aug 2008, Shawn O. Pearce wrote:\n> > +# Clone patch_clone indirectly by cloning base and fetching.\n> > +#\n> > +test_expect_success \\\n> > + 'indirectly clone patch_clone' \\\n> > + '(mkdir user_clone &&\n> > + cd user_clone &&\n> > + git init &&\n> > + git pull ../.git &&\n> > + test $(git rev-parse HEAD) = $B\n> > +\n> > + git pull ../patch_clone/.git &&\n> > + test $(git rev-parse HEAD) = $C\n> > + )\n> > + '\n> \n> What if the first test command fails? Won't its result be ignored?\n\nIsn't the exit status of the subshell the exit status of the last\ncommand in the subshell?\n\nI just changed the test line to compare to \"x$C\" instead of $C\nand it correctly detected the error condition:\n\n$ git diff\ndiff --git a/t/t5306-pack-nobase.sh b/t/t5306-pack-nobase.sh\nindex 503e9d4..7c55e9e 100755\n--- a/t/t5306-pack-nobase.sh\n+++ b/t/t5306-pack-nobase.sh\n@@ -62,7 +62,7 @@ test_expect_success \\\n test $(git rev-parse HEAD) = $B\n\n git pull ../patch_clone/.git &&\n- test $(git rev-parse HEAD) = $C\n+ test $(git rev-parse HEAD) = x$C\n )\n '\n\n$ ./t5306-pack-nobase.sh\n* ok 1: setup base\n* ok 2: setup patch_clone\n* FAIL 3: indirectly clone patch_clone\n (mkdir user_clone &&\n cd user_clone &&\n git init &&\n git pull ../.git &&\n test $(git rev-parse HEAD) = $B\n\n git pull ../patch_clone/.git &&\n test $(git rev-parse HEAD) = x$C\n )\n\n* ok 4: clone of patch_clone is incomplete\n* failed 1 among 4 test(s)\n\n-- \nShawn.\n"},{"id":"86959","messageId":"alpine.LFD.1.10.0808121426100.22892@xanadu.home","threadId":"14936","inReplyTo":"20080812181843.GD31092@spearce.org","subject":"Re: [PATCH] pack-objects: Allow missing base objects when creating thin packs","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-12T18:26:52Z","receivedAt":"2008-08-12T18:26:52Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 12 Aug 2008, Shawn O. Pearce wrote:\n\n> Nicolas Pitre wrote:\n> > On Tue, 12 Aug 2008, Shawn O. Pearce wrote:\n> > > +# Clone patch_clone indirectly by cloning base and fetching.\n> > > +#\n> > > +test_expect_success \\\n> > > + 'indirectly clone patch_clone' \\\n> > > + '(mkdir user_clone &&\n> > > + cd user_clone &&\n> > > + git init &&\n> > > + git pull ../.git &&\n> > > + test $(git rev-parse HEAD) = $B\n> > > +\n> > > + git pull ../patch_clone/.git &&\n> > > + test $(git rev-parse HEAD) = $C\n> > > + )\n> > > + '\n> > \n> > What if the first test command fails? Won't its result be ignored?\n> \n> Isn't the exit status of the subshell the exit status of the last\n> command in the subshell?\n\nI'm not talking about the last command but the \"test $(git rev-parse \nHEAD) = $B\" line.\n\n\nNicolas\n"},{"id":"86960","messageId":"20080812183106.GE31092@spearce.org","threadId":"14936","inReplyTo":"alpine.LFD.1.10.0808121426100.22892@xanadu.home","subject":"[PATCH v2] pack-objects: Allow missing base objects when creating thin packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-12T18:31:06Z","receivedAt":"2008-08-12T18:31:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If we are building a thin pack and one of the base objects we would\nconsider for deltification is missing its OK, the other side already\nhas that base object. We may be able to get a delta from another\nobject, or we can simply send the new object whole (no delta).\n\nThis change allows a shallow clone to store only the objects which\nare unique to it, as well as the boundary commit and its trees, but\navoids storing the boundary blobs. This special form of a shallow\nclone is able to represent just the difference between two trees.\n\nPack objects change suggested by Nicolas Pitre.\n\nSigned-off-by: Shawn O. Pearce \n---\n\n Nicolas Pitre wrote:\n > On Tue, 12 Aug 2008, Shawn O. Pearce wrote:\n > I'm not talking about the last command but the \"test $(git rev-parse \n > HEAD) = $B\" line.\n \n Oh, right. Good catch.\n\n builtin-pack-objects.c | 15 ++++++--\n t/t5306-pack-nobase.sh | 80 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 91 insertions(+), 4 deletions(-)\n create mode 100755 t/t5306-pack-nobase.sh\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 2dadec1..a90302d 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1096,9 +1096,12 @@ static void check_object(struct object_entry *entry)\n \t}\n \n \tentry->type = sha1_object_info(entry->idx.sha1, &entry->size);\n-\tif (entry->type < 0)\n-\t\tdie(\"unable to get type of object %s\",\n-\t\t sha1_to_hex(entry->idx.sha1));\n+\t/*\n+ \t * The error condition is checked in prepare_pack(). This is\n+ \t * to permit a missing preferred base object to be ignored\n+ \t * as a preferred base. Doing so can result in a larger\n+ \t * pack file, but the transfer will still take place.\n+ \t */\n }\n \n static int pack_offset_sort(const void *_a, const void *_b)\n@@ -1722,8 +1725,12 @@ static void prepare_pack(int window, int depth)\n \t\tif (entry->no_try_delta)\n \t\t\tcontinue;\n \n-\t\tif (!entry->preferred_base)\n+\t\tif (!entry->preferred_base) {\n \t\t\tnr_deltas++;\n+\t\t\tif (entry->type < 0)\n+\t\t\t\tdie(\"unable to get type of object %s\",\n+\t\t\t\t sha1_to_hex(entry->idx.sha1));\n+\t\t}\n \n \t\tdelta_list[n++] = entry;\n \t}\ndiff --git a/t/t5306-pack-nobase.sh b/t/t5306-pack-nobase.sh\nnew file mode 100755\nindex 0000000..f4931c0\n--- /dev/null\n+++ b/t/t5306-pack-nobase.sh\n@@ -0,0 +1,80 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2008 Google Inc.\n+#\n+\n+test_description='git-pack-object with missing base\n+\n+'\n+. ./test-lib.sh\n+\n+# Create A-B chain\n+#\n+test_expect_success \\\n+ 'setup base' \\\n+ 'for a in a b c d e f g h i; do echo $a >>text; done &&\n+ echo side >side &&\n+ git update-index --add text side &&\n+ A=$(echo A | git commit-tree $(git write-tree)) &&\n+\n+ echo m >>text &&\n+ git update-index text &&\n+ B=$(echo B | git commit-tree $(git write-tree) -p $A) &&\n+ git update-ref HEAD $B\n+ '\n+\n+# Create repository with C whose parent is B.\n+# Repository contains C, C^{tree}, C:text, B, B^{tree}.\n+# Repository is missing B:text (best delta base for C:text).\n+# Repository is missing A (parent of B).\n+# Repository is missing A:side.\n+#\n+test_expect_success \\\n+ 'setup patch_clone' \\\n+ 'base_objects=$(pwd)/.git/objects &&\n+ (mkdir patch_clone &&\n+ cd patch_clone &&\n+ git init &&\n+ echo \"$base_objects\" >.git/objects/info/alternates &&\n+ echo q >>text &&\n+ git read-tree $B &&\n+ git update-index text &&\n+ git update-ref HEAD $(echo C | git commit-tree $(git write-tree) -p $B) &&\n+ rm .git/objects/info/alternates &&\n+\n+ git --git-dir=../.git cat-file commit $B |\n+ git hash-object -t commit -w --stdin &&\n+\n+ git --git-dir=../.git cat-file tree \"$B^{tree}\" |\n+ git hash-object -t tree -w --stdin\n+ ) &&\n+ C=$(git --git-dir=patch_clone/.git rev-parse HEAD)\n+ '\n+\n+# Clone patch_clone indirectly by cloning base and fetching.\n+#\n+test_expect_success \\\n+ 'indirectly clone patch_clone' \\\n+ '(mkdir user_clone &&\n+ cd user_clone &&\n+ git init &&\n+ git pull ../.git &&\n+ test $(git rev-parse HEAD) = $B &&\n+\n+ git pull ../patch_clone/.git &&\n+ test $(git rev-parse HEAD) = $C\n+ )\n+ '\n+\n+# Cloning the patch_clone directly should fail.\n+#\n+test_expect_success \\\n+ 'clone of patch_clone is incomplete' \\\n+ '(mkdir user_direct &&\n+ cd user_direct &&\n+ git init &&\n+ test_must_fail git fetch ../patch_clone/.git\n+ )\n+ '\n+\n+test_done\n-- \n1.6.0.rc2.22.g71b99\n\n\n-- \nShawn.\n"},{"id":"86962","messageId":"alpine.LFD.1.10.0808121439150.22892@xanadu.home","threadId":"14936","inReplyTo":"20080812183106.GE31092@spearce.org","subject":"Re: [PATCH v2] pack-objects: Allow missing base objects when creating thin packs","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-12T18:43:45Z","receivedAt":"2008-08-12T18:43:45Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 12 Aug 2008, Shawn O. Pearce wrote:\n\n> If we are building a thin pack and one of the base objects we would\n> consider for deltification is missing its OK, the other side already\n> has that base object. We may be able to get a delta from another\n> object, or we can simply send the new object whole (no delta).\n> \n> This change allows a shallow clone to store only the objects which\n> are unique to it, as well as the boundary commit and its trees, but\n> avoids storing the boundary blobs. This special form of a shallow\n> clone is able to represent just the difference between two trees.\n> \n> Pack objects change suggested by Nicolas Pitre.\n> \n> Signed-off-by: Shawn O. Pearce \n> ---\n> \n> Nicolas Pitre wrote:\n> > On Tue, 12 Aug 2008, Shawn O. Pearce wrote:\n> > I'm not talking about the last command but the \"test $(git rev-parse \n> > HEAD) = $B\" line.\n> \n> Oh, right. Good catch.\n\nAcked-by: Nicolas Pitre \n\n> +++ b/t/t5306-pack-nobase.sh\n> @@ -0,0 +1,80 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2008 Google Inc.\n\nHeh. ;-)\n\n\nNicolas\n"}]}