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

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

On Sat, 12 Oct 2024, 20:35 Kristoffer Haugsbakk,
<[email protected]> 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.

immeëmosol <[email protected]> writes:

> On Sat, 12 Oct 2024, 20:35 Kristoffer Haugsbakk,
> <[email protected]> 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.

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

Resending this mail to the list 'cause mail-client added html to previous mail.

On Sat, 12 Oct 2024 at 22:55, Jeff King <[email protected]> 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 <[email protected]>
---
 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

On 2024-10-13 01:41:06+0200, immeëmosol <[email protected]> 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 <[email protected]>
> ---
>  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

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