{"thread":{"id":"54478","subject":"[PATCH] SKIP_DASHED_BUILT_INS: do not skip the bin/ programs","startedAt":"2020-10-21T15:13:36Z","lastAt":"2020-10-22T09:30:00Z","messageCount":3,"participants":["Johannes Schindelin via GitGitGadget","Taylor Blau","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"408066","messageId":"pull.768.git.1603293211428.gitgitgadget@gmail.com","threadId":"54478","inReplyTo":null,"subject":"[PATCH] SKIP_DASHED_BUILT_INS: do not skip the bin/ programs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-21T15:13:31Z","receivedAt":"2020-10-21T15:13:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe idea of the `SKIP_DASHED_BUILT_INS` option is to stop hard-linking\nthe built-in commands as separate executables. The patches to do that\nspecifically excluded the three commands `receive-pack`,\n`upload-archive` and `upload-pack`, though: these commands are expected\nto be present in the `PATH` in their dashed form on the server side of\nany fetch/push.\n\nHowever, due to an oversight by myself, even if those commands were\nstill hard-linked, they were not installed into `bin/`.\n\nNoticed-by: Michael Forney \nSigned-off-by: Johannes Schindelin \n---\n SKIP_DASHED_BUILT_INS: do not skip the bin/ programs\n \n As reported by Michael in \n https://lore.kernel.org/git/CAGw6cBsEjOnh-ZqXCPfFha=NYEdy7JDddha=UzAau0Z1tBrWKg@mail.gmail.com\n , the SKIP_DASHED_BUILT_INS feature had a bug I wish we had caught in\n the -rc cycle. But at least SKIP_DASHED_BUILT_INS is still strictly\n opt-in, so the damage is somewhat contained.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-768%2Fdscho%2Fskip-dashed-built-ins-still-must-install-install_bindir_programs-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-768/dscho/skip-dashed-built-ins-still-must-install-install_bindir_programs-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/768\n\n Makefile | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 95571ee3fc..1fb0ec1705 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2981,15 +2981,12 @@ endif\n \t} && \\\n \tfor p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \\\n \t\t$(RM) \"$$bindir/$$p\" && \\\n-\t\tif test -z \"$(SKIP_DASHED_BUILT_INS)\"; \\\n-\t\tthen \\\n-\t\t\ttest -n \"$(INSTALL_SYMLINKS)\" && \\\n-\t\t\tln -s \"git$X\" \"$$bindir/$$p\" || \\\n-\t\t\t{ test -z \"$(NO_INSTALL_HARDLINKS)\" && \\\n-\t\t\t ln \"$$bindir/git$X\" \"$$bindir/$$p\" 2>/dev/null || \\\n-\t\t\t ln -s \"git$X\" \"$$bindir/$$p\" 2>/dev/null || \\\n-\t\t\t cp \"$$bindir/git$X\" \"$$bindir/$$p\" || exit; }; \\\n-\t\tfi \\\n+\t\ttest -n \"$(INSTALL_SYMLINKS)\" && \\\n+\t\tln -s \"git$X\" \"$$bindir/$$p\" || \\\n+\t\t{ test -z \"$(NO_INSTALL_HARDLINKS)\" && \\\n+\t\t ln \"$$bindir/git$X\" \"$$bindir/$$p\" 2>/dev/null || \\\n+\t\t ln -s \"git$X\" \"$$bindir/$$p\" 2>/dev/null || \\\n+\t\t cp \"$$bindir/git$X\" \"$$bindir/$$p\" || exit; }; \\\n \tdone && \\\n \tfor p in $(BUILT_INS); do \\\n \t\t$(RM) \"$$execdir/$$p\" && \\\n\nbase-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a\n-- \ngitgitgadget\n"},{"id":"408072","messageId":"20201021172516.GB1237181@nand.local","threadId":"54478","inReplyTo":"pull.768.git.1603293211428.gitgitgadget@gmail.com","subject":"Re: [PATCH] SKIP_DASHED_BUILT_INS: do not skip the bin/ programs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-21T17:25:16Z","receivedAt":"2020-10-21T17:25:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 21, 2020 at 03:13:31PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n>\n> The idea of the `SKIP_DASHED_BUILT_INS` option is to stop hard-linking\n> the built-in commands as separate executables. The patches to do that\n> specifically excluded the three commands `receive-pack`,\n> `upload-archive` and `upload-pack`, though: these commands are expected\n> to be present in the `PATH` in their dashed form on the server side of\n> any fetch/push.\n>\n> However, due to an oversight by myself, even if those commands were\n> still hard-linked, they were not installed into `bin/`.\n>\n> Noticed-by: Michael Forney \n> Signed-off-by: Johannes Schindelin \n> ---\n> SKIP_DASHED_BUILT_INS: do not skip the bin/ programs\n>\n> As reported by Michael in\n> https://lore.kernel.org/git/CAGw6cBsEjOnh-ZqXCPfFha=NYEdy7JDddha=UzAau0Z1tBrWKg@mail.gmail.com\n> , the SKIP_DASHED_BUILT_INS feature had a bug I wish we had caught in\n> the -rc cycle. But at least SKIP_DASHED_BUILT_INS is still strictly\n> opt-in, so the damage is somewhat contained.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-768%2Fdscho%2Fskip-dashed-built-ins-still-must-install-install_bindir_programs-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-768/dscho/skip-dashed-built-ins-still-must-install-install_bindir_programs-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/768\n>\n> Makefile | 15 ++++++---------\n> 1 file changed, 6 insertions(+), 9 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 95571ee3fc..1fb0ec1705 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2981,15 +2981,12 @@ endif\n> \t} && \\\n> \tfor p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \\\n\nThis does still install 'git-shell' into my installation's 'bin'\ndirectory. I'm not up-to-date whether or not we mean to still be\ninstalling that, but, doing the same 'SKIP_DASHED_BUILT_INS=1 make\ninstall' on 'master' also installs git-shell, so I'm fine with that.\n\n> \t\t$(RM) \"$$bindir/$$p\" && \\\n> -\t\tif test -z \"$(SKIP_DASHED_BUILT_INS)\"; \\\n> -\t\tthen \\\n> -\t\t\ttest -n \"$(INSTALL_SYMLINKS)\" && \\\n> -\t\t\tln -s \"git$X\" \"$$bindir/$$p\" || \\\n> -\t\t\t{ test -z \"$(NO_INSTALL_HARDLINKS)\" && \\\n> -\t\t\t ln \"$$bindir/git$X\" \"$$bindir/$$p\" 2>/dev/null || \\\n> -\t\t\t ln -s \"git$X\" \"$$bindir/$$p\" 2>/dev/null || \\\n> -\t\t\t cp \"$$bindir/git$X\" \"$$bindir/$$p\" || exit; }; \\\n> -\t\tfi \\\n> +\t\ttest -n \"$(INSTALL_SYMLINKS)\" && \\\n> +\t\tln -s \"git$X\" \"$$bindir/$$p\" || \\\n> +\t\t{ test -z \"$(NO_INSTALL_HARDLINKS)\" && \\\n> +\t\t ln \"$$bindir/git$X\" \"$$bindir/$$p\" 2>/dev/null || \\\n> +\t\t ln -s \"git$X\" \"$$bindir/$$p\" 2>/dev/null || \\\n> +\t\t cp \"$$bindir/git$X\" \"$$bindir/$$p\" || exit; }; \\\n\nOK. $(install_bindir_programs) is the set of programs that we want to\ninstall; that makes sense. And for those, we definitely don't want to\nskip them because SKIP_DASHED_BUILT_INS was set. So removing that test\nmakes sense to me.\n\n> \tdone && \\\n> \tfor p in $(BUILT_INS); do \\\n> \t\t$(RM) \"$$execdir/$$p\" && \\\n>\n> base-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a\n> --\n> gitgitgadget\n\nLooks great, thanks for turning my thoughts into a real patch.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"408157","messageId":"nycvar.QRO.7.76.6.2010221126240.56@tvgsbejvaqbjf.bet","threadId":"54478","inReplyTo":"20201021172516.GB1237181@nand.local","subject":"Re: [PATCH] SKIP_DASHED_BUILT_INS: do not skip the bin/ programs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-22T09:29:50Z","receivedAt":"2020-10-22T09:30:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Taylor,\n\nOn Wed, 21 Oct 2020, Taylor Blau wrote:\n\n> On Wed, Oct 21, 2020 at 03:13:31PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > The idea of the `SKIP_DASHED_BUILT_INS` option is to stop hard-linking\n> > the built-in commands as separate executables. The patches to do that\n> > specifically excluded the three commands `receive-pack`,\n> > `upload-archive` and `upload-pack`, though: these commands are expected\n> > to be present in the `PATH` in their dashed form on the server side of\n> > any fetch/push.\n> >\n> > However, due to an oversight by myself, even if those commands were\n> > still hard-linked, they were not installed into `bin/`.\n> >\n> > Noticed-by: Michael Forney \n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > SKIP_DASHED_BUILT_INS: do not skip the bin/ programs\n> >\n> > As reported by Michael in\n> > https://lore.kernel.org/git/CAGw6cBsEjOnh-ZqXCPfFha=NYEdy7JDddha=UzAau0Z1tBrWKg@mail.gmail.com\n> > , the SKIP_DASHED_BUILT_INS feature had a bug I wish we had caught in\n> > the -rc cycle. But at least SKIP_DASHED_BUILT_INS is still strictly\n> > opt-in, so the damage is somewhat contained.\n> >\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-768%2Fdscho%2Fskip-dashed-built-ins-still-must-install-install_bindir_programs-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-768/dscho/skip-dashed-built-ins-still-must-install-install_bindir_programs-v1\n> > Pull-Request: https://github.com/gitgitgadget/git/pull/768\n> >\n> > Makefile | 15 ++++++---------\n> > 1 file changed, 6 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/Makefile b/Makefile\n> > index 95571ee3fc..1fb0ec1705 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -2981,15 +2981,12 @@ endif\n> > \t} && \\\n> > \tfor p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \\\n>\n> This does still install 'git-shell' into my installation's 'bin'\n> directory. I'm not up-to-date whether or not we mean to still be\n> installing that, but, doing the same 'SKIP_DASHED_BUILT_INS=1 make\n> install' on 'master' also installs git-shell, so I'm fine with that.\n\nYes, `git-shell` is not a built-in, that's why.\n\nIt is added as `PROGRAM_OBJS` in the `Makefile`, and it has its own small\n`cmd_main()`. On purpose: the idea for `git-shell` is to serve as a pseudo\nlogin shell for `git fetch`/`git push` to do the minimum amount of work to\nensure that only `git receive-pack`, `git upload-pack` and `git\nupload-archive` commands are allowed. Everything else is blocked.\n\nSo yes, it is installed into `bin/`, expressly so, and it is not a\nhard-linked copy of `bin/git`.\n\n> > \t\t$(RM) \"$$bindir/$$p\" && \\\n> > -\t\tif test -z \"$(SKIP_DASHED_BUILT_INS)\"; \\\n> > -\t\tthen \\\n> > -\t\t\ttest -n \"$(INSTALL_SYMLINKS)\" && \\\n> > -\t\t\tln -s \"git$X\" \"$$bindir/$$p\" || \\\n> > -\t\t\t{ test -z \"$(NO_INSTALL_HARDLINKS)\" && \\\n> > -\t\t\t ln \"$$bindir/git$X\" \"$$bindir/$$p\" 2>/dev/null || \\\n> > -\t\t\t ln -s \"git$X\" \"$$bindir/$$p\" 2>/dev/null || \\\n> > -\t\t\t cp \"$$bindir/git$X\" \"$$bindir/$$p\" || exit; }; \\\n> > -\t\tfi \\\n> > +\t\ttest -n \"$(INSTALL_SYMLINKS)\" && \\\n> > +\t\tln -s \"git$X\" \"$$bindir/$$p\" || \\\n> > +\t\t{ test -z \"$(NO_INSTALL_HARDLINKS)\" && \\\n> > +\t\t ln \"$$bindir/git$X\" \"$$bindir/$$p\" 2>/dev/null || \\\n> > +\t\t ln -s \"git$X\" \"$$bindir/$$p\" 2>/dev/null || \\\n> > +\t\t cp \"$$bindir/git$X\" \"$$bindir/$$p\" || exit; }; \\\n>\n> OK. $(install_bindir_programs) is the set of programs that we want to\n> install; that makes sense. And for those, we definitely don't want to\n> skip them because SKIP_DASHED_BUILT_INS was set. So removing that test\n> makes sense to me.\n>\n> > \tdone && \\\n> > \tfor p in $(BUILT_INS); do \\\n> > \t\t$(RM) \"$$execdir/$$p\" && \\\n> >\n> > base-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a\n> > --\n> > gitgitgadget\n>\n> Looks great, thanks for turning my thoughts into a real patch.\n>\n> Reviewed-by: Taylor Blau \n\nAwesome, thanks!\nDscho\n"}]}