Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix rev-index #1695

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix rev-index #1695

wants to merge 2 commits into from

Conversation

dscho
Copy link
Member

@dscho dscho commented Mar 15, 2024

git index-pack fails when the -o argument is used and the requested .idx file does not have the standard .idx suffix because the reverse index code expects a .idx suffix when it generates the .rev file name.

Let's just silently disable the reverse index when the suffix is different from .idx.

This has been in the microsoft/git fork of Git for over half a year now, via microsoft#599, and it is time that this bug fix is upstreamed.

@dscho dscho self-assigned this Mar 15, 2024
Copy link

gitgitgadget bot commented Mar 15, 2024

There are issues in commit 19d951f:
t5300: confirm failure of git index-pack when non-idx suffix requested
Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
Indented lines, and lines without whitespace, are exempt

Copy link

gitgitgadget bot commented Mar 15, 2024

There are issues in commit eb6431b:
index-pack: disable rev-index if index file has non .idx suffix
Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
Indented lines, and lines without whitespace, are exempt

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>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
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.

Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants