mlawren / githook-perltidy

Run perltidy as a Git pre-commit hook
41 stars 10 forks source link

Need full path in the pre/post-commit files #3

Closed shmuelfomberg closed 11 years ago

shmuelfomberg commented 11 years ago

I've install githook-perltidy, and installed it in one of my gits.

The Perl is installed using mac-port, that install perl itself in /opt/local/bin/perl, and when a CPAN module install a script it is done inside /opt/local/libexec/perl5.XX/sitebin/ (with XX stand for the Perl version) This is not necessarily inside the PATH. (I work with multiple Perl-s, and each have its own installation)

Inside the pre/post-commit file, githook is simply writing "githook-perltidy pre-commit". which fails on my machine.

Is it possible to use the same path that was used to run the install command? If someone ran "githook-perltidy install" then use the same. But if someone ran "/opt/local/libexec/perl5.12/sitebin/githook-perltidy install" then use this path.

Does it make sense? Shmuel.

dolmen commented 11 years ago

+1

mlawren commented 11 years ago

I've install githook-perltidy, and installed it in one of my gits.

The Perl is installed using mac-port, that install perl itself in /opt/local/bin/perl, and when a CPAN module install a script it is done inside /opt/local/libexec/perl5.XX/sitebin/ (with XX stand for the Perl version) This is not necessarily inside the PATH. (I work with multiple Perl-s, and each have its own installation)

Inside the pre/post-commit file, githook is simply writing "githook-perltidy pre-commit". which fails on my machine.

This appears to be a more general problem than just githook-perltidy. What do you do for other Perl scripts/commands you have installed and use from the command line? Cpanminus (cpanm) for example? Are you using something like perlbrew to change Perl's? That usually takes care of the PATH problem, although you do of course have to have the same commands installed in every Perl you have.

Is it possible to use the same path that was used to run the install command? If someone ran "githook-perltidy install" then use the same. But if someone ran "/opt/local/libexec/perl5.12/sitebin/githook-perltidy install" then use this path.

Does it make sense? Shmuel.

Fairly trivial to make that happen, but I think an option would be a better way to trigger that behaviour. It makes it a little more obvious that something different will be done, instead of some hidden automatic effect. For example what if someone ran this:

../$SOME_PATH/bin/githook-perltidy

Checking for absolute paths is more code to test and check for bugs. So would the following be ok?

usage: githook-perltidy install [-f | --full-path]

Mark.

shmuelfomberg commented 11 years ago

Hi Mark.

Of course it is a general problem, but most scripts does not install themselves in third place. cpanm is being installed in that perl's specific directory, and that is the user's problem to call it in the right location. However, cpanm does not put references to itself in other locations. githook-perldity does.

I prefer a automatic operation (how about checking if running githook-perltidy --selftest runs it, and if not install the full path?) but using a parameter is OK too, I think.

Shmuel.

mlawren commented 11 years ago

Of course it is a general problem, but most scripts does not install themselves in third place. cpanm is being installed in that perl's specific directory, and that is the user's problem to call it in the right location. However, cpanm does not put references to itself in other locations. githook-perldity does.

A good argument, and in fact with some consideration I now believe that the full path should always be used in the hook. The advent of tools like perlbrew and local::lib (which I use a lot) could make this issue a not-uncommon problem, and without a downside I see no need to add an auto on/off feature to the code at all.

The next release will fix this. Probably with a new '--force, -f' option to reinstall the hooks to save you having to edit files (again).

shmuelfomberg commented 11 years ago

thanks you.

mlawren commented 11 years ago

The latest 'devel' branch now has the full path capability. As soon as I get positive reports from cpan testers on the changes I'll push out a new release.