SKIP_DASHED_BUILT_INS does not install git-*-pack

SKIP_DASHED_BUILT_INS does not install git-*-pack

[Michael Forney]

I saw that git 2.29.0 introduced a new make variable
SKIP_DASHED_BUILT_INS. However, after testing it out I noticed that it
skips installation of bin/git-receive-pack bin/git-upload-archive and
bin/git-upload-pack as well.

There is a comment that says these commands are special and expected
to be in the bin/ directory in dashed form, so unless I'm missing
something, I believe this is unintended.

This seems to be the offending hunk:
https://github.com/git/git/commit/94de88c986712e79c20813ba54e797c4ca83137b#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L2979-R2993

Reverting that hunk restores git-receive-pack, git-upload-archive, and
git-upload-pack (and only those).

Re: SKIP_DASHED_BUILT_INS does not install git-*-pack

[Junio C Hamano]

Michael Forney <mforney@mforney.org> writes:

> I saw that git 2.29.0 introduced a new make variable
> SKIP_DASHED_BUILT_INS. However, after testing it out I noticed that it
> skips installation of bin/git-receive-pack bin/git-upload-archive and
> bin/git-upload-pack as well.
>
> There is a comment that says these commands are special and expected
> to be in the bin/ directory in dashed form, so unless I'm missing
> something, I believe this is unintended.
>
> This seems to be the offending hunk:
> https://github.com/git/git/commit/94de88c986712e79c20813ba54e797c4ca83137b#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L2979-R2993
>
> Reverting that hunk restores git-receive-pack, git-upload-archive, and
> git-upload-pack (and only those).

Thanks for a report.  Dscho?

Re: SKIP_DASHED_BUILT_INS does not install git-*-pack

[Taylor Blau]

On Mon, Oct 19, 2020 at 06:54:29PM -0700, Junio C Hamano wrote:
> Michael Forney <mforney@mforney.org> writes:
>
> > I saw that git 2.29.0 introduced a new make variable
> > SKIP_DASHED_BUILT_INS. However, after testing it out I noticed that it
> > skips installation of bin/git-receive-pack bin/git-upload-archive and
> > bin/git-upload-pack as well.
> >
> > There is a comment that says these commands are special and expected
> > to be in the bin/ directory in dashed form, so unless I'm missing
> > something, I believe this is unintended.
> >
> > This seems to be the offending hunk:
> > https://github.com/git/git/commit/94de88c986712e79c20813ba54e797c4ca83137b#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L2979-R2993
> >
> > Reverting that hunk restores git-receive-pack, git-upload-archive, and
> > git-upload-pack (and only those).
>
> Thanks for a report.  Dscho?

I'm not Dscho, but I wonder if something as simple as the following
(totally untested) patch might work. The rationale is that we already
build 'ALL_COMMANDS_TO_INSTALL' to only contain the programs and
builtins (iff SKIP_DASHED_BUILT_INS is unset) we want, so we could
install each entry in that list unconditionally instead of iterating
through BUILT_INS and checking SKIP_DASHED_BUILT_INS each time.

If Dscho or someone else wants to ack that this is a patch in the right
directions, I'd be happy to clean it up.

--- 8< ---

diff --git a/Makefile b/Makefile
index 95571ee3fc..1b2b085765 100644
--- a/Makefile
+++ b/Makefile
@@ -2991,17 +2991,14 @@ endif
 			  cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \
 		fi \
 	done && \
-	for p in $(BUILT_INS); do \
+	for p in $(ALL_COMMANDS_TO_INSTALL); do \
 		$(RM) "$$execdir/$$p" && \
-		if test -z "$(SKIP_DASHED_BUILT_INS)"; \
-		then \
-			test -n "$(INSTALL_SYMLINKS)" && \
-			ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \
-			{ test -z "$(NO_INSTALL_HARDLINKS)" && \
-			  ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
-			  ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
-			  cp "$$execdir/git$X" "$$execdir/$$p" || exit; }; \
-		fi \
+		test -n "$(INSTALL_SYMLINKS)" && \
+		ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \
+		{ test -z "$(NO_INSTALL_HARDLINKS)" && \
+			ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
+			ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
+			cp "$$execdir/git$X" "$$execdir/$$p" || exit; }; \
 	done && \
 	remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
 	for p in $$remote_curl_aliases; do \

Re: SKIP_DASHED_BUILT_INS does not install git-*-pack

[Johannes Schindelin]

Hi Junio & Michael,

On Mon, 19 Oct 2020, Junio C Hamano wrote:

> Michael Forney <mforney@mforney.org> writes:
>
> > I saw that git 2.29.0 introduced a new make variable
> > SKIP_DASHED_BUILT_INS. However, after testing it out I noticed that it
> > skips installation of bin/git-receive-pack bin/git-upload-archive and
> > bin/git-upload-pack as well.
> >
> > There is a comment that says these commands are special and expected
> > to be in the bin/ directory in dashed form, so unless I'm missing
> > something, I believe this is unintended.
> >
> > This seems to be the offending hunk:
> > https://github.com/git/git/commit/94de88c986712e79c20813ba54e797c4ca83137b#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L2979-R2993
> >
> > Reverting that hunk restores git-receive-pack, git-upload-archive, and
> > git-upload-pack (and only those).
>
> Thanks for a report.  Dscho?

Hrm. We have this, specifically:

```
ifeq (,$(SKIP_DASHED_BUILT_INS))
ALL_COMMANDS_TO_INSTALL += $(BUILT_INS)
else
# git-upload-pack, git-receive-pack and git-upload-archive are special: they
# are _expected_ to be present in the `bin/` directory in their dashed form.
ALL_COMMANDS_TO_INSTALL += git-receive-pack$(X)
ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)
ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)
endif

```

Will have a look,
Dscho

Re: SKIP_DASHED_BUILT_INS does not install git-*-pack

[Johannes Schindelin]

Hi Taylor,

On Mon, 19 Oct 2020, Taylor Blau wrote:

> diff --git a/Makefile b/Makefile
> index 95571ee3fc..1b2b085765 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2991,17 +2991,14 @@ endif
>  			  cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \
>  		fi \
>  	done && \
> -	for p in $(BUILT_INS); do \
> +	for p in $(ALL_COMMANDS_TO_INSTALL); do \
>  		$(RM) "$$execdir/$$p" && \
> -		if test -z "$(SKIP_DASHED_BUILT_INS)"; \
> -		then \
> -			test -n "$(INSTALL_SYMLINKS)" && \
> -			ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \
> -			{ test -z "$(NO_INSTALL_HARDLINKS)" && \
> -			  ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
> -			  ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
> -			  cp "$$execdir/git$X" "$$execdir/$$p" || exit; }; \
> -		fi \
> +		test -n "$(INSTALL_SYMLINKS)" && \
> +		ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \
> +		{ test -z "$(NO_INSTALL_HARDLINKS)" && \
> +			ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
> +			ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
> +			cp "$$execdir/git$X" "$$execdir/$$p" || exit; }; \

This code talks about `execdir`; I am fairly certain that we do not need
anything specific in `libexec/git-core/`. However, for purposes of
fetching/pushing, the mentioned executables should be present in the
`bin/` directory in dashed form, no matter whether they are built-ins or
not.

In a not-so-distant future, we might even be able to teach Git to call for
the undashed form. But the problem there is that the Git doing the asking
is on the client side, while the Git possibly missing the dashed forms is
on the remote side.

Ciao,
Dscho

Re: SKIP_DASHED_BUILT_INS does not install git-*-pack

[Johannes Schindelin]

Hi Michael,

On Mon, 19 Oct 2020, Michael Forney wrote:

> I saw that git 2.29.0 introduced a new make variable
> SKIP_DASHED_BUILT_INS. However, after testing it out I noticed that it
> skips installation of bin/git-receive-pack bin/git-upload-archive and
> bin/git-upload-pack as well.
>
> There is a comment that says these commands are special and expected
> to be in the bin/ directory in dashed form, so unless I'm missing
> something, I believe this is unintended.
>
> This seems to be the offending hunk:
> https://github.com/git/git/commit/94de88c986712e79c20813ba54e797c4ca83137b#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L2979-R2993
>
> Reverting that hunk restores git-receive-pack, git-upload-archive, and
> git-upload-pack (and only those).

Indeed. I purposefully investigated without having a look at what you
linked at, and came to the exact same conclusion.

As soon as the build passed, I will send this to the list:
https://github.com/gitgitgadget/git/pull/768

Thanks,
Dscho

Re: SKIP_DASHED_BUILT_INS does not install git-*-pack

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Reverting that hunk restores git-receive-pack, git-upload-archive, and
>> git-upload-pack (and only those).
>
> Indeed. I purposefully investigated without having a look at what you
> linked at, and came to the exact same conclusion.

Good---that's sort-of "independent verification" ;-)

Thanks, let's fast-track the fix for 2.29.1.