Re: RFC: Allow missing objects during packing

"Shawn O. Pearce" [off-list ref] writes:

... It seems pretty harmless to allow an object we
aren't going to transmit but that we want to use as a delta base
in a thin pack to be missing.  At worst we just get a little bit
more data transfer.

If the check is only about a thin delta base that is not going to be
transmit, I'd agree.  But I do not see how you are distinguishing that
case and the case where an object you are actually sending is missing (in
which case we would want to error out, wouldn't we?)

Junio C Hamano [off-list ref] wrote:

"Shawn O. Pearce" [off-list ref] writes:

quoted

... It seems pretty harmless to allow an object we
aren't going to transmit but that we want to use as a delta base
in a thin pack to be missing.  At worst we just get a little bit
more data transfer.

If the check is only about a thin delta base that is not going to be
transmit, I'd agree.  But I do not see how you are distinguishing that
case and the case where an object you are actually sending is missing (in
which case we would want to error out, wouldn't we?)

Arrgh.  Good catch.  My patch is flawed in that it does not correctly
fail if we really needed the missing object in this output pack.

I don't think that would be hard to fix.  I'll respin something
shortly.

-- 
Shawn.

Junio C Hamano [off-list ref] wrote:

quoted

If the check is only about a thin delta base that is not going to be
transmit, I'd agree.  But I do not see how you are distinguishing that
case and the case where an object you are actually sending is missing (in
which case we would want to error out, wouldn't we?)

Turns out to be pretty simple I think.  We just delay the
error handling for ->type < 0 until write_object().  If we
get this far we know we wanted to include the object but
we really don't have it.  Up until that point its fine
for us to get objects which are missing, we'll just wind
up with a suboptimal pack.

We also don't even need to report the error from sha1_object_info
as it already issues an error message (see sha1_loose_object_info).

--8<--
pack-objects: Allow missing base objects when creating thin packs

If we are building a thin pack and one of the base objects we would
consider for deltification is missing its OK, the other side already
has that base object.  We may be able to get a delta from another
object, or we can simply send the new object whole (no delta).

This allows a shallow clone which may have only commits and trees
(but only partial blobs) to generate a pack for a fetch client,
so the shallow clone only needs to contain objects that are not
in the common base.

Signed-off-by: Shawn O. Pearce <redacted>
---
 builtin-pack-objects.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 2dadec1..187cb19 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c

@@ -243,6 +243,8 @@ static unsigned long write_object(struct sha1file *f,
 		crc32_begin(f);
 
 	type = entry->type;
+	if (type < 0)
+		die("unable to read object %s", sha1_to_hex(entry->idx.sha1));
 
 	/* write limit if limited packsize and not first object */
 	limit = pack_size_limit && nr_written ?

@@ -1096,9 +1098,6 @@ static void check_object(struct object_entry *entry)
 	}
 
 	entry->type = sha1_object_info(entry->idx.sha1, &entry->size);
-	if (entry->type < 0)
-		die("unable to get type of object %s",
-		    sha1_to_hex(entry->idx.sha1));
 }
 
 static int pack_offset_sort(const void *_a, const void *_b)

-- 
1.6.0.rc2.22.g71b99

-- 
Shawn.

On Mon, 11 Aug 2008, Shawn O. Pearce wrote:

quoted

Junio C Hamano [off-list ref] wrote:

quoted

If the check is only about a thin delta base that is not going to be
transmit, I'd agree.  But I do not see how you are distinguishing that
case and the case where an object you are actually sending is missing (in
which case we would want to error out, wouldn't we?)

Turns out to be pretty simple I think.  We just delay the
error handling for ->type < 0 until write_object().  If we
get this far we know we wanted to include the object but
we really don't have it.  Up until that point its fine
for us to get objects which are missing, we'll just wind
up with a suboptimal pack.

If you're going to die anyway due to an object with unknown type, better 
do so _before_ going through the delta search phase and leaving a 
partial pack behind.  IOW, the type check can be performed in 
prepare_pack() instead of write_object() like:

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 2dadec1..01ab49c 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c

@@ -1722,8 +1733,12 @@ static void prepare_pack(int window, int depth)
 		if (entry->no_try_delta)
 			continue;
 
-		if (!entry->preferred_base)
+		if (!entry->preferred_base) {
 			nr_deltas++;
+			if (entry->type < 0)
+				die("unable to get type of object %s",
+				    sha1_to_hex(entry->idx.sha1));
+		}
 
 		delta_list[n++] = entry;
 	}

Also a comment in check_object() mentioning where the return value of 
sha1_object_info() is verified would be in order.

And I also agree with Junio about a test script for this so the usage is 
fully demonstrated, and to ensure it keeps on working as intended 
(most people will simply never exercise this otherwise).


Nicolas

If we are building a thin pack and one of the base objects we would
consider for deltification is missing its OK, the other side already
has that base object.  We may be able to get a delta from another
object, or we can simply send the new object whole (no delta).

This change allows a shallow clone to store only the objects which
are unique to it, as well as the boundary commit and its trees, but
avoids storing the boundary blobs.  This special form of a shallow
clone is able to represent just the difference between two trees.

Pack objects change suggested by Nicolas Pitre.

Signed-off-by: Shawn O. Pearce <redacted>
---

 Nicolas Pitre [off-list ref] wrote:
 > On Mon, 11 Aug 2008, Shawn O. Pearce wrote:
 > > > Junio C Hamano [off-list ref] wrote:
 > > > > If the check is only about a thin delta base that is not going to be
 > > > > transmit, I'd agree.
 > 
 > If you're going to die anyway due to an object with unknown type, better 
 > do so _before_ going through the delta search phase and leaving a 
 > partial pack behind.

 Now with a test! :-)
 
 builtin-pack-objects.c |   15 ++++++--
 t/t5306-pack-nobase.sh |   80 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 4 deletions(-)
 create mode 100755 t/t5306-pack-nobase.sh

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 2dadec1..a90302d 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c

@@ -1096,9 +1096,12 @@ static void check_object(struct object_entry *entry)
 	}
 
 	entry->type = sha1_object_info(entry->idx.sha1, &entry->size);
-	if (entry->type < 0)
-		die("unable to get type of object %s",
-		    sha1_to_hex(entry->idx.sha1));
+	/*
+ 	 * The error condition is checked in prepare_pack().  This is
+ 	 * to permit a missing preferred base object to be ignored
+ 	 * as a preferred base.  Doing so can result in a larger
+ 	 * pack file, but the transfer will still take place.
+ 	 */
 }
 
 static int pack_offset_sort(const void *_a, const void *_b)

@@ -1722,8 +1725,12 @@ static void prepare_pack(int window, int depth)
 		if (entry->no_try_delta)
 			continue;
 
-		if (!entry->preferred_base)
+		if (!entry->preferred_base) {
 			nr_deltas++;
+			if (entry->type < 0)
+				die("unable to get type of object %s",
+				    sha1_to_hex(entry->idx.sha1));
+		}
 
 		delta_list[n++] = entry;
 	}

diff --git a/t/t5306-pack-nobase.sh b/t/t5306-pack-nobase.sh
new file mode 100755
index 0000000..503e9d4
--- /dev/null
+++ b/t/t5306-pack-nobase.sh

@@ -0,0 +1,80 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Google Inc.
+#
+
+test_description='git-pack-object with missing base
+
+'
+. ./test-lib.sh
+
+# Create A-B chain
+#
+test_expect_success \
+    'setup base' \
+    'for a in a b c d e f g h i; do echo $a >>text; done &&
+     echo side >side &&
+     git update-index --add text side &&
+     A=$(echo A | git commit-tree $(git write-tree)) &&
+
+     echo m >>text &&
+     git update-index text &&
+     B=$(echo B | git commit-tree $(git write-tree) -p $A) &&
+     git update-ref HEAD $B
+    '
+
+# Create repository with C whose parent is B.
+# Repository contains C, C^{tree}, C:text, B, B^{tree}.
+# Repository is missing B:text (best delta base for C:text).
+# Repository is missing A (parent of B).
+# Repository is missing A:side.
+#
+test_expect_success \
+    'setup patch_clone' \
+    'base_objects=$(pwd)/.git/objects &&
+     (mkdir patch_clone &&
+      cd patch_clone &&
+      git init &&
+      echo "$base_objects" >.git/objects/info/alternates &&
+      echo q >>text &&
+      git read-tree $B &&
+      git update-index text &&
+      git update-ref HEAD $(echo C | git commit-tree $(git write-tree) -p $B) &&
+      rm .git/objects/info/alternates &&
+
+      git --git-dir=../.git cat-file commit $B |
+      git hash-object -t commit -w --stdin &&
+
+      git --git-dir=../.git cat-file tree "$B^{tree}" |
+      git hash-object -t tree -w --stdin
+     ) &&
+     C=$(git --git-dir=patch_clone/.git rev-parse HEAD)
+    '
+
+# Clone patch_clone indirectly by cloning base and fetching.
+#
+test_expect_success \
+    'indirectly clone patch_clone' \
+    '(mkdir user_clone &&
+      cd user_clone &&
+      git init &&
+      git pull ../.git &&
+      test $(git rev-parse HEAD) = $B
+
+      git pull ../patch_clone/.git &&
+      test $(git rev-parse HEAD) = $C
+     )
+    '
+
+# Cloning the patch_clone directly should fail.
+#
+test_expect_success \
+    'clone of patch_clone is incomplete' \
+    '(mkdir user_direct &&
+      cd user_direct &&
+      git init &&
+      test_must_fail git fetch ../patch_clone/.git
+     )
+    '
+
+test_done

-- 
1.6.0.rc2.22.g71b99


-- 
Shawn.

On Tue, 12 Aug 2008, Shawn O. Pearce wrote:

 Now with a test! :-)
 

[...]

+# Clone patch_clone indirectly by cloning base and fetching.
+#
+test_expect_success \
+    'indirectly clone patch_clone' \
+    '(mkdir user_clone &&
+      cd user_clone &&
+      git init &&
+      git pull ../.git &&
+      test $(git rev-parse HEAD) = $B
+
+      git pull ../patch_clone/.git &&
+      test $(git rev-parse HEAD) = $C
+     )
+    '

What if the first test command fails?  Won't its result be ignored?


Nicolas

Nicolas Pitre [off-list ref] wrote:

On Tue, 12 Aug 2008, Shawn O. Pearce wrote:

quoted

+# Clone patch_clone indirectly by cloning base and fetching.
+#
+test_expect_success \
+    'indirectly clone patch_clone' \
+    '(mkdir user_clone &&
+      cd user_clone &&
+      git init &&
+      git pull ../.git &&
+      test $(git rev-parse HEAD) = $B
+
+      git pull ../patch_clone/.git &&
+      test $(git rev-parse HEAD) = $C
+     )
+    '

What if the first test command fails?  Won't its result be ignored?

Isn't the exit status of the subshell the exit status of the last
command in the subshell?

I just changed the test line to compare to "x$C" instead of $C
and it correctly detected the error condition:

$ git diff

diff --git a/t/t5306-pack-nobase.sh b/t/t5306-pack-nobase.sh
index 503e9d4..7c55e9e 100755
--- a/t/t5306-pack-nobase.sh
+++ b/t/t5306-pack-nobase.sh

@@ -62,7 +62,7 @@ test_expect_success \
       test $(git rev-parse HEAD) = $B

       git pull ../patch_clone/.git &&
-      test $(git rev-parse HEAD) = $C
+      test $(git rev-parse HEAD) = x$C
      )
     '

$ ./t5306-pack-nobase.sh
*   ok 1: setup base
*   ok 2: setup patch_clone
* FAIL 3: indirectly clone patch_clone
        (mkdir user_clone &&
              cd user_clone &&
              git init &&
              git pull ../.git &&
              test $(git rev-parse HEAD) = $B

              git pull ../patch_clone/.git &&
              test $(git rev-parse HEAD) = x$C
             )

*   ok 4: clone of patch_clone is incomplete
* failed 1 among 4 test(s)

-- 
Shawn.

On Tue, 12 Aug 2008, Shawn O. Pearce wrote:

Nicolas Pitre [off-list ref] wrote:

quoted

On Tue, 12 Aug 2008, Shawn O. Pearce wrote:

quoted

+# Clone patch_clone indirectly by cloning base and fetching.
+#
+test_expect_success \
+    'indirectly clone patch_clone' \
+    '(mkdir user_clone &&
+      cd user_clone &&
+      git init &&
+      git pull ../.git &&
+      test $(git rev-parse HEAD) = $B
+
+      git pull ../patch_clone/.git &&
+      test $(git rev-parse HEAD) = $C
+     )
+    '

What if the first test command fails?  Won't its result be ignored?

Isn't the exit status of the subshell the exit status of the last
command in the subshell?

I'm not talking about the last command but the "test $(git rev-parse 
HEAD) = $B" line.


Nicolas

If we are building a thin pack and one of the base objects we would
consider for deltification is missing its OK, the other side already
has that base object.  We may be able to get a delta from another
object, or we can simply send the new object whole (no delta).

This change allows a shallow clone to store only the objects which
are unique to it, as well as the boundary commit and its trees, but
avoids storing the boundary blobs.  This special form of a shallow
clone is able to represent just the difference between two trees.

Pack objects change suggested by Nicolas Pitre.

Signed-off-by: Shawn O. Pearce <redacted>
---

 Nicolas Pitre [off-list ref] wrote:
 > On Tue, 12 Aug 2008, Shawn O. Pearce wrote:
 > I'm not talking about the last command but the "test $(git rev-parse 
 > HEAD) = $B" line.
 
 Oh, right.  Good catch.

 builtin-pack-objects.c |   15 ++++++--
 t/t5306-pack-nobase.sh |   80 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 4 deletions(-)
 create mode 100755 t/t5306-pack-nobase.sh

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 2dadec1..a90302d 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c

@@ -1096,9 +1096,12 @@ static void check_object(struct object_entry *entry)
 	}
 
 	entry->type = sha1_object_info(entry->idx.sha1, &entry->size);
-	if (entry->type < 0)
-		die("unable to get type of object %s",
-		    sha1_to_hex(entry->idx.sha1));
+	/*
+ 	 * The error condition is checked in prepare_pack().  This is
+ 	 * to permit a missing preferred base object to be ignored
+ 	 * as a preferred base.  Doing so can result in a larger
+ 	 * pack file, but the transfer will still take place.
+ 	 */
 }
 
 static int pack_offset_sort(const void *_a, const void *_b)

@@ -1722,8 +1725,12 @@ static void prepare_pack(int window, int depth)
 		if (entry->no_try_delta)
 			continue;
 
-		if (!entry->preferred_base)
+		if (!entry->preferred_base) {
 			nr_deltas++;
+			if (entry->type < 0)
+				die("unable to get type of object %s",
+				    sha1_to_hex(entry->idx.sha1));
+		}
 
 		delta_list[n++] = entry;
 	}

diff --git a/t/t5306-pack-nobase.sh b/t/t5306-pack-nobase.sh
new file mode 100755
index 0000000..f4931c0
--- /dev/null
+++ b/t/t5306-pack-nobase.sh

@@ -0,0 +1,80 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Google Inc.
+#
+
+test_description='git-pack-object with missing base
+
+'
+. ./test-lib.sh
+
+# Create A-B chain
+#
+test_expect_success \
+    'setup base' \
+    'for a in a b c d e f g h i; do echo $a >>text; done &&
+     echo side >side &&
+     git update-index --add text side &&
+     A=$(echo A | git commit-tree $(git write-tree)) &&
+
+     echo m >>text &&
+     git update-index text &&
+     B=$(echo B | git commit-tree $(git write-tree) -p $A) &&
+     git update-ref HEAD $B
+    '
+
+# Create repository with C whose parent is B.
+# Repository contains C, C^{tree}, C:text, B, B^{tree}.
+# Repository is missing B:text (best delta base for C:text).
+# Repository is missing A (parent of B).
+# Repository is missing A:side.
+#
+test_expect_success \
+    'setup patch_clone' \
+    'base_objects=$(pwd)/.git/objects &&
+     (mkdir patch_clone &&
+      cd patch_clone &&
+      git init &&
+      echo "$base_objects" >.git/objects/info/alternates &&
+      echo q >>text &&
+      git read-tree $B &&
+      git update-index text &&
+      git update-ref HEAD $(echo C | git commit-tree $(git write-tree) -p $B) &&
+      rm .git/objects/info/alternates &&
+
+      git --git-dir=../.git cat-file commit $B |
+      git hash-object -t commit -w --stdin &&
+
+      git --git-dir=../.git cat-file tree "$B^{tree}" |
+      git hash-object -t tree -w --stdin
+     ) &&
+     C=$(git --git-dir=patch_clone/.git rev-parse HEAD)
+    '
+
+# Clone patch_clone indirectly by cloning base and fetching.
+#
+test_expect_success \
+    'indirectly clone patch_clone' \
+    '(mkdir user_clone &&
+      cd user_clone &&
+      git init &&
+      git pull ../.git &&
+      test $(git rev-parse HEAD) = $B &&
+
+      git pull ../patch_clone/.git &&
+      test $(git rev-parse HEAD) = $C
+     )
+    '
+
+# Cloning the patch_clone directly should fail.
+#
+test_expect_success \
+    'clone of patch_clone is incomplete' \
+    '(mkdir user_direct &&
+      cd user_direct &&
+      git init &&
+      test_must_fail git fetch ../patch_clone/.git
+     )
+    '
+
+test_done

-- 
1.6.0.rc2.22.g71b99


-- 
Shawn.

On Tue, 12 Aug 2008, Shawn O. Pearce wrote:

If we are building a thin pack and one of the base objects we would
consider for deltification is missing its OK, the other side already
has that base object.  We may be able to get a delta from another
object, or we can simply send the new object whole (no delta).

This change allows a shallow clone to store only the objects which
are unique to it, as well as the boundary commit and its trees, but
avoids storing the boundary blobs.  This special form of a shallow
clone is able to represent just the difference between two trees.

Pack objects change suggested by Nicolas Pitre.

Signed-off-by: Shawn O. Pearce <redacted>
---

 Nicolas Pitre [off-list ref] wrote:
 > On Tue, 12 Aug 2008, Shawn O. Pearce wrote:
 > I'm not talking about the last command but the "test $(git rev-parse 
 > HEAD) = $B" line.
 
 Oh, right.  Good catch.

Acked-by: Nicolas Pitre <redacted>

Heh.  ;-)


Nicolas