From: Taylor Blau <me@ttaylorr.com>
To: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Michael Forney <mforney@mforney.org>,
Taylor Blau <me@ttaylorr.com>, Junio C Hamano <gitster@pobox.com>,
Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH] SKIP_DASHED_BUILT_INS: do not skip the bin/ programs
Date: Wed, 21 Oct 2020 13:25:16 -0400 [thread overview]
Message-ID: <20201021172516.GB1237181@nand.local> (raw)
In-Reply-To: <pull.768.git.1603293211428.gitgitgadget@gmail.com>
On Wed, Oct 21, 2020 at 03:13:31PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The idea of the `SKIP_DASHED_BUILT_INS` option is to stop hard-linking
> the built-in commands as separate executables. The patches to do that
> specifically excluded the three commands `receive-pack`,
> `upload-archive` and `upload-pack`, though: these commands are expected
> to be present in the `PATH` in their dashed form on the server side of
> any fetch/push.
>
> However, due to an oversight by myself, even if those commands were
> still hard-linked, they were not installed into `bin/`.
>
> Noticed-by: Michael Forney <mforney@mforney.org>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> SKIP_DASHED_BUILT_INS: do not skip the bin/ programs
>
> As reported by Michael in
> https://lore.kernel.org/git/CAGw6cBsEjOnh-ZqXCPfFha=NYEdy7JDddha=UzAau0Z1tBrWKg@mail.gmail.com
> , the SKIP_DASHED_BUILT_INS feature had a bug I wish we had caught in
> the -rc cycle. But at least SKIP_DASHED_BUILT_INS is still strictly
> opt-in, so the damage is somewhat contained.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-768%2Fdscho%2Fskip-dashed-built-ins-still-must-install-install_bindir_programs-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-768/dscho/skip-dashed-built-ins-still-must-install-install_bindir_programs-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/768
>
> Makefile | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 95571ee3fc..1fb0ec1705 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2981,15 +2981,12 @@ endif
> } && \
> for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
This does still install 'git-shell' into my installation's 'bin'
directory. I'm not up-to-date whether or not we mean to still be
installing that, but, doing the same 'SKIP_DASHED_BUILT_INS=1 make
install' on 'master' also installs git-shell, so I'm fine with that.
> $(RM) "$$bindir/$$p" && \
> - if test -z "$(SKIP_DASHED_BUILT_INS)"; \
> - then \
> - test -n "$(INSTALL_SYMLINKS)" && \
> - ln -s "git$X" "$$bindir/$$p" || \
> - { test -z "$(NO_INSTALL_HARDLINKS)" && \
> - ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
> - ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
> - cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \
> - fi \
> + test -n "$(INSTALL_SYMLINKS)" && \
> + ln -s "git$X" "$$bindir/$$p" || \
> + { test -z "$(NO_INSTALL_HARDLINKS)" && \
> + ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
> + ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
> + cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \
OK. $(install_bindir_programs) is the set of programs that we want to
install; that makes sense. And for those, we definitely don't want to
skip them because SKIP_DASHED_BUILT_INS was set. So removing that test
makes sense to me.
> done && \
> for p in $(BUILT_INS); do \
> $(RM) "$$execdir/$$p" && \
>
> base-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a
> --
> gitgitgadget
Looks great, thanks for turning my thoughts into a real patch.
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Thanks,
Taylor
next prev parent reply other threads:[~2020-10-21 17:25 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-21 15:13 [PATCH] SKIP_DASHED_BUILT_INS: do not skip the bin/ programs Johannes Schindelin via GitGitGadget
2020-10-21 17:25 ` Taylor Blau [this message]
2020-10-22 9:29 ` Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201021172516.GB1237181@nand.local \
--to=me@ttaylorr.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=johannes.schindelin@gmx.de \
--cc=mforney@mforney.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).