# SKIP_DASHED_BUILT_INS does not install git-*-pack 7 messages from 2020-10-20 to 2020-10-21. Participants: Michael Forney, Junio C Hamano, Taylor Blau, Johannes Schindelin. Thread: https://gitlist.dev/t/54466 ## Michael Forney, 2020-10-20 01:27 Subject: SKIP_DASHED_BUILT_INS does not install git-*-pack Message-ID: URL: https://gitlist.dev/e/CAGw6cBsEjOnh-ZqXCPfFha%3DNYEdy7JDddha%3DUzAau0Z1tBrWKg%40mail.gmail.com ``` 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). ``` ## Junio C Hamano, 2020-10-20 01:54 Subject: Re: SKIP_DASHED_BUILT_INS does not install git-*-pack Message-ID: URL: https://gitlist.dev/e/xmqqtuupd5m2.fsf%40gitster.c.googlers.com In-Reply-To: ``` Michael Forney 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? ``` ## Taylor Blau, 2020-10-20 02:52 Subject: Re: SKIP_DASHED_BUILT_INS does not install git-*-pack Message-ID: <20201020025247.GF54484@nand.local> URL: https://gitlist.dev/e/20201020025247.GF54484%40nand.local In-Reply-To: ``` On Mon, Oct 19, 2020 at 06:54:29PM -0700, Junio C Hamano wrote: > Michael Forney 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 \ ``` ## Johannes Schindelin, 2020-10-21 14:10 Subject: Re: SKIP_DASHED_BUILT_INS does not install git-*-pack Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.2010211609520.56%40tvgsbejvaqbjf.bet In-Reply-To: ``` Hi Junio & Michael, On Mon, 19 Oct 2020, Junio C Hamano wrote: > Michael Forney 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 ``` ## Johannes Schindelin, 2020-10-21 14:16 Subject: Re: SKIP_DASHED_BUILT_INS does not install git-*-pack Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.2010211614300.56%40tvgsbejvaqbjf.bet In-Reply-To: <20201020025247.GF54484@nand.local> ``` 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 ``` ## Johannes Schindelin, 2020-10-21 14:27 Subject: Re: SKIP_DASHED_BUILT_INS does not install git-*-pack Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.2010211626430.56%40tvgsbejvaqbjf.bet In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2020-10-21 18:34 Subject: Re: SKIP_DASHED_BUILT_INS does not install git-*-pack Message-ID: URL: https://gitlist.dev/e/xmqqd01b8m2q.fsf%40gitster.c.googlers.com In-Reply-To: ``` Johannes Schindelin 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. ```