Skip to content

fix: allow .git directory on parent directory #225

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

Merged

Conversation

roman
Copy link

@roman roman commented Jan 11, 2023

Currently, the check to validate pre-commit-hooks is getting installed on a git repository only considers the .git directory if the pre-commit-hooks config is at the root path; if the pre-commit-hooks config lives in another path that is not the root of the repository, pre-commit-hooks won't install a script.

This change allows flake.nix files in sub-directories to have the pre-commit-hooks configuration. This is especially common in monorepos.

Note, this change requires a PR on nixpkgs to be merged before this code works.

resolves #224

@roman roman mentioned this pull request Jan 11, 2023
@roman roman force-pushed the rgonzalez/fix/allow-pre-commit-on-subdir branch from d9c9f63 to c67006f Compare January 11, 2023 22:00
@roman roman force-pushed the rgonzalez/fix/allow-pre-commit-on-subdir branch from c67006f to ee2300b Compare January 27, 2023 00:03
@roman
Copy link
Author

roman commented Jan 28, 2023

Hey @domenkozar, this has been here for a while, and recently started working due to the dependency being merged to nixpkgs, would you consider adding it?

@domenkozar
Copy link
Member

@roman could you backport it to 22.11? We also want to support the stable version in pre-commit-hooks.nix

@@ -366,7 +366,7 @@ in
if ! type -t git >/dev/null; then
# This happens in pure shells, including lorri
echo 1>&2 "WARNING: pre-commit-hooks.nix: git command not found; skipping installation."
elif [[ ! -e .git ]]; then
elif ! ${pkgs.locate-dominating-file}/bin/locate-dominating-file .git >/dev/null; then
Copy link

@soupglasses soupglasses Jan 30, 2023

Choose a reason for hiding this comment

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

This should probably be written with git rev-parse --git-dir, unless there is a specific reason to avoid the git binary for this lookup.

Copy link
Author

@roman roman Jan 30, 2023

Choose a reason for hiding this comment

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

I was trying to understand the nix runtime dependencies of this generated script, but I'm unable to find any via nix-store --query.

We must have the assumption that git is installed already, so I don't think avoiding depending on it here gains us anything.

All this to say, I agree @imsofi, let me change that to the git command.

Currently, the check to validate pre-commit-hooks are getting installed
on a git repository only consider the `.git` directory, if the
pre-commit-hooks config lives in another path that is not the root of
the repository, pre-commit-hooks won't install a script.

This change allows flake.nix files in sub-directories to have the
pre-commit-hooks configuration. This is specially common in monorepos.
@roman roman force-pushed the rgonzalez/fix/allow-pre-commit-on-subdir branch from ee2300b to 3973f37 Compare January 30, 2023 19:39
@domenkozar domenkozar merged commit ce4efee into cachix:master Jan 31, 2023
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.

Support for mono-repos?
3 participants