From ac1b218973fe8f9671ae46720c794ad9b8b587a6 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 25 Aug 2023 09:58:27 -0400 Subject: [PATCH 1/2] t5300: confirm failure of git index-pack when non-idx suffix requested Add test case to demonstrate that `git index-pack -o pack-path` fails if does not end in ".idx" when `--rev-index` is enabled. In e37d0b8730b (builtin/index-pack.c: write reverse indexes, 2021-01-25) we learned to create `.rev` reverse indexes in addition to `.idx` index files. The `.rev` file pathname is constructed by replacing the suffix on the `.idx` file. The code assumes a hard-coded "idx" suffix. In a8dd7e05b1c (config: enable `pack.writeReverseIndex` by default, 2023-04-12) reverse indexes were enabled by default. If the `-o ` argument is used, the index file may have a different suffix. This causes an error when it tries to create the reverse index pathname. The test here demonstrates the failure. (The test forces `--rev-index` to avoid interaction with `GIT_TEST_NO_WRITE_REV_INDEX` during CI runs.) Signed-off-by: Jeff Hostetler Signed-off-by: Johannes Schindelin --- t/t5300-pack-object.sh | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index a58f91035d124d..652e12a99f081f 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -355,6 +355,31 @@ test_expect_success 'build pack index for an existing pack' ' : ' +# The `--rev-index` option of `git index-pack` is now the default, so +# a `foo.rev` REV file will be created when a `foo.idx` IDX file is +# created. Normally, these pathnames are based upon the `foo.pack` +# PACK file pathname. +# +# However, the `-o` option lets you set the pathname of the IDX file +# indepdent of the PACK file. +# +# Verify what happens if these suffixes are changed. +# +test_expect_success 'complain about index name' ' + # Normal case { .pack, .idx, .rev } + cat test-1-${packname_1}.pack >test-complain-0.pack && + git index-pack -o test-complain-0.idx --rev-index test-complain-0.pack && + test -f test-complain-0.idx && + test -f test-complain-0.rev && + + # Non .idx suffix + cat test-1-${packname_1}.pack >test-complain-1.pack && + test_must_fail git index-pack -o test-complain-1.idx-suffix --rev-index test-complain-1.pack 2>err && + grep "does not end" err && + ! test -f test-complain-1.idx-suffix && + ! test -f test-complain-1.rev +' + test_expect_success 'unpacking with --strict' ' for j in a b c d e f g From 554af334042dea1f24153a657eb366eab5a5a5c9 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 25 Aug 2023 11:06:28 -0400 Subject: [PATCH 2/2] index-pack: disable rev-index if index file has non .idx suffix Teach index-pack to silently omit the reverse index if the index file does not have the standard ".idx" suffix. In e37d0b8730b (builtin/index-pack.c: write reverse indexes, 2021-01-25) we learned to create `.rev` reverse indexes in addition to `.idx` index files. The `.rev` file pathname is constructed by replacing the suffix on the `.idx` file. The code assumes a hard-coded "idx" suffix. In a8dd7e05b1c (config: enable `pack.writeReverseIndex` by default, 2023-04-12) reverse indexes were enabled by default. If the `-o ` argument is used, the index file may have a different suffix. This causes an error when it tries to create the reverse index pathname. Since we do not know why the user requested a non-standard suffix for the index, we cannot guess what the proper corresponding suffix should be for the reverse index. So we disable it. The t5300 test has been updated to verify that we no longer error out and that the .rev file is not created. Signed-off-by: Jeff Hostetler Signed-off-by: Johannes Schindelin --- builtin/index-pack.c | 4 ++++ t/t5300-pack-object.sh | 7 +++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index a3a37bd215d844..f80f0397ecd5ac 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1744,6 +1744,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) unsigned foreign_nr = 1; /* zero is a "good" value, assume bad */ int report_end_of_input = 0; int hash_algo = 0; + int dash_o = 0; /* * index-pack never needs to fetch missing objects except when @@ -1837,6 +1838,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) if (index_name || (i+1) >= argc) usage(index_pack_usage); index_name = argv[++i]; + dash_o = 1; } else if (starts_with(arg, "--index-version=")) { char *c; opts.version = strtoul(arg + 16, &c, 10); @@ -1879,6 +1881,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) index_name = derive_filename(pack_name, "pack", "idx", &index_name_buf); opts.flags &= ~(WRITE_REV | WRITE_REV_VERIFY); + if (rev_index && dash_o && !ends_with(index_name, ".idx")) + rev_index = 0; if (rev_index) { opts.flags |= verify ? WRITE_REV_VERIFY : WRITE_REV; if (index_name) diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index 652e12a99f081f..f334312b6858c1 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -372,11 +372,10 @@ test_expect_success 'complain about index name' ' test -f test-complain-0.idx && test -f test-complain-0.rev && - # Non .idx suffix + # Non .idx suffix -- implicitly omits the .rev cat test-1-${packname_1}.pack >test-complain-1.pack && - test_must_fail git index-pack -o test-complain-1.idx-suffix --rev-index test-complain-1.pack 2>err && - grep "does not end" err && - ! test -f test-complain-1.idx-suffix && + git index-pack -o test-complain-1.idx-suffix --rev-index test-complain-1.pack && + test -f test-complain-1.idx-suffix && ! test -f test-complain-1.rev '