On Sat, Oct 12, 2024 at 08:23:33PM +0000, immeëmosol via GitGitGadget wrote:
> diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile
> index f2be7cc9243..a53e09e0bdd 100644
> --- a/contrib/diff-highlight/Makefile
> +++ b/contrib/diff-highlight/Makefile
> @@ -10,6 +10,9 @@ diff-highlight: shebang.perl DiffHighlight.pm diff-highlight.perl
> chmod +x $@+
> mv $@+ $@
>
> +install: diff-highlight
> + test -w $(DESTDIR) && ln -s $(abspath $<) $(DESTDIR)
> +
I'm not opposed to having an install target here, like we do in the main
Makefile and in a few other contrib directories.
But in that case, I think it should behave more like those other
targets:
1. Actually copy the program rather than making a symlink. Preferably
using $(INSTALL).
2. Respect $(prefix) in the usual way.
And also...
> clean:
> + test ! -L $(DESTDIR)/diff-highlight || \
> + $(RM) -f $(DESTDIR)/diff-highlight
> $(RM) diff-highlight
3. It's unusual for "clean" to reach outside of the build directory.
What you're doing here is more like an "uninstall" target, but we
don't usually provide one.
There are a few different approaches other contrib/ items take to
work like the rest of the Git:
- in contrib/contacts, we source config.mak from the top-level, and
then define a default $(prefix). This gives some repeated
boilerplate, but is pretty independent from the top-level Makefile.
- in contrib/credential/netrc, we piggy-back on the top-level
Makefile's "install-perl-script", which knows where the user has
asked us to install things. That might not be appropriate here,
though, as I think it only puts things in libexec/, so
"diff-highlight" wouldn't be generally available in the user's $PATH
(though it would be enough to use as a pager within git).
-Peff
