# [PATCH] diff-highlight: link to diff-highlight in DESTDIR #Makefile #diff-highlight 11 messages from 2024-10-12 to 2024-10-14. Participants: immeëmosol via GitGitGadget, Taylor Blau, Kristoffer Haugsbakk, immeëmosol, Junio C Hamano, Jeff King, Đoàn Trần Công Danh. Thread: https://gitlist.dev/t/62324 ## immeëmosol via GitGitGadget, 2024-10-12 15:03 Subject: [PATCH] diff-highlight: link to diff-highlight in DESTDIR #Makefile #diff-highlight Message-ID: URL: https://gitlist.dev/e/pull.938.git.git.1728745399993.gitgitgadget%40gmail.com ``` From: =?UTF-8?q?imme=C3=ABmosol?= 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ëmosol --- add symlinking diff-highlight into DESTDIR In order to immediately make git's diff-highlight program available for command-line instructions; add a symlink to the generated/concatenated diff-highlight perl script(s) in DESTDIR Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-938%2Fimme-emosol%2Fpatch-1-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-938/imme-emosol/patch-1-v1 Pull-Request: https://github.com/git/git/pull/938 contrib/diff-highlight/Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile index f2be7cc9243..10c588a7929 100644 --- a/contrib/diff-highlight/Makefile +++ b/contrib/diff-highlight/Makefile @@ -9,6 +9,7 @@ diff-highlight: shebang.perl DiffHighlight.pm diff-highlight.perl cat $^ >$@+ chmod +x $@+ mv $@+ $@ + ln --symbolic --target-directory=$(DESTDIR) $(abspath $@) shebang.perl: FORCE @echo '#!$(PERL_PATH_SQ)' >$@+ @@ -19,5 +20,6 @@ test: all clean: $(RM) diff-highlight + $(RM) --force $(DESTDIR)diff-highlight .PHONY: FORCE base-commit: ef8ce8f3d4344fd3af049c17eeba5cd20d98b69f -- gitgitgadget ``` ## Taylor Blau, 2024-10-12 15:50 Subject: Re: [PATCH] diff-highlight: link to diff-highlight in DESTDIR #Makefile #diff-highlight Message-ID: URL: https://gitlist.dev/e/Zwqa3YFt9%2BdfKmHM%40nand.local In-Reply-To: ``` On Sat, Oct 12, 2024 at 03:03:19PM +0000, immeëmosol via GitGitGadget wrote: > diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile > index f2be7cc9243..10c588a7929 100644 > --- a/contrib/diff-highlight/Makefile > +++ b/contrib/diff-highlight/Makefile > @@ -9,6 +9,7 @@ diff-highlight: shebang.perl DiffHighlight.pm diff-highlight.perl > cat $^ >$@+ > chmod +x $@+ > mv $@+ $@ > + ln --symbolic --target-directory=$(DESTDIR) $(abspath $@) Hmm. I am not opposed to having diff-highlight's Makefile be responsible for installing a symbolic link to the generated script, but I do not think that this Makefile recipe is the right place to do it. This recipe is about building the executable, not installing it. If you want to introduce a separate .PHONY recipe for installing the script, I think that would be a better place to introduce this change. Thanks, Taylor ``` ## immeëmosol via GitGitGadget, 2024-10-12 17:30 Subject: [PATCH v2] diff-highlight: make install link into DESTDIR #Makefile Message-ID: URL: https://gitlist.dev/e/pull.938.v2.git.git.1728754230466.gitgitgadget%40gmail.com In-Reply-To: ``` From: =?UTF-8?q?imme=C3=ABmosol?= 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ëmosol --- add symlinking diff-highlight into DESTDIR Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-938%2Fimme-emosol%2Fpatch-1-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-938/imme-emosol/patch-1-v2 Pull-Request: https://github.com/git/git/pull/938 Range-diff vs v1: 1: e23aeb1a76c ! 1: ca76f336ded diff-highlight: link to diff-highlight in DESTDIR #Makefile #diff-highlight @@ Metadata Author: immeëmosol ## Commit message ## - diff-highlight: link to diff-highlight in DESTDIR #Makefile #diff-highlight + diff-highlight: make install link into DESTDIR #Makefile Make git's diff-highlight program immediately available to the command-line. Create a link in DESTDIR that @@ Commit message ## contrib/diff-highlight/Makefile ## @@ contrib/diff-highlight/Makefile: diff-highlight: shebang.perl DiffHighlight.pm diff-highlight.perl - cat $^ >$@+ chmod +x $@+ mv $@+ $@ -+ ln --symbolic --target-directory=$(DESTDIR) $(abspath $@) ++install: diff-highlight ++ test -w $(DESTDIR) && \ ++ ln --symbolic --target-directory=$(DESTDIR) $(abspath $<) ++ shebang.perl: FORCE @echo '#!$(PERL_PATH_SQ)' >$@+ + @cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@ @@ contrib/diff-highlight/Makefile: test: all + $(MAKE) -C t clean: ++ test ! -L $(DESTDIR)/diff-highlight || \ ++ $(RM) --force $(DESTDIR)/diff-highlight $(RM) diff-highlight -+ $(RM) --force $(DESTDIR)diff-highlight .PHONY: FORCE ++.PHONY: install contrib/diff-highlight/Makefile | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile index f2be7cc9243..19e0cf17ccd 100644 --- a/contrib/diff-highlight/Makefile +++ b/contrib/diff-highlight/Makefile @@ -10,6 +10,10 @@ diff-highlight: shebang.perl DiffHighlight.pm diff-highlight.perl chmod +x $@+ mv $@+ $@ +install: diff-highlight + test -w $(DESTDIR) && \ + ln --symbolic --target-directory=$(DESTDIR) $(abspath $<) + shebang.perl: FORCE @echo '#!$(PERL_PATH_SQ)' >$@+ @cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@ @@ -18,6 +22,9 @@ test: all $(MAKE) -C t clean: + test ! -L $(DESTDIR)/diff-highlight || \ + $(RM) --force $(DESTDIR)/diff-highlight $(RM) diff-highlight .PHONY: FORCE +.PHONY: install base-commit: ef8ce8f3d4344fd3af049c17eeba5cd20d98b69f -- gitgitgadget ``` ## Kristoffer Haugsbakk, 2024-10-12 18:35 Subject: Re: [PATCH v2] diff-highlight: make install link into DESTDIR #Makefile Message-ID: URL: https://gitlist.dev/e/d2aeda41-84fe-4ddb-89e2-5cafbbb213fc%40app.fastmail.com In-Reply-To: ``` On Sat, Oct 12, 2024, at 19:30, immeëmosol via GitGitGadget wrote: > [PATCH v2] diff-highlight: make install link into DESTDIR #Makefile What does `#Makefile` mean? Previous subject had two of these: diff-highlight: link to diff-highlight in DESTDIR #Makefile #diff-highlight -- Kristoffer Haugsbakk ``` ## immeëmosol, 2024-10-12 19:10 Subject: Re: [PATCH v2] diff-highlight: make install link into DESTDIR #Makefile Message-ID: URL: https://gitlist.dev/e/CABOUa4aY6GdoBH%2BXZFCRknpiOP-F%3DSE_b%3DB3RLjO-WdS_Quikw%40mail.gmail.com In-Reply-To: ``` On Sat, 12 Oct 2024, 20:35 Kristoffer Haugsbakk, wrote: > > On Sat, Oct 12, 2024, at 19:30, immeëmosol via GitGitGadget wrote: > > [PATCH v2] diff-highlight: make install link into DESTDIR #Makefile > > What does `#Makefile` mean? […] not much. It is an attempt to signify what the commit relates to. In this case it might hint at the commit not impacting the sources of what is built, but the build process itself. Maybe #🏗️ would be more clear. ``` ## Junio C Hamano, 2024-10-12 19:22 Subject: Re: [PATCH v2] diff-highlight: make install link into DESTDIR #Makefile Message-ID: URL: https://gitlist.dev/e/xmqq8qut9ldz.fsf%40gitster.g In-Reply-To: ``` immeëmosol writes: > On Sat, 12 Oct 2024, 20:35 Kristoffer Haugsbakk, > wrote: >> >> On Sat, Oct 12, 2024, at 19:30, immeëmosol via GitGitGadget wrote: >> > [PATCH v2] diff-highlight: make install link into DESTDIR #Makefile >> >> What does `#Makefile` mean? […] > > not much. > It is an attempt to signify what the commit relates to. > In this case it might hint at the commit not impacting the sources of > what is built, but the build process itself. > Maybe #🏗️ would be more clear. Drop all these. Study "git shortlog --no-merges" from the recent history and try to blend in. Don't try to be original where the originality does not matter. Also, resist the temptation to go outside POSIX.1 needlessly. https://pubs.opengroup.org/onlinepubs/9799919799/utilities/rm.html#tag_20_104 does not say "rm" takes "--force" and https://pubs.opengroup.org/onlinepubs/9799919799/utilities/ln.html#tag_20_67 does not say "ln" takes "--symbolic" nor "--target-directory". Be inclusive and help those whose userland tools are not tainted by GNUisms. Thanks. ``` ## immeëmosol via GitGitGadget, 2024-10-12 20:23 Subject: [PATCH v3] diff-highlight: make install link into DESTDIR Message-ID: URL: https://gitlist.dev/e/pull.938.v3.git.git.1728764613835.gitgitgadget%40gmail.com In-Reply-To: ``` From: =?UTF-8?q?imme=C3=ABmosol?= 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ëmosol --- add symlinking diff-highlight into DESTDIR Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-938%2Fimme-emosol%2Fpatch-1-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-938/imme-emosol/patch-1-v3 Pull-Request: https://github.com/git/git/pull/938 Range-diff vs v2: 1: ca76f336ded ! 1: af4bea815fa diff-highlight: make install link into DESTDIR #Makefile @@ Metadata Author: immeëmosol ## Commit message ## - diff-highlight: make install link into DESTDIR #Makefile + diff-highlight: make install link into DESTDIR Make git's diff-highlight program immediately available to the command-line. Create a link in DESTDIR that @@ contrib/diff-highlight/Makefile: diff-highlight: shebang.perl DiffHighlight.pm d mv $@+ $@ +install: diff-highlight -+ test -w $(DESTDIR) && \ -+ ln --symbolic --target-directory=$(DESTDIR) $(abspath $<) ++ test -w $(DESTDIR) && ln -s $(abspath $<) $(DESTDIR) + shebang.perl: FORCE @echo '#!$(PERL_PATH_SQ)' >$@+ @@ contrib/diff-highlight/Makefile: test: all clean: + test ! -L $(DESTDIR)/diff-highlight || \ -+ $(RM) --force $(DESTDIR)/diff-highlight ++ $(RM) -f $(DESTDIR)/diff-highlight $(RM) diff-highlight .PHONY: FORCE contrib/diff-highlight/Makefile | 6 ++++++ 1 file changed, 6 insertions(+) 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) + shebang.perl: FORCE @echo '#!$(PERL_PATH_SQ)' >$@+ @cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@ @@ -18,6 +21,9 @@ test: all $(MAKE) -C t clean: + test ! -L $(DESTDIR)/diff-highlight || \ + $(RM) -f $(DESTDIR)/diff-highlight $(RM) diff-highlight .PHONY: FORCE +.PHONY: install base-commit: ef8ce8f3d4344fd3af049c17eeba5cd20d98b69f -- gitgitgadget ``` ## Jeff King, 2024-10-12 20:55 Subject: Re: [PATCH v3] diff-highlight: make install link into DESTDIR Message-ID: <20241012205506.GA55242@coredump.intra.peff.net> URL: https://gitlist.dev/e/20241012205506.GA55242%40coredump.intra.peff.net In-Reply-To: ``` 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 ``` ## immeëmosol, 2024-10-12 23:41 Subject: Re: [PATCH v3] diff-highlight: make install link into DESTDIR Message-ID: URL: https://gitlist.dev/e/CABOUa4bqPiUM%2B%3D6dxW5TTHOdMzXiFD02vWoVJcgAQDhZs8DoXg%40mail.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 wrote: > > 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 --- 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 ``` ## Đoàn Trần Công Danh, 2024-10-14 03:29 Subject: Re: [PATCH v3] diff-highlight: make install link into DESTDIR Message-ID: URL: https://gitlist.dev/e/ZwyQE-BMTn3Fj-BO%40danh.dev In-Reply-To: ``` On 2024-10-13 01:41:06+0200, immeëmosol wrote: > 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 ) > ˋˋˋ Nah, it isn't DESTDIR's usage, it's prefix job! make prefix=${HOME}/.local install > --- > 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 > --- > 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) So it would be something like this: install: diff-highlight $(INSTALL) diff-highlight '$(DESTDIR)$(bindir_SQ)' > + > 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 > -- Danh ``` ## Taylor Blau, 2024-10-14 22:17 Subject: Re: [PATCH v3] diff-highlight: make install link into DESTDIR Message-ID: URL: https://gitlist.dev/e/Zw2YXD6XEiQVKj9j%40nand.local In-Reply-To: ``` On Sun, Oct 13, 2024 at 01:41:06AM +0200, immeëmosol wrote: > 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= It is a bit sad to hear that you do not have time to bring this patch over the finish line, having received some useful pointers from others on the list. Any takers who might want to pick this up instead? Thanks, Taylor ```