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 get_versioned_files() failing when GIT_INDEX_FILE is set #123

Merged
merged 3 commits into from
Sep 21, 2020
Merged

Fix get_versioned_files() failing when GIT_INDEX_FILE is set #123

merged 3 commits into from
Sep 21, 2020

Conversation

phijor
Copy link
Contributor

@phijor phijor commented Sep 20, 2020

Instead of running git ls-files in each git submodule, run

    $ git ls-file --recurse-submodules

from the git root directory and list all files under version control in one go.

This fixes a bug when check-manifest would fail with

    ['git', 'ls-files', '-z'] failed (status 128):
    fatal: .git/index: index file open failed: Not a directory

when run from a pre-commit hook. When running from a pre-commit hook, git sets GIT_INDEX_FILE=.git/index, which should be interpreted relative to the git root directory. When changing directories into a submodule directory (say ./submodule/) and running git ls-files there, git expects to find a git index file at ./submodule/.git/index, which fails since ./submodule/.git is a regular file in a submodule.

The easy solution is to just pass --recurse-submodules when listing files under version control and remove all logic listing submodules files one-by-one.

A test is added that sets GIT_INDEX_FILE=.git/index and checks that tests dealing with submodules still pass. This simulates the "running from git-hook" setting

Fixes #122.

Instead of running `git ls-files` in each git submodule, run

        $ git ls-file --recurse-submodules

from the git root directory and list all files  under version control
in one go.

This fixes a bug when check-manifest would fail with

        ['git', 'ls-files', '-z'] failed (status 128):
        fatal: .git/index: index file open failed: Not a directory

when run from a pre-commit hook.  When running from a pre-commit hook,
git sets   `GIT_INDEX_FILE=.git/index`,   which should be interpreted
relative to the git root directory.  When changing directories into a
submodule directory  (say `./submodule/`)  and running `git ls-files`
there, git expects to find a git index file at `./submodule/.git/index`,
which fails since `./submodule/.git` is a regular file in a submodule.

The easy solution is to just pass `--recurse-submodules` when listing
files under  version control and remove all logic  listing submodules
files one-by-one.

A test is added that sets `GIT_INDEX_FILE=.git/index` and checks that
tests dealing with submodules still pass. This simulates the "running
from git-hook" setting
Copy link
Owner

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Would you mind adding a small changelog note for this change?

check_manifest.py Show resolved Hide resolved
@mgedmin
Copy link
Owner

mgedmin commented Sep 21, 2020

(BTW thank you very much for the very comprehensive bug analysis and the PR fixing it while simultaneously simplifying the code!)

This is not needed anymore since b8f814f.
@phijor
Copy link
Contributor Author

phijor commented Sep 21, 2020

You're welcome :) I had been scratching my head why check-manifest would fail with such a cryptic error, but it turns out it's easy-to-understand code so I could debug the issue easily. It always feels nice to fix a bug by deleting code 👍

I'll push an addition to the changelog later; if you are happy with the changes I could squash and rebase then.

@phijor
Copy link
Contributor Author

phijor commented Sep 21, 2020

Well, this is weird:

https://travis-ci.org/github/mgedmin/check-manifest/jobs/728972962

I'd be interested to know how I was able to trigger that without changing any code. Has to be the worst timing ever.

@phijor phijor requested a review from mgedmin September 21, 2020 13:22
@mgedmin
Copy link
Owner

mgedmin commented Sep 21, 2020

I've retried the pypy job and it passed!

@mgedmin mgedmin merged commit ccf4f1c into mgedmin:master Sep 21, 2020
@mgedmin
Copy link
Owner

mgedmin commented Sep 22, 2020

BTW I released check-manifest 0.43 with this fix yesterday.

mgedmin added a commit that referenced this pull request Mar 13, 2022
This reverts commit cc7c176.

Fixes #153: a `.gitmodules` might exist in a subdirectory, so it's not
sufficient to check for it at the project root.

Also, Git versions that don't support --recurse-submodules are even
further in the past now, so I'm hoping it's fine to ignore them.

Note that this removes a fix for #123.
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.

pre-commit hook fails for projects with submodules
2 participants