Merge branch 'jk/pack-objects-with-bitmap-fix'
authorJunio C Hamano <gitster@pobox.com>
Mon, 17 Sep 2018 20:53:58 +0000 (13:53 -0700)
committerJunio C Hamano <gitster@pobox.com>
Mon, 17 Sep 2018 20:53:58 +0000 (13:53 -0700)
Hotfix of the base topic.

* jk/pack-objects-with-bitmap-fix:
pack-bitmap: drop "loaded" flag
traverse_bitmap_commit_list(): don't free result
t5310: test delta reuse with bitmaps
bitmap_has_sha1_in_uninteresting(): drop BUG check

1  2 
pack-bitmap.c
t/t5310-pack-bitmaps.sh
diff --combined pack-bitmap.c
index c7d593c91c7a76497407a703d9f0c50925e2475a,40debd5e202e7fc0cc3aaa5eb01ecde62010c748..5848cc93aa254b8549f4c569882b5d781984ca47
@@@ -91,8 -91,6 +91,6 @@@ struct bitmap_index 
  
        /* Version of the bitmap index */
        unsigned int version;
-       unsigned loaded : 1;
  };
  
  static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)
@@@ -306,7 -304,7 +304,7 @@@ static int open_pack_bitmap_1(struct bi
  
  static int load_pack_bitmap(struct bitmap_index *bitmap_git)
  {
-       assert(bitmap_git->map && !bitmap_git->loaded);
+       assert(bitmap_git->map);
  
        bitmap_git->bitmaps = kh_init_sha1();
        bitmap_git->ext_index.positions = kh_init_sha1_pos();
        if (load_bitmap_entries_v1(bitmap_git) < 0)
                goto failed;
  
-       bitmap_git->loaded = 1;
        return 0;
  
  failed:
@@@ -336,9 -333,9 +333,9 @@@ static int open_pack_bitmap(struct bitm
        struct packed_git *p;
        int ret = -1;
  
-       assert(!bitmap_git->map && !bitmap_git->loaded);
+       assert(!bitmap_git->map);
  
 -      for (p = get_packed_git(the_repository); p; p = p->next) {
 +      for (p = get_all_packs(the_repository); p; p = p->next) {
                if (open_pack_bitmap_1(bitmap_git, p) == 0)
                        ret = 0;
        }
@@@ -738,7 -735,7 +735,7 @@@ struct bitmap_index *prepare_bitmap_wal
         * from disk. this is the point of no return; after this the rev_list
         * becomes invalidated and we must perform the revwalk through bitmaps
         */
-       if (!bitmap_git->loaded && load_pack_bitmap(bitmap_git) < 0)
+       if (load_pack_bitmap(bitmap_git) < 0)
                goto cleanup;
  
        object_array_clear(&revs->pending);
@@@ -848,9 -845,6 +845,6 @@@ void traverse_bitmap_commit_list(struc
                OBJ_TAG, show_reachable);
  
        show_extended_objects(bitmap_git, show_reachable);
-       bitmap_free(bitmap_git->result);
-       bitmap_git->result = NULL;
  }
  
  static uint32_t count_object_type(struct bitmap_index *bitmap_git,
@@@ -1128,8 -1122,6 +1122,6 @@@ int bitmap_has_sha1_in_uninteresting(st
  
        if (!bitmap_git)
                return 0; /* no bitmap loaded */
-       if (!bitmap_git->result)
-               BUG("failed to perform bitmap walk before querying");
        if (!bitmap_git->haves)
                return 0; /* walk had no "haves" */
  
diff --combined t/t5310-pack-bitmaps.sh
index 7bff7923f2a91165b5a0b5251351798519d63270,af387760549b62d1f537b3fc25be20c9c56e82e2..1be3459c5b8aa331a646e3ead24f7422110cdc08
@@@ -335,11 -335,104 +335,104 @@@ test_expect_success 'truncated bitmap f
        git rev-list --use-bitmap-index --count --all >expect &&
        bitmap=$(ls .git/objects/pack/*.bitmap) &&
        test_when_finished "rm -f $bitmap" &&
 -      head -c 512 <$bitmap >$bitmap.tmp &&
 +      test_copy_bytes 512 <$bitmap >$bitmap.tmp &&
        mv -f $bitmap.tmp $bitmap &&
        git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
        test_cmp expect actual &&
        test_i18ngrep corrupt stderr
  '
  
+ # have_delta <obj> <expected_base>
+ #
+ # Note that because this relies on cat-file, it might find _any_ copy of an
+ # object in the repository. The caller is responsible for making sure
+ # there's only one (e.g., via "repack -ad", or having just fetched a copy).
+ have_delta () {
+       echo $2 >expect &&
+       echo $1 | git cat-file --batch-check="%(deltabase)" >actual &&
+       test_cmp expect actual
+ }
+ # Create a state of history with these properties:
+ #
+ #  - refs that allow a client to fetch some new history, while sharing some old
+ #    history with the server; we use branches delta-reuse-old and
+ #    delta-reuse-new here
+ #
+ #  - the new history contains an object that is stored on the server as a delta
+ #    against a base that is in the old history
+ #
+ #  - the base object is not immediately reachable from the tip of the old
+ #    history; finding it would involve digging down through history we know the
+ #    other side has
+ #
+ # This should result in a state where fetching from old->new would not
+ # traditionally reuse the on-disk delta (because we'd have to dig to realize
+ # that the client has it), but we will do so if bitmaps can tell us cheaply
+ # that the other side has it.
+ test_expect_success 'set up thin delta-reuse parent' '
+       # This first commit contains the buried base object.
+       test-tool genrandom delta 16384 >file &&
+       git add file &&
+       git commit -m "delta base" &&
+       base=$(git rev-parse --verify HEAD:file) &&
+       # These intermediate commits bury the base back in history.
+       # This becomes the "old" state.
+       for i in 1 2 3 4 5
+       do
+               echo $i >file &&
+               git commit -am "intermediate $i" || return 1
+       done &&
+       git branch delta-reuse-old &&
+       # And now our new history has a delta against the buried base. Note
+       # that this must be smaller than the original file, since pack-objects
+       # prefers to create deltas from smaller objects to larger.
+       test-tool genrandom delta 16300 >file &&
+       git commit -am "delta result" &&
+       delta=$(git rev-parse --verify HEAD:file) &&
+       git branch delta-reuse-new &&
+       # Repack with bitmaps and double check that we have the expected delta
+       # relationship.
+       git repack -adb &&
+       have_delta $delta $base
+ '
+ # Now we can sanity-check the non-bitmap behavior (that the server is not able
+ # to reuse the delta). This isn't strictly something we care about, so this
+ # test could be scrapped in the future. But it makes sure that the next test is
+ # actually triggering the feature we want.
+ #
+ # Note that our tools for working with on-the-wire "thin" packs are limited. So
+ # we actually perform the fetch, retain the resulting pack, and inspect the
+ # result.
+ test_expect_success 'fetch without bitmaps ignores delta against old base' '
+       test_config pack.usebitmaps false &&
+       test_when_finished "rm -rf client.git" &&
+       git init --bare client.git &&
+       (
+               cd client.git &&
+               git config transfer.unpackLimit 1 &&
+               git fetch .. delta-reuse-old:delta-reuse-old &&
+               git fetch .. delta-reuse-new:delta-reuse-new &&
+               have_delta $delta $ZERO_OID
+       )
+ '
+ # And do the same for the bitmap case, where we do expect to find the delta.
+ test_expect_success 'fetch with bitmaps can reuse old base' '
+       test_config pack.usebitmaps true &&
+       test_when_finished "rm -rf client.git" &&
+       git init --bare client.git &&
+       (
+               cd client.git &&
+               git config transfer.unpackLimit 1 &&
+               git fetch .. delta-reuse-old:delta-reuse-old &&
+               git fetch .. delta-reuse-new:delta-reuse-new &&
+               have_delta $delta $base
+       )
+ '
  test_done