Re: [PATCH 1/2] fsck: verify checksums of all .bitmap files

[Junio C Hamano <gitster@pobox.com>]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +static int verify_bitmap_file(const char *name)
> +{
> +	struct stat st;
> +	unsigned char *data;
> +	int fd = git_open(name);
> +	int res = 0;
> +
> +	/* A non-existent file is valid. */
> +	if (fstat(fd, &st)) {
> +		close(fd);
> +		return 0;
> +	}

git_open() may return a negative value to signal an error, and
fstat() would return negative to signal its own error, and feeding
the negative fd to close() would also be an error without molesting
other open file descriptors we would want to keep, and we do not
check the return value from close(), so all of the above may be
safe, but makes us feel a bit dirty.

	/* It is OK not to have the file */
	if (fd < 0 || fstat(fd, &st)) {
		if (0 <= fd)
			close(fd);
		return 0;
	}

is probably not overly too verbose and makes the result a bit more
palatable, perhaps.

> +	data = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
> +	close(fd);
> +	if (!hashfile_checksum_valid(data, st.st_size))
> +		res = error(_("bitmap file '%s' has invalid checksum"),
> +			    name);
> +
> +	munmap(data, st.st_size);
> +	return res;
> +}

OK.

> +int verify_bitmap_files(struct repository *r)
> +{
> +	int res = 0;
> +
> +	for (struct multi_pack_index *m = get_multi_pack_index(r);
> +	     m; m = m->next) {
> +		char *midx_bitmap_name = midx_bitmap_filename(m);
> +		res |= verify_bitmap_file(midx_bitmap_name);
> +		free(midx_bitmap_name);
> +	}
> +
> +	for (struct packed_git *p = get_all_packs(r);
> +	     p; p = p->next) {
> +		char *pack_bitmap_name = pack_bitmap_filename(p);
> +		res |= verify_bitmap_file(pack_bitmap_name);
> +		free(pack_bitmap_name);
> +	}
> +
> +	return res;
> +}

OK.

> diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
> index 0882cbb6e4a..d7353d8d443 100755
> --- a/t/t5326-multi-pack-bitmaps.sh
> +++ b/t/t5326-multi-pack-bitmaps.sh
> @@ -434,4 +434,42 @@ test_expect_success 'tagged commits are selected for bitmapping' '
>  	)
>  '
>  
> +corrupt_file () {
> +	chmod a+w "$1" &&
> +	printf "bogus" | dd of="$1" bs=1 seek="12" conv=notrunc
> +}
> +
> +test_expect_success 'git fsck correctly identifies good and bad bitmaps' '
> +	git init valid &&
> +	test_when_finished rm -rf valid &&
> +
> +	test_commit_bulk 20 &&
> +	git repack -adbf &&
> +
> +	# Move pack-bitmap aside so it is not deleted
> +	# in next repack.
> +	packbitmap=$(ls .git/objects/pack/pack-*.bitmap) &&
> +	mv "$packbitmap" "$packbitmap.bak" &&
> +
> +	test_commit_bulk 10 &&
> +	git repack -b --write-midx &&
> +	midxbitmap=$(ls .git/objects/pack/multi-pack-index-*.bitmap) &&
> +
> +	# Copy MIDX bitmap to backup. Copy pack bitmap from backup.
> +	cp "$midxbitmap" "$midxbitmap.bak" &&
> +	cp "$packbitmap.bak" "$packbitmap" &&

So, at this point, we have only one pack, and we could enable the
midx bitmap and/or pack bitmap from their .bak.  We first enable
only the pack bitmap while leaving midx bitmap disabled.

> +	# fsck works at first
> +	git fsck &&

OK.

> +	corrupt_file "$packbitmap" &&
> +	test_must_fail git fsck 2>err &&
> +	grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err &&

And when the only thing enabled, i.e. pack bitmap, is broken, we
notice the breakage.  OK.

> +	cp "$packbitmap.bak" "$packbitmap" &&
> +	corrupt_file "$midxbitmap" &&

Now we repair the pack bitmap and break midx bitmap.  Both are
enabled in this case.

> +	test_must_fail git fsck 2>err &&
> +	grep "bitmap file '\''$midxbitmap'\'' has invalid checksum" err

And we notice midx bitmap is corrupt.

Is it worth checking at this point if we detect breakages in both
bitmaps, if we have pack bitmap and midx bitmap, both broken?

Other than these tiny nits, looking very good.

Thanks.