[PATCH] diff-highlight: link to diff-highlight in DESTDIR #Makefile #diff-highlight

[PATCH] diff-highlight: link to diff-highlight in DESTDIR #Makefile #diff-highlight

[immeëmosol via GitGitGadget]

From: =?UTF-8?q?imme=C3=ABmosol?= <will+developer@willfris.nl>

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 <will+developer@willfris.nl>
---
    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

Re: [PATCH] diff-highlight: link to diff-highlight in DESTDIR #Makefile #diff-highlight

[Taylor Blau]

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

[PATCH v2] diff-highlight: make install link into DESTDIR #Makefile

[immeëmosol via GitGitGadget]

From: =?UTF-8?q?imme=C3=ABmosol?= <will+developer@willfris.nl>

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 <will+developer@willfris.nl>
---
    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 <will+developer@willfris.nl>
      
       ## 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

Re: [PATCH v2] diff-highlight: make install link into DESTDIR #Makefile

[Kristoffer Haugsbakk]

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

Re: [PATCH v2] diff-highlight: make install link into DESTDIR #Makefile

[immeëmosol]

On Sat, 12 Oct 2024, 20:35 Kristoffer Haugsbakk,
<kristofferhaugsbakk@fastmail.com> 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.

Re: [PATCH v2] diff-highlight: make install link into DESTDIR #Makefile

[Junio C Hamano]

immeëmosol <will+developer@willfris.nl> writes:

> On Sat, 12 Oct 2024, 20:35 Kristoffer Haugsbakk,
> <kristofferhaugsbakk@fastmail.com> 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.

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

[immeëmosol via GitGitGadget]

From: =?UTF-8?q?imme=C3=ABmosol?= <will+developer@willfris.nl>

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 <will+developer@willfris.nl>
---
    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 <will+developer@willfris.nl>
      
       ## 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

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

[Jeff King]

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

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

[immeëmosol]

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:
>
> 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

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

[Đoàn Trần Công Danh]

On 2024-10-13 01:41:06+0200, immeëmosol <will+developer@willfris.nl> 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 <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)

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

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

[Taylor Blau]

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