Re: What's cooking in git.git (Sep 2018, #04; Thu, 20)

["René Scharfe" <l.s.r@web.de>]

Am 21.09.2018 um 07:22 schrieb Junio C Hamano:
> * cc/delta-islands (2018-08-16) 7 commits
>   (merged to 'next' on 2018-08-27 at cf3d7bd93f)
>  + pack-objects: move 'layer' into 'struct packing_data'
>  + pack-objects: move tree_depth into 'struct packing_data'
>  + t5320: tests for delta islands
>  + repack: add delta-islands support
>  + pack-objects: add delta-islands support
>  + pack-objects: refactor code into compute_layer_order()
>  + Add delta-islands.{c,h}
> 
>  Lift code from GitHub to restrict delta computation so that an
>  object that exists in one fork is not made into a delta against
>  another object that does not appear in the same forked repository.

> * jk/pack-delta-reuse-with-bitmap (2018-08-21) 6 commits
>   (merged to 'next' on 2018-08-22 at fc50b59dab)
>  + pack-objects: reuse on-disk deltas for thin "have" objects
>  + pack-bitmap: save "have" bitmap from walk
>  + t/perf: add perf tests for fetches from a bitmapped server
>  + t/perf: add infrastructure for measuring sizes
>  + t/perf: factor out percent calculations
>  + t/perf: factor boilerplate out of test_perf
>  (this branch is used by jk/pack-objects-with-bitmap-fix.)
> 
>  When creating a thin pack, which allows objects to be made into a
>  delta against another object that is not in the resulting pack but
>  is known to be present on the receiving end, the code learned to
>  take advantage of the reachability bitmap; this allows the server
>  to send a delta against a base beyond the "boundary" commit.

Not sure if it's the interaction of the two topics or if only one of
them is to blame, but the result of the merge can dereference a NULL
pointer.  Found using Clang's UBSan and t5310.

Here's a patch that avoids the issue, but I don't know if it's the
right thing to do -- should we rather treat a non-existing base as
"not from the same island" instead?

And it's certainly ugly -- that condition is complicated enough
already.  Splitting it up in a nice way would probably help, but how?

---
 builtin/pack-objects.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index e6316d294d..9abed4a0f0 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1559,7 +1559,8 @@ static void check_object(struct object_entry *entry)
 		    (base_entry = packlist_find(&to_pack, base_ref, NULL)) ||
 		    (thin &&
 		     bitmap_has_sha1_in_uninteresting(bitmap_git, base_ref))) &&
-		    in_same_island(&entry->idx.oid, &base_entry->idx.oid)) {
+		    (!base_entry ||
+		     in_same_island(&entry->idx.oid, &base_entry->idx.oid))) {
 			/*
 			 * If base_ref was set above that means we wish to
 			 * reuse delta data, and either we found that object in
-- 
2.19.0