Re: [PATCH v3] diff-highlight: make install link into DESTDIR
- From
immeëmosol <will+developer@willfris.nl>
- Date
- Oct 12, 2024, 23:41 UTC
- Message-ID
- <CABOUa4bqPiUM+=6dxW5TTHOdMzXiFD02vWoVJcgAQDhZs8DoXg@mail.gmail.com>
- In-Reply-To
- <20241012205506.GA55242@coredump.intra.peff.net>
Resending this mail to the list 'cause mail-client added html to previous mail.
On Sat, 12 Oct 2024 at 22:55, Jeff King <peff@peff.net> wrote:
Show 52 quoted lines
> > 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
As mentioned, `contrib/diff-highlight` is less like other perl contribs like `contrib/contacts` and `contrib/credential/netrc`, those two seem to be git subcommands (`git-*`) where diff-highlight is more of a "standalone" command.
My usecase was to peek at what the command does by making it available in a `$PATH` writable by a non-root user. (Much like what is mentioned in `contrib/diff-highlight/README#Use`: `git log -p --color | diff-highlight`.= )
```sh echo '# Given ~/.local/bin is in $PATH,' ( export DESTDIR=3D"${HOME?}/.local/bin/" ; make linked-in-destdir ) echo '# In another already open shell, try suggestion from readme.' ( export DESTDIR=3D"${HOME?}/.local/bin/" ; make clean ) ```
Thanks to all of you for the introduction into how contributions to git/git are handled. Though it has been quite an informative introduction, and i can understand the suggestion of making it a install-target like other contrib-parts, i am currently not spending more time on this. Thanks again, and have a good day= .
--- Make git's diff-highlight program immediately available to the command-line= . Create a link in DESTDIR that refers to the generated/concatenated diff-highlight perl script
Signed-off-by: imme=C3=ABmosol <will+developer@willfris.nl> --- contrib/diff-highlight/Makefile | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile index f2be7cc9243719..84f6e65c730380 100644 --- a/contrib/diff-highlight/Makefile +++ b/contrib/diff-highlight/Makefile @@ -10,6 +10,11 @@ diff-highlight: shebang.perl DiffHighlight.pm diff-highlight.perl chmod +x $@+ mv $@+ $@ +linked-in-destdir: diff-highlight + test -n "$(DESTDIR)" && \ + test -w $(DESTDIR) && \ + ln -s $(abspath $<) $(DESTDIR) + shebang.perl: FORCE @echo '#!$(PERL_PATH_SQ)' >$@+ @cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@ @@ -17,7 +22,13 @@ shebang.perl: FORCE test: all $(MAKE) -C t -clean: +unlink-from-destdir: + test -z "$(DESTDIR)" || \ + test ! -L $(DESTDIR)/diff-highlight || \ + $(RM) $(DESTDIR)/diff-highlight + +clean: unlink-from-destdir $(RM) diff-highlight .PHONY: FORCE +.PHONY: linked-in-destdir unlink-from-destdir