Re: What's cooking in git.git (Sep 2021, #02; Wed, 8)

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 08/09/2021 16:38, Junio C Hamano wrote:

test t5319-multi-pack-index.sh on 'seen' has been failing for
several days, so I had a quick look tonight. This issue relates
to the following two topics:

> * jb/midx-revindex-fix (2021-08-23) 1 commit
>  - multi-pack-index: fix *.rev cleanups with --object-dir
> 
>  An implementation in "multi-pack-index write" that takes object
>  directory inconsistently used that and the object store of the
>  default repository, causing segfaults and possibly corrupting
>  repositories.
> 
>  On hold.
>  cf. <xmqqo89jbf49.fsf@gitster.g>
> 

...

> * tb/multi-pack-bitmaps (2021-09-01) 27 commits
>  - p5326: perf tests for MIDX bitmaps
>  - p5310: extract full and partial bitmap tests
>  - midx: respect 'GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP'
>  - t7700: update to work with MIDX bitmap test knob
>  - t5319: don't write MIDX bitmaps in t5319
>  - t5310: disable GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP
>  - t0410: disable GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP
>  - t5326: test multi-pack bitmap behavior
>  - t/helper/test-read-midx.c: add --checksum mode
>  - t5310: move some tests to lib-bitmap.sh
>  - pack-bitmap: write multi-pack bitmaps
>  - pack-bitmap: read multi-pack bitmaps
>  - pack-bitmap.c: avoid redundant calls to try_partial_reuse
>  - pack-bitmap.c: introduce 'bitmap_is_preferred_refname()'
>  - pack-bitmap.c: introduce 'nth_bitmap_object_oid()'
>  - pack-bitmap.c: introduce 'bitmap_num_objects()'
>  - midx: avoid opening multiple MIDXs when writing
>  - midx: close linked MIDXs, avoid leaking memory
>  - midx: infer preferred pack when not given one
>  - midx: reject empty `--preferred-pack`'s
>  - midx: clear auxiliary .rev after replacing the MIDX
>  - midx: fix `*.rev` cleanups with `--object-dir`
>  - midx: disallow running outside of a repository
>  - Documentation: describe MIDX-based bitmaps
>  - pack-bitmap-write.c: free existing bitmaps
>  - pack-bitmap-write.c: gracefully fail to write non-closed bitmaps
>  - pack-bitmap.c: harden 'test_bitmap_walk()' to check type bitmaps
> 
>  The reachability bitmap file used to be generated only for a single
>  pack, but now we've learned to generate bitmaps for history that
>  span across multiple packfiles.
> 
>  Will merge to 'next'.

The merge commit 24cade3ceb ("Merge branch 'jb/midx-revindex-fix' into seen",
2021-09-07) basically takes two versions of the same patch and keeps both
versions of the same test. The two patches are:

  - c575d4a2d9 ("multi-pack-index: fix *.rev cleanups with --object-dir",
    2021-08-23)
  - 426c00e454 ("midx: fix `*.rev` cleanups with `--object-dir`", 2021-08-31)

(the second patch is from the 'tb/multi-pack-bitmaps' branch).

The second patch even includes an acknowledgment of 'Original-patch-by:
Johannes Berg <johannes@sipsolutions.net>' (ie the first patch).

  $ ./t5319-multi-pack-index.sh -i -v
...
  expecting success of 5319.19 'multi-pack-index *.rev cleanup with --object-dir':
...
  ok 19 - multi-pack-index *.rev cleanup with --object-dir

  expecting success of 5319.20 'multi-pack-index *.rev cleanup with --object-dir':
...
  fatal: not a git repository (or any of the parent directories): .git
...
  $

The 'fatal' message is issued by a call to 'nongit git multi-pack-index ... write'
command, which implies that 'git multi-pack-index' dies when not run from
a repository. When the original patch was written, that command would not
have died, so somewhere between v2.33.0 and 'seen' (maybe as part of the
'tb/multi-pack-bitmaps' branch, I haven't looked), that was changed and
that test no longer works.

If I had to change that test to take account of that change in behaviour,
then it would look pretty much the same as the new test in Taylor's commit
(and which can now be seen as test #19 above).

So, a solution would be to simply drop 'jb/midx-revindex-fix', since it
has effectively been incorporated into Taylor's branch. You could also
go the other way around, drop commit 426c00e454 from Taylor's branch and
fix up the test in Johannes's patch ... :-P

Well, I only had a quick look so (hopefully) I haven't missed something
obvious!

ATB,
Ramsay Jones