Re: [PATCH v3] diff-highlight: make install link into DESTDIR

From: Jeff King <peff@peff.net>
Date: Oct 12, 2024, 20:55 UTC
Message-ID: <20241012205506.GA55242@coredump.intra.peff.net>
In-Reply-To: <pull.938.v3.git.git.1728764613835.gitgitgadget@gmail.com>

On Sat, Oct 12, 2024 at 08:23:33PM +0000, immeëmosol via GitGitGadget wrote:

Show 11 quoted lines

> 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