Skip to content

Fix git-lfs unnecessary hook installation #1416

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

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

Conversation

yaegashi
Copy link

@yaegashi yaegashi commented Jul 20, 2025

Fix an issue where git lfs install was running unnecessarily in repositories without LFS files, causing hooks to be installed when they shouldn't be.

Changes:

  • Update src/git-lfs/install.sh to properly check for empty git lfs ls-files output
  • Add comprehensive test (test/git-lfs/noLfsFiles.sh) to verify hooks are not installed when no LFS files are present
  • Update test scenarios to include the new test case

The fix changes the condition from checking command exit status to checking if the output is empty, preventing unnecessary hook installation in non-LFS repos.

Related issues/PRs:

The changes above introduced new problems in the following scenario:

  • Starting a dev container from the default universal image with the git-lfs feature

  • Add devcontainer.json without the git-lfs feature and rebuild the container

  • Encounter the following warnings/errors when trying to use git checkout, commit, or push:

    $ git push origin new-branch
    
    This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks').
     
    error: failed to push some refs to 'https://github.com/yaegashi/foo'

@yaegashi yaegashi marked this pull request as ready for review July 20, 2025 08:05
@Copilot Copilot AI review requested due to automatic review settings July 20, 2025 08:05
@yaegashi yaegashi requested a review from a team as a code owner July 20, 2025 08:05
Copilot

This comment was marked as outdated.

@yaegashi yaegashi marked this pull request as draft July 20, 2025 08:23
@yaegashi yaegashi force-pushed the fix-git-lfs-unnecessary-hook-installation branch from f20c448 to a95c315 Compare July 20, 2025 10:37
@yaegashi yaegashi marked this pull request as ready for review July 20, 2025 10:47
@yaegashi yaegashi requested a review from Copilot July 20, 2025 10:47
Copilot

This comment was marked as outdated.

@yaegashi yaegashi force-pushed the fix-git-lfs-unnecessary-hook-installation branch from a95c315 to a2774d7 Compare July 20, 2025 10:51
@yaegashi yaegashi requested a review from Copilot July 20, 2025 10:52
Copilot

This comment was marked as outdated.

@yaegashi yaegashi force-pushed the fix-git-lfs-unnecessary-hook-installation branch from a2774d7 to 3a2c97e Compare July 20, 2025 11:00
@yaegashi yaegashi requested a review from Copilot July 20, 2025 11:01
Copilot

This comment was marked as outdated.

Fix an issue where git lfs install was running unnecessarily in repositories
without LFS files, causing hooks to be installed when they shouldn't be.

Changes:
- Update src/git-lfs/install.sh to properly check for empty git lfs ls-files output
- Add comprehensive test (test/git-lfs/noLfsFiles.sh) to verify hooks are not installed
  when no LFS files are present
- Update test scenarios to include the new test case

The fix changes the condition from checking command exit status to checking
if the output is empty, preventing unnecessary hook installation in non-LFS repos.
@yaegashi yaegashi force-pushed the fix-git-lfs-unnecessary-hook-installation branch from 3a2c97e to 2c73a25 Compare July 20, 2025 11:06
@yaegashi yaegashi requested a review from Copilot July 20, 2025 11:07
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where Git LFS hooks were being installed unnecessarily in repositories without LFS files. The fix changes the detection logic from checking command exit status to verifying if the output is empty, preventing hook installation in non-LFS repositories.

  • Modified the LFS detection logic in the install script to check for empty output instead of command exit status
  • Added a comprehensive test case to verify hooks are not installed when no LFS files are present
  • Updated test scenarios to include the new test case for repositories without LFS files

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/git-lfs/install.sh Updated LFS detection logic to check for empty output and separate error handling
test/git-lfs/noLfsFiles.sh Added new test to verify no hooks are installed in repositories without LFS files
test/git-lfs/scenarios.json Added test scenario configuration for repositories without LFS files
Comments suppressed due to low confidence (1)

test/git-lfs/noLfsFiles.sh:22

  • This test redirects stderr to /dev/null which may hide important error messages that could help diagnose test failures. Consider capturing and checking the error output separately or allowing it to be visible during test execution.
check "git lfs ls-files returns empty output" test -z "$(git lfs ls-files 2>/dev/null)"

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.

1 participant