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

Don't set any project path by default #137

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

Conversation

qgadrian
Copy link
Owner

@qgadrian qgadrian commented Nov 18, 2022

This PR removes the auto directory change on the project. To change the path where to execute the hooks, a project_path config will need to be set.

The changes here can potentially break a current setup for a project that is relying on the auto cd.

Fixes #123 #133

This commit removes the auto directory change on the project. To change
the path where to execute the hooks, a `project_path` config will need
to be set.

This commit can potentially break a current setup for a project that is
relying on the _auto cd_.
@djthread
Copy link

Thanks for looking at this, Adrián! I just gave this another try and noticed the same thing as I mentioned on #123. Checking out my project freshly and setting the git_hooks dep in mix.exs with 'github' and 'branch' set to this PR, 'mix compile' shows the installing message, but at the end, my pre-push file is not created under .git/hooks.

↗ Installing git hooks...

I even looked at your diff here and tried swapping back the line in lib/mix/tasks/git_hooks/install.ex which seemed to be the only relevant change... Same result every time I rm -rf _build/dev/lib/git_hooks and mix compile: no hook file generated!

However, if I then switch back to 0.7.3 and do mix deps.get, mix compile, of course the installing message appears here, too... but the git hook file is created as expected, consistently!

I can't seem to even theorize what would make the difference! ...

@qgadrian
Copy link
Owner Author

qgadrian commented Dec 5, 2022

Thanks for looking at this, Adrián! I just gave this another try and noticed the same thing as I mentioned on #123. Checking out my project freshly and setting the git_hooks dep in mix.exs with 'github' and 'branch' set to this PR, 'mix compile' shows the installing message, but at the end, my pre-push file is not created under .git/hooks.

↗ Installing git hooks...

I even looked at your diff here and tried swapping back the line in lib/mix/tasks/git_hooks/install.ex which seemed to be the only relevant change... Same result every time I rm -rf _build/dev/lib/git_hooks and mix compile: no hook file generated!

However, if I then switch back to 0.7.3 and do mix deps.get, mix compile, of course the installing message appears here, too... but the git hook file is created as expected, consistently!

I can't seem to even theorize what would make the difference! ...

That is weird, @djthread can you please add the config :git_hooks, verbose: true config and try again? What is the path used to install the hooks?

I've just tried on a fresh install and it worked fine. Just to be clear, the hooks install will only occur when the dep is compiled or when you execute mix git_hooks.install, but that doesn't seem to be your case :/

@MajdSehwail
Copy link

Hi @qgadrian

Thanks for working on this fix. I've pulled this PR and the auto installation is working as expected when specifying the project's root 👍

config :git_hooks, auto_install: true, verbose: true, project_path: File.cwd!()

Would really appreciate if included in the next version release, cheers!

@qgadrian
Copy link
Owner Author

Hi @qgadrian

Thanks for working on this fix. I've pulled this PR and the auto installation is working as expected when specifying the project's root 👍

config :git_hooks, auto_install: true, verbose: true, project_path: File.cwd!()

Would really appreciate if included in the next version release, cheers!

Thanks for the feedback. Do you need to explicitly specify the :project_path field?

IMO this dependency should work out of the box with _default project architecture, and use that option only for edge cases. That means, without setting :project_path it should work on any mix new project.

@MajdSehwail
Copy link

IMO this dependency should work out of the box with _default project architecture, and use that option only for edge cases. That means, without setting :project_path it should work on any mix new project.

Yes we had to specifically set it. Perhaps it's due to having multiple config files (git_hooks config only exists in dev.exs)?

.
├── mix.exs
├── mix.lock
├── config
   ├──config.exs
   ├──dev.exs
   ├──prod.exs
   ├──releases.exs
   ├──test.exs
├── apps
├── deploy
├── rel
├── .github
├── .credo.exs
├── .formatter.exs
├── .tools-versions
├── LICENSE
└── README.md

@sergiolc
Copy link

Hi @qgadrian,

the problem seems to be here:

  def resolve_app_path do
    repo_path =
      resolve_git_path_based_on_git_version()
      |> Path.dirname()

    Path.relative_to(File.cwd!(), repo_path)
  end

During auto install, File.cwd!() returns /<project>/deps/git_hooks and repo_path returns ../../.git/.

I couldn't figure out what's the purpose of Path.relative_to(File.cwd!(), repo_path) here.

When running mix git_hooks.install, File.cwd!() returns /<project> and repo_path returns .git/, so the repo_path is not doing anything.

Can you double check this and provide some details please.

Thanks

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/push hooks fail in the latest version
4 participants