* Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git
2018-03-16 12:43 ` Ævar Arnfjörð Bjarmason
@ 2018-03-19 11:34 ` Johannes Schindelin
2018-03-19 21:21 ` Linus Torvalds
2018-11-02 22:37 ` [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init" Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
6 siblings, 1 reply; 53+ messages in thread
From: Johannes Schindelin @ 2018-03-19 11:34 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Linus Torvalds, Johannes Sixt, Git Mailing List, Junio C Hamano,
Daniel Jacques, Steffen Prohaska, John Keeping, Stan Hu,
Richard Clamp
[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]
Hi Ævar,
On Fri, 16 Mar 2018, Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, Mar 16 2018, Johannes Schindelin jotted:
>
> > On Thu, 15 Mar 2018, Linus Torvalds wrote:
> >
> >> We do end up still using the dashed form for certain things, but they
> >> are already special-cased (ie things like "git-receive-pack" and
> >> "git-shell" that very much get executed directly, and for fundamental
> >> reasons).
> >
> > Please do elaborate.
>
> If you were to set set "/bin/git shell" in /etc/password it would not do
> the right thing as far as I know. Is that a shell name with a space in
> it, or the "shell" argument to /bin/git?
True. And `git-shell` is not a builtin, so it does not even matter with
regards to this discussion.
> There's also the fully dashed forms of stuff like git-receive-pack is
> part of the over-ssh convention, i.e.:
>
> ssh <host> git-upload-pack ...
Even if upload-pack is not a builtin (and thus still has to be its own
executable), receive-pack *is*, so this does affect our current
discussion.
This is a real problem. And it is our own darned fault because we let an
implementation detail bleed into a protocol. We could have designed that a
lot better.
Of course we should fix this, though. There is literally no good reason
that I can think of why we should not change this to `ssh <host> git
upload-pack ...` (of course with an insanely long deprecation period).
Ciao,
Dscho
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git
2018-03-19 11:34 ` Johannes Schindelin
@ 2018-03-19 21:21 ` Linus Torvalds
0 siblings, 0 replies; 53+ messages in thread
From: Linus Torvalds @ 2018-03-19 21:21 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Ævar Arnfjörð Bjarmason, Johannes Sixt,
Git Mailing List, Junio C Hamano, Daniel Jacques,
Steffen Prohaska, John Keeping, Stan Hu, Richard Clamp
On Mon, Mar 19, 2018, 04:34 Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> This is a real problem.
No it isn't.
We already handle those special cases specially, and install them in
the bin directory (as opposed to libexec). And it all works fine.
Look into the bin directory some day. You'll find things like
git-cvsserver
gitk
git-receive-pack
git-shell
git-upload-archive
git-upload-pack
there, and the fact that a couple of them happen to be built-ins is an
IMPLEMENTATION DETAIL, not a "Oh we should have used just 'git' for
them".
The design of having separate programs is the *good* conceptual
design. And we damn well should keep it for these things that are used
for special purposes.
The fact that two of them have become built-ins as part of the git
binary is incidental. It shouldn't be visible in the names, because it
really is just an internal implementation thing, not anything
fundamental.
> And it is our own darned fault because we let an
> implementation detail bleed into a protocol. We could have designed that a
> lot better.
And by "we" you clearly mean "not you", and by "we could have designed
that a lot better" you must mean "and it was very well designed by
competent people who didn't use bad operating systems".
> Of course we should fix this, though. There is literally no good reason
Go away.
We shouldn't fix it, it's all fine as-is, and there were tons of
f*cking good reasons for why git did what it did. The main one being
"it's a collection of scripts", which was what git _was_, for
chrissake. And using spaces and running some idiotic and
hard-to-verify script de-multiplexer is the WRONG THING for things
like "git-shell" and "git-receive-pack" and friends.
Right now you can actually verify exactly what "git-shell" does. Or
you could replace - or remove - it entirely if you don't like it. And
never have to worry about running "git" with some "shell" subcommand.
And you know that it's not an alias, for example. Because "git-xyz"
simply does not look up aliases.
So really. Go away, Johannes. Your concerns are complete and utter BS.
The real problem is that Windows is badly designed, but since it's
easy to work around (by using hard-linking on Windows), nobody sane
cares.
The solution is simple, and was already suggested: use symlinks (like
we used to!) on non-windows systems. End of story.
And for the libexec thing, we might want to deprecate those names, if
somebody wants to, but it's not like it actually hurts, and it gives
backwards compatibility.
Btw, real Windows people know all about backwards compatibility. Ask
around competent people inside MS whether it's an important thing.
So stop this idiotic "bad design" crap. Somebody working on Windows
simply can't afford your attitude.
Somebody who didn't design it in the first place can't afford your attitude.
Linus
^ permalink raw reply [flat|nested] 53+ messages in thread
* [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
2018-03-16 12:43 ` Ævar Arnfjörð Bjarmason
2018-03-19 11:34 ` Johannes Schindelin
@ 2018-11-02 22:37 ` Ævar Arnfjörð Bjarmason
2018-11-03 1:17 ` Junio C Hamano
` (2 more replies)
2018-11-02 22:37 ` [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
6 siblings, 3 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-11-02 22:37 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Daniel Jacques, Johannes Schindelin,
Steffen Prohaska, John Keeping, Stan Hu, Richard Clamp, Jeff King,
Ævar Arnfjörð Bjarmason
I think up to patch 4 here should be near a state that's ready for
inclusion.
Although I'm on the fence with the approach in 1/5. Should this be a
giant getopt switch statement like that in a helper script? An
alternative would be to write out a shell file similar to
GIT-BUILD-OPTIONS and source that from this thing. I don't know, what
do you all think?
The idea with 4/5 was to make this symlink mode the default in
config.mak.uname and have a blacklist of systems like Windows that
couldn't deal with it.
Since my ad874608d8 ("Makefile: optionally symlink libexec/git-core
binaries to bin/git", 2018-03-13) I see that e.g. Debian and GitLab
have started shipping with the INSTALL_SYMLINKS flag, so making that
unconditional is the next logical step.
The 5th one is more radical. See
https://public-inbox.org/git/87woyfdkoi.fsf@evledraar.gmail.com/ from
back in March for context.
I'd like to say it's ready, but I've spotted some fallout:
* Help like "git ninit" suggesting "git init" doesn't work, this is
because load_command_list() in help.c doesn't look out our
in-memory idea of builtins, it reads the libexecdir, so if we don't
have the programs there it doesn't know about it.
* GIT_TEST_INSTALLED breaks entirely under this, as early as the
heuristic for "are we built?" being "do we have git-init in
libexecdir?". I tried a bit to make this work, but there's a lot of
dependencies there.
* We still (and this is also true of my ad874608d8) hardlink
everything in the build dir via a different part of the Makefile,
ideally we should do exactly the same thing there so also normal
tests and not just GIT_TEST_INSTALLED (if that worked) would test
in the same mode.
I gave making that work a bit of a try and gave up in the Makefile
jungle.
Ævar Arnfjörð Bjarmason (5):
Makefile: move long inline shell loops in "install" into helper
Makefile: conform some of the code to our coding standards
Makefile: stop hiding failures during "install"
Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch
Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
Makefile | 65 +++++++++++--------------
install_programs | 124 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 151 insertions(+), 38 deletions(-)
create mode 100755 install_programs
--
2.19.1.930.g4563a0d9d0
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
2018-11-02 22:37 ` [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init" Ævar Arnfjörð Bjarmason
@ 2018-11-03 1:17 ` Junio C Hamano
2018-11-05 11:36 ` Ævar Arnfjörð Bjarmason
2018-11-12 13:33 ` Johannes Schindelin
2018-11-16 10:38 ` Ævar Arnfjörð Bjarmason
2 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2018-11-03 1:17 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Daniel Jacques, Johannes Schindelin, Steffen Prohaska,
John Keeping, Stan Hu, Richard Clamp, Jeff King
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Although I'm on the fence with the approach in 1/5. Should this be a
> giant getopt switch statement like that in a helper script?
>
> An alternative would be to write out a shell file similar to
> GIT-BUILD-OPTIONS and source that from this thing. I don't know,
> what do you all think?
Not really. Why do we iterate over these in a shell loop, rather
than having make to figure them out, just like we do when we "loop
over the source files and turn them into object files" without using
a shell loop? What's so special about enumerating the installation
targets and iterating over the enumeration to perform an action on
each of them? I think that is the first question we should be
asking before patch 1/5, which already assumes that it has been
decided that it must be done with a shell loop.
I think "first install 'git' itself, and then make these
other things derived from it" should let $(MAKE) install
things in parallel just like it can naturally do many things
in parallel, and the dependency rule to do so should not be
so bad, I suspect.
This is a tangent, but I have long been wishing that somebody would
notice that output during install and (dist)clean without V=1 is so
different from the normal targets and do something about it, and
hoped that that somebody finally turned out to be you doing so in
this series X-<.
> I'd like to say it's ready, but I've spotted some fallout:
I still have not recovered from the trauma I suffered post 1.6.0
era, so I would rather *not* engage in a long discussion like this
one (it is a long thread; reserve a solid hour to read it through if
you are interested),
https://public-inbox.org/git/alpine.LFD.1.10.0808261435470.3363@nehalem.linux-foundation.org/
which would be needed to defend the choice, if we decide to omit
installing the git-foo on disk in a released version.
I personally have no objection to offer a knob that can e used to
force installation of symlinks without falling back to other
methods. I think it would be ideal to do so without special casing
symbolic links---rather, it would be ideal if it were a single knob
INSTALL_GIT_FOO_METHOD=(symlinks|hardlinks|copies) that says "I want
them to be installed as (symlinks|hardlinks|copies), no fallbacks".
> Ævar Arnfjörð Bjarmason (5):
> Makefile: move long inline shell loops in "install" into helper
> Makefile: conform some of the code to our coding standards
> Makefile: stop hiding failures during "install"
> Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch
> Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
>
> Makefile | 65 +++++++++++--------------
> install_programs | 124 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 151 insertions(+), 38 deletions(-)
> create mode 100755 install_programs
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
2018-11-03 1:17 ` Junio C Hamano
@ 2018-11-05 11:36 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-11-05 11:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Daniel Jacques, Johannes Schindelin, Steffen Prohaska,
John Keeping, Stan Hu, Richard Clamp, Jeff King
On Sat, Nov 03 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Although I'm on the fence with the approach in 1/5. Should this be a
>> giant getopt switch statement like that in a helper script?
>>
>> An alternative would be to write out a shell file similar to
>> GIT-BUILD-OPTIONS and source that from this thing. I don't know,
>> what do you all think?
>
> Not really. Why do we iterate over these in a shell loop, rather
> than having make to figure them out, just like we do when we "loop
> over the source files and turn them into object files" without using
> a shell loop? What's so special about enumerating the installation
> targets and iterating over the enumeration to perform an action on
> each of them? I think that is the first question we should be
> asking before patch 1/5, which already assumes that it has been
> decided that it must be done with a shell loop.
>
> I think "first install 'git' itself, and then make these
> other things derived from it" should let $(MAKE) install
> things in parallel just like it can naturally do many things
> in parallel, and the dependency rule to do so should not be
> so bad, I suspect.
>
> This is a tangent, but I have long been wishing that somebody would
> notice that output during install and (dist)clean without V=1 is so
> different from the normal targets and do something about it, and
> hoped that that somebody finally turned out to be you doing so in
> this series X-<.
I'm all for this, but don't have enough Make skills to make it
happen. Can you or someone else post a WIP patch showing how to do this?
What would the targets look like? Something that's a build target for
the target file in the installation directory, so e.g. if you ran "all
install" and had modified just one file (and no recursive rebuilds)
you'd install just that one file?
Early on in the "install" target we install many of these programs, and
then some of these for-loops re-install on top of them. Actually now
that I think of this this is one of the reasons for the 2>/dev/null
probably, i.e. run "install" twice and you don't want to get errors.
Anyway, regardless of how the for-loop looks like (shell or
make-powered) I split this up because it was getting really hard to
maintain the *inner* part of those loops. I.e. needing to specially
quote everything, end lines with \ etc.
But reading on...
>> I'd like to say it's ready, but I've spotted some fallout:
>
> I still have not recovered from the trauma I suffered post 1.6.0
> era, so I would rather *not* engage in a long discussion like this
> one (it is a long thread; reserve a solid hour to read it through if
> you are interested),
>
> https://public-inbox.org/git/alpine.LFD.1.10.0808261435470.3363@nehalem.linux-foundation.org/
>
> which would be needed to defend the choice, if we decide to omit
> installing the git-foo on disk in a released version.
Thanks. I'll read that later.
> I personally have no objection to offer a knob that can e used to
> force installation of symlinks without falling back to other
> methods. I think it would be ideal to do so without special casing
> symbolic links---rather, it would be ideal if it were a single knob
> INSTALL_GIT_FOO_METHOD=(symlinks|hardlinks|copies) that says "I want
> them to be installed as (symlinks|hardlinks|copies), no fallbacks".
... If you're happy to accept a patch that rips out this whole
conditional fallback logic and just makes it an if/elsif/elsif for
symlink/hardlink/copy this makes things a lot easier.
>> Ævar Arnfjörð Bjarmason (5):
>> Makefile: move long inline shell loops in "install" into helper
>> Makefile: conform some of the code to our coding standards
>> Makefile: stop hiding failures during "install"
>> Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch
>> Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
>>
>> Makefile | 65 +++++++++++--------------
>> install_programs | 124 +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 151 insertions(+), 38 deletions(-)
>> create mode 100755 install_programs
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
2018-11-02 22:37 ` [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init" Ævar Arnfjörð Bjarmason
2018-11-03 1:17 ` Junio C Hamano
@ 2018-11-12 13:33 ` Johannes Schindelin
2018-11-16 10:38 ` Ævar Arnfjörð Bjarmason
2 siblings, 0 replies; 53+ messages in thread
From: Johannes Schindelin @ 2018-11-12 13:33 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Daniel Jacques, Steffen Prohaska,
John Keeping, Stan Hu, Richard Clamp, Jeff King
[-- Attachment #1: Type: text/plain, Size: 888 bytes --]
Hi Ævar,
On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> * GIT_TEST_INSTALLED breaks entirely under this, as early as the
> heuristic for "are we built?" being "do we have git-init in
> libexecdir?". I tried a bit to make this work, but there's a lot of
> dependencies there.
I have a couple of patches in the pipeline to improve
`GIT_TEST_INSTALLED`, as I needed it to work without hardlinked copies of
the built-ins. These patches might help this here isue.
> * We still (and this is also true of my ad874608d8) hardlink
> everything in the build dir via a different part of the Makefile,
> ideally we should do exactly the same thing there so also normal
> tests and not just GIT_TEST_INSTALLED (if that worked) would test
> in the same mode.
>
> I gave making that work a bit of a try and gave up in the Makefile
> jungle.
Yep.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
2018-11-02 22:37 ` [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init" Ævar Arnfjörð Bjarmason
2018-11-03 1:17 ` Junio C Hamano
2018-11-12 13:33 ` Johannes Schindelin
@ 2018-11-16 10:38 ` Ævar Arnfjörð Bjarmason
2018-11-16 16:00 ` Michael Haggerty
2 siblings, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-11-16 10:38 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Daniel Jacques, Johannes Schindelin,
Steffen Prohaska, John Keeping, Stan Hu, Richard Clamp, Jeff King,
Michael Haggerty
On Fri, Nov 02 2018, Ævar Arnfjörð Bjarmason wrote:
> I think up to patch 4 here should be near a state that's ready for
> inclusion.
>
> Although I'm on the fence with the approach in 1/5. Should this be a
> giant getopt switch statement like that in a helper script? An
> alternative would be to write out a shell file similar to
> GIT-BUILD-OPTIONS and source that from this thing. I don't know, what
> do you all think?
>
> The idea with 4/5 was to make this symlink mode the default in
> config.mak.uname and have a blacklist of systems like Windows that
> couldn't deal with it.
>
> Since my ad874608d8 ("Makefile: optionally symlink libexec/git-core
> binaries to bin/git", 2018-03-13) I see that e.g. Debian and GitLab
> have started shipping with the INSTALL_SYMLINKS flag, so making that
> unconditional is the next logical step.
>
> The 5th one is more radical. See
> https://public-inbox.org/git/87woyfdkoi.fsf@evledraar.gmail.com/ from
> back in March for context.
>
> I'd like to say it's ready, but I've spotted some fallout:
>
> * Help like "git ninit" suggesting "git init" doesn't work, this is
> because load_command_list() in help.c doesn't look out our
> in-memory idea of builtins, it reads the libexecdir, so if we don't
> have the programs there it doesn't know about it.
A follow-up on this: We should really fix this for other
reasons. I.e. compile in some "this is stuff we ourselves think is in
git".
There's other manifestations of this, e.g.:
git-sizer --help # => shows you help
git sizer --help # => says it doesn't have a manpage
Because we aren't aware that git-sizer is some external tool, and that
we should route --help to it.
Non-withstanding the arguable bug that things like git-sizer shouldn't
be allowing themselves to be invoked by "git" like that without
guaranteeing that it can consume all the options 'git' expects. When I
had to deal with a similar problem in an external git-* command I was
maintaining I simply made it an error to invoke it as "git mything"
instead of "git-mything".
> * GIT_TEST_INSTALLED breaks entirely under this, as early as the
> heuristic for "are we built?" being "do we have git-init in
> libexecdir?". I tried a bit to make this work, but there's a lot of
> dependencies there.
>
> * We still (and this is also true of my ad874608d8) hardlink
> everything in the build dir via a different part of the Makefile,
> ideally we should do exactly the same thing there so also normal
> tests and not just GIT_TEST_INSTALLED (if that worked) would test
> in the same mode.
>
> I gave making that work a bit of a try and gave up in the Makefile
> jungle.
>
> Ævar Arnfjörð Bjarmason (5):
> Makefile: move long inline shell loops in "install" into helper
> Makefile: conform some of the code to our coding standards
> Makefile: stop hiding failures during "install"
> Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch
> Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
>
> Makefile | 65 +++++++++++--------------
> install_programs | 124 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 151 insertions(+), 38 deletions(-)
> create mode 100755 install_programs
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
2018-11-16 10:38 ` Ævar Arnfjörð Bjarmason
@ 2018-11-16 16:00 ` Michael Haggerty
2018-11-16 19:22 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 53+ messages in thread
From: Michael Haggerty @ 2018-11-16 16:00 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano, dnj, Johannes Schindelin,
prohaska, john, stanhu, richardc, Jeff King
On Fri, Nov 16, 2018 at 11:38 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> [...]
> A follow-up on this: We should really fix this for other
> reasons. I.e. compile in some "this is stuff we ourselves think is in
> git".
>
> There's other manifestations of this, e.g.:
>
> git-sizer --help # => shows you help
> git sizer --help # => says it doesn't have a manpage
>
> Because we aren't aware that git-sizer is some external tool, and that
> we should route --help to it.
That would be nice. This has been an annoying for several tools named
`git-foo` that I have worked on (e.g., git-sizer, git-imerge,
git-when-merged, plus many internal tools).
> Non-withstanding the arguable bug that things like git-sizer shouldn't
> be allowing themselves to be invoked by "git" like that without
> guaranteeing that it can consume all the options 'git' expects. When I
> had to deal with a similar problem in an external git-* command I was
> maintaining I simply made it an error to invoke it as "git mything"
> instead of "git-mything".
Hmmm, I always thought that it was intended and supported that an
external tool can name itself `git-foo` so that it can be invoked as
`git foo`.
Which git options do you think that such a tool should be expected to
support? Many useful ones, like `-C <path>`, `--git-dir=<path>`,
`--work-tree=<path>`, `-c <name>=<value>`, and `--no-replace-objects`,
work pretty much for free if the external tool uses `git` to interact
with the repository. I use such options regularly with external tools.
IMO it would be a regression for these tools to refuse to run when
invoked as, say, `git -C path/to/repo foo`.
Michael
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
2018-11-16 16:00 ` Michael Haggerty
@ 2018-11-16 19:22 ` Ævar Arnfjörð Bjarmason
2018-11-17 6:39 ` Jeff King
0 siblings, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-11-16 19:22 UTC (permalink / raw)
To: mhagger
Cc: Git Mailing List, Junio C Hamano, dnj, Johannes Schindelin,
prohaska, john, stanhu, richardc, Jeff King, Joey Hess
On Fri, Nov 16 2018, Michael Haggerty wrote:
> On Fri, Nov 16, 2018 at 11:38 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> [...]
>> A follow-up on this: We should really fix this for other
>> reasons. I.e. compile in some "this is stuff we ourselves think is in
>> git".
>>
>> There's other manifestations of this, e.g.:
>>
>> git-sizer --help # => shows you help
>> git sizer --help # => says it doesn't have a manpage
>>
>> Because we aren't aware that git-sizer is some external tool, and that
>> we should route --help to it.
>
> That would be nice. This has been an annoying for several tools named
> `git-foo` that I have worked on (e.g., git-sizer, git-imerge,
> git-when-merged, plus many internal tools).
>
>> Non-withstanding the arguable bug that things like git-sizer shouldn't
>> be allowing themselves to be invoked by "git" like that without
>> guaranteeing that it can consume all the options 'git' expects. When I
>> had to deal with a similar problem in an external git-* command I was
>> maintaining I simply made it an error to invoke it as "git mything"
>> instead of "git-mything".
>
> Hmmm, I always thought that it was intended and supported that an
> external tool can name itself `git-foo` so that it can be invoked as
> `git foo`.
>
> Which git options do you think that such a tool should be expected to
> support? Many useful ones, like `-C <path>`, `--git-dir=<path>`,
> `--work-tree=<path>`, `-c <name>=<value>`, and `--no-replace-objects`,
> work pretty much for free if the external tool uses `git` to interact
> with the repository. I use such options regularly with external tools.
> IMO it would be a regression for these tools to refuse to run when
> invoked as, say, `git -C path/to/repo foo`.
I don't mean we should forbid it, just that if you have an external
git-foo tool meant to be invoked like "git-foo" that and not "git foo"
it might be worth to save yourself the trouble and not support the
latter. I thought git-sizer was one such tool, since it docs just
mention "git-sizer".
But yeah, "-c" etc. are useful, and we definitely want to support this
in the general case. E.g. for "git-annex" and others that are meant to
be used like that.
So maybe we should just document this interface better. It seems one
implicit dependency is that we expect a manpage for the tool to exist
for --help.
Or, keep some list of internal git tools and treat them specially. But I
see now that would break "git annex --help" in the other direction, but
maybe that would be better. I don't know.
As I recall I stopped supporting "git" invocation for the tool I was
fixing a long time ago because of some funny interaction with terminals
& signals. I.e. git itself would eat some of them, but the tool wanted
to handle it instead.
But I don't remember the details, and can't reproduce it now with:
$ cat ~/bin/git-sigint
#!/usr/bin/env perl
$SIG{INT} = sub { warn localtime . " " . $$ };
sleep 1 while 1;
$ git sigint # or git-sigint
[... behaves the same either way ...]
So maybe it was something else, or I'm misremembering...
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
2018-11-16 19:22 ` Ævar Arnfjörð Bjarmason
@ 2018-11-17 6:39 ` Jeff King
2018-11-22 12:48 ` Johannes Schindelin
0 siblings, 1 reply; 53+ messages in thread
From: Jeff King @ 2018-11-17 6:39 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: mhagger, Git Mailing List, Junio C Hamano, dnj,
Johannes Schindelin, prohaska, john, stanhu, richardc, Joey Hess
On Fri, Nov 16, 2018 at 08:22:11PM +0100, Ævar Arnfjörð Bjarmason wrote:
> So maybe we should just document this interface better. It seems one
> implicit dependency is that we expect a manpage for the tool to exist
> for --help.
Yeah, I think this really the only problematic assumption. The rest of
"-c", "--git-dir", etc, are just manipulating the environment, and that
gets passed through to sub-invocations of Git (so if I have a script
which calls git-config, it will pick up the "-c" config).
It would be nice if there was a way to ask "is there a manpage?", and
fallback to running "git-cmd --help". But:
1. I don't think there is a portable way to query that via man. And
anyway, depending platform and config, it may be opening a browser
to show HTML documentation (or I think even texinfo?).
2. We can just ask whether "man git-sizer" (or whatever help display
command) returned a non-zero exit code, and fall back to "git-sizer
--help". But there's an infinite-loop possibility here: running
"git-sizer --help" does what we want. But if "man git-log" failed,
we'd run "git-log --help", which in turn runs "git help log", which
runs "man git-log", and so on.
You can break that loop with an environment variable for "I already
tried to show the manpage", which would I guess convert "--help" to
"-h".
So it's maybe do-able, but not quite as trivial as one might hope.
> But I don't remember the details, and can't reproduce it now with:
>
> $ cat ~/bin/git-sigint
> #!/usr/bin/env perl
> $SIG{INT} = sub { warn localtime . " " . $$ };
> sleep 1 while 1;
> $ git sigint # or git-sigint
> [... behaves the same either way ...]
>
> So maybe it was something else, or I'm misremembering...
I think that generally works because hitting ^C is going to send SIGINT
to the whole process group. A more interesting case is:
git sigint &
kill -INT $!
There $! is a parent "git" process that is just waiting on git-sigint to
die. But that works, too, because the parent relays common signals due
to 10c6cddd92 (dashed externals: kill children on exit, 2012-01-08). So
you might have been remembering issues prior to that commit (or uncommon
signals; these come from sigchain_push_common).
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
2018-11-17 6:39 ` Jeff King
@ 2018-11-22 12:48 ` Johannes Schindelin
2018-11-22 16:06 ` Jeff King
0 siblings, 1 reply; 53+ messages in thread
From: Johannes Schindelin @ 2018-11-22 12:48 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, mhagger, Git Mailing List,
Junio C Hamano, dnj, prohaska, john, stanhu, richardc, Joey Hess
[-- Attachment #1: Type: text/plain, Size: 3352 bytes --]
Hi Peff,
On Sat, 17 Nov 2018, Jeff King wrote:
> On Fri, Nov 16, 2018 at 08:22:11PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> > So maybe we should just document this interface better. It seems one
> > implicit dependency is that we expect a manpage for the tool to exist
> > for --help.
>
> Yeah, I think this really the only problematic assumption. The rest of
> "-c", "--git-dir", etc, are just manipulating the environment, and that
> gets passed through to sub-invocations of Git (so if I have a script
> which calls git-config, it will pick up the "-c" config).
>
> It would be nice if there was a way to ask "is there a manpage?", and
> fallback to running "git-cmd --help". But:
>
> 1. I don't think there is a portable way to query that via man. And
> anyway, depending platform and config, it may be opening a browser
> to show HTML documentation (or I think even texinfo?).
>
> 2. We can just ask whether "man git-sizer" (or whatever help display
> command) returned a non-zero exit code, and fall back to "git-sizer
> --help". But there's an infinite-loop possibility here: running
> "git-sizer --help" does what we want. But if "man git-log" failed,
> we'd run "git-log --help", which in turn runs "git help log", which
> runs "man git-log", and so on.
>
> You can break that loop with an environment variable for "I already
> tried to show the manpage", which would I guess convert "--help" to
> "-h".
>
> So it's maybe do-able, but not quite as trivial as one might hope.
A trivial alternative would be to recommend adding a man page for
3rd-party git-<tool>s.
In other words, as soon as `git-sizer` is accompanied by `git-sizer.1` in
one of the appropriate locations, it is set.
(Actually, it is not: on Windows, it would have to add git-sizer.html in
the appropriate location, but we can deal with this if needed.)
> > But I don't remember the details, and can't reproduce it now with:
> >
> > $ cat ~/bin/git-sigint
> > #!/usr/bin/env perl
> > $SIG{INT} = sub { warn localtime . " " . $$ };
> > sleep 1 while 1;
> > $ git sigint # or git-sigint
> > [... behaves the same either way ...]
> >
> > So maybe it was something else, or I'm misremembering...
>
> I think that generally works because hitting ^C is going to send SIGINT
> to the whole process group. A more interesting case is:
>
> git sigint &
> kill -INT $!
>
> There $! is a parent "git" process that is just waiting on git-sigint to
> die. But that works, too, because the parent relays common signals due
> to 10c6cddd92 (dashed externals: kill children on exit, 2012-01-08). So
> you might have been remembering issues prior to that commit (or uncommon
> signals; these come from sigchain_push_common).
FWIW I do have a couple of scripts I use that I install into
`$HOME/bin/git-<tool>`. Although, granted, I essentially switched to
aliases for most of them, where the aliases still call a script that is
checked out in some folder in my home directory. The reason: this works in
more circumstances, as I do not have to add `$HOME/bin` to the `PATH`,
say, in PowerShell.
So YMMV with git-<tool>s. My rule of thumb is: if I want to use this
myself only, I'll make it an alias. If I want to ship it (e.g. with Git
for Windows), I'll make it a git-<tool>.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
2018-11-22 12:48 ` Johannes Schindelin
@ 2018-11-22 16:06 ` Jeff King
2018-11-23 11:19 ` Johannes Schindelin
0 siblings, 1 reply; 53+ messages in thread
From: Jeff King @ 2018-11-22 16:06 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Ævar Arnfjörð Bjarmason, mhagger, Git Mailing List,
Junio C Hamano, dnj, prohaska, john, stanhu, richardc, Joey Hess
On Thu, Nov 22, 2018 at 01:48:53PM +0100, Johannes Schindelin wrote:
> > So it's maybe do-able, but not quite as trivial as one might hope.
>
> A trivial alternative would be to recommend adding a man page for
> 3rd-party git-<tool>s.
>
> In other words, as soon as `git-sizer` is accompanied by `git-sizer.1` in
> one of the appropriate locations, it is set.
Yes, it would be nice if everything did ship with a manpage.
Unfortunately that complicates installation, where the installation for
many such tools is "save this file to your $PATH".
Tools like git-sizer may be getting big and popular enough to merit the
extra infrastructure, but I think there are many smaller scripts which
don't.
> FWIW I do have a couple of scripts I use that I install into
> `$HOME/bin/git-<tool>`. Although, granted, I essentially switched to
> aliases for most of them, where the aliases still call a script that is
> checked out in some folder in my home directory. The reason: this works in
> more circumstances, as I do not have to add `$HOME/bin` to the `PATH`,
> say, in PowerShell.
>
> So YMMV with git-<tool>s. My rule of thumb is: if I want to use this
> myself only, I'll make it an alias. If I want to ship it (e.g. with Git
> for Windows), I'll make it a git-<tool>.
I have a handful of personal git-* scripts: mostly ones that are big
enough to be unwieldy as an alias. But then, $PATH management is pretty
straightforward on my platforms, so it's easier to drop a script there
than to point an alias to a non-git-* script.
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
2018-11-22 16:06 ` Jeff King
@ 2018-11-23 11:19 ` Johannes Schindelin
0 siblings, 0 replies; 53+ messages in thread
From: Johannes Schindelin @ 2018-11-23 11:19 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, mhagger, Git Mailing List,
Junio C Hamano, dnj, prohaska, john, stanhu, richardc, Joey Hess
Hi Peff,
On Thu, 22 Nov 2018, Jeff King wrote:
> On Thu, Nov 22, 2018 at 01:48:53PM +0100, Johannes Schindelin wrote:
>
> > So YMMV with git-<tool>s. My rule of thumb is: if I want to use this
> > myself only, I'll make it an alias. If I want to ship it (e.g. with Git
> > for Windows), I'll make it a git-<tool>.
>
> I have a handful of personal git-* scripts: mostly ones that are big
> enough to be unwieldy as an alias. But then, $PATH management is pretty
> straightforward on my platforms, so it's easier to drop a script there
> than to point an alias to a non-git-* script.
Oh, my Git aliases pretty much *all* point to a single script that takes
subcommands.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 53+ messages in thread
* [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper
2018-03-16 12:43 ` Ævar Arnfjörð Bjarmason
2018-03-19 11:34 ` Johannes Schindelin
2018-11-02 22:37 ` [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init" Ævar Arnfjörð Bjarmason
@ 2018-11-02 22:37 ` Ævar Arnfjörð Bjarmason
2018-11-04 1:09 ` Eric Sunshine
2018-11-12 14:03 ` Johannes Schindelin
2018-11-02 22:37 ` [RFC/PATCH 2/5] Makefile: conform some of the code to our coding standards Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
6 siblings, 2 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-11-02 22:37 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Daniel Jacques, Johannes Schindelin,
Steffen Prohaska, John Keeping, Stan Hu, Richard Clamp, Jeff King,
Ævar Arnfjörð Bjarmason
Move a 37 line for-loop mess out of "install" and into a helper
script. This started out fairly innocent but over the years has grown
into a hard-to-maintain monster, and my recent ad874608d8 ("Makefile:
optionally symlink libexec/git-core binaries to bin/git", 2018-03-13)
certainly didn't help.
The shell code is ported pretty much as-is (with getopts added), it'll
be fixed & prettified in subsequent commits.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Makefile | 52 ++++++++--------------------
install_programs | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 103 insertions(+), 38 deletions(-)
create mode 100755 install_programs
diff --git a/Makefile b/Makefile
index bbfbb4292d..aa6ca1fa68 100644
--- a/Makefile
+++ b/Makefile
@@ -2808,44 +2808,20 @@ endif
bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 's|[^/][^/]*|..|g') && \
- { test "$$bindir/" = "$$execdir/" || \
- for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
- $(RM) "$$execdir/$$p" && \
- test -n "$(INSTALL_SYMLINKS)" && \
- ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" "$$execdir/$$p" || \
- { test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
- ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
- cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \
- done; \
- } && \
- for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
- $(RM) "$$bindir/$$p" && \
- 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" && \
- 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 \
- $(RM) "$$execdir/$$p" && \
- test -n "$(INSTALL_SYMLINKS)" && \
- ln -s "git-remote-http$X" "$$execdir/$$p" || \
- { test -z "$(NO_INSTALL_HARDLINKS)" && \
- ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
- ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
- cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
- done && \
+ ./install_programs \
+ --X="$$X" \
+ --RM="$(RM)" \
+ --bindir="$$bindir" \
+ --bindir-relative="$(bindir_relative_SQ)" \
+ --execdir="$$execdir" \
+ --destdir-from-execdir="$$destdir_from_execdir_SQ" \
+ --flag-install-symlinks="$(INSTALL_SYMLINKS)" \
+ --flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
+ --flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
+ --list-bindir-standalone="git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS))" \
+ --list-bindir-git-dashed="$(filter $(install_bindir_programs),$(BUILT_INS))" \
+ --list-execdir-git-dashed="$(BUILT_INS)" \
+ --list-execdir-curl-aliases="$(REMOTE_CURL_ALIASES)" && \
./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
.PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf
diff --git a/install_programs b/install_programs
new file mode 100755
index 0000000000..e287108112
--- /dev/null
+++ b/install_programs
@@ -0,0 +1,89 @@
+#!/bin/sh
+
+while test $# != 0
+do
+ case "$1" in
+ --X=*)
+ X="${1#--X=}"
+ ;;
+ --RM=*)
+ RM="${1#--RM=}"
+ ;;
+ --bindir=*)
+ bindir="${1#--bindir=}"
+ ;;
+ --bindir-relative=*)
+ bindir_relative="${1#--bindir-relative=}"
+ ;;
+ --execdir=*)
+ execdir="${1#--execdir=}"
+ ;;
+ --destdir-from-execdir=*)
+ destdir_from_execdir="${1#--destdir-from-execdir=}"
+ ;;
+ --flag-install-symlinks=*)
+ INSTALL_SYMLINKS="${1#--flag-install-symlinks=}"
+ ;;
+ --flag-no-install-hardlinks=*)
+ NO_INSTALL_HARDLINKS="${1#--flag-no-install-hardlinks=}"
+ ;;
+ --flag-no-cross-directory-hardlinks=*)
+ NO_CROSS_DIRECTORY_HARDLINKS="${1#--flag-no-cross-directory-hardlinks=}"
+ ;;
+ --list-bindir-standalone=*)
+ list_bindir_standalone="${1#--list-bindir-standalone=}"
+ ;;
+ --list-bindir-git-dashed=*)
+ list_bindir_git_dashed="${1#--list-bindir-git-dashed=}"
+ ;;
+ --list-execdir-git-dashed=*)
+ list_execdir_git_dashed="${1#--list-execdir-git-dashed=}"
+ ;;
+ --list-execdir-curl-aliases=*)
+ list_execdir_curl_aliases="${1#--list-execdir-curl-aliases=}"
+ ;;
+
+ *)
+ echo "Unknown option $1"
+ exit 1
+ ;;
+ esac
+ shift
+done &&
+{ test "$bindir/" = "$execdir/" ||
+ for p in $list_bindir_standalone; do
+ $RM "$execdir/$p" &&
+ test -n "$INSTALL_SYMLINKS" &&
+ ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p" ||
+ { test -z "$NO_INSTALL_HARDLINKS$NO_CROSS_DIRECTORY_HARDLINKS" &&
+ ln "$bindir/$p" "$execdir/$p" 2>/dev/null ||
+ cp "$bindir/$p" "$execdir/$p" || exit; }
+ done;
+} &&
+for p in $list_bindir_git_dashed; do
+ $RM "$bindir/$p" &&
+ 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 $list_execdir_git_dashed; do
+ $RM "$execdir/$p" &&
+ test -n "$INSTALL_SYMLINKS" &&
+ ln -s "$destdir_from_execdir/$bindir_relative/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 &&
+for p in $list_execdir_curl_aliases; do
+ $RM "$execdir/$p" &&
+ test -n "$INSTALL_SYMLINKS" &&
+ ln -s "git-remote-http$X" "$execdir/$p" ||
+ { test -z "$NO_INSTALL_HARDLINKS" &&
+ ln "$execdir/git-remote-http$X" "$execdir/$p" 2>/dev/null ||
+ ln -s "git-remote-http$X" "$execdir/$p" 2>/dev/null ||
+ cp "$execdir/git-remote-http$X" "$execdir/$p" || exit; }
+done
--
2.19.1.930.g4563a0d9d0
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper
2018-11-02 22:37 ` [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper Ævar Arnfjörð Bjarmason
@ 2018-11-04 1:09 ` Eric Sunshine
2018-11-12 14:03 ` Johannes Schindelin
1 sibling, 0 replies; 53+ messages in thread
From: Eric Sunshine @ 2018-11-04 1:09 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Dan Jacques, Johannes Schindelin,
Steffen Prohaska, John Keeping, stanhu, richardc, Jeff King
'sb/filenames-with-dashes'On Fri, Nov 2, 2018 at 6:38 PM Ævar Arnfjörð
Bjarmason <avarab@gmail.com> wrote:
> Move a 37 line for-loop mess out of "install" and into a helper
> script. This started out fairly innocent but over the years has grown
> into a hard-to-maintain monster, and my recent ad874608d8 ("Makefile:
> optionally symlink libexec/git-core binaries to bin/git", 2018-03-13)
> certainly didn't help.
>
> The shell code is ported pretty much as-is (with getopts added), it'll
> be fixed & prettified in subsequent commits.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> install_programs | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
Pure nitpick: Earlier this year, Stefan made an effort[1] to eradicate
filenames with underscores and replace them with hyphenated filenames.
Perhaps name this "install-programs", instead.
[1]: sb/filenames-with-dashes
> diff --git a/install_programs b/install_programs
> @@ -0,0 +1,89 @@
> +while test $# != 0
> +do
> + case "$1" in
> + --X=*)
> + X="${1#--X=}"
> + ;;
> + --RM=*)
> + RM="${1#--RM=}"
> + ;;
> + --bindir=*)
> + bindir="${1#--bindir=}"
> + ;;
Is the intention that the user might have X, RM, 'bindir', etc.
already in the environment, and the switches in this script merely
override those values? Or is the intention that X, RM, 'bindir, etc.
should all start out unset? If the latter, perhaps start the script
with an initialization block which clears all these variables first:
X=
RM=
bindir=
...
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper
2018-11-02 22:37 ` [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper Ævar Arnfjörð Bjarmason
2018-11-04 1:09 ` Eric Sunshine
@ 2018-11-12 14:03 ` Johannes Schindelin
2018-11-12 14:42 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 53+ messages in thread
From: Johannes Schindelin @ 2018-11-12 14:03 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Daniel Jacques, Steffen Prohaska,
John Keeping, Stan Hu, Richard Clamp, Jeff King
[-- Attachment #1: Type: text/plain, Size: 8138 bytes --]
Hi,
On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> Move a 37 line for-loop mess out of "install" and into a helper
> script. This started out fairly innocent but over the years has grown
> into a hard-to-maintain monster, and my recent ad874608d8 ("Makefile:
> optionally symlink libexec/git-core binaries to bin/git", 2018-03-13)
> certainly didn't help.
>
> The shell code is ported pretty much as-is (with getopts added), it'll
> be fixed & prettified in subsequent commits.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> Makefile | 52 ++++++++--------------------
> install_programs | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 103 insertions(+), 38 deletions(-)
> create mode 100755 install_programs
>
> diff --git a/Makefile b/Makefile
> index bbfbb4292d..aa6ca1fa68 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2808,44 +2808,20 @@ endif
> bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
> execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
> destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 's|[^/][^/]*|..|g') && \
> - { test "$$bindir/" = "$$execdir/" || \
> - for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
> - $(RM) "$$execdir/$$p" && \
> - test -n "$(INSTALL_SYMLINKS)" && \
> - ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" "$$execdir/$$p" || \
> - { test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
> - ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
> - cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \
> - done; \
> - } && \
> - for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
> - $(RM) "$$bindir/$$p" && \
> - 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" && \
> - 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 \
> - $(RM) "$$execdir/$$p" && \
> - test -n "$(INSTALL_SYMLINKS)" && \
> - ln -s "git-remote-http$X" "$$execdir/$$p" || \
> - { test -z "$(NO_INSTALL_HARDLINKS)" && \
> - ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
> - ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
> - cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
> - done && \
This indeed looks like a mess...
> + ./install_programs \
> + --X="$$X" \
> + --RM="$(RM)" \
> + --bindir="$$bindir" \
> + --bindir-relative="$(bindir_relative_SQ)" \
> + --execdir="$$execdir" \
> + --destdir-from-execdir="$$destdir_from_execdir_SQ" \
> + --flag-install-symlinks="$(INSTALL_SYMLINKS)" \
> + --flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
> + --flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
> + --list-bindir-standalone="git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS))" \
> + --list-bindir-git-dashed="$(filter $(install_bindir_programs),$(BUILT_INS))" \
> + --list-execdir-git-dashed="$(BUILT_INS)" \
> + --list-execdir-curl-aliases="$(REMOTE_CURL_ALIASES)" && \
> ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
>
> .PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf
> diff --git a/install_programs b/install_programs
> new file mode 100755
> index 0000000000..e287108112
> --- /dev/null
> +++ b/install_programs
> @@ -0,0 +1,89 @@
> +#!/bin/sh
> +
> +while test $# != 0
> +do
> + case "$1" in
> + --X=*)
> + X="${1#--X=}"
> + ;;
> + --RM=*)
> + RM="${1#--RM=}"
> + ;;
> + --bindir=*)
> + bindir="${1#--bindir=}"
> + ;;
> + --bindir-relative=*)
> + bindir_relative="${1#--bindir-relative=}"
> + ;;
> + --execdir=*)
> + execdir="${1#--execdir=}"
> + ;;
> + --destdir-from-execdir=*)
> + destdir_from_execdir="${1#--destdir-from-execdir=}"
> + ;;
> + --flag-install-symlinks=*)
> + INSTALL_SYMLINKS="${1#--flag-install-symlinks=}"
> + ;;
> + --flag-no-install-hardlinks=*)
> + NO_INSTALL_HARDLINKS="${1#--flag-no-install-hardlinks=}"
> + ;;
> + --flag-no-cross-directory-hardlinks=*)
> + NO_CROSS_DIRECTORY_HARDLINKS="${1#--flag-no-cross-directory-hardlinks=}"
> + ;;
> + --list-bindir-standalone=*)
> + list_bindir_standalone="${1#--list-bindir-standalone=}"
> + ;;
> + --list-bindir-git-dashed=*)
> + list_bindir_git_dashed="${1#--list-bindir-git-dashed=}"
> + ;;
> + --list-execdir-git-dashed=*)
> + list_execdir_git_dashed="${1#--list-execdir-git-dashed=}"
> + ;;
> + --list-execdir-curl-aliases=*)
> + list_execdir_curl_aliases="${1#--list-execdir-curl-aliases=}"
> + ;;
> +
> + *)
> + echo "Unknown option $1"
> + exit 1
> + ;;
> + esac
> + shift
> +done &&
> +{ test "$bindir/" = "$execdir/" ||
> + for p in $list_bindir_standalone; do
> + $RM "$execdir/$p" &&
> + test -n "$INSTALL_SYMLINKS" &&
> + ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p" ||
> + { test -z "$NO_INSTALL_HARDLINKS$NO_CROSS_DIRECTORY_HARDLINKS" &&
> + ln "$bindir/$p" "$execdir/$p" 2>/dev/null ||
> + cp "$bindir/$p" "$execdir/$p" || exit; }
> + done;
> +} &&
> +for p in $list_bindir_git_dashed; do
> + $RM "$bindir/$p" &&
> + 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 $list_execdir_git_dashed; do
> + $RM "$execdir/$p" &&
> + test -n "$INSTALL_SYMLINKS" &&
> + ln -s "$destdir_from_execdir/$bindir_relative/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 &&
> +for p in $list_execdir_curl_aliases; do
> + $RM "$execdir/$p" &&
> + test -n "$INSTALL_SYMLINKS" &&
> + ln -s "git-remote-http$X" "$execdir/$p" ||
> + { test -z "$NO_INSTALL_HARDLINKS" &&
> + ln "$execdir/git-remote-http$X" "$execdir/$p" 2>/dev/null ||
> + ln -s "git-remote-http$X" "$execdir/$p" 2>/dev/null ||
> + cp "$execdir/git-remote-http$X" "$execdir/$p" || exit; }
> +done
... but so does this. I would be very surprised if these four very
similar-looking constructs could not be refactored into a single shell
script that is then called four times with different parameters.
Something like
#!/bin/sh
from=
while case "$1" in
--no-hardlinks)
NO_INSTALL_HARDLINKS=t
;;
--from=*)
from="${1#*=}"
;;
*)
break
;;
esac; do
shift
done
test $# -gt 3 || {
echo "Usage: $0 [--no-hardlinks] <from-dir> <to-dir> <file>..." >&2
exit 1
}
fromdir="$1"
todir="$2"
shift
shift
for p in "$@"
do
$RM "$todir/$p" &&
test -n "$INSTALL_SYMLINKS" &&
ln -s "$fromdir/${from:-$p}" "$todir/$p" ||
{ test -z "$NO_INSTALL_HARDLINKS" &&
ln "$fromdir/${from:-$p}" "$todir/$p" ||
ln -s "$fromdir/${from:-$p}" "$todir/$p" ||
cp "$fromdir/${from:-$p}" "$todir/$p" || exit; }
done
and then calling it using
test "$bindir/" = "$execdir/" ||
link-or-copy ${NO_CROSS_DIRECTORY_HARDLINKS:+--no-hardlinks} \
"$bindir" "$execdir" $list_bindir_standalone
link-or-copy --from=git$X "$bindir" "$bindir" $list_bindir_git_dashed
link-or-copy --from=git$X "$bindir" "$execdir" $list_bindir_git_dashed
link-or-copy --from=git-remote-http$X "$bindir" "$execdir" $list_execdir_curl_aliases
That would at least DRY up this mess a bit.
Ciao,
Dscho
> --
> 2.19.1.930.g4563a0d9d0
>
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper
2018-11-12 14:03 ` Johannes Schindelin
@ 2018-11-12 14:42 ` Ævar Arnfjörð Bjarmason
2018-11-12 16:32 ` Johannes Schindelin
0 siblings, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-11-12 14:42 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Daniel Jacques, Steffen Prohaska,
John Keeping, Stan Hu, Richard Clamp, Jeff King
On Mon, Nov 12 2018, Johannes Schindelin wrote:
> Hi,
>
> On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> Move a 37 line for-loop mess out of "install" and into a helper
>> script. This started out fairly innocent but over the years has grown
>> into a hard-to-maintain monster, and my recent ad874608d8 ("Makefile:
>> optionally symlink libexec/git-core binaries to bin/git", 2018-03-13)
>> certainly didn't help.
>>
>> The shell code is ported pretty much as-is (with getopts added), it'll
>> be fixed & prettified in subsequent commits.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> Makefile | 52 ++++++++--------------------
>> install_programs | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 103 insertions(+), 38 deletions(-)
>> create mode 100755 install_programs
>>
>> diff --git a/Makefile b/Makefile
>> index bbfbb4292d..aa6ca1fa68 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2808,44 +2808,20 @@ endif
>> bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
>> execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
>> destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 's|[^/][^/]*|..|g') && \
>> - { test "$$bindir/" = "$$execdir/" || \
>> - for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
>> - $(RM) "$$execdir/$$p" && \
>> - test -n "$(INSTALL_SYMLINKS)" && \
>> - ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" "$$execdir/$$p" || \
>> - { test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
>> - ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
>> - cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \
>> - done; \
>> - } && \
>> - for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
>> - $(RM) "$$bindir/$$p" && \
>> - 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" && \
>> - 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 \
>> - $(RM) "$$execdir/$$p" && \
>> - test -n "$(INSTALL_SYMLINKS)" && \
>> - ln -s "git-remote-http$X" "$$execdir/$$p" || \
>> - { test -z "$(NO_INSTALL_HARDLINKS)" && \
>> - ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
>> - ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
>> - cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
>> - done && \
>
> This indeed looks like a mess...
>
>> + ./install_programs \
>> + --X="$$X" \
>> + --RM="$(RM)" \
>> + --bindir="$$bindir" \
>> + --bindir-relative="$(bindir_relative_SQ)" \
>> + --execdir="$$execdir" \
>> + --destdir-from-execdir="$$destdir_from_execdir_SQ" \
>> + --flag-install-symlinks="$(INSTALL_SYMLINKS)" \
>> + --flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
>> + --flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
>> + --list-bindir-standalone="git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS))" \
>> + --list-bindir-git-dashed="$(filter $(install_bindir_programs),$(BUILT_INS))" \
>> + --list-execdir-git-dashed="$(BUILT_INS)" \
>> + --list-execdir-curl-aliases="$(REMOTE_CURL_ALIASES)" && \
>> ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
>>
>> .PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf
>> diff --git a/install_programs b/install_programs
>> new file mode 100755
>> index 0000000000..e287108112
>> --- /dev/null
>> +++ b/install_programs
>> @@ -0,0 +1,89 @@
>> +#!/bin/sh
>> +
>> +while test $# != 0
>> +do
>> + case "$1" in
>> + --X=*)
>> + X="${1#--X=}"
>> + ;;
>> + --RM=*)
>> + RM="${1#--RM=}"
>> + ;;
>> + --bindir=*)
>> + bindir="${1#--bindir=}"
>> + ;;
>> + --bindir-relative=*)
>> + bindir_relative="${1#--bindir-relative=}"
>> + ;;
>> + --execdir=*)
>> + execdir="${1#--execdir=}"
>> + ;;
>> + --destdir-from-execdir=*)
>> + destdir_from_execdir="${1#--destdir-from-execdir=}"
>> + ;;
>> + --flag-install-symlinks=*)
>> + INSTALL_SYMLINKS="${1#--flag-install-symlinks=}"
>> + ;;
>> + --flag-no-install-hardlinks=*)
>> + NO_INSTALL_HARDLINKS="${1#--flag-no-install-hardlinks=}"
>> + ;;
>> + --flag-no-cross-directory-hardlinks=*)
>> + NO_CROSS_DIRECTORY_HARDLINKS="${1#--flag-no-cross-directory-hardlinks=}"
>> + ;;
>> + --list-bindir-standalone=*)
>> + list_bindir_standalone="${1#--list-bindir-standalone=}"
>> + ;;
>> + --list-bindir-git-dashed=*)
>> + list_bindir_git_dashed="${1#--list-bindir-git-dashed=}"
>> + ;;
>> + --list-execdir-git-dashed=*)
>> + list_execdir_git_dashed="${1#--list-execdir-git-dashed=}"
>> + ;;
>> + --list-execdir-curl-aliases=*)
>> + list_execdir_curl_aliases="${1#--list-execdir-curl-aliases=}"
>> + ;;
>> +
>> + *)
>> + echo "Unknown option $1"
>> + exit 1
>> + ;;
>> + esac
>> + shift
>> +done &&
>> +{ test "$bindir/" = "$execdir/" ||
>> + for p in $list_bindir_standalone; do
>> + $RM "$execdir/$p" &&
>> + test -n "$INSTALL_SYMLINKS" &&
>> + ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p" ||
>> + { test -z "$NO_INSTALL_HARDLINKS$NO_CROSS_DIRECTORY_HARDLINKS" &&
>> + ln "$bindir/$p" "$execdir/$p" 2>/dev/null ||
>> + cp "$bindir/$p" "$execdir/$p" || exit; }
>> + done;
>> +} &&
>> +for p in $list_bindir_git_dashed; do
>> + $RM "$bindir/$p" &&
>> + 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 $list_execdir_git_dashed; do
>> + $RM "$execdir/$p" &&
>> + test -n "$INSTALL_SYMLINKS" &&
>> + ln -s "$destdir_from_execdir/$bindir_relative/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 &&
>> +for p in $list_execdir_curl_aliases; do
>> + $RM "$execdir/$p" &&
>> + test -n "$INSTALL_SYMLINKS" &&
>> + ln -s "git-remote-http$X" "$execdir/$p" ||
>> + { test -z "$NO_INSTALL_HARDLINKS" &&
>> + ln "$execdir/git-remote-http$X" "$execdir/$p" 2>/dev/null ||
>> + ln -s "git-remote-http$X" "$execdir/$p" 2>/dev/null ||
>> + cp "$execdir/git-remote-http$X" "$execdir/$p" || exit; }
>> +done
>
> ... but so does this. I would be very surprised if these four very
> similar-looking constructs could not be refactored into a single shell
> script that is then called four times with different parameters.
>
> Something like
>
> #!/bin/sh
>
> from=
> while case "$1" in
> --no-hardlinks)
> NO_INSTALL_HARDLINKS=t
> ;;
> --from=*)
> from="${1#*=}"
> ;;
> *)
> break
> ;;
> esac; do
> shift
> done
>
> test $# -gt 3 || {
> echo "Usage: $0 [--no-hardlinks] <from-dir> <to-dir> <file>..." >&2
> exit 1
> }
>
> fromdir="$1"
> todir="$2"
> shift
> shift
>
> for p in "$@"
> do
> $RM "$todir/$p" &&
> test -n "$INSTALL_SYMLINKS" &&
> ln -s "$fromdir/${from:-$p}" "$todir/$p" ||
> { test -z "$NO_INSTALL_HARDLINKS" &&
> ln "$fromdir/${from:-$p}" "$todir/$p" ||
> ln -s "$fromdir/${from:-$p}" "$todir/$p" ||
> cp "$fromdir/${from:-$p}" "$todir/$p" || exit; }
> done
>
> and then calling it using
>
> test "$bindir/" = "$execdir/" ||
> link-or-copy ${NO_CROSS_DIRECTORY_HARDLINKS:+--no-hardlinks} \
> "$bindir" "$execdir" $list_bindir_standalone
> link-or-copy --from=git$X "$bindir" "$bindir" $list_bindir_git_dashed
> link-or-copy --from=git$X "$bindir" "$execdir" $list_bindir_git_dashed
> link-or-copy --from=git-remote-http$X "$bindir" "$execdir" $list_execdir_curl_aliases
>
> That would at least DRY up this mess a bit.
I'll try for a non-RFC re-submission of this which won't have the 5/5
NO_INSTALL_BUILTIN_EXECDIR_ALIASES (for now) which'll hopefully be ready
for inclusion.
I tried to fold up some of these special cases into one thing before,
but managing the exceptions gets messier to read in my opinion than just
having some duplication.
But more to the point, between your suggestion and Junio's to do all of
this with some make construct: Yeah we should make it awesome, but I
think a logical first step for a patch series like this is to just as-is
use the existing logic we have now with some minor fixes for bugs.
We can refactor this later, I'm mainly interested in moving this over to
a *.sh first so that process is less of a mess, fixing some minor bugs,
and not getting a first iteration of improvements stuck on a much bigger
review for "I rewrote the way the whole install process works" series.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper
2018-11-12 14:42 ` Ævar Arnfjörð Bjarmason
@ 2018-11-12 16:32 ` Johannes Schindelin
2018-11-16 10:32 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 53+ messages in thread
From: Johannes Schindelin @ 2018-11-12 16:32 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Daniel Jacques, Steffen Prohaska,
John Keeping, Stan Hu, Richard Clamp, Jeff King
[-- Attachment #1: Type: text/plain, Size: 10157 bytes --]
Hi Ævar,
On Mon, 12 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> On Mon, Nov 12 2018, Johannes Schindelin wrote:
>
> > On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> Move a 37 line for-loop mess out of "install" and into a helper
> >> script. This started out fairly innocent but over the years has grown
> >> into a hard-to-maintain monster, and my recent ad874608d8 ("Makefile:
> >> optionally symlink libexec/git-core binaries to bin/git", 2018-03-13)
> >> certainly didn't help.
> >>
> >> The shell code is ported pretty much as-is (with getopts added), it'll
> >> be fixed & prettified in subsequent commits.
> >>
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> ---
> >> Makefile | 52 ++++++++--------------------
> >> install_programs | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 103 insertions(+), 38 deletions(-)
> >> create mode 100755 install_programs
> >>
> >> diff --git a/Makefile b/Makefile
> >> index bbfbb4292d..aa6ca1fa68 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -2808,44 +2808,20 @@ endif
> >> bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
> >> execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
> >> destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 's|[^/][^/]*|..|g') && \
> >> - { test "$$bindir/" = "$$execdir/" || \
> >> - for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
> >> - $(RM) "$$execdir/$$p" && \
> >> - test -n "$(INSTALL_SYMLINKS)" && \
> >> - ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" "$$execdir/$$p" || \
> >> - { test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
> >> - ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
> >> - cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \
> >> - done; \
> >> - } && \
> >> - for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
> >> - $(RM) "$$bindir/$$p" && \
> >> - 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" && \
> >> - 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 \
> >> - $(RM) "$$execdir/$$p" && \
> >> - test -n "$(INSTALL_SYMLINKS)" && \
> >> - ln -s "git-remote-http$X" "$$execdir/$$p" || \
> >> - { test -z "$(NO_INSTALL_HARDLINKS)" && \
> >> - ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
> >> - ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
> >> - cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
> >> - done && \
> >
> > This indeed looks like a mess...
> >
> >> + ./install_programs \
> >> + --X="$$X" \
> >> + --RM="$(RM)" \
> >> + --bindir="$$bindir" \
> >> + --bindir-relative="$(bindir_relative_SQ)" \
> >> + --execdir="$$execdir" \
> >> + --destdir-from-execdir="$$destdir_from_execdir_SQ" \
> >> + --flag-install-symlinks="$(INSTALL_SYMLINKS)" \
> >> + --flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
> >> + --flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
> >> + --list-bindir-standalone="git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS))" \
> >> + --list-bindir-git-dashed="$(filter $(install_bindir_programs),$(BUILT_INS))" \
> >> + --list-execdir-git-dashed="$(BUILT_INS)" \
> >> + --list-execdir-curl-aliases="$(REMOTE_CURL_ALIASES)" && \
> >> ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
> >>
> >> .PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf
> >> diff --git a/install_programs b/install_programs
> >> new file mode 100755
> >> index 0000000000..e287108112
> >> --- /dev/null
> >> +++ b/install_programs
> >> @@ -0,0 +1,89 @@
> >> +#!/bin/sh
> >> +
> >> +while test $# != 0
> >> +do
> >> + case "$1" in
> >> + --X=*)
> >> + X="${1#--X=}"
> >> + ;;
> >> + --RM=*)
> >> + RM="${1#--RM=}"
> >> + ;;
> >> + --bindir=*)
> >> + bindir="${1#--bindir=}"
> >> + ;;
> >> + --bindir-relative=*)
> >> + bindir_relative="${1#--bindir-relative=}"
> >> + ;;
> >> + --execdir=*)
> >> + execdir="${1#--execdir=}"
> >> + ;;
> >> + --destdir-from-execdir=*)
> >> + destdir_from_execdir="${1#--destdir-from-execdir=}"
> >> + ;;
> >> + --flag-install-symlinks=*)
> >> + INSTALL_SYMLINKS="${1#--flag-install-symlinks=}"
> >> + ;;
> >> + --flag-no-install-hardlinks=*)
> >> + NO_INSTALL_HARDLINKS="${1#--flag-no-install-hardlinks=}"
> >> + ;;
> >> + --flag-no-cross-directory-hardlinks=*)
> >> + NO_CROSS_DIRECTORY_HARDLINKS="${1#--flag-no-cross-directory-hardlinks=}"
> >> + ;;
> >> + --list-bindir-standalone=*)
> >> + list_bindir_standalone="${1#--list-bindir-standalone=}"
> >> + ;;
> >> + --list-bindir-git-dashed=*)
> >> + list_bindir_git_dashed="${1#--list-bindir-git-dashed=}"
> >> + ;;
> >> + --list-execdir-git-dashed=*)
> >> + list_execdir_git_dashed="${1#--list-execdir-git-dashed=}"
> >> + ;;
> >> + --list-execdir-curl-aliases=*)
> >> + list_execdir_curl_aliases="${1#--list-execdir-curl-aliases=}"
> >> + ;;
> >> +
> >> + *)
> >> + echo "Unknown option $1"
> >> + exit 1
> >> + ;;
> >> + esac
> >> + shift
> >> +done &&
> >> +{ test "$bindir/" = "$execdir/" ||
> >> + for p in $list_bindir_standalone; do
> >> + $RM "$execdir/$p" &&
> >> + test -n "$INSTALL_SYMLINKS" &&
> >> + ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p" ||
> >> + { test -z "$NO_INSTALL_HARDLINKS$NO_CROSS_DIRECTORY_HARDLINKS" &&
> >> + ln "$bindir/$p" "$execdir/$p" 2>/dev/null ||
> >> + cp "$bindir/$p" "$execdir/$p" || exit; }
> >> + done;
> >> +} &&
> >> +for p in $list_bindir_git_dashed; do
> >> + $RM "$bindir/$p" &&
> >> + 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 $list_execdir_git_dashed; do
> >> + $RM "$execdir/$p" &&
> >> + test -n "$INSTALL_SYMLINKS" &&
> >> + ln -s "$destdir_from_execdir/$bindir_relative/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 &&
> >> +for p in $list_execdir_curl_aliases; do
> >> + $RM "$execdir/$p" &&
> >> + test -n "$INSTALL_SYMLINKS" &&
> >> + ln -s "git-remote-http$X" "$execdir/$p" ||
> >> + { test -z "$NO_INSTALL_HARDLINKS" &&
> >> + ln "$execdir/git-remote-http$X" "$execdir/$p" 2>/dev/null ||
> >> + ln -s "git-remote-http$X" "$execdir/$p" 2>/dev/null ||
> >> + cp "$execdir/git-remote-http$X" "$execdir/$p" || exit; }
> >> +done
> >
> > ... but so does this. I would be very surprised if these four very
> > similar-looking constructs could not be refactored into a single shell
> > script that is then called four times with different parameters.
> >
> > Something like
> >
> > #!/bin/sh
> >
> > from=
> > while case "$1" in
> > --no-hardlinks)
> > NO_INSTALL_HARDLINKS=t
> > ;;
> > --from=*)
> > from="${1#*=}"
> > ;;
> > *)
> > break
> > ;;
> > esac; do
> > shift
> > done
> >
> > test $# -gt 3 || {
> > echo "Usage: $0 [--no-hardlinks] <from-dir> <to-dir> <file>..." >&2
> > exit 1
> > }
> >
> > fromdir="$1"
> > todir="$2"
> > shift
> > shift
> >
> > for p in "$@"
> > do
> > $RM "$todir/$p" &&
> > test -n "$INSTALL_SYMLINKS" &&
> > ln -s "$fromdir/${from:-$p}" "$todir/$p" ||
> > { test -z "$NO_INSTALL_HARDLINKS" &&
> > ln "$fromdir/${from:-$p}" "$todir/$p" ||
> > ln -s "$fromdir/${from:-$p}" "$todir/$p" ||
> > cp "$fromdir/${from:-$p}" "$todir/$p" || exit; }
> > done
> >
> > and then calling it using
> >
> > test "$bindir/" = "$execdir/" ||
> > link-or-copy ${NO_CROSS_DIRECTORY_HARDLINKS:+--no-hardlinks} \
> > "$bindir" "$execdir" $list_bindir_standalone
> > link-or-copy --from=git$X "$bindir" "$bindir" $list_bindir_git_dashed
> > link-or-copy --from=git$X "$bindir" "$execdir" $list_bindir_git_dashed
> > link-or-copy --from=git-remote-http$X "$bindir" "$execdir" $list_execdir_curl_aliases
> >
> > That would at least DRY up this mess a bit.
>
> I'll try for a non-RFC re-submission of this which won't have the 5/5
> NO_INSTALL_BUILTIN_EXECDIR_ALIASES (for now) which'll hopefully be ready
> for inclusion.
>
> I tried to fold up some of these special cases into one thing before,
> but managing the exceptions gets messier to read in my opinion than just
> having some duplication.
>
> But more to the point, between your suggestion and Junio's to do all of
> this with some make construct: Yeah we should make it awesome, but I
> think a logical first step for a patch series like this is to just as-is
> use the existing logic we have now with some minor fixes for bugs.
>
> We can refactor this later, I'm mainly interested in moving this over to
> a *.sh first so that process is less of a mess, fixing some minor bugs,
> and not getting a first iteration of improvements stuck on a much bigger
> review for "I rewrote the way the whole install process works" series.
Did you seen any mistake in my version? Because if you didn't, then it
would be unfortunate to leave this technical debt unaddressed. Just moving
the mess really does nothing to address it, and my version should be
almost there to actually do address the mess.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper
2018-11-12 16:32 ` Johannes Schindelin
@ 2018-11-16 10:32 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-11-16 10:32 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Daniel Jacques, Steffen Prohaska,
John Keeping, Stan Hu, Richard Clamp, Jeff King
On Mon, Nov 12 2018, Johannes Schindelin wrote:
> Hi Ævar,
>
> On Mon, 12 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> On Mon, Nov 12 2018, Johannes Schindelin wrote:
>>
>> > On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> Move a 37 line for-loop mess out of "install" and into a helper
>> >> script. This started out fairly innocent but over the years has grown
>> >> into a hard-to-maintain monster, and my recent ad874608d8 ("Makefile:
>> >> optionally symlink libexec/git-core binaries to bin/git", 2018-03-13)
>> >> certainly didn't help.
>> >>
>> >> The shell code is ported pretty much as-is (with getopts added), it'll
>> >> be fixed & prettified in subsequent commits.
>> >>
>> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> >> ---
>> >> Makefile | 52 ++++++++--------------------
>> >> install_programs | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
>> >> 2 files changed, 103 insertions(+), 38 deletions(-)
>> >> create mode 100755 install_programs
>> >>
>> >> diff --git a/Makefile b/Makefile
>> >> index bbfbb4292d..aa6ca1fa68 100644
>> >> --- a/Makefile
>> >> +++ b/Makefile
>> >> @@ -2808,44 +2808,20 @@ endif
>> >> bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
>> >> execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
>> >> destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 's|[^/][^/]*|..|g') && \
>> >> - { test "$$bindir/" = "$$execdir/" || \
>> >> - for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
>> >> - $(RM) "$$execdir/$$p" && \
>> >> - test -n "$(INSTALL_SYMLINKS)" && \
>> >> - ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" "$$execdir/$$p" || \
>> >> - { test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
>> >> - ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
>> >> - cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \
>> >> - done; \
>> >> - } && \
>> >> - for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
>> >> - $(RM) "$$bindir/$$p" && \
>> >> - 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" && \
>> >> - 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 \
>> >> - $(RM) "$$execdir/$$p" && \
>> >> - test -n "$(INSTALL_SYMLINKS)" && \
>> >> - ln -s "git-remote-http$X" "$$execdir/$$p" || \
>> >> - { test -z "$(NO_INSTALL_HARDLINKS)" && \
>> >> - ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
>> >> - ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
>> >> - cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
>> >> - done && \
>> >
>> > This indeed looks like a mess...
>> >
>> >> + ./install_programs \
>> >> + --X="$$X" \
>> >> + --RM="$(RM)" \
>> >> + --bindir="$$bindir" \
>> >> + --bindir-relative="$(bindir_relative_SQ)" \
>> >> + --execdir="$$execdir" \
>> >> + --destdir-from-execdir="$$destdir_from_execdir_SQ" \
>> >> + --flag-install-symlinks="$(INSTALL_SYMLINKS)" \
>> >> + --flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
>> >> + --flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
>> >> + --list-bindir-standalone="git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS))" \
>> >> + --list-bindir-git-dashed="$(filter $(install_bindir_programs),$(BUILT_INS))" \
>> >> + --list-execdir-git-dashed="$(BUILT_INS)" \
>> >> + --list-execdir-curl-aliases="$(REMOTE_CURL_ALIASES)" && \
>> >> ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
>> >>
>> >> .PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf
>> >> diff --git a/install_programs b/install_programs
>> >> new file mode 100755
>> >> index 0000000000..e287108112
>> >> --- /dev/null
>> >> +++ b/install_programs
>> >> @@ -0,0 +1,89 @@
>> >> +#!/bin/sh
>> >> +
>> >> +while test $# != 0
>> >> +do
>> >> + case "$1" in
>> >> + --X=*)
>> >> + X="${1#--X=}"
>> >> + ;;
>> >> + --RM=*)
>> >> + RM="${1#--RM=}"
>> >> + ;;
>> >> + --bindir=*)
>> >> + bindir="${1#--bindir=}"
>> >> + ;;
>> >> + --bindir-relative=*)
>> >> + bindir_relative="${1#--bindir-relative=}"
>> >> + ;;
>> >> + --execdir=*)
>> >> + execdir="${1#--execdir=}"
>> >> + ;;
>> >> + --destdir-from-execdir=*)
>> >> + destdir_from_execdir="${1#--destdir-from-execdir=}"
>> >> + ;;
>> >> + --flag-install-symlinks=*)
>> >> + INSTALL_SYMLINKS="${1#--flag-install-symlinks=}"
>> >> + ;;
>> >> + --flag-no-install-hardlinks=*)
>> >> + NO_INSTALL_HARDLINKS="${1#--flag-no-install-hardlinks=}"
>> >> + ;;
>> >> + --flag-no-cross-directory-hardlinks=*)
>> >> + NO_CROSS_DIRECTORY_HARDLINKS="${1#--flag-no-cross-directory-hardlinks=}"
>> >> + ;;
>> >> + --list-bindir-standalone=*)
>> >> + list_bindir_standalone="${1#--list-bindir-standalone=}"
>> >> + ;;
>> >> + --list-bindir-git-dashed=*)
>> >> + list_bindir_git_dashed="${1#--list-bindir-git-dashed=}"
>> >> + ;;
>> >> + --list-execdir-git-dashed=*)
>> >> + list_execdir_git_dashed="${1#--list-execdir-git-dashed=}"
>> >> + ;;
>> >> + --list-execdir-curl-aliases=*)
>> >> + list_execdir_curl_aliases="${1#--list-execdir-curl-aliases=}"
>> >> + ;;
>> >> +
>> >> + *)
>> >> + echo "Unknown option $1"
>> >> + exit 1
>> >> + ;;
>> >> + esac
>> >> + shift
>> >> +done &&
>> >> +{ test "$bindir/" = "$execdir/" ||
>> >> + for p in $list_bindir_standalone; do
>> >> + $RM "$execdir/$p" &&
>> >> + test -n "$INSTALL_SYMLINKS" &&
>> >> + ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p" ||
>> >> + { test -z "$NO_INSTALL_HARDLINKS$NO_CROSS_DIRECTORY_HARDLINKS" &&
>> >> + ln "$bindir/$p" "$execdir/$p" 2>/dev/null ||
>> >> + cp "$bindir/$p" "$execdir/$p" || exit; }
>> >> + done;
>> >> +} &&
>> >> +for p in $list_bindir_git_dashed; do
>> >> + $RM "$bindir/$p" &&
>> >> + 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 $list_execdir_git_dashed; do
>> >> + $RM "$execdir/$p" &&
>> >> + test -n "$INSTALL_SYMLINKS" &&
>> >> + ln -s "$destdir_from_execdir/$bindir_relative/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 &&
>> >> +for p in $list_execdir_curl_aliases; do
>> >> + $RM "$execdir/$p" &&
>> >> + test -n "$INSTALL_SYMLINKS" &&
>> >> + ln -s "git-remote-http$X" "$execdir/$p" ||
>> >> + { test -z "$NO_INSTALL_HARDLINKS" &&
>> >> + ln "$execdir/git-remote-http$X" "$execdir/$p" 2>/dev/null ||
>> >> + ln -s "git-remote-http$X" "$execdir/$p" 2>/dev/null ||
>> >> + cp "$execdir/git-remote-http$X" "$execdir/$p" || exit; }
>> >> +done
>> >
>> > ... but so does this. I would be very surprised if these four very
>> > similar-looking constructs could not be refactored into a single shell
>> > script that is then called four times with different parameters.
>> >
>> > Something like
>> >
>> > #!/bin/sh
>> >
>> > from=
>> > while case "$1" in
>> > --no-hardlinks)
>> > NO_INSTALL_HARDLINKS=t
>> > ;;
>> > --from=*)
>> > from="${1#*=}"
>> > ;;
>> > *)
>> > break
>> > ;;
>> > esac; do
>> > shift
>> > done
>> >
>> > test $# -gt 3 || {
>> > echo "Usage: $0 [--no-hardlinks] <from-dir> <to-dir> <file>..." >&2
>> > exit 1
>> > }
>> >
>> > fromdir="$1"
>> > todir="$2"
>> > shift
>> > shift
>> >
>> > for p in "$@"
>> > do
>> > $RM "$todir/$p" &&
>> > test -n "$INSTALL_SYMLINKS" &&
>> > ln -s "$fromdir/${from:-$p}" "$todir/$p" ||
>> > { test -z "$NO_INSTALL_HARDLINKS" &&
>> > ln "$fromdir/${from:-$p}" "$todir/$p" ||
>> > ln -s "$fromdir/${from:-$p}" "$todir/$p" ||
>> > cp "$fromdir/${from:-$p}" "$todir/$p" || exit; }
>> > done
>> >
>> > and then calling it using
>> >
>> > test "$bindir/" = "$execdir/" ||
>> > link-or-copy ${NO_CROSS_DIRECTORY_HARDLINKS:+--no-hardlinks} \
>> > "$bindir" "$execdir" $list_bindir_standalone
>> > link-or-copy --from=git$X "$bindir" "$bindir" $list_bindir_git_dashed
>> > link-or-copy --from=git$X "$bindir" "$execdir" $list_bindir_git_dashed
>> > link-or-copy --from=git-remote-http$X "$bindir" "$execdir" $list_execdir_curl_aliases
>> >
>> > That would at least DRY up this mess a bit.
>>
>> I'll try for a non-RFC re-submission of this which won't have the 5/5
>> NO_INSTALL_BUILTIN_EXECDIR_ALIASES (for now) which'll hopefully be ready
>> for inclusion.
>>
>> I tried to fold up some of these special cases into one thing before,
>> but managing the exceptions gets messier to read in my opinion than just
>> having some duplication.
>>
>> But more to the point, between your suggestion and Junio's to do all of
>> this with some make construct: Yeah we should make it awesome, but I
>> think a logical first step for a patch series like this is to just as-is
>> use the existing logic we have now with some minor fixes for bugs.
>>
>> We can refactor this later, I'm mainly interested in moving this over to
>> a *.sh first so that process is less of a mess, fixing some minor bugs,
>> and not getting a first iteration of improvements stuck on a much bigger
>> review for "I rewrote the way the whole install process works" series.
>
> Did you seen any mistake in my version? Because if you didn't, then it
> would be unfortunate to leave this technical debt unaddressed. Just moving
> the mess really does nothing to address it, and my version should be
> almost there to actually do address the mess.
It's not that I found some bug, and I'm not saying it should be left
unaddressed. Just pointing out that saying "let's make this even better
while we're at it" can make perfect the enemy of the good, particularly
in this case since we have no tests for this build procedure and there's
a myriad of option combinations that need to be tested.
^ permalink raw reply [flat|nested] 53+ messages in thread
* [RFC/PATCH 2/5] Makefile: conform some of the code to our coding standards
2018-03-16 12:43 ` Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2018-11-02 22:37 ` [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper Ævar Arnfjörð Bjarmason
@ 2018-11-02 22:37 ` Ævar Arnfjörð Bjarmason
2018-11-02 22:37 ` [RFC/PATCH 3/5] Makefile: stop hiding failures during "install" Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
6 siblings, 0 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-11-02 22:37 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Daniel Jacques, Johannes Schindelin,
Steffen Prohaska, John Keeping, Stan Hu, Richard Clamp, Jeff King,
Ævar Arnfjörð Bjarmason
This code is still very much unlike our usual style since it was
lifted from the Makefile, but we can at least make some of it use the
usual style and line spacing.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
install_programs | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/install_programs b/install_programs
index e287108112..d3333cd25f 100755
--- a/install_programs
+++ b/install_programs
@@ -50,17 +50,21 @@ do
esac
shift
done &&
-{ test "$bindir/" = "$execdir/" ||
- for p in $list_bindir_standalone; do
- $RM "$execdir/$p" &&
- test -n "$INSTALL_SYMLINKS" &&
- ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p" ||
- { test -z "$NO_INSTALL_HARDLINKS$NO_CROSS_DIRECTORY_HARDLINKS" &&
- ln "$bindir/$p" "$execdir/$p" 2>/dev/null ||
- cp "$bindir/$p" "$execdir/$p" || exit; }
- done;
-} &&
-for p in $list_bindir_git_dashed; do
+
+if test "$bindir/" != "$execdir/"
+then
+ for p in $list_bindir_standalone; do
+ $RM "$execdir/$p" &&
+ test -n "$INSTALL_SYMLINKS" &&
+ ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p" ||
+ { test -z "$NO_INSTALL_HARDLINKS$NO_CROSS_DIRECTORY_HARDLINKS" &&
+ ln "$bindir/$p" "$execdir/$p" 2>/dev/null ||
+ cp "$bindir/$p" "$execdir/$p" || exit; }
+ done
+fi &&
+
+for p in $list_bindir_git_dashed
+do
$RM "$bindir/$p" &&
test -n "$INSTALL_SYMLINKS" &&
ln -s "git$X" "$bindir/$p" ||
@@ -69,6 +73,7 @@ for p in $list_bindir_git_dashed; do
ln -s "git$X" "$bindir/$p" 2>/dev/null ||
cp "$bindir/git$X" "$bindir/$p" || exit; }
done &&
+
for p in $list_execdir_git_dashed; do
$RM "$execdir/$p" &&
test -n "$INSTALL_SYMLINKS" &&
@@ -78,6 +83,7 @@ for p in $list_execdir_git_dashed; do
ln -s "git$X" "$execdir/$p" 2>/dev/null ||
cp "$execdir/git$X" "$execdir/$p" || exit; }
done &&
+
for p in $list_execdir_curl_aliases; do
$RM "$execdir/$p" &&
test -n "$INSTALL_SYMLINKS" &&
--
2.19.1.930.g4563a0d9d0
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [RFC/PATCH 3/5] Makefile: stop hiding failures during "install"
2018-03-16 12:43 ` Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2018-11-02 22:37 ` [RFC/PATCH 2/5] Makefile: conform some of the code to our coding standards Ævar Arnfjörð Bjarmason
@ 2018-11-02 22:37 ` Ævar Arnfjörð Bjarmason
2018-11-02 22:37 ` [RFC/PATCH 4/5] Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch Ævar Arnfjörð Bjarmason
2018-11-02 22:37 ` [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag Ævar Arnfjörð Bjarmason
6 siblings, 0 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-11-02 22:37 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Daniel Jacques, Johannes Schindelin,
Steffen Prohaska, John Keeping, Stan Hu, Richard Clamp, Jeff King,
Ævar Arnfjörð Bjarmason
Change the fallback mechanism where we try to create hardlinks and
ultimately fall back on a plain copy to emit the errors it encounters
instead of hiding them away and silently falling back to copying.
Hiding these errors dates back to 3e073dc561 ("Makefile: always
provide a fallback when hardlinks fail", 2008-08-25) when the existing
"hardlink or copy" logic was amended to hide the errors.
At that time "make install" hadn't yet been taught any of the
NO_*_HARDLINK options, that happened later in 3426e34fed ("Add
NO_CROSS_DIRECTORY_HARDLINKS support to the Makefile", 2009-05-11) and
was finally finished to roughly the current form in
70de5e65e8 ("Makefile: NO_INSTALL_HARDLINKS", 2012-05-02).
If someone is building a git in an environment where hard linking
fails, they can now specify some combination of
NO_INSTALL_HARDLINKS=YesPlease and NO_INSTALL_HARDLINKS=YesPlease, it
doesn't make sense anymore to not only implicitly fall back to
copying, but to do so silently.
This change leaves no way to not get errors spewed if we're trying and
failing to e.g. make symlinks and having to fall back on "cp". I think
that's OK.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
install_programs | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/install_programs b/install_programs
index d3333cd25f..367e9a6cdf 100755
--- a/install_programs
+++ b/install_programs
@@ -58,7 +58,7 @@ then
test -n "$INSTALL_SYMLINKS" &&
ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p" ||
{ test -z "$NO_INSTALL_HARDLINKS$NO_CROSS_DIRECTORY_HARDLINKS" &&
- ln "$bindir/$p" "$execdir/$p" 2>/dev/null ||
+ ln "$bindir/$p" "$execdir/$p" ||
cp "$bindir/$p" "$execdir/$p" || exit; }
done
fi &&
@@ -69,8 +69,8 @@ do
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 ||
+ ln "$bindir/git$X" "$bindir/$p" ||
+ ln -s "git$X" "$bindir/$p" ||
cp "$bindir/git$X" "$bindir/$p" || exit; }
done &&
@@ -79,8 +79,8 @@ for p in $list_execdir_git_dashed; do
test -n "$INSTALL_SYMLINKS" &&
ln -s "$destdir_from_execdir/$bindir_relative/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 ||
+ ln "$execdir/git$X" "$execdir/$p" ||
+ ln -s "git$X" "$execdir/$p" ||
cp "$execdir/git$X" "$execdir/$p" || exit; }
done &&
@@ -89,7 +89,7 @@ for p in $list_execdir_curl_aliases; do
test -n "$INSTALL_SYMLINKS" &&
ln -s "git-remote-http$X" "$execdir/$p" ||
{ test -z "$NO_INSTALL_HARDLINKS" &&
- ln "$execdir/git-remote-http$X" "$execdir/$p" 2>/dev/null ||
- ln -s "git-remote-http$X" "$execdir/$p" 2>/dev/null ||
+ ln "$execdir/git-remote-http$X" "$execdir/$p" ||
+ ln -s "git-remote-http$X" "$execdir/$p" ||
cp "$execdir/git-remote-http$X" "$execdir/$p" || exit; }
done
--
2.19.1.930.g4563a0d9d0
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [RFC/PATCH 4/5] Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch
2018-03-16 12:43 ` Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2018-11-02 22:37 ` [RFC/PATCH 3/5] Makefile: stop hiding failures during "install" Ævar Arnfjörð Bjarmason
@ 2018-11-02 22:37 ` Ævar Arnfjörð Bjarmason
2018-11-04 1:01 ` Eric Sunshine
2018-11-02 22:37 ` [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag Ævar Arnfjörð Bjarmason
6 siblings, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-11-02 22:37 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Daniel Jacques, Johannes Schindelin,
Steffen Prohaska, John Keeping, Stan Hu, Richard Clamp, Jeff King,
Ævar Arnfjörð Bjarmason
Add a switch for use in conjunction with the INSTALL_SYMLINKS flag
added in ad874608d8 ("Makefile: optionally symlink libexec/git-core
binaries to bin/git", 2018-03-13).
Now it's possible to install git with:
INSTALL_SYMLINKS=YesPlease NO_INSTALL_SYMLINKS_FALLBACK=YesPlease
And know for sure that there's not going to be any silent fallbacks on
hardlinks or copying of files if symlinking fails.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Makefile | 5 ++++
install_programs | 69 ++++++++++++++++++++++++++++++++----------------
2 files changed, 51 insertions(+), 23 deletions(-)
diff --git a/Makefile b/Makefile
index aa6ca1fa68..07c8b74353 100644
--- a/Makefile
+++ b/Makefile
@@ -342,6 +342,10 @@ all::
# within the same directory in some cases, INSTALL_SYMLINKS will
# always symlink to the final target directly.
#
+# Define NO_INSTALL_SYMLINKS_FALLBACK if in conjunction with
+# INSTALL_SYMLINKS if you'd prefer not to have the install procedure
+# fallack on hardlinking or copying if "ln -s" fails.
+#
# Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
# programs as a tar, where bin/ and libexec/ might be on different file systems.
#
@@ -2818,6 +2822,7 @@ endif
--flag-install-symlinks="$(INSTALL_SYMLINKS)" \
--flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
--flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
+ --flag-no-install-symlinks-fallback="$(NO_INSTALL_SYMLINKS_FALLBACK)" \
--list-bindir-standalone="git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS))" \
--list-bindir-git-dashed="$(filter $(install_bindir_programs),$(BUILT_INS))" \
--list-execdir-git-dashed="$(BUILT_INS)" \
diff --git a/install_programs b/install_programs
index 367e9a6cdf..51e08019dd 100755
--- a/install_programs
+++ b/install_programs
@@ -30,6 +30,9 @@ do
--flag-no-cross-directory-hardlinks=*)
NO_CROSS_DIRECTORY_HARDLINKS="${1#--flag-no-cross-directory-hardlinks=}"
;;
+ --flag-no-install-symlinks-fallback=*)
+ NO_INSTALL_SYMLINKS_FALLBACK="${1#--flag-no-install-symlinks-fallback=}"
+ ;;
--list-bindir-standalone=*)
list_bindir_standalone="${1#--list-bindir-standalone=}"
;;
@@ -55,41 +58,61 @@ if test "$bindir/" != "$execdir/"
then
for p in $list_bindir_standalone; do
$RM "$execdir/$p" &&
- test -n "$INSTALL_SYMLINKS" &&
- ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p" ||
- { test -z "$NO_INSTALL_HARDLINKS$NO_CROSS_DIRECTORY_HARDLINKS" &&
- ln "$bindir/$p" "$execdir/$p" ||
- cp "$bindir/$p" "$execdir/$p" || exit; }
+ if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
+ then
+ ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p"
+ else
+ test -n "$INSTALL_SYMLINKS" &&
+ ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p" ||
+ { test -z "$NO_INSTALL_HARDLINKS$NO_CROSS_DIRECTORY_HARDLINKS" &&
+ ln "$bindir/$p" "$execdir/$p" ||
+ cp "$bindir/$p" "$execdir/$p" || exit; }
+ fi
done
fi &&
for p in $list_bindir_git_dashed
do
$RM "$bindir/$p" &&
- test -n "$INSTALL_SYMLINKS" &&
- ln -s "git$X" "$bindir/$p" ||
- { test -z "$NO_INSTALL_HARDLINKS" &&
- ln "$bindir/git$X" "$bindir/$p" ||
- ln -s "git$X" "$bindir/$p" ||
- cp "$bindir/git$X" "$bindir/$p" || exit; }
+ if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
+ then
+ ln -s "git$X" "$bindir/$p"
+ else
+ test -n "$INSTALL_SYMLINKS" &&
+ ln -s "git$X" "$bindir/$p" ||
+ { test -z "$NO_INSTALL_HARDLINKS" &&
+ ln "$bindir/git$X" "$bindir/$p" ||
+ ln -s "git$X" "$bindir/$p" ||
+ cp "$bindir/git$X" "$bindir/$p" || exit; }
+ fi
done &&
for p in $list_execdir_git_dashed; do
$RM "$execdir/$p" &&
- test -n "$INSTALL_SYMLINKS" &&
- ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p" ||
- { test -z "$NO_INSTALL_HARDLINKS" &&
- ln "$execdir/git$X" "$execdir/$p" ||
- ln -s "git$X" "$execdir/$p" ||
- cp "$execdir/git$X" "$execdir/$p" || exit; }
+ if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
+ then
+ ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p"
+ else
+ test -n "$INSTALL_SYMLINKS" &&
+ ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p" ||
+ { test -z "$NO_INSTALL_HARDLINKS" &&
+ ln "$execdir/git$X" "$execdir/$p" ||
+ ln -s "git$X" "$execdir/$p" ||
+ cp "$execdir/git$X" "$execdir/$p" || exit; }
+ fi
done &&
for p in $list_execdir_curl_aliases; do
$RM "$execdir/$p" &&
- test -n "$INSTALL_SYMLINKS" &&
- ln -s "git-remote-http$X" "$execdir/$p" ||
- { test -z "$NO_INSTALL_HARDLINKS" &&
- ln "$execdir/git-remote-http$X" "$execdir/$p" ||
- ln -s "git-remote-http$X" "$execdir/$p" ||
- cp "$execdir/git-remote-http$X" "$execdir/$p" || exit; }
+ if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
+ then
+ ln -s "git-remote-http$X" "$execdir/$p"
+ else
+ test -n "$INSTALL_SYMLINKS" &&
+ ln -s "git-remote-http$X" "$execdir/$p" ||
+ { test -z "$NO_INSTALL_HARDLINKS" &&
+ ln "$execdir/git-remote-http$X" "$execdir/$p" ||
+ ln -s "git-remote-http$X" "$execdir/$p" ||
+ cp "$execdir/git-remote-http$X" "$execdir/$p" || exit; }
+ fi
done
--
2.19.1.930.g4563a0d9d0
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 4/5] Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch
2018-11-02 22:37 ` [RFC/PATCH 4/5] Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch Ævar Arnfjörð Bjarmason
@ 2018-11-04 1:01 ` Eric Sunshine
0 siblings, 0 replies; 53+ messages in thread
From: Eric Sunshine @ 2018-11-04 1:01 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Dan Jacques, Johannes Schindelin,
Steffen Prohaska, John Keeping, stanhu, richardc, Jeff King
On Fri, Nov 2, 2018 at 6:38 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Add a switch for use in conjunction with the INSTALL_SYMLINKS flag
> added in ad874608d8 ("Makefile: optionally symlink libexec/git-core
> binaries to bin/git", 2018-03-13).
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/Makefile b/Makefile
> @@ -342,6 +342,10 @@ all::
> +# Define NO_INSTALL_SYMLINKS_FALLBACK if in conjunction with
s/if in/in/
> +# INSTALL_SYMLINKS if you'd prefer not to have the install procedure
> +# fallack on hardlinking or copying if "ln -s" fails.
> +#
^ permalink raw reply [flat|nested] 53+ messages in thread
* [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
2018-03-16 12:43 ` Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2018-11-02 22:37 ` [RFC/PATCH 4/5] Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch Ævar Arnfjörð Bjarmason
@ 2018-11-02 22:37 ` Ævar Arnfjörð Bjarmason
2018-11-04 1:04 ` Eric Sunshine
2018-11-12 14:14 ` Johannes Schindelin
6 siblings, 2 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-11-02 22:37 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Daniel Jacques, Johannes Schindelin,
Steffen Prohaska, John Keeping, Stan Hu, Richard Clamp, Jeff King,
Ævar Arnfjörð Bjarmason
Back when git was initially written the likes of "git-add", "git-init"
etc. were installed in the user's $PATH. A few years later everything,
with a few exceptions like git-upload-pack and git-receive-pack, was
expected to be invoked as "git $cmd".
Now something like a decade later we're still installing these old
commands in gitexecdir. This is so someone with a shellscript that
still targets e.g. "git-init" can add $(git --exec-path) to their
$PATH and not have to change their script.
Let's add an option to break this backwards compatibility. Now with
NO_INSTALL_BUILTIN_EXECDIR_ALIASES=YesPlease there's only 3 programs
in the bindir that are hardlinked to "git" (receive-pack,
upload-archive & upload-pack), and 3 in the
gitexecdir (git-remote-{ftp,ftps,https} linked to git-remote-http).
There's no cross-directory links anymore, so the
"NO_CROSS_DIRECTORY_HARDLINKS" flag becomes redundant under this new
option.
1. https://public-inbox.org/git/87woyfdkoi.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Makefile | 8 ++++++++
install_programs | 36 +++++++++++++++++++++---------------
2 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/Makefile b/Makefile
index 07c8b74353..a849a7b6d1 100644
--- a/Makefile
+++ b/Makefile
@@ -346,6 +346,13 @@ all::
# INSTALL_SYMLINKS if you'd prefer not to have the install procedure
# fallack on hardlinking or copying if "ln -s" fails.
#
+# Define NO_INSTALL_BUILTIN_EXECDIR_ALIASES if you'd like to skip
+# installing legacy such as "git-init" and "git-add" in the
+# gitexecdir. Unless you're on a system where "which git-init" is
+# expected to returns something set this. Users have been expected to
+# use the likes of "git init" for ages now, these programs were only
+# provided for legacy compatibility.
+#
# Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
# programs as a tar, where bin/ and libexec/ might be on different file systems.
#
@@ -2823,6 +2830,7 @@ endif
--flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
--flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
--flag-no-install-symlinks-fallback="$(NO_INSTALL_SYMLINKS_FALLBACK)" \
+ --flag-no-install-builtin-execdir-aliases="$(NO_INSTALL_BUILTIN_EXECDIR_ALIASES)" \
--list-bindir-standalone="git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS))" \
--list-bindir-git-dashed="$(filter $(install_bindir_programs),$(BUILT_INS))" \
--list-execdir-git-dashed="$(BUILT_INS)" \
diff --git a/install_programs b/install_programs
index 51e08019dd..8d89cd9984 100755
--- a/install_programs
+++ b/install_programs
@@ -33,6 +33,9 @@ do
--flag-no-install-symlinks-fallback=*)
NO_INSTALL_SYMLINKS_FALLBACK="${1#--flag-no-install-symlinks-fallback=}"
;;
+ --flag-no-install-builtin-execdir-aliases=*)
+ NO_INSTALL_BUILTIN_EXECDIR_ALIASES="${1#--flag-no-install-builtin-execdir-aliases=}"
+ ;;
--list-bindir-standalone=*)
list_bindir_standalone="${1#--list-bindir-standalone=}"
;;
@@ -54,7 +57,7 @@ do
shift
done &&
-if test "$bindir/" != "$execdir/"
+if test "$bindir/" != "$execdir/" -a -z "$NO_INSTALL_BUILTIN_EXECDIR_ALIASES"
then
for p in $list_bindir_standalone; do
$RM "$execdir/$p" &&
@@ -87,20 +90,23 @@ do
fi
done &&
-for p in $list_execdir_git_dashed; do
- $RM "$execdir/$p" &&
- if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
- then
- ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p"
- else
- test -n "$INSTALL_SYMLINKS" &&
- ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p" ||
- { test -z "$NO_INSTALL_HARDLINKS" &&
- ln "$execdir/git$X" "$execdir/$p" ||
- ln -s "git$X" "$execdir/$p" ||
- cp "$execdir/git$X" "$execdir/$p" || exit; }
- fi
-done &&
+if test -z "$NO_INSTALL_BUILTIN_EXECDIR_ALIASES"
+then
+ for p in $list_execdir_git_dashed; do
+ $RM "$execdir/$p" &&
+ if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
+ then
+ ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p"
+ else
+ test -n "$INSTALL_SYMLINKS" &&
+ ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p" ||
+ { test -z "$NO_INSTALL_HARDLINKS" &&
+ ln "$execdir/git$X" "$execdir/$p" ||
+ ln -s "git$X" "$execdir/$p" ||
+ cp "$execdir/git$X" "$execdir/$p" || exit; }
+ fi
+ done
+fi &&
for p in $list_execdir_curl_aliases; do
$RM "$execdir/$p" &&
--
2.19.1.930.g4563a0d9d0
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
2018-11-02 22:37 ` [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag Ævar Arnfjörð Bjarmason
@ 2018-11-04 1:04 ` Eric Sunshine
2018-11-12 14:14 ` Johannes Schindelin
1 sibling, 0 replies; 53+ messages in thread
From: Eric Sunshine @ 2018-11-04 1:04 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Dan Jacques, Johannes Schindelin,
Steffen Prohaska, John Keeping, stanhu, richardc, Jeff King
On Fri, Nov 2, 2018 at 6:38 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Let's add an option to break this backwards compatibility. Now with
> NO_INSTALL_BUILTIN_EXECDIR_ALIASES=YesPlease there's only 3 programs
> in the bindir that are hardlinked to "git" (receive-pack,
> upload-archive & upload-pack), and 3 in the
> gitexecdir (git-remote-{ftp,ftps,https} linked to git-remote-http).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/Makefile b/Makefile
> @@ -346,6 +346,13 @@ all::
> +# Define NO_INSTALL_BUILTIN_EXECDIR_ALIASES if you'd like to skip
> +# installing legacy such as "git-init" and "git-add" in the
> +# gitexecdir. Unless you're on a system where "which git-init" is
> +# expected to returns something set this. Users have been expected to
s/returns/return/
s/something/&,/
Although, it's not clear what "return something" means. Perhaps rephrase it as:
...git-init is expected to exist, set this.
> +# use the likes of "git init" for ages now, these programs were only
> +# provided for legacy compatibility.
> +#
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
2018-11-02 22:37 ` [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag Ævar Arnfjörð Bjarmason
2018-11-04 1:04 ` Eric Sunshine
@ 2018-11-12 14:14 ` Johannes Schindelin
1 sibling, 0 replies; 53+ messages in thread
From: Johannes Schindelin @ 2018-11-12 14:14 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Daniel Jacques, Steffen Prohaska,
John Keeping, Stan Hu, Richard Clamp, Jeff King
[-- Attachment #1: Type: text/plain, Size: 5119 bytes --]
Hi Ævar,
On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> Back when git was initially written the likes of "git-add", "git-init"
> etc. were installed in the user's $PATH. A few years later everything,
> with a few exceptions like git-upload-pack and git-receive-pack, was
> expected to be invoked as "git $cmd".
>
> Now something like a decade later we're still installing these old
> commands in gitexecdir. This is so someone with a shellscript that
> still targets e.g. "git-init" can add $(git --exec-path) to their
> $PATH and not have to change their script.
>
> Let's add an option to break this backwards compatibility. Now with
> NO_INSTALL_BUILTIN_EXECDIR_ALIASES=YesPlease there's only 3 programs
> in the bindir that are hardlinked to "git" (receive-pack,
> upload-archive & upload-pack), and 3 in the
> gitexecdir (git-remote-{ftp,ftps,https} linked to git-remote-http).
>
> There's no cross-directory links anymore, so the
> "NO_CROSS_DIRECTORY_HARDLINKS" flag becomes redundant under this new
> option.
>
> 1. https://public-inbox.org/git/87woyfdkoi.fsf@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
I like the idea.
With my suggested refactoring that avoids the non-DRY code, this patch
would also become much simpler (as would 2/5 -- 4/5).
However, I would not call these "aliases". That's just confusing. Maybe
NO_INSTALL_DASHED_BUILTINS would be better? It certainly would not have
confused me.
Ciao,
Dscho
> Makefile | 8 ++++++++
> install_programs | 36 +++++++++++++++++++++---------------
> 2 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 07c8b74353..a849a7b6d1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -346,6 +346,13 @@ all::
> # INSTALL_SYMLINKS if you'd prefer not to have the install procedure
> # fallack on hardlinking or copying if "ln -s" fails.
> #
> +# Define NO_INSTALL_BUILTIN_EXECDIR_ALIASES if you'd like to skip
> +# installing legacy such as "git-init" and "git-add" in the
> +# gitexecdir. Unless you're on a system where "which git-init" is
> +# expected to returns something set this. Users have been expected to
> +# use the likes of "git init" for ages now, these programs were only
> +# provided for legacy compatibility.
> +#
> # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
> # programs as a tar, where bin/ and libexec/ might be on different file systems.
> #
> @@ -2823,6 +2830,7 @@ endif
> --flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
> --flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
> --flag-no-install-symlinks-fallback="$(NO_INSTALL_SYMLINKS_FALLBACK)" \
> + --flag-no-install-builtin-execdir-aliases="$(NO_INSTALL_BUILTIN_EXECDIR_ALIASES)" \
> --list-bindir-standalone="git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS))" \
> --list-bindir-git-dashed="$(filter $(install_bindir_programs),$(BUILT_INS))" \
> --list-execdir-git-dashed="$(BUILT_INS)" \
> diff --git a/install_programs b/install_programs
> index 51e08019dd..8d89cd9984 100755
> --- a/install_programs
> +++ b/install_programs
> @@ -33,6 +33,9 @@ do
> --flag-no-install-symlinks-fallback=*)
> NO_INSTALL_SYMLINKS_FALLBACK="${1#--flag-no-install-symlinks-fallback=}"
> ;;
> + --flag-no-install-builtin-execdir-aliases=*)
> + NO_INSTALL_BUILTIN_EXECDIR_ALIASES="${1#--flag-no-install-builtin-execdir-aliases=}"
> + ;;
> --list-bindir-standalone=*)
> list_bindir_standalone="${1#--list-bindir-standalone=}"
> ;;
> @@ -54,7 +57,7 @@ do
> shift
> done &&
>
> -if test "$bindir/" != "$execdir/"
> +if test "$bindir/" != "$execdir/" -a -z "$NO_INSTALL_BUILTIN_EXECDIR_ALIASES"
> then
> for p in $list_bindir_standalone; do
> $RM "$execdir/$p" &&
> @@ -87,20 +90,23 @@ do
> fi
> done &&
>
> -for p in $list_execdir_git_dashed; do
> - $RM "$execdir/$p" &&
> - if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
> - then
> - ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p"
> - else
> - test -n "$INSTALL_SYMLINKS" &&
> - ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p" ||
> - { test -z "$NO_INSTALL_HARDLINKS" &&
> - ln "$execdir/git$X" "$execdir/$p" ||
> - ln -s "git$X" "$execdir/$p" ||
> - cp "$execdir/git$X" "$execdir/$p" || exit; }
> - fi
> -done &&
> +if test -z "$NO_INSTALL_BUILTIN_EXECDIR_ALIASES"
> +then
> + for p in $list_execdir_git_dashed; do
> + $RM "$execdir/$p" &&
> + if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
> + then
> + ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p"
> + else
> + test -n "$INSTALL_SYMLINKS" &&
> + ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p" ||
> + { test -z "$NO_INSTALL_HARDLINKS" &&
> + ln "$execdir/git$X" "$execdir/$p" ||
> + ln -s "git$X" "$execdir/$p" ||
> + cp "$execdir/git$X" "$execdir/$p" || exit; }
> + fi
> + done
> +fi &&
>
> for p in $list_execdir_curl_aliases; do
> $RM "$execdir/$p" &&
> --
> 2.19.1.930.g4563a0d9d0
>
>
^ permalink raw reply [flat|nested] 53+ messages in thread