Fix rev index #599
Merged
Fix rev index #599
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add test case to demonstrate that `git index-pack -o <idx-path> pack-path` fails if <idx-path> does not end in ".idx" when `--rev-index` is enabled. In e37d0b8 (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 a8dd7e0 (config: enable `pack.writeReverseIndex` by default, 2023-04-12) reverse indexes were enabled by default. If the `-o <idx-path>` 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 <jeffhostetler@github.com>
Teach index-pack to silently omit the reverse index if the index file does not have the standard ".idx" suffix. In e37d0b8 (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 a8dd7e0 (config: enable `pack.writeReverseIndex` by default, 2023-04-12) reverse indexes were enabled by default. If the `-o <idx-path>` 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. TODO We could warn the user that we skipped it (perhaps only if they TODO explicitly requested `--rev-index` on the command line). TODO TODO Ideally, we should add an `--rev-index-path=<path>` argument TODO or change `--rev-index` to take a pathname. TODO TODO I'll leave these questions for a future series. Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
jeffhostetler
force-pushed
the
fix-rev-index
branch
from
ebbda58
to
728d78d
Compare
derrickstolee
approved these changes
Aug 25, 2023
There was a problem hiding this comment.
This will definitely unblock the release. I have doubts that upstream would settle for this disabling, though it does bring the behavior back in line with pre-2.41.0 behavior. It's strange that the index name would disable the rev-index feature.
I was expecting that the behavior would instead strip the suffix from the -o option (if possible, and append to the filename otherwise).
For the current strategy, I only have a comment that the --no-rev-index behavior could be documented as working with a non-.idx filename.
t/t5300-pack-object.sh
Outdated
|
|
||
| # 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 && |
There was a problem hiding this comment.
Is it possible to demonstrate the command succeeds when given .idx-suffix to -o and --no-rev-index, isolating the issue to the rev-index option?
|
An alternative to disabling the revindex in the case of a file name that does not end in
I imagine that that variant could potentially be welcomed on the Git mailing list. |
|
I've created an issue to track and upstream this. |
dscho
pushed a commit
that referenced
this pull request
Aug 28, 2023
`git index-pack` fails when the `-o` argument is used and the requested IDX file does not have the standard `.idx` suffix. The reverse index code assumes the `.idx` suffix when it generates the `.rev` pathname. Silently disable the reverse index when the suffix is different. Added a test to confirm the breakage and then updated it to confirm the fix. This is being patched into vfs-2.41.0.5 directly, but the fixes should be backported to upstream Git and to vfs-2.42.0.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
git index-packfails when the-oargument is used and the requested IDX file does not have the standard.idxsuffix. The reverse index code assumes the.idxsuffix when it generates the.revpathname.Silently disable the reverse index when the suffix is different.
Added a test to confirm the breakage and then updated it to confirm the fix.
This is being patched into vfs-2.41.0.5 directly, but the fixes should be backported to upstream Git and to vfs-2.42.0.