* [PATCH] SKIP_DASHED_BUILT_INS: do not skip the bin/ programs
@ 2020-10-21 15:13 Johannes Schindelin via GitGitGadget
2020-10-21 17:25 ` Taylor Blau
0 siblings, 1 reply; 3+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-10-21 15:13 UTC (permalink / raw)
To: git
Cc: Michael Forney, Taylor Blau, Junio C Hamano, Johannes Schindelin,
Johannes Schindelin
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 \
$(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; }; \
done && \
for p in $(BUILT_INS); do \
$(RM) "$$execdir/$$p" && \
base-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a
--
gitgitgadget
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] SKIP_DASHED_BUILT_INS: do not skip the bin/ programs
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
2020-10-22 9:29 ` Johannes Schindelin
0 siblings, 1 reply; 3+ messages in thread
From: Taylor Blau @ 2020-10-21 17:25 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Michael Forney, Taylor Blau, Junio C Hamano,
Johannes Schindelin
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] SKIP_DASHED_BUILT_INS: do not skip the bin/ programs
2020-10-21 17:25 ` Taylor Blau
@ 2020-10-22 9:29 ` Johannes Schindelin
0 siblings, 0 replies; 3+ messages in thread
From: Johannes Schindelin @ 2020-10-22 9:29 UTC (permalink / raw)
To: Taylor Blau
Cc: Johannes Schindelin via GitGitGadget, git, Michael Forney,
Junio C Hamano
Hi Taylor,
On Wed, 21 Oct 2020, Taylor Blau wrote:
> 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.
Yes, `git-shell` is not a built-in, that's why.
It is added as `PROGRAM_OBJS` in the `Makefile`, and it has its own small
`cmd_main()`. On purpose: the idea for `git-shell` is to serve as a pseudo
login shell for `git fetch`/`git push` to do the minimum amount of work to
ensure that only `git receive-pack`, `git upload-pack` and `git
upload-archive` commands are allowed. Everything else is blocked.
So yes, it is installed into `bin/`, expressly so, and it is not a
hard-linked copy of `bin/git`.
> > $(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>
Awesome, thanks!
Dscho
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-22 9:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2020-10-22 9:29 ` Johannes Schindelin
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).