threads / rfc / 11033

RFC patchMove all dashed form git commands to libexecdir

Subject: [PATCH RFC] Move all dashed form git commands to libexecdir

## tl;dr

92 messages between Nov 27, 2007 and Dec 2, 2007. Diffs are folded; open one to read it.

replies: 91people: 15as markdown or json

Nguyễn Thái Ngọc Duy· Nov 27, 2007, 15:02 UTC · lore
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 We have talked about it for quite some time now. How about
 making it happen? I won't miss dashed form commands much :)
 A compromised approach could be keeping porcelain commands
 in bindir, only plumbings are moved to libexecdir. That would
 be less shock than this.
 config.mak.in |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to config.mak.in +1 −1
diff --git a/config.mak.in b/config.mak.in
index 11d256e..1db0338 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -11,7 +11,7 @@ TCLTK_PATH = @TCLTK_PATH@
 prefix = @prefix@
 exec_prefix = @exec_prefix@
 bindir = @bindir@
-#gitexecdir = @libexecdir@/git-core/
+gitexecdir = @libexecdir@/git-core/
 datarootdir = @datarootdir@
 template_dir = @datadir@/git-core/templates/
 
-- 
1.5.3.GIT
Johannes Schindelin· Nov 27, 2007, 15:12 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH RFC] Move all dashed form git commands to libexecdir

Hi,
On Tue, 27 Nov 2007, Nguyễn Thái Ngọc Duy wrote:
> diff --git a/config.mak.in b/config.mak.in

I don't use configure, and don't expect distros to use it either. Maybe you want to change Makefile first, so that the hard-core "next" followers test it first? Then, when everything is worked out and deemed to be stable, Junio can merge it to "master", and we can adjust the rpm spec and bug packagers to change their setup?

Ciao, Dscho

Nicolas Pitre· Nov 27, 2007, 15:25 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH RFC] Move all dashed form git commands to libexecdir

On Tue, 27 Nov 2007, Nguyễn Thái Ngọc Duy wrote:
Show 6 quoted lines
>  We have talked about it for quite some time now. How about
>  making it happen? I won't miss dashed form commands much :)
> 
>  A compromised approach could be keeping porcelain commands
>  in bindir, only plumbings are moved to libexecdir. That would
>  be less shock than this.
I think that would be an excellent idea.
Nicolas
Nguyễn Thái Ngoc Duy· Nov 27, 2007, 16:04 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH] Move all dashed form git commands to libexecdir

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Both configure and make-only ways should work now
 Makefile      |    2 +-
 config.mak.in |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

Makefile, config.mak.in

diff --git a/Makefile b/Makefile
index 313f9a2..377d7be 100644
--- a/Makefile
+++ b/Makefile
@@ -154,7 +154,7 @@ STRIP ?= strip
 
 prefix = $(HOME)
 bindir = $(prefix)/bin
-gitexecdir = $(bindir)
+gitexecdir = $(prefix)/libexec/git-core
 sharedir = $(prefix)/share
 template_dir = $(sharedir)/git-core/templates
 ifeq ($(prefix),/usr)
diff --git a/config.mak.in b/config.mak.in
index 11d256e..1db0338 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -11,7 +11,7 @@ TCLTK_PATH = @TCLTK_PATH@
 prefix = @prefix@
 exec_prefix = @exec_prefix@
 bindir = @bindir@
-#gitexecdir = @libexecdir@/git-core/
+gitexecdir = @libexecdir@/git-core/
 datarootdir = @datarootdir@
 template_dir = @datadir@/git-core/templates/
 
-- 
1.5.3.GIT
Jan Hudec· Nov 28, 2007, 00:07 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Tue, Nov 27, 2007 at 16:18:01 +0000, Johannes Schindelin wrote:
Show 7 quoted lines
> Hi,
> 
> On Tue, 27 Nov 2007, Nguyễn Thái Ngoc Duy wrote:
> 
> >  Both configure and make-only ways should work now
> 
> I thought your plan was to put the non-porcelain into the libexecdir only?

I had the impression that deprecating the dash notation for /all/ use was approved some time ago. Though I don't want to search through the list archives this late in the night to check it.

-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>
Junio C Hamano· Nov 28, 2007, 01:13 UTC · re: Jan Hudec · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Jan Hudec <bulb@ucw.cz> writes:
Show 12 quoted lines
> On Tue, Nov 27, 2007 at 16:18:01 +0000, Johannes Schindelin wrote:
>> Hi,
>> 
>> On Tue, 27 Nov 2007, Nguyễn Thái Ngoc Duy wrote:
>> 
>> >  Both configure and make-only ways should work now
>> 
>> I thought your plan was to put the non-porcelain into the libexecdir only?
>
> I had the impression that deprecating the dash notation for /all/ use was
> approved some time ago. Though I don't want to search through the list
> archives this late in the night to check it.

Yes. Moving the dash-form commands out of end user's PATH was something distros and users have been allowed to do since forever, and strictly speaking, using dash form from the command line was already deprecated at that point.

If your script runs git-foo without first asking "git --exec-path" and prepending it to the path, your script would not find git-foo if the installation uses gitexecdir that is not on the usual $PATH, either.

I essentially just said that your patch is unnecessary, but at the same time, your patch does not go far enough. As Nico earlier pointed out, we ship a sample rpm spec, which would also need to be updated. We do not ship a sample debian/rules anymore, thank $DEITY ;-)

Also, because we do not remove existing files from the installation target directory when we do "make install", the commit log message should carry a big fat warning that says "remove old installation of git from your $(bindir) when you try this," for people who build from the source, and we need to repeat the deprecation notice in bold red letters in the Release Notes for perhaps git 1.6.0.

In case somebody is thinking about 36e5e70e0f40 (Start deprecating "git-command" in favor of "git command"), that is a somewhat different issue. What Linus suggested is not installing git-foo link for built-in commands _anywhere_ on the filesystem. Not just "out of user's PATH". That is not deprecating dash form but removing the support for it. We need to give ample time for users to adjust to such a change.

Jan Hudec· Nov 28, 2007, 08:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Tue, Nov 27, 2007 at 17:13:58 -0800, Junio C Hamano wrote:
Show 20 quoted lines
> Jan Hudec <bulb@ucw.cz> writes:
> > On Tue, Nov 27, 2007 at 16:18:01 +0000, Johannes Schindelin wrote:
> >> On Tue, 27 Nov 2007, Nguyễn Thái Ngoc Duy wrote:
> >> 
> >> >  Both configure and make-only ways should work now
> >> 
> >> I thought your plan was to put the non-porcelain into the libexecdir only?
> >
> > I had the impression that deprecating the dash notation for /all/ use was
> > approved some time ago. Though I don't want to search through the list
> > archives this late in the night to check it.
> 
> [...]
>
> In case somebody is thinking about 36e5e70e0f40 (Start deprecating
> "git-command" in favor of "git command"), that is a somewhat different
> issue.  What Linus suggested is not installing git-foo link for built-in
> commands _anywhere_ on the filesystem.  Not just "out of user's PATH".
> That is not deprecating dash form but removing the support for it.  We
> need to give ample time for users to adjust to such a change.

Yes, that is what I said I recall seeing. Installing out of user's PATH is a step towards not installing at all and the change suggests it has been already accepted as a general direction for future. Or not?

-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>
Nguyen Thai Ngoc Duy· Nov 28, 2007, 08:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Nov 28, 2007 8:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> In case somebody is thinking about 36e5e70e0f40 (Start deprecating
> "git-command" in favor of "git command"), that is a somewhat different
> issue.  What Linus suggested is not installing git-foo link for built-in
> commands _anywhere_ on the filesystem.  Not just "out of user's PATH".
> That is not deprecating dash form but removing the support for it.  We
> need to give ample time for users to adjust to such a change.

A little note on this one. I've been using git without builtin links for a while with my git-box port. There are still some builtin fixups needed. And because execv_git_cmd() always uses dash form, so it's impossible to use vanilla git without builtin links.

-- 
Duy
Junio C Hamano· Nov 28, 2007, 23:14 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

"Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
Show 12 quoted lines
> On Nov 28, 2007 8:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> In case somebody is thinking about 36e5e70e0f40 (Start deprecating
>> "git-command" in favor of "git command"), that is a somewhat different
>> issue.  What Linus suggested is not installing git-foo link for built-in
>> commands _anywhere_ on the filesystem.  Not just "out of user's PATH".
>> That is not deprecating dash form but removing the support for it.  We
>> need to give ample time for users to adjust to such a change.
>
> A little note on this one. I've been using git without builtin links
> for a while with my git-box port. There are still some builtin fixups
> needed. And because execv_git_cmd() always uses dash form, so it's
> impossible to use vanilla git without builtin links.
Thanks for a heads up.
Would people agree with a rough roadmap like this?
 - v1.5.4 will ship with gitexecdir=$(bindir) in Makefile.  But the
   release notes for the version will warn users that:
   (1) using git-foo from the command line, and
   (2) using git-foo from your scripts without first prepending the
       return value of "git --exec-path" to the PATH
   is now officially deprecated (it has been deprecated for a long time
   since January 2006, v1.2.0~149) and upcoming v1.5.5 will ship with
   the default configuration that does not install git-foo form in
   user's PATH.
 - Post v1.5.4, start cooking gitexecdir=$(libexecdir)/git-core, aiming
   for inclusion in v1.5.5, perhaps in Mar-Feb 2008 timeframe.
 - The release notes for v1.5.5 will warn users that git-foo will be
   removed in v1.6.0 for many commands and it will be merely an accident
   if some of them still work.
 - Post v1.5.5, start cooking the change that does not install hardlinks
   for built-in commands, aiming for inclusion in v1.6.0, by the end of
   2008.
Johannes Schindelin· Nov 28, 2007, 23:40 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Hi,
On Wed, 28 Nov 2007, Junio C Hamano wrote:
Show 31 quoted lines
> "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
> 
> > On Nov 28, 2007 8:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> >> In case somebody is thinking about 36e5e70e0f40 (Start deprecating
> >> "git-command" in favor of "git command"), that is a somewhat different
> >> issue.  What Linus suggested is not installing git-foo link for built-in
> >> commands _anywhere_ on the filesystem.  Not just "out of user's PATH".
> >> That is not deprecating dash form but removing the support for it.  We
> >> need to give ample time for users to adjust to such a change.
> >
> > A little note on this one. I've been using git without builtin links
> > for a while with my git-box port. There are still some builtin fixups
> > needed. And because execv_git_cmd() always uses dash form, so it's
> > impossible to use vanilla git without builtin links.
> 
> Thanks for a heads up.
> 
> Would people agree with a rough roadmap like this?
> 
>  - v1.5.4 will ship with gitexecdir=$(bindir) in Makefile.  But the
>    release notes for the version will warn users that:
> 
>    (1) using git-foo from the command line, and
> 
>    (2) using git-foo from your scripts without first prepending the
>        return value of "git --exec-path" to the PATH
> 
>    is now officially deprecated (it has been deprecated for a long time
>    since January 2006, v1.2.0~149) and upcoming v1.5.5 will ship with
>    the default configuration that does not install git-foo form in
>    user's PATH.

Maybe we can squeeze a step in here where only porcelains are installed in the bindir?

FWIW I think that we should fix the problem with the builtins being called via their hard links. But how? As of now, libgit.a has no idea what the builtins are; this information is buried in git.c.

The fundamental problem is that we cannot move handle_internal_command() into libgit.a, because it has pointers to all builtin cmd_*() functions.

So maybe the best solution would be to try "git <command>" first, and then "git-<command>"? But this means another exec() call :-(

Ciao, Dscho

Junio C Hamano· Nov 28, 2007, 23:48 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> The fundamental problem is that we cannot move handle_internal_command() 
> into libgit.a, because it has pointers to all builtin cmd_*() functions.
What's wrong with having cmd_* functions in the library to begin with?
Johannes Schindelin· Nov 29, 2007, 00:01 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Hi,
On Wed, 28 Nov 2007, Junio C Hamano wrote:
Show 7 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > The fundamental problem is that we cannot move 
> > handle_internal_command() into libgit.a, because it has pointers to 
> > all builtin cmd_*() functions.
> 
> What's wrong with having cmd_* functions in the library to begin with?

I was considering it not desirable (after all, the library is meant to have common functions in it), but you're right...

Ciao, Dscho

A Large Angry SCM· Nov 29, 2007, 00:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Junio C Hamano wrote: [...]

Show 25 quoted lines
> Would people agree with a rough roadmap like this?
> 
>  - v1.5.4 will ship with gitexecdir=$(bindir) in Makefile.  But the
>    release notes for the version will warn users that:
> 
>    (1) using git-foo from the command line, and
> 
>    (2) using git-foo from your scripts without first prepending the
>        return value of "git --exec-path" to the PATH
> 
>    is now officially deprecated (it has been deprecated for a long time
>    since January 2006, v1.2.0~149) and upcoming v1.5.5 will ship with
>    the default configuration that does not install git-foo form in
>    user's PATH.
> 
>  - Post v1.5.4, start cooking gitexecdir=$(libexecdir)/git-core, aiming
>    for inclusion in v1.5.5, perhaps in Mar-Feb 2008 timeframe.
> 
>  - The release notes for v1.5.5 will warn users that git-foo will be
>    removed in v1.6.0 for many commands and it will be merely an accident
>    if some of them still work.
> 
>  - Post v1.5.5, start cooking the change that does not install hardlinks
>    for built-in commands, aiming for inclusion in v1.6.0, by the end of
>    2008.

So long as there remains the option in the Makefile to install the "dashed" commands in $(bindir) for those of us that wish it.

Junio C Hamano· Nov 29, 2007, 01:02 UTC · re: A Large Angry SCM · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

A Large Angry SCM <gitzilla@gmail.com> writes:
Show 30 quoted lines
> Junio C Hamano wrote:
> [...]
>> Would people agree with a rough roadmap like this?
>>
>>  - v1.5.4 will ship with gitexecdir=$(bindir) in Makefile.  But the
>>    release notes for the version will warn users that:
>>
>>    (1) using git-foo from the command line, and
>>
>>    (2) using git-foo from your scripts without first prepending the
>>        return value of "git --exec-path" to the PATH
>>
>>    is now officially deprecated (it has been deprecated for a long time
>>    since January 2006, v1.2.0~149) and upcoming v1.5.5 will ship with
>>    the default configuration that does not install git-foo form in
>>    user's PATH.
>>
>>  - Post v1.5.4, start cooking gitexecdir=$(libexecdir)/git-core, aiming
>>    for inclusion in v1.5.5, perhaps in Mar-Feb 2008 timeframe.
>>
>>  - The release notes for v1.5.5 will warn users that git-foo will be
>>    removed in v1.6.0 for many commands and it will be merely an accident
>>    if some of them still work.
>>
>>  - Post v1.5.5, start cooking the change that does not install hardlinks
>>    for built-in commands, aiming for inclusion in v1.6.0, by the end of
>>    2008.
>
> So long as there remains the option in the Makefile to install the
> "dashed" commands in $(bindir) for those of us that wish it.
Surely.

Actually they already have that option of going dashless themselves, so in a sense it is not strictly necessary to make a big deal out of this.

Nguyen Thai Ngoc Duy· Nov 29, 2007, 03:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Nov 29, 2007 6:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 43 quoted lines
>
> "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
>
> > On Nov 28, 2007 8:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> >> In case somebody is thinking about 36e5e70e0f40 (Start deprecating
> >> "git-command" in favor of "git command"), that is a somewhat different
> >> issue.  What Linus suggested is not installing git-foo link for built-in
> >> commands _anywhere_ on the filesystem.  Not just "out of user's PATH".
> >> That is not deprecating dash form but removing the support for it.  We
> >> need to give ample time for users to adjust to such a change.
> >
> > A little note on this one. I've been using git without builtin links
> > for a while with my git-box port. There are still some builtin fixups
> > needed. And because execv_git_cmd() always uses dash form, so it's
> > impossible to use vanilla git without builtin links.
>
> Thanks for a heads up.
>
> Would people agree with a rough roadmap like this?
>
>  - v1.5.4 will ship with gitexecdir=$(bindir) in Makefile.  But the
>    release notes for the version will warn users that:
>
>    (1) using git-foo from the command line, and
>
>    (2) using git-foo from your scripts without first prepending the
>        return value of "git --exec-path" to the PATH
>
>    is now officially deprecated (it has been deprecated for a long time
>    since January 2006, v1.2.0~149) and upcoming v1.5.5 will ship with
>    the default configuration that does not install git-foo form in
>    user's PATH.
>
>  - Post v1.5.4, start cooking gitexecdir=$(libexecdir)/git-core, aiming
>    for inclusion in v1.5.5, perhaps in Mar-Feb 2008 timeframe.
>
>  - The release notes for v1.5.5 will warn users that git-foo will be
>    removed in v1.6.0 for many commands and it will be merely an accident
>    if some of them still work.
>
>  - Post v1.5.5, start cooking the change that does not install hardlinks
>    for built-in commands, aiming for inclusion in v1.6.0, by the end of
>    2008.

There won't be a stage when only porcelain git-foos are in $(bindir)? I could stop working on the relevant patch then.

-- 
Duy
Nicolas Pitre· Nov 29, 2007, 14:09 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, 29 Nov 2007, Nguyen Thai Ngoc Duy wrote:
Show 47 quoted lines
> On Nov 29, 2007 6:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
> >
> > > On Nov 28, 2007 8:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > >> In case somebody is thinking about 36e5e70e0f40 (Start deprecating
> > >> "git-command" in favor of "git command"), that is a somewhat different
> > >> issue.  What Linus suggested is not installing git-foo link for built-in
> > >> commands _anywhere_ on the filesystem.  Not just "out of user's PATH".
> > >> That is not deprecating dash form but removing the support for it.  We
> > >> need to give ample time for users to adjust to such a change.
> > >
> > > A little note on this one. I've been using git without builtin links
> > > for a while with my git-box port. There are still some builtin fixups
> > > needed. And because execv_git_cmd() always uses dash form, so it's
> > > impossible to use vanilla git without builtin links.
> >
> > Thanks for a heads up.
> >
> > Would people agree with a rough roadmap like this?
> >
> >  - v1.5.4 will ship with gitexecdir=$(bindir) in Makefile.  But the
> >    release notes for the version will warn users that:
> >
> >    (1) using git-foo from the command line, and
> >
> >    (2) using git-foo from your scripts without first prepending the
> >        return value of "git --exec-path" to the PATH
> >
> >    is now officially deprecated (it has been deprecated for a long time
> >    since January 2006, v1.2.0~149) and upcoming v1.5.5 will ship with
> >    the default configuration that does not install git-foo form in
> >    user's PATH.
> >
> >  - Post v1.5.4, start cooking gitexecdir=$(libexecdir)/git-core, aiming
> >    for inclusion in v1.5.5, perhaps in Mar-Feb 2008 timeframe.
> >
> >  - The release notes for v1.5.5 will warn users that git-foo will be
> >    removed in v1.6.0 for many commands and it will be merely an accident
> >    if some of them still work.
> >
> >  - Post v1.5.5, start cooking the change that does not install hardlinks
> >    for built-in commands, aiming for inclusion in v1.6.0, by the end of
> >    2008.
> 
> There won't be a stage when only porcelain git-foos are in $(bindir)?
> I could stop working on the relevant patch then.

Well, I personally found your effort really nice. I think Junio is overly cautious in this case, and I would prefer to see the number of git commands in the default path drop rather sooner than later.

Nicolas
Junio C Hamano· Nov 29, 2007, 22:36 UTC · re: Nicolas Pitre · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Nicolas Pitre <nico@cam.org> writes:
Show 8 quoted lines
> On Thu, 29 Nov 2007, Nguyen Thai Ngoc Duy wrote:
>
>> There won't be a stage when only porcelain git-foos are in $(bindir)?
>> I could stop working on the relevant patch then.
>
> Well, I personally found your effort really nice.  I think Junio is 
> overly cautious in this case, and I would prefer to see the number of 
> git commands in the default path drop rather sooner than later.

I agree with the first sentence. And yes I am playing it safe, and at the same time I do not think the "default" really matters as much as people think.

If people are really serious about reducing the number of commands in the path, I would expect fixes and bugreports saying "I am setting gitexecdir different from bindir in _my_ installation when I build git, and here are the things that does not work if I do so". Within the span of more than 20 months (77cb17e9 introduced gitexecdir in Jan 2006), I do not think there was a single such report or patch, other than the message from Nguyen that started this thread.

Which means one of two things (1) we got everything right and there is nothing to fix, other than changing the default like Nguyen's patch does, or (2) nobody is interested in moving git-foo out of their PATH for _his_ own use, but pushing changes that would affect _other_ people without testing.

I am of course hoping that (1) is the case. And it could be that in open-source settings often the silent majority is content with what's already there, and that many people who are indeed interested in moving git-foo out of their PATH are doing so happily without telling the others of their success.

But it still worries me.

And people's scripts, especially old/unmaintained ones that google still knows about, are worrysome too. Didn't we just see a message that says "git-update-cache in a script I picked up from google does not work" on the list?

Wincent Colaiuta· Nov 30, 2007, 07:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

El 29/11/2007, a las 23:36, Junio C Hamano escribió:
Show 9 quoted lines
> If people are really serious about reducing the number of commands in
> the path, I would expect fixes and bugreports saying "I am setting
> gitexecdir different from bindir in _my_ installation when I build  
> git,
> and here are the things that does not work if I do so".  Within the  
> span
> of more than 20 months (77cb17e9 introduced gitexecdir in Jan 2006), I
> do not think there was a single such report or patch, other than the
> message from Nguyen that started this thread.

One reason why there have been no reports is probably because 99% of people have never heard of gitexecdir nor know what it does.

$ git grep gitexecdir | awk -F : '{print $1}' | uniq Makefile config.mak.in git-gui/Makefile git-gui/macosx/AppMain.tcl git.c

Try googling for "site:git.or.cz gitexecdir"

Basically, seems the only way you could know about it is if you've heard it mentioned on this mailing list and decided to study the Makefile.

Cheers, Wincent

Eyvind Bernhardsen· Nov 30, 2007, 11:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On 29. nov. 2007, at 23.36, Junio C Hamano wrote:
[...]
Show 9 quoted lines
> If people are really serious about reducing the number of commands in
> the path, I would expect fixes and bugreports saying "I am setting
> gitexecdir different from bindir in _my_ installation when I build  
> git,
> and here are the things that does not work if I do so".  Within the  
> span
> of more than 20 months (77cb17e9 introduced gitexecdir in Jan 2006), I
> do not think there was a single such report or patch, other than the
> message from Nguyen that started this thread.

I'm setting gitexecdir different from bindir in my installation, and here are the things that don't work:

- When pushing to my system over ssh, git-receive-pack and git-upload- 
pack are expected to be in $PATH.  I resolved the problem by putting  
symlinks in /usr/local/bin.

I haven't seen any other problems, but then again, I only use git plumbing commands and my own scripts.

Eyvind
Johannes Schindelin· Nov 30, 2007, 12:08 UTC · re: Eyvind Bernhardsen · lore

[PATCH] transport.c: call dash-less form of receive-pack and upload-pack on remote

Since we plan to move the dash-form (git-<whatever>) into an execdir, it make sense to prepare our git protocol users for it.

Noticed by Eyvind Bernhardsen.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	On Fri, 30 Nov 2007, Eyvind Bernhardsen wrote:
	> - When pushing to my system over ssh, git-receive-pack and
	> git-upload-pack are expected to be in $PATH.  I resolved the 
	> problem by putting symlinks in /usr/local/bin.
	How about this?  (I only compile-tested it...)
 transport.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to transport.c +2 −2
diff --git a/transport.c b/transport.c
index 50db980..7bd0846 100644
--- a/transport.c
+++ b/transport.c
@@ -736,10 +736,10 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->disconnect = disconnect_git;
 
 		data->thin = 1;
-		data->uploadpack = "git-upload-pack";
+		data->uploadpack = "git upload-pack";
 		if (remote && remote->uploadpack)
 			data->uploadpack = remote->uploadpack;
-		data->receivepack = "git-receive-pack";
+		data->receivepack = "git receive-pack";
 		if (remote && remote->receivepack)
 			data->receivepack = remote->receivepack;
 	}
-- 
1.5.3.6.2088.g8c260
Junio C Hamano· Dec 1, 2007, 02:36 UTC · re: Johannes Schindelin · lore

Re: [PATCH] transport.c: call dash-less form of receive-pack and upload-pack on remote

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 15 quoted lines
> Since we plan to move the dash-form (git-<whatever>) into an execdir, it
> make sense to prepare our git protocol users for it.
>
> Noticed by Eyvind Bernhardsen.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	On Fri, 30 Nov 2007, Eyvind Bernhardsen wrote:
>
> 	> - When pushing to my system over ssh, git-receive-pack and
> 	> git-upload-pack are expected to be in $PATH.  I resolved the 
> 	> problem by putting symlinks in /usr/local/bin.
>
> 	How about this?  (I only compile-tested it...)

I only eyeball-tested it and looks Okay, but that does not assure us much ;-). Is this change easy to test using local transport?

Johannes Schindelin· Dec 1, 2007, 10:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] transport.c: call dash-less form of receive-pack and upload-pack on remote

Hi,
On Fri, 30 Nov 2007, Junio C Hamano wrote:
Show 20 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Since we plan to move the dash-form (git-<whatever>) into an execdir, it
> > make sense to prepare our git protocol users for it.
> >
> > Noticed by Eyvind Bernhardsen.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >
> > 	On Fri, 30 Nov 2007, Eyvind Bernhardsen wrote:
> >
> > 	> - When pushing to my system over ssh, git-receive-pack and
> > 	> git-upload-pack are expected to be in $PATH.  I resolved the 
> > 	> problem by putting symlinks in /usr/local/bin.
> >
> > 	How about this?  (I only compile-tested it...)
> 
> I only eyeball-tested it and looks Okay, but that does not assure us
> much ;-).  Is this change easy to test using local transport?

Seems like it breaks down with git-shell. But then, I think that we just have to fix execv_git_cmd() to call builtins via "git" instead.

Ciao, Dscho

Junio C Hamano· Dec 1, 2007, 19:30 UTC · re: Johannes Schindelin · lore

Re: [PATCH] transport.c: call dash-less form of receive-pack and upload-pack on remote

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted lines
>> I only eyeball-tested it and looks Okay, but that does not assure us
>> much ;-).  Is this change easy to test using local transport?
>
> Seems like it breaks down with git-shell.  But then, I think that we just 
> have to fix execv_git_cmd() to call builtins via "git" instead.

So in execv_git_cmd(), instead of doing the strbuf_addf(), we do something like this (and drop your patch)?

---
 exec_cmd.c |   34 +++++++++++++---------------------
 1 files changed, 13 insertions(+), 21 deletions(-)
Show changes to exec_cmd.c +13 −21
diff --git a/exec_cmd.c b/exec_cmd.c
index 2d0a758..2920335 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -65,32 +65,24 @@ void setup_path(const char *cmd_path)
 
 int execv_git_cmd(const char **argv)
 {
-	struct strbuf cmd;
-	const char *tmp;
-
-	strbuf_init(&cmd, 0);
-	strbuf_addf(&cmd, "git-%s", argv[0]);
-
-	/*
-	 * argv[0] must be the git command, but the argv array
-	 * belongs to the caller, and may be reused in
-	 * subsequent loop iterations. Save argv[0] and
-	 * restore it on error.
-	 */
-	tmp = argv[0];
-	argv[0] = cmd.buf;
-
-	trace_argv_printf(argv, -1, "trace: exec:");
+	int i;
+	const char **args;
+
+	for (i = 0; argv[i]; i++)
+		;
+	args = xcalloc(i + 1, sizeof(*args));
+	for (i = 0; argv[i]; i++)
+		args[i+1] = argv[i];
+	args[0] = "git";
+	args[i+1] = NULL;
+	trace_argv_printf(args, -1, "trace: exec:");
 
 	/* execvp() can only ever return if it fails */
-	execvp(cmd.buf, (char **)argv);
+	execvp(args[0], (char **)args);
 
 	trace_printf("trace: exec failed: %s\n", strerror(errno));
 
-	argv[0] = tmp;
-
-	strbuf_release(&cmd);
-
+	free(args);
 	return -1;
 }
 
Johannes Schindelin· Dec 1, 2007, 23:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] transport.c: call dash-less form of receive-pack and upload-pack on remote

Hi,
On Sat, 1 Dec 2007, Junio C Hamano wrote:
Show 10 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I only eyeball-tested it and looks Okay, but that does not assure us
> >> much ;-).  Is this change easy to test using local transport?
> >
> > Seems like it breaks down with git-shell.  But then, I think that we just 
> > have to fix execv_git_cmd() to call builtins via "git" instead.
> 
> So in execv_git_cmd(), instead of doing the strbuf_addf(), we do
> something like this (and drop your patch)?
You know what is really funny?  I have this in my stash:
-- snip --
 exec_cmd.c      |   30 ++++++++++++------------------
 t/t0020-crlf.sh |    2 +-
 2 files changed, 13 insertions(+), 19 deletions(-)
Show changes to 2 files +13 −20

exec_cmd.c, t/t0020-crlf.sh

diff --git a/exec_cmd.c b/exec_cmd.c
index 2d0a758..7d022a2 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -65,31 +65,25 @@ void setup_path(const char *cmd_path)
 
 int execv_git_cmd(const char **argv)
 {
-	struct strbuf cmd;
-	const char *tmp;
+	int i;
+	char **new_argv;
 
-	strbuf_init(&cmd, 0);
-	strbuf_addf(&cmd, "git-%s", argv[0]);
+	for (i = 0; argv[i]; i++)
+		; /* do nothing */
+	new_argv = xmalloc((i + 2) * sizeof(*new_argv));
+	new_argv[0] = "git";
+	for (i = 0; argv[i]; i++)
+		new_argv[i + 1] = (char *)argv[i];
+	new_argv[i] = NULL;
 
-	/*
-	 * argv[0] must be the git command, but the argv array
-	 * belongs to the caller, and may be reused in
-	 * subsequent loop iterations. Save argv[0] and
-	 * restore it on error.
-	 */
-	tmp = argv[0];
-	argv[0] = cmd.buf;
-
-	trace_argv_printf(argv, -1, "trace: exec:");
+	trace_argv_printf((const char **)new_argv, -1, "trace: exec:");
 
 	/* execvp() can only ever return if it fails */
-	execvp(cmd.buf, (char **)argv);
+	execvp(new_argv[0], new_argv);
 
 	trace_printf("trace: exec failed: %s\n", strerror(errno));
 
-	argv[0] = tmp;
-
-	strbuf_release(&cmd);
+	free(new_argv);
 
 	return -1;
 }
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 62bc4bb..275379d 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -36,7 +36,7 @@ test_expect_success setup '
 
 	for w in Some extra lines here; do echo $w; done >>one &&
 	git diff >patch.file &&
-	patched=`git hash-object --stdin <one` &&
+	patched=`GIT_TRACE=2 git hash-object --stdin <one` &&
 	git read-tree --reset -u HEAD &&
 
 	echo happy.
-- snap --

Which looks awfully like your patch (except that I called it new_argv, I 
think).

Now you might ask why there is such a funny patch to t0020?  Well, the 
patch does not work as-is.

Will investigate right now,
Dscho
Johannes Schindelin· Dec 1, 2007, 23:15 UTC · re: Johannes Schindelin · lore

Re: [PATCH] transport.c: call dash-less form of receive-pack and upload-pack on remote

Hi,
On Sat, 1 Dec 2007, Johannes Schindelin wrote:
> Will investigate right now,

The problem is that "git <command>" will call execv_git_cmd() for non-builtins, which in turn will execute "git <command>", ... ad infinitum.

Ciao, Dscho

Junio C Hamano· Dec 2, 2007, 01:57 UTC · re: Johannes Schindelin · lore

Re: [PATCH] transport.c: call dash-less form of receive-pack and upload-pack on remote

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 7 quoted lines
> On Sat, 1 Dec 2007, Johannes Schindelin wrote:
>
>> Will investigate right now,
>
> The problem is that "git <command>" will call execv_git_cmd() for 
> non-builtins, which in turn will execute "git <command>", ... ad 
> infinitum.

Ok, then the ".. then try the external ones" in git.c needs to do the execvp itself, which should not be a big deal.

Very funny ;-).
Johannes Schindelin· Dec 2, 2007, 02:52 UTC · re: Johannes Schindelin · lore

[PATCH 0/3] Call builtin functions directly, was Re: [PATCH] transport.c: call dash-less form of receive-pack and upload-pack on remote

Hi,
On Sat, 1 Dec 2007, Johannes Schindelin wrote:
Show 7 quoted lines
> On Sat, 1 Dec 2007, Johannes Schindelin wrote:
> 
> > Will investigate right now,
> 
> The problem is that "git <command>" will call execv_git_cmd() for 
> non-builtins, which in turn will execute "git <command>", ... ad 
> infinitum.

Okay, I bit the apple and tried to move the builtins into the library, and rename handle_internal_command into execv_git_builtin(), moving it into exec-cmd.c.

Big mistake.

Why? Because there is at least one caller, git-bundle, which relies on execv_git_cmd() _not_ reusing all those "nice" one-shot static variables, like for example the object hashmap and the objects themselves.

Now, it seems that we can get away for the moment with just introducing an object release mechanism and calling that in execv_git_builtin() before calling a builtin function, because the existing callers do not rely on more than a cleanup of the objects.

But it is hairy, since it is such an essential part of git. And since I was utterly tired while preparing this patch series. So I suggest maybe putting this into pu, but no further for the moment. I will use the patched git in the next days, though, to catch breakages (hopefully).

Ciao, Dscho

Johannes Schindelin· Dec 2, 2007, 02:54 UTC · re: Johannes Schindelin · lore

[PATCH 1/3] Introduce release_all_objects()

The new function release_all_objects() can be used to flush the object cache. This will be needed for the upcoming change in execv_git_cmd(), which should call the builtin functions directly instead of calling execvp().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	Guess how surprised I was when "free(commit);" would work the
	first time, but not the second...
	ATM I use an ugly way to cope with the static "nr" variables
	in alloc.c: I just do not free() the last block.
	It might be a better idea to refactor the (quite ugly) code in
	alloc.c to have global structures a la
		#define BLOCKING 1024
		struct object_block {
			size_t struct_size;
			int nr;
			/* first (uint32_t *) is pointer to previous block */
			void *block;
		};
		static void *alloc_node(struct object_block *block)
		{
			if (!block || block->nr >= BLOCKING) {
				void *next = xmalloc(sizeof(void *)
					+ BLOCKING * block->struct_size);
				*(void **)next = block->block;
				block->block = next;
				block->nr = 0;
			}
			return block->block + sizeof(void *)
				+ block->struct_size * block->nr++;
		}
		static void release_nodes(struct object_block *block)
		{
			while (block->block) {
				void *previous = *(void **)block->block;
				free(block->block);
				block->block = previous;
			}
			block->nr = 0; /* not strictly necessary */
		}
		#define DEFINE_ALLOCATOR(type)				\
		static object_block type##s = { sizeof(struct type) };	\
		struct type *alloc_##type##_node(void)			\
		{							\
			return alloc_node(&type##s);			\
		}
		DEFINE_ALLOCATOR(object)
		DEFINE_ALLOCATOR(blob)
		DEFINE_ALLOCATOR(tree)
		DEFINE_ALLOCATOR(commit)
		DEFINE_ALLOCATOR(tag)
	but I am waaay too tired to brush this up, test it and submit it
	(hint, hint).
 alloc.c  |   31 ++++++++++++++++++++++++++++++-
 blob.c   |    3 +++
 blob.h   |    1 +
 cache.h  |    6 ++++++
 commit.c |    8 ++++++++
 commit.h |    2 ++
 object.c |   24 ++++++++++++++++++++++++
 object.h |    2 ++
 tag.c    |    8 ++++++++
 tag.h    |    2 ++
 tree.c   |    6 ++++++
 tree.h   |    1 +
 12 files changed, 93 insertions(+), 1 deletions(-)
Show changes to 12 files +93 −1

alloc.c, blob.c, blob.h, cache.h, commit.c, commit.h, object.c, object.h, tag.c, tag.h, tree.c, tree.h

diff --git a/alloc.c b/alloc.c
index 216c23a..8c5e5e0 100644
--- a/alloc.c
+++ b/alloc.c
@@ -20,6 +20,7 @@
 
 #define DEFINE_ALLOCATOR(name, type)				\
 static unsigned int name##_allocs;				\
+static void *last_alloced_##name;				\
 void *alloc_##name##_node(void)					\
 {								\
 	static int nr;						\
@@ -28,13 +29,32 @@ void *alloc_##name##_node(void)					\
 								\
 	if (!nr) {						\
 		nr = BLOCKING;					\
-		block = xmalloc(BLOCKING * sizeof(type));	\
+		struct {					\
+			void *previous;				\
+			type block[BLOCKING];			\
+		} *buf = xmalloc(sizeof(*buf));			\
+		buf->previous = last_alloced_##name;		\
+		last_alloced_##name = buf;			\
+		block = buf->block;				\
 	}							\
 	nr--;							\
 	name##_allocs++;					\
 	ret = block++;						\
 	memset(ret, 0, sizeof(type));				\
 	return ret;						\
+}								\
+								\
+void release_all_##name##_nodes(void)				\
+{								\
+	void *buf = last_alloced_##name;			\
+	if (!buf)						\
+		return;						\
+	buf = *(void **)buf;					\
+	while (buf) {						\
+		void *next = *(void **)buf;			\
+		free(buf);					\
+		buf = next;					\
+	}							\
 }
 
 union any_object {
@@ -74,3 +94,12 @@ void alloc_report(void)
 	REPORT(commit);
 	REPORT(tag);
 }
+
+void release_all_nodes(void)
+{
+	release_all_blob_nodes();
+	release_all_tree_nodes();
+	release_all_commit_nodes();
+	release_all_tag_nodes();
+	release_all_object_nodes();
+}
diff --git a/blob.c b/blob.c
index bd7d078..63756e6 100644
--- a/blob.c
+++ b/blob.c
@@ -18,6 +18,9 @@ struct blob *lookup_blob(const unsigned char *sha1)
 	return (struct blob *) obj;
 }
 
+void release_blob(struct blob *blob) {
+}
+
 int parse_blob_buffer(struct blob *item, void *buffer, unsigned long size)
 {
 	item->object.parsed = 1;
diff --git a/blob.h b/blob.h
index ea5d9e9..7560671 100644
--- a/blob.h
+++ b/blob.h
@@ -10,6 +10,7 @@ struct blob {
 };
 
 struct blob *lookup_blob(const unsigned char *sha1);
+void release_blob(struct blob *blob);
 
 int parse_blob_buffer(struct blob *item, void *buffer, unsigned long size);
 
diff --git a/cache.h b/cache.h
index 4e59646..cc50f1c 100644
--- a/cache.h
+++ b/cache.h
@@ -613,6 +613,12 @@ extern void *alloc_tree_node(void);
 extern void *alloc_commit_node(void);
 extern void *alloc_tag_node(void);
 extern void *alloc_object_node(void);
+extern void release_all_blob_nodes(void);
+extern void release_all_tree_nodes(void);
+extern void release_all_commit_nodes(void);
+extern void release_all_tag_nodes(void);
+extern void release_all_object_nodes(void);
+extern void release_all_nodes(void);
 extern void alloc_report(void);
 
 /* trace.c */
diff --git a/commit.c b/commit.c
index f074811..59c2236 100644
--- a/commit.c
+++ b/commit.c
@@ -48,6 +48,14 @@ struct commit *lookup_commit(const unsigned char *sha1)
 	return check_commit(obj, sha1, 0);
 }
 
+void release_commit(struct commit *commit)
+{
+	if (commit->parents)
+		free_commit_list(commit->parents);
+	if (commit->buffer)
+		free(commit->buffer);
+}
+
 static unsigned long parse_commit_date(const char *buf)
 {
 	unsigned long date;
diff --git a/commit.h b/commit.h
index 10e2b5d..363b9fb 100644
--- a/commit.h
+++ b/commit.h
@@ -21,6 +21,7 @@ struct commit {
 	char *buffer;
 };
 
+
 extern int save_commit_buffer;
 extern const char *commit_type;
 
@@ -35,6 +36,7 @@ struct commit *lookup_commit(const unsigned char *sha1);
 struct commit *lookup_commit_reference(const unsigned char *sha1);
 struct commit *lookup_commit_reference_gently(const unsigned char *sha1,
 					      int quiet);
+void release_commit(struct commit *commit);
 
 int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size);
 
diff --git a/object.c b/object.c
index 16793d9..f217122 100644
--- a/object.c
+++ b/object.c
@@ -192,6 +192,30 @@ struct object *parse_object(const unsigned char *sha1)
 	return NULL;
 }
 
+void release_all_objects(void)
+{
+	int i;
+	for (i = 0; i < obj_hash_size; i++)
+		if (obj_hash[i]) {
+			switch (obj_hash[i]->type) {
+			case OBJ_BLOB:
+				release_blob((struct blob *)obj_hash[i]);
+				break;
+			case OBJ_COMMIT:
+				release_commit((struct commit *)obj_hash[i]);
+				break;
+			/*case OBJ_TREE:
+				release_tree((struct tree *)obj_hash[i]);
+				break;
+			case OBJ_TAG:
+				release_tag((struct tag *)obj_hash[i]);
+				break;*/
+			}
+			obj_hash[i] = NULL;
+		}
+	release_all_nodes();
+}
+
 struct object_list *object_list_insert(struct object *item,
 				       struct object_list **list_p)
 {
diff --git a/object.h b/object.h
index 397bbfa..ad6184c 100644
--- a/object.h
+++ b/object.h
@@ -44,6 +44,8 @@ extern unsigned int get_max_object_index(void);
 extern struct object *get_indexed_object(unsigned int);
 extern struct object_refs *lookup_object_refs(struct object *);
 
+extern void release_all_objects(void);
+
 /** Internal only **/
 struct object *lookup_object(const unsigned char *sha1);
 
diff --git a/tag.c b/tag.c
index f62bcdd..8bc6840 100644
--- a/tag.c
+++ b/tag.c
@@ -33,6 +33,14 @@ struct tag *lookup_tag(const unsigned char *sha1)
         return (struct tag *) obj;
 }
 
+void release_tag(struct tag *tag)
+{
+	if (tag->tag)
+		free(tag->tag);
+	if (tag->signature)
+		free(tag->signature);
+}
+
 int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 {
 	int typelen, taglen;
diff --git a/tag.h b/tag.h
index 7a0cb00..fbc6048 100644
--- a/tag.h
+++ b/tag.h
@@ -12,6 +12,8 @@ struct tag {
 	char *signature; /* not actually implemented */
 };
 
+void release_tag(struct tag *tag);
+
 extern struct tag *lookup_tag(const unsigned char *sha1);
 extern int parse_tag_buffer(struct tag *item, void *data, unsigned long size);
 extern int parse_tag(struct tag *item);
diff --git a/tree.c b/tree.c
index 8c0819f..ee99bc6 100644
--- a/tree.c
+++ b/tree.c
@@ -202,6 +202,12 @@ struct tree *lookup_tree(const unsigned char *sha1)
 	return (struct tree *) obj;
 }
 
+void release_tree(struct tree *tree)
+{
+	if (tree->buffer)
+		free(tree->buffer);
+}
+
 /*
  * NOTE! Tree refs to external git repositories
  * (ie gitlinks) do not count as real references.
diff --git a/tree.h b/tree.h
index dd25c53..f8372a2 100644
--- a/tree.h
+++ b/tree.h
@@ -12,6 +12,7 @@ struct tree {
 };
 
 struct tree *lookup_tree(const unsigned char *sha1);
+void release_tree(struct tree *tree);
 
 int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size);
 
-- 
1.5.3.6.2112.ge2263
Johannes Schindelin· Dec 2, 2007, 02:54 UTC · re: Johannes Schindelin · lore

[PATCH 2/3] Include the objects needed for the builtin functions into libgit.a

For the upcoming change in execv_git_cmd() to call builtin functions directly, it is necessary to be able to access the builtins, so move the corresponding objects into libgit.a.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)
Show changes to Makefile +9 −5
diff --git a/Makefile b/Makefile
index f000a5e..f9a62eb 100644
--- a/Makefile
+++ b/Makefile
@@ -314,7 +314,8 @@ LIB_OBJS = \
 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
 	color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
 	convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \
-	transport.o bundle.o walker.o parse-options.o
+	transport.o bundle.o walker.o parse-options.o \
+	$(BUILTIN_OBJS)
 
 BUILTIN_OBJS = \
 	builtin-add.o \
@@ -785,12 +786,12 @@ strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
 git.o: git.c common-cmds.h GIT-CFLAGS
-	$(QUIET_CC)$(CC) -DGIT_VERSION='"$(GIT_VERSION)"' \
+	$(QUIET_CC)$(CC) \
 		$(ALL_CFLAGS) -c $(filter %.c,$^)
 
 git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
-		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
+		$(ALL_LDFLAGS) $(LIBS)
 
 help.o: common-cmds.h
 
@@ -894,7 +895,10 @@ git.o git.spec \
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 
 exec_cmd.o: exec_cmd.c GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) '-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' $<
+	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) \
+		-DGIT_EXEC_PATH='"$(gitexecdir_SQ)"' \
+		-DGIT_VERSION='"$(GIT_VERSION)"' \
+		$<
 builtin-init-db.o: builtin-init-db.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"' $<
 
@@ -920,7 +924,7 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
-$(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)
+$(LIB_OBJS): $(LIB_H)
 $(patsubst git-%$X,%.o,$(PROGRAMS)): $(LIB_H) $(wildcard */*.h)
 builtin-revert.o builtin-runstatus.o wt-status.o: wt-status.h
 
-- 
1.5.3.6.2112.ge2263
Johannes Schindelin· Dec 2, 2007, 02:55 UTC · re: Johannes Schindelin · lore

[PATCH 3/3] Introduce execv_git_builtin() and use it

You can call execv_git_builtin() to execute a builtin command. This function is a reborn handle_internal_command() from git.c, and has the semantics of execv_git_cmd(), i.e. it exits with the exit code of the builtin if there is a matching builtin, but it avoids the real execvp() call.

This function calls release_all_objects() and discard_cache() to start from a clean slate (this, along with the calculation of argc, is the only difference from a straight code move).

The test suite passes, which at least does not contradict the hypothesis that this is good enough. However, it might be necessary to de-initialize more global variables.

The function execv_git_cmd() and git.c's main() function were changed to take advantage of execv_git_builtin().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 exec_cmd.c |  178 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 exec_cmd.h |    1 +
 git.c      |  172 +---------------------------------------------------------
 3 files changed, 180 insertions(+), 171 deletions(-)
Show changes to 3 files +180 −171

exec_cmd.c, exec_cmd.h, git.c

diff --git a/exec_cmd.c b/exec_cmd.c
index 2d0a758..ac21181 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -1,6 +1,8 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "quote.h"
+#include "builtin.h"
+#include "object.h"
 #define MAX_ARGS	32
 
 extern char **environ;
@@ -63,11 +65,187 @@ void setup_path(const char *cmd_path)
 	strbuf_release(&new_path);
 }
 
+const char git_usage_string[] =
+	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
+
+const char git_version_string[] = GIT_VERSION;
+
+#define RUN_SETUP	(1<<0)
+#define USE_PAGER	(1<<1)
+/*
+ * require working tree to be present -- anything uses this needs
+ * RUN_SETUP for reading from the configuration file.
+ */
+#define NEED_WORK_TREE	(1<<2)
+
+struct cmd_struct {
+	const char *cmd;
+	int (*fn)(int, const char **, const char *);
+	int option;
+};
+
+static int run_command(struct cmd_struct *p, int argc, const char **argv)
+{
+	int status;
+	struct stat st;
+	const char *prefix;
+
+	prefix = NULL;
+	if (p->option & RUN_SETUP)
+		prefix = setup_git_directory();
+	if (p->option & USE_PAGER)
+		setup_pager();
+	if (p->option & NEED_WORK_TREE)
+		setup_work_tree();
+
+	trace_argv_printf(argv, argc, "trace: built-in: git");
+
+	status = p->fn(argc, argv, prefix);
+	if (status)
+		return status & 0xff;
+
+	/* Somebody closed stdout? */
+	if (fstat(fileno(stdout), &st))
+		return 0;
+	/* Ignore write errors for pipes and sockets.. */
+	if (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode))
+		return 0;
+
+	/* Check for ENOSPC and EIO errors.. */
+	if (fflush(stdout))
+		die("write failure on standard output: %s", strerror(errno));
+	if (ferror(stdout))
+		die("unknown write failure on standard output");
+	if (fclose(stdout))
+		die("close failed on standard output: %s", strerror(errno));
+	return 0;
+}
+
+int execv_git_builtin(const char **argv)
+{
+	const char *cmd = argv[0];
+	static struct cmd_struct commands[] = {
+		{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
+		{ "annotate", cmd_annotate, RUN_SETUP },
+		{ "apply", cmd_apply },
+		{ "archive", cmd_archive },
+		{ "blame", cmd_blame, RUN_SETUP },
+		{ "branch", cmd_branch, RUN_SETUP },
+		{ "bundle", cmd_bundle },
+		{ "cat-file", cmd_cat_file, RUN_SETUP },
+		{ "checkout-index", cmd_checkout_index,
+			RUN_SETUP | NEED_WORK_TREE},
+		{ "check-ref-format", cmd_check_ref_format },
+		{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
+		{ "cherry", cmd_cherry, RUN_SETUP },
+		{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+		{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
+		{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
+		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
+		{ "config", cmd_config },
+		{ "count-objects", cmd_count_objects, RUN_SETUP },
+		{ "describe", cmd_describe, RUN_SETUP },
+		{ "diff", cmd_diff },
+		{ "diff-files", cmd_diff_files },
+		{ "diff-index", cmd_diff_index, RUN_SETUP },
+		{ "diff-tree", cmd_diff_tree, RUN_SETUP },
+		{ "fast-export", cmd_fast_export, RUN_SETUP },
+		{ "fetch", cmd_fetch, RUN_SETUP },
+		{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
+		{ "fetch--tool", cmd_fetch__tool, RUN_SETUP },
+		{ "fmt", cmd_fmt },
+		{ "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
+		{ "for-each-ref", cmd_for_each_ref, RUN_SETUP },
+		{ "format-patch", cmd_format_patch, RUN_SETUP },
+		{ "fsck", cmd_fsck, RUN_SETUP },
+		{ "fsck-objects", cmd_fsck, RUN_SETUP },
+		{ "gc", cmd_gc, RUN_SETUP },
+		{ "get-tar-commit-id", cmd_get_tar_commit_id },
+		{ "grep", cmd_grep, RUN_SETUP | USE_PAGER },
+		{ "help", cmd_help },
+#ifndef NO_CURL
+		{ "http-fetch", cmd_http_fetch, RUN_SETUP },
+#endif
+		{ "init", cmd_init_db },
+		{ "init-db", cmd_init_db },
+		{ "log", cmd_log, RUN_SETUP | USE_PAGER },
+		{ "ls-files", cmd_ls_files, RUN_SETUP },
+		{ "ls-tree", cmd_ls_tree, RUN_SETUP },
+		{ "ls-remote", cmd_ls_remote },
+		{ "mailinfo", cmd_mailinfo },
+		{ "mailsplit", cmd_mailsplit },
+		{ "merge-base", cmd_merge_base, RUN_SETUP },
+		{ "merge-file", cmd_merge_file },
+		{ "merge-ours", cmd_merge_ours, RUN_SETUP },
+		{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
+		{ "name-rev", cmd_name_rev, RUN_SETUP },
+		{ "pack-objects", cmd_pack_objects, RUN_SETUP },
+		{ "peek-remote", cmd_ls_remote },
+		{ "pickaxe", cmd_blame, RUN_SETUP },
+		{ "prune", cmd_prune, RUN_SETUP },
+		{ "prune-packed", cmd_prune_packed, RUN_SETUP },
+		{ "push", cmd_push, RUN_SETUP },
+		{ "read-tree", cmd_read_tree, RUN_SETUP },
+		{ "reflog", cmd_reflog, RUN_SETUP },
+		{ "repo-config", cmd_config },
+		{ "rerere", cmd_rerere, RUN_SETUP },
+		{ "reset", cmd_reset, RUN_SETUP },
+		{ "rev-list", cmd_rev_list, RUN_SETUP },
+		{ "rev-parse", cmd_rev_parse },
+		{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
+		{ "rm", cmd_rm, RUN_SETUP },
+		{ "send-pack", cmd_send_pack, RUN_SETUP },
+		{ "shortlog", cmd_shortlog, RUN_SETUP | USE_PAGER },
+		{ "show-branch", cmd_show_branch, RUN_SETUP },
+		{ "show", cmd_show, RUN_SETUP | USE_PAGER },
+		{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
+		{ "stripspace", cmd_stripspace },
+		{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
+		{ "tag", cmd_tag, RUN_SETUP },
+		{ "tar-tree", cmd_tar_tree },
+		{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
+		{ "update-index", cmd_update_index, RUN_SETUP },
+		{ "update-ref", cmd_update_ref, RUN_SETUP },
+		{ "upload-archive", cmd_upload_archive },
+		{ "verify-tag", cmd_verify_tag, RUN_SETUP },
+		{ "version", cmd_version },
+		{ "whatchanged", cmd_whatchanged, RUN_SETUP | USE_PAGER },
+		{ "write-tree", cmd_write_tree, RUN_SETUP },
+		{ "verify-pack", cmd_verify_pack },
+		{ "show-ref", cmd_show_ref, RUN_SETUP },
+		{ "pack-refs", cmd_pack_refs, RUN_SETUP },
+	};
+	int i;
+
+	/* Turn "git cmd --help" into "git help cmd" */
+	if (argv[0] && argv[1] && !strcmp(argv[1], "--help")) {
+		argv[1] = argv[0];
+		argv[0] = cmd = "help";
+	}
+
+	for (i = 0; i < ARRAY_SIZE(commands); i++) {
+		int argc;
+		struct cmd_struct *p = commands+i;
+		if (strcmp(p->cmd, cmd))
+			continue;
+		release_all_objects();
+		discard_cache();
+		for (argc = 0; argv[argc]; argc++)
+			; /* do nothing */
+		exit(run_command(p, argc, argv));
+	}
+	return -1;
+}
+
 int execv_git_cmd(const char **argv)
 {
 	struct strbuf cmd;
 	const char *tmp;
 
+	/* Try builtin first... */
+	execv_git_builtin(argv);
+
+	/* ... and then external commands */
 	strbuf_init(&cmd, 0);
 	strbuf_addf(&cmd, "git-%s", argv[0]);
 
diff --git a/exec_cmd.h b/exec_cmd.h
index a892355..bb15425 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -4,6 +4,7 @@
 extern void git_set_argv_exec_path(const char *exec_path);
 extern const char* git_exec_path(void);
 extern void setup_path(const char *);
+extern int execv_git_builtin(const char **argv); /* NULL terminated */
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 extern int execl_git_cmd(const char *cmd, ...);
 
diff --git a/git.c b/git.c
index 38b01ca..1523c4a 100644
--- a/git.c
+++ b/git.c
@@ -3,9 +3,6 @@
 #include "cache.h"
 #include "quote.h"
 
-const char git_usage_string[] =
-	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
-
 static int handle_options(const char*** argv, int* argc, int* envchanged)
 {
 	int handled = 0;
@@ -231,169 +228,6 @@ static int handle_alias(int *argcp, const char ***argv)
 	return ret;
 }
 
-const char git_version_string[] = GIT_VERSION;
-
-#define RUN_SETUP	(1<<0)
-#define USE_PAGER	(1<<1)
-/*
- * require working tree to be present -- anything uses this needs
- * RUN_SETUP for reading from the configuration file.
- */
-#define NEED_WORK_TREE	(1<<2)
-
-struct cmd_struct {
-	const char *cmd;
-	int (*fn)(int, const char **, const char *);
-	int option;
-};
-
-static int run_command(struct cmd_struct *p, int argc, const char **argv)
-{
-	int status;
-	struct stat st;
-	const char *prefix;
-
-	prefix = NULL;
-	if (p->option & RUN_SETUP)
-		prefix = setup_git_directory();
-	if (p->option & USE_PAGER)
-		setup_pager();
-	if (p->option & NEED_WORK_TREE)
-		setup_work_tree();
-
-	trace_argv_printf(argv, argc, "trace: built-in: git");
-
-	status = p->fn(argc, argv, prefix);
-	if (status)
-		return status & 0xff;
-
-	/* Somebody closed stdout? */
-	if (fstat(fileno(stdout), &st))
-		return 0;
-	/* Ignore write errors for pipes and sockets.. */
-	if (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode))
-		return 0;
-
-	/* Check for ENOSPC and EIO errors.. */
-	if (fflush(stdout))
-		die("write failure on standard output: %s", strerror(errno));
-	if (ferror(stdout))
-		die("unknown write failure on standard output");
-	if (fclose(stdout))
-		die("close failed on standard output: %s", strerror(errno));
-	return 0;
-}
-
-static void handle_internal_command(int argc, const char **argv)
-{
-	const char *cmd = argv[0];
-	static struct cmd_struct commands[] = {
-		{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
-		{ "annotate", cmd_annotate, RUN_SETUP },
-		{ "apply", cmd_apply },
-		{ "archive", cmd_archive },
-		{ "blame", cmd_blame, RUN_SETUP },
-		{ "branch", cmd_branch, RUN_SETUP },
-		{ "bundle", cmd_bundle },
-		{ "cat-file", cmd_cat_file, RUN_SETUP },
-		{ "checkout-index", cmd_checkout_index,
-			RUN_SETUP | NEED_WORK_TREE},
-		{ "check-ref-format", cmd_check_ref_format },
-		{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
-		{ "cherry", cmd_cherry, RUN_SETUP },
-		{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
-		{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
-		{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
-		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
-		{ "config", cmd_config },
-		{ "count-objects", cmd_count_objects, RUN_SETUP },
-		{ "describe", cmd_describe, RUN_SETUP },
-		{ "diff", cmd_diff },
-		{ "diff-files", cmd_diff_files },
-		{ "diff-index", cmd_diff_index, RUN_SETUP },
-		{ "diff-tree", cmd_diff_tree, RUN_SETUP },
-		{ "fast-export", cmd_fast_export, RUN_SETUP },
-		{ "fetch", cmd_fetch, RUN_SETUP },
-		{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
-		{ "fetch--tool", cmd_fetch__tool, RUN_SETUP },
-		{ "fmt", cmd_fmt },
-		{ "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
-		{ "for-each-ref", cmd_for_each_ref, RUN_SETUP },
-		{ "format-patch", cmd_format_patch, RUN_SETUP },
-		{ "fsck", cmd_fsck, RUN_SETUP },
-		{ "fsck-objects", cmd_fsck, RUN_SETUP },
-		{ "gc", cmd_gc, RUN_SETUP },
-		{ "get-tar-commit-id", cmd_get_tar_commit_id },
-		{ "grep", cmd_grep, RUN_SETUP | USE_PAGER },
-		{ "help", cmd_help },
-#ifndef NO_CURL
-		{ "http-fetch", cmd_http_fetch, RUN_SETUP },
-#endif
-		{ "init", cmd_init_db },
-		{ "init-db", cmd_init_db },
-		{ "log", cmd_log, RUN_SETUP | USE_PAGER },
-		{ "ls-files", cmd_ls_files, RUN_SETUP },
-		{ "ls-tree", cmd_ls_tree, RUN_SETUP },
-		{ "ls-remote", cmd_ls_remote },
-		{ "mailinfo", cmd_mailinfo },
-		{ "mailsplit", cmd_mailsplit },
-		{ "merge-base", cmd_merge_base, RUN_SETUP },
-		{ "merge-file", cmd_merge_file },
-		{ "merge-ours", cmd_merge_ours, RUN_SETUP },
-		{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
-		{ "name-rev", cmd_name_rev, RUN_SETUP },
-		{ "pack-objects", cmd_pack_objects, RUN_SETUP },
-		{ "peek-remote", cmd_ls_remote },
-		{ "pickaxe", cmd_blame, RUN_SETUP },
-		{ "prune", cmd_prune, RUN_SETUP },
-		{ "prune-packed", cmd_prune_packed, RUN_SETUP },
-		{ "push", cmd_push, RUN_SETUP },
-		{ "read-tree", cmd_read_tree, RUN_SETUP },
-		{ "reflog", cmd_reflog, RUN_SETUP },
-		{ "repo-config", cmd_config },
-		{ "rerere", cmd_rerere, RUN_SETUP },
-		{ "reset", cmd_reset, RUN_SETUP },
-		{ "rev-list", cmd_rev_list, RUN_SETUP },
-		{ "rev-parse", cmd_rev_parse },
-		{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
-		{ "rm", cmd_rm, RUN_SETUP },
-		{ "send-pack", cmd_send_pack, RUN_SETUP },
-		{ "shortlog", cmd_shortlog, RUN_SETUP | USE_PAGER },
-		{ "show-branch", cmd_show_branch, RUN_SETUP },
-		{ "show", cmd_show, RUN_SETUP | USE_PAGER },
-		{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
-		{ "stripspace", cmd_stripspace },
-		{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
-		{ "tag", cmd_tag, RUN_SETUP },
-		{ "tar-tree", cmd_tar_tree },
-		{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
-		{ "update-index", cmd_update_index, RUN_SETUP },
-		{ "update-ref", cmd_update_ref, RUN_SETUP },
-		{ "upload-archive", cmd_upload_archive },
-		{ "verify-tag", cmd_verify_tag, RUN_SETUP },
-		{ "version", cmd_version },
-		{ "whatchanged", cmd_whatchanged, RUN_SETUP | USE_PAGER },
-		{ "write-tree", cmd_write_tree, RUN_SETUP },
-		{ "verify-pack", cmd_verify_pack },
-		{ "show-ref", cmd_show_ref, RUN_SETUP },
-		{ "pack-refs", cmd_pack_refs, RUN_SETUP },
-	};
-	int i;
-
-	/* Turn "git cmd --help" into "git help cmd" */
-	if (argc > 1 && !strcmp(argv[1], "--help")) {
-		argv[1] = argv[0];
-		argv[0] = cmd = "help";
-	}
-
-	for (i = 0; i < ARRAY_SIZE(commands); i++) {
-		struct cmd_struct *p = commands+i;
-		if (strcmp(p->cmd, cmd))
-			continue;
-		exit(run_command(p, argc, argv));
-	}
-}
-
 int main(int argc, const char **argv)
 {
 	const char *cmd = argv[0] ? argv[0] : "git-help";
@@ -425,7 +259,7 @@ int main(int argc, const char **argv)
 	if (!prefixcmp(cmd, "git-")) {
 		cmd += 4;
 		argv[0] = cmd;
-		handle_internal_command(argc, argv);
+		execv_git_builtin(argv);
 		die("cannot handle %s internally", cmd);
 	}
 
@@ -453,10 +287,6 @@ int main(int argc, const char **argv)
 	setup_path(cmd_path);
 
 	while (1) {
-		/* See if it's an internal command */
-		handle_internal_command(argc, argv);
-
-		/* .. then try the external ones */
 		execv_git_cmd(argv);
 
 		/* It could be an alias -- this works around the insanity
-- 
1.5.3.6.2112.ge2263
Johannes Schindelin· Dec 2, 2007, 03:16 UTC · re: Johannes Schindelin · lore

[REPLACEMENT PATCH 3/3] Introduce execv_git_builtin() and use it

You can call execv_git_builtin() to execute a builtin command. This function is a reborn handle_internal_command() from git.c, and has the semantics of execv_git_cmd(), i.e. it exits with the exit code of the builtin if there is a matching builtin, but it avoids the real execvp() call.

This function calls release_all_objects() and discard_cache() to start from a clean slate (this, along with the calculation of argc, is the only difference from a straight code move).

The test suite passes, which at least does not contradict the hypothesis that this is good enough. However, it might be necessary to de-initialize more global variables.

The function execv_git_cmd() and git.c's main() function were changed to take advantage of execv_git_builtin().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	On Sun, 2 Dec 2007, Johannes Schindelin wrote:
	> Hi,
	> 
	> On Sun, 2 Dec 2007, Johannes Schindelin wrote:
	> 
	> > +		{ "fmt", cmd_fmt },
	> 
	> Ah, well.  This slipped in by mistake.  Will resend in a few 
	> minutes.
	Here we go.
 exec_cmd.c |  176 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 exec_cmd.h |    1 +
 git.c      |  170 +---------------------------------------------------------
 3 files changed, 178 insertions(+), 169 deletions(-)
Show changes to 3 files +178 −169

exec_cmd.c, exec_cmd.h, git.c

diff --git a/exec_cmd.c b/exec_cmd.c
index 2d0a758..745b951 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -1,6 +1,8 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "quote.h"
+#include "builtin.h"
+#include "object.h"
 #define MAX_ARGS	32
 
 extern char **environ;
@@ -63,11 +65,185 @@ void setup_path(const char *cmd_path)
 	strbuf_release(&new_path);
 }
 
+const char git_usage_string[] =
+	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
+
+const char git_version_string[] = GIT_VERSION;
+
+#define RUN_SETUP	(1<<0)
+#define USE_PAGER	(1<<1)
+/*
+ * require working tree to be present -- anything uses this needs
+ * RUN_SETUP for reading from the configuration file.
+ */
+#define NEED_WORK_TREE	(1<<2)
+
+struct cmd_struct {
+	const char *cmd;
+	int (*fn)(int, const char **, const char *);
+	int option;
+};
+
+static int run_command(struct cmd_struct *p, int argc, const char **argv)
+{
+	int status;
+	struct stat st;
+	const char *prefix;
+
+	prefix = NULL;
+	if (p->option & RUN_SETUP)
+		prefix = setup_git_directory();
+	if (p->option & USE_PAGER)
+		setup_pager();
+	if (p->option & NEED_WORK_TREE)
+		setup_work_tree();
+
+	trace_argv_printf(argv, argc, "trace: built-in: git");
+
+	status = p->fn(argc, argv, prefix);
+	if (status)
+		return status & 0xff;
+
+	/* Somebody closed stdout? */
+	if (fstat(fileno(stdout), &st))
+		return 0;
+	/* Ignore write errors for pipes and sockets.. */
+	if (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode))
+		return 0;
+
+	/* Check for ENOSPC and EIO errors.. */
+	if (fflush(stdout))
+		die("write failure on standard output: %s", strerror(errno));
+	if (ferror(stdout))
+		die("unknown write failure on standard output");
+	if (fclose(stdout))
+		die("close failed on standard output: %s", strerror(errno));
+	return 0;
+}
+
+int execv_git_builtin(const char **argv)
+{
+	const char *cmd = argv[0];
+	static struct cmd_struct commands[] = {
+		{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
+		{ "annotate", cmd_annotate, RUN_SETUP },
+		{ "apply", cmd_apply },
+		{ "archive", cmd_archive },
+		{ "blame", cmd_blame, RUN_SETUP },
+		{ "branch", cmd_branch, RUN_SETUP },
+		{ "bundle", cmd_bundle },
+		{ "cat-file", cmd_cat_file, RUN_SETUP },
+		{ "checkout-index", cmd_checkout_index,
+			RUN_SETUP | NEED_WORK_TREE},
+		{ "check-ref-format", cmd_check_ref_format },
+		{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
+		{ "cherry", cmd_cherry, RUN_SETUP },
+		{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+		{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
+		{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
+		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
+		{ "config", cmd_config },
+		{ "count-objects", cmd_count_objects, RUN_SETUP },
+		{ "describe", cmd_describe, RUN_SETUP },
+		{ "diff", cmd_diff },
+		{ "diff-files", cmd_diff_files },
+		{ "diff-index", cmd_diff_index, RUN_SETUP },
+		{ "diff-tree", cmd_diff_tree, RUN_SETUP },
+		{ "fetch", cmd_fetch, RUN_SETUP },
+		{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
+		{ "fetch--tool", cmd_fetch__tool, RUN_SETUP },
+		{ "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
+		{ "for-each-ref", cmd_for_each_ref, RUN_SETUP },
+		{ "format-patch", cmd_format_patch, RUN_SETUP },
+		{ "fsck", cmd_fsck, RUN_SETUP },
+		{ "fsck-objects", cmd_fsck, RUN_SETUP },
+		{ "gc", cmd_gc, RUN_SETUP },
+		{ "get-tar-commit-id", cmd_get_tar_commit_id },
+		{ "grep", cmd_grep, RUN_SETUP | USE_PAGER },
+		{ "help", cmd_help },
+#ifndef NO_CURL
+		{ "http-fetch", cmd_http_fetch, RUN_SETUP },
+#endif
+		{ "init", cmd_init_db },
+		{ "init-db", cmd_init_db },
+		{ "log", cmd_log, RUN_SETUP | USE_PAGER },
+		{ "ls-files", cmd_ls_files, RUN_SETUP },
+		{ "ls-tree", cmd_ls_tree, RUN_SETUP },
+		{ "ls-remote", cmd_ls_remote },
+		{ "mailinfo", cmd_mailinfo },
+		{ "mailsplit", cmd_mailsplit },
+		{ "merge-base", cmd_merge_base, RUN_SETUP },
+		{ "merge-file", cmd_merge_file },
+		{ "merge-ours", cmd_merge_ours, RUN_SETUP },
+		{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
+		{ "name-rev", cmd_name_rev, RUN_SETUP },
+		{ "pack-objects", cmd_pack_objects, RUN_SETUP },
+		{ "peek-remote", cmd_ls_remote },
+		{ "pickaxe", cmd_blame, RUN_SETUP },
+		{ "prune", cmd_prune, RUN_SETUP },
+		{ "prune-packed", cmd_prune_packed, RUN_SETUP },
+		{ "push", cmd_push, RUN_SETUP },
+		{ "read-tree", cmd_read_tree, RUN_SETUP },
+		{ "reflog", cmd_reflog, RUN_SETUP },
+		{ "repo-config", cmd_config },
+		{ "rerere", cmd_rerere, RUN_SETUP },
+		{ "reset", cmd_reset, RUN_SETUP },
+		{ "rev-list", cmd_rev_list, RUN_SETUP },
+		{ "rev-parse", cmd_rev_parse },
+		{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
+		{ "rm", cmd_rm, RUN_SETUP },
+		{ "send-pack", cmd_send_pack, RUN_SETUP },
+		{ "shortlog", cmd_shortlog, RUN_SETUP | USE_PAGER },
+		{ "show-branch", cmd_show_branch, RUN_SETUP },
+		{ "show", cmd_show, RUN_SETUP | USE_PAGER },
+		{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
+		{ "stripspace", cmd_stripspace },
+		{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
+		{ "tag", cmd_tag, RUN_SETUP },
+		{ "tar-tree", cmd_tar_tree },
+		{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
+		{ "update-index", cmd_update_index, RUN_SETUP },
+		{ "update-ref", cmd_update_ref, RUN_SETUP },
+		{ "upload-archive", cmd_upload_archive },
+		{ "verify-tag", cmd_verify_tag, RUN_SETUP },
+		{ "version", cmd_version },
+		{ "whatchanged", cmd_whatchanged, RUN_SETUP | USE_PAGER },
+		{ "write-tree", cmd_write_tree, RUN_SETUP },
+		{ "verify-pack", cmd_verify_pack },
+		{ "show-ref", cmd_show_ref, RUN_SETUP },
+		{ "pack-refs", cmd_pack_refs, RUN_SETUP },
+	};
+	int i;
+
+	/* Turn "git cmd --help" into "git help cmd" */
+	if (argv[0] && argv[1] && !strcmp(argv[1], "--help")) {
+		argv[1] = argv[0];
+		argv[0] = cmd = "help";
+	}
+
+	for (i = 0; i < ARRAY_SIZE(commands); i++) {
+		int argc;
+		struct cmd_struct *p = commands+i;
+		if (strcmp(p->cmd, cmd))
+			continue;
+		release_all_objects();
+		discard_cache();
+		for (argc = 0; argv[argc]; argc++)
+			; /* do nothing */
+		exit(run_command(p, argc, argv));
+	}
+	return -1;
+}
+
 int execv_git_cmd(const char **argv)
 {
 	struct strbuf cmd;
 	const char *tmp;
 
+	/* Try builtin first... */
+	execv_git_builtin(argv);
+
+	/* ... and then external commands */
 	strbuf_init(&cmd, 0);
 	strbuf_addf(&cmd, "git-%s", argv[0]);
 
diff --git a/exec_cmd.h b/exec_cmd.h
index a892355..bb15425 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -4,6 +4,7 @@
 extern void git_set_argv_exec_path(const char *exec_path);
 extern const char* git_exec_path(void);
 extern void setup_path(const char *);
+extern int execv_git_builtin(const char **argv); /* NULL terminated */
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 extern int execl_git_cmd(const char *cmd, ...);
 
diff --git a/git.c b/git.c
index 95296aa..1523c4a 100644
--- a/git.c
+++ b/git.c
@@ -3,9 +3,6 @@
 #include "cache.h"
 #include "quote.h"
 
-const char git_usage_string[] =
-	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
-
 static int handle_options(const char*** argv, int* argc, int* envchanged)
 {
 	int handled = 0;
@@ -231,167 +228,6 @@ static int handle_alias(int *argcp, const char ***argv)
 	return ret;
 }
 
-const char git_version_string[] = GIT_VERSION;
-
-#define RUN_SETUP	(1<<0)
-#define USE_PAGER	(1<<1)
-/*
- * require working tree to be present -- anything uses this needs
- * RUN_SETUP for reading from the configuration file.
- */
-#define NEED_WORK_TREE	(1<<2)
-
-struct cmd_struct {
-	const char *cmd;
-	int (*fn)(int, const char **, const char *);
-	int option;
-};
-
-static int run_command(struct cmd_struct *p, int argc, const char **argv)
-{
-	int status;
-	struct stat st;
-	const char *prefix;
-
-	prefix = NULL;
-	if (p->option & RUN_SETUP)
-		prefix = setup_git_directory();
-	if (p->option & USE_PAGER)
-		setup_pager();
-	if (p->option & NEED_WORK_TREE)
-		setup_work_tree();
-
-	trace_argv_printf(argv, argc, "trace: built-in: git");
-
-	status = p->fn(argc, argv, prefix);
-	if (status)
-		return status & 0xff;
-
-	/* Somebody closed stdout? */
-	if (fstat(fileno(stdout), &st))
-		return 0;
-	/* Ignore write errors for pipes and sockets.. */
-	if (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode))
-		return 0;
-
-	/* Check for ENOSPC and EIO errors.. */
-	if (fflush(stdout))
-		die("write failure on standard output: %s", strerror(errno));
-	if (ferror(stdout))
-		die("unknown write failure on standard output");
-	if (fclose(stdout))
-		die("close failed on standard output: %s", strerror(errno));
-	return 0;
-}
-
-static void handle_internal_command(int argc, const char **argv)
-{
-	const char *cmd = argv[0];
-	static struct cmd_struct commands[] = {
-		{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
-		{ "annotate", cmd_annotate, RUN_SETUP },
-		{ "apply", cmd_apply },
-		{ "archive", cmd_archive },
-		{ "blame", cmd_blame, RUN_SETUP },
-		{ "branch", cmd_branch, RUN_SETUP },
-		{ "bundle", cmd_bundle },
-		{ "cat-file", cmd_cat_file, RUN_SETUP },
-		{ "checkout-index", cmd_checkout_index,
-			RUN_SETUP | NEED_WORK_TREE},
-		{ "check-ref-format", cmd_check_ref_format },
-		{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
-		{ "cherry", cmd_cherry, RUN_SETUP },
-		{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
-		{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
-		{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
-		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
-		{ "config", cmd_config },
-		{ "count-objects", cmd_count_objects, RUN_SETUP },
-		{ "describe", cmd_describe, RUN_SETUP },
-		{ "diff", cmd_diff },
-		{ "diff-files", cmd_diff_files },
-		{ "diff-index", cmd_diff_index, RUN_SETUP },
-		{ "diff-tree", cmd_diff_tree, RUN_SETUP },
-		{ "fetch", cmd_fetch, RUN_SETUP },
-		{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
-		{ "fetch--tool", cmd_fetch__tool, RUN_SETUP },
-		{ "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
-		{ "for-each-ref", cmd_for_each_ref, RUN_SETUP },
-		{ "format-patch", cmd_format_patch, RUN_SETUP },
-		{ "fsck", cmd_fsck, RUN_SETUP },
-		{ "fsck-objects", cmd_fsck, RUN_SETUP },
-		{ "gc", cmd_gc, RUN_SETUP },
-		{ "get-tar-commit-id", cmd_get_tar_commit_id },
-		{ "grep", cmd_grep, RUN_SETUP | USE_PAGER },
-		{ "help", cmd_help },
-#ifndef NO_CURL
-		{ "http-fetch", cmd_http_fetch, RUN_SETUP },
-#endif
-		{ "init", cmd_init_db },
-		{ "init-db", cmd_init_db },
-		{ "log", cmd_log, RUN_SETUP | USE_PAGER },
-		{ "ls-files", cmd_ls_files, RUN_SETUP },
-		{ "ls-tree", cmd_ls_tree, RUN_SETUP },
-		{ "ls-remote", cmd_ls_remote },
-		{ "mailinfo", cmd_mailinfo },
-		{ "mailsplit", cmd_mailsplit },
-		{ "merge-base", cmd_merge_base, RUN_SETUP },
-		{ "merge-file", cmd_merge_file },
-		{ "merge-ours", cmd_merge_ours, RUN_SETUP },
-		{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
-		{ "name-rev", cmd_name_rev, RUN_SETUP },
-		{ "pack-objects", cmd_pack_objects, RUN_SETUP },
-		{ "peek-remote", cmd_ls_remote },
-		{ "pickaxe", cmd_blame, RUN_SETUP },
-		{ "prune", cmd_prune, RUN_SETUP },
-		{ "prune-packed", cmd_prune_packed, RUN_SETUP },
-		{ "push", cmd_push, RUN_SETUP },
-		{ "read-tree", cmd_read_tree, RUN_SETUP },
-		{ "reflog", cmd_reflog, RUN_SETUP },
-		{ "repo-config", cmd_config },
-		{ "rerere", cmd_rerere, RUN_SETUP },
-		{ "reset", cmd_reset, RUN_SETUP },
-		{ "rev-list", cmd_rev_list, RUN_SETUP },
-		{ "rev-parse", cmd_rev_parse },
-		{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
-		{ "rm", cmd_rm, RUN_SETUP },
-		{ "send-pack", cmd_send_pack, RUN_SETUP },
-		{ "shortlog", cmd_shortlog, RUN_SETUP | USE_PAGER },
-		{ "show-branch", cmd_show_branch, RUN_SETUP },
-		{ "show", cmd_show, RUN_SETUP | USE_PAGER },
-		{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
-		{ "stripspace", cmd_stripspace },
-		{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
-		{ "tag", cmd_tag, RUN_SETUP },
-		{ "tar-tree", cmd_tar_tree },
-		{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
-		{ "update-index", cmd_update_index, RUN_SETUP },
-		{ "update-ref", cmd_update_ref, RUN_SETUP },
-		{ "upload-archive", cmd_upload_archive },
-		{ "verify-tag", cmd_verify_tag, RUN_SETUP },
-		{ "version", cmd_version },
-		{ "whatchanged", cmd_whatchanged, RUN_SETUP | USE_PAGER },
-		{ "write-tree", cmd_write_tree, RUN_SETUP },
-		{ "verify-pack", cmd_verify_pack },
-		{ "show-ref", cmd_show_ref, RUN_SETUP },
-		{ "pack-refs", cmd_pack_refs, RUN_SETUP },
-	};
-	int i;
-
-	/* Turn "git cmd --help" into "git help cmd" */
-	if (argc > 1 && !strcmp(argv[1], "--help")) {
-		argv[1] = argv[0];
-		argv[0] = cmd = "help";
-	}
-
-	for (i = 0; i < ARRAY_SIZE(commands); i++) {
-		struct cmd_struct *p = commands+i;
-		if (strcmp(p->cmd, cmd))
-			continue;
-		exit(run_command(p, argc, argv));
-	}
-}
-
 int main(int argc, const char **argv)
 {
 	const char *cmd = argv[0] ? argv[0] : "git-help";
@@ -423,7 +259,7 @@ int main(int argc, const char **argv)
 	if (!prefixcmp(cmd, "git-")) {
 		cmd += 4;
 		argv[0] = cmd;
-		handle_internal_command(argc, argv);
+		execv_git_builtin(argv);
 		die("cannot handle %s internally", cmd);
 	}
 
@@ -451,10 +287,6 @@ int main(int argc, const char **argv)
 	setup_path(cmd_path);
 
 	while (1) {
-		/* See if it's an internal command */
-		handle_internal_command(argc, argv);
-
-		/* .. then try the external ones */
 		execv_git_cmd(argv);
 
 		/* It could be an alias -- this works around the insanity
-- 
1.5.3.6.2112.ge2263
Junio C Hamano· Dec 2, 2007, 05:19 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/3] Call builtin functions directly, was Re: [PATCH] transport.c: call dash-less form of receive-pack and upload-pack on remote

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted lines
> Okay, I bit the apple and tried to move the builtins into the library, and 
> rename handle_internal_command into execv_git_builtin(), moving it into 
> exec-cmd.c.
>
> Big mistake.

I really feel this should not go in. Anything called exec _should_ assure the callers that the new command will start from a clean slate, and the way to give that assurance is by actually doing exec(), not introducing "clean-up" functions for random things we can think of (like cached objects) and risking of forgetting some others. I do not think the complexity is worth it.

The first step we have decided to take is to move git-foo form out of users' PATH. This would reduce the cluttered PATH problem, and it means not all of external commands have to become built-ins on a single flag day. I also think it has always been a nice touch that we allowed users to drop their own custom git-foo script to their path and call "git foo" as if it is part of the official git suite, so spawning commands in git-foo form needs to be supported via GIT_EXEC_PATH even if everything eventually becomes built-in.

So I would prefer doing something like this instead for v1.5.5 (see the top of updated release notes for 1.5.4 for deprecation notice).

 * execv_git_cmd() function will exec "git" with the given subcommand
   and its arguments;
 * The command dispatcher of git potty itself will first try the
   built-ins, and then try externals in dash form (which cannot be done
   with execv_git_cmd() anymore), and then aliases.
 * Just to be nice, we allow git-shell to treat "git foo arg" as if
   "git-foo arg" was given, but it continues to use execv_git_cmd(), and
   starts from a clean slate.
---
 exec_cmd.c |   31 ++++++++++++-------------------
 git.c      |   32 +++++++++++++++++++++++++++++++-
 shell.c    |   26 +++++++++++++++-----------
 3 files changed, 58 insertions(+), 31 deletions(-)
Show changes to 3 files +58 −31

exec_cmd.c, git.c, shell.c

diff --git a/exec_cmd.c b/exec_cmd.c
index 2d0a758..10b2908 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -65,32 +65,25 @@ void setup_path(const char *cmd_path)
 
 int execv_git_cmd(const char **argv)
 {
-	struct strbuf cmd;
-	const char *tmp;
-
-	strbuf_init(&cmd, 0);
-	strbuf_addf(&cmd, "git-%s", argv[0]);
+	int argc;
+	const char **nargv;
 
-	/*
-	 * argv[0] must be the git command, but the argv array
-	 * belongs to the caller, and may be reused in
-	 * subsequent loop iterations. Save argv[0] and
-	 * restore it on error.
-	 */
-	tmp = argv[0];
-	argv[0] = cmd.buf;
+	for (argc = 0; argv[argc]; argc++)
+		; /* just counting */
+	nargv = xmalloc(sizeof(*nargv) * (argc + 2));
 
-	trace_argv_printf(argv, -1, "trace: exec:");
+	nargv[0] = "git";
+	for (argc = 0; argv[argc]; argc++)
+		nargv[argc + 1] = argv[argc];
+	nargv[argc + 1] = NULL;
+	trace_argv_printf(nargv, -1, "trace: exec:");
 
 	/* execvp() can only ever return if it fails */
-	execvp(cmd.buf, (char **)argv);
+	execvp("git", (char **)nargv);
 
 	trace_printf("trace: exec failed: %s\n", strerror(errno));
 
-	argv[0] = tmp;
-
-	strbuf_release(&cmd);
-
+	free(nargv);
 	return -1;
 }
 
diff --git a/git.c b/git.c
index 01bbbc7..d690426 100644
--- a/git.c
+++ b/git.c
@@ -382,6 +382,36 @@ static void handle_internal_command(int argc, const char **argv)
 	}
 }
 
+static void execv_dashed_external(const char **argv)
+{
+	struct strbuf cmd;
+	const char *tmp;
+
+	strbuf_init(&cmd, 0);
+	strbuf_addf(&cmd, "git-%s", argv[0]);
+
+	/*
+	 * argv[0] must be the git command, but the argv array
+	 * belongs to the caller, and may be reused in
+	 * subsequent loop iterations. Save argv[0] and
+	 * restore it on error.
+	 */
+	tmp = argv[0];
+	argv[0] = cmd.buf;
+
+	trace_argv_printf(argv, -1, "trace: exec:");
+
+	/* execvp() can only ever return if it fails */
+	execvp(cmd.buf, (char **)argv);
+
+	trace_printf("trace: exec failed: %s\n", strerror(errno));
+
+	argv[0] = tmp;
+
+	strbuf_release(&cmd);
+}
+
+
 int main(int argc, const char **argv)
 {
 	const char *cmd = argv[0] ? argv[0] : "git-help";
@@ -445,7 +475,7 @@ int main(int argc, const char **argv)
 		handle_internal_command(argc, argv);
 
 		/* .. then try the external ones */
-		execv_git_cmd(argv);
+		execv_dashed_external(argv);
 
 		/* It could be an alias -- this works around the insanity
 		 * of overriding "git log" with "git show" by having
diff --git a/shell.c b/shell.c
index 9826109..729797c 100644
--- a/shell.c
+++ b/shell.c
@@ -19,17 +19,13 @@ static int do_generic_cmd(const char *me, char *arg)
 	return execv_git_cmd(my_argv);
 }
 
-static int do_cvs_cmd(const char *me, char *arg)
+static int do_cvs_cmd(void)
 {
 	const char *cvsserver_argv[3] = {
 		"cvsserver", "server", NULL
 	};
 
-	if (!arg || strcmp(arg, "server"))
-		die("git-cvsserver only handles server: %s", arg);
-
 	setup_path(NULL);
-
 	return execv_git_cmd(cvsserver_argv);
 }
 
@@ -40,7 +36,6 @@ static struct commands {
 } cmd_list[] = {
 	{ "git-receive-pack", do_generic_cmd },
 	{ "git-upload-pack", do_generic_cmd },
-	{ "cvs", do_cvs_cmd },
 	{ NULL },
 };
 
@@ -49,15 +44,24 @@ int main(int argc, char **argv)
 	char *prog;
 	struct commands *cmd;
 
+	/*
+	 * Special hack to pretend to be a CVS server
+	 */
 	if (argc == 2 && !strcmp(argv[1], "cvs server"))
-		argv--;
-	/* We want to see "-c cmd args", and nothing else */
-	else if (argc != 3 || strcmp(argv[1], "-c"))
+		exit(do_cvs_cmd());
+
+	/*
+	 * We do not accept anything but "-c" followed by "cmd arg",
+	 * where "cmd" is a very limited subset of git commands.
+	 */
+	if (argc != 3 || strcmp(argv[1], "-c"))
 		die("What do you think I am? A shell?");
 
 	prog = argv[2];
-	argv += 2;
-	argc -= 2;
+	if (!strncmp(prog, "git", 3) && isspace(prog[3]))
+		/* Accept "git foo" as if the caller said "git-foo". */
+		prog[3] = '-';
+
 	for (cmd = cmd_list ; cmd->name ; cmd++) {
 		int len = strlen(cmd->name);
 		char *arg;
Johannes Schindelin· Dec 2, 2007, 11:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Call builtin functions directly, was Re: [PATCH] transport.c: call dash-less form of receive-pack and upload-pack on remote

Hi,
On Sat, 1 Dec 2007, Junio C Hamano wrote:
Show 9 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Okay, I bit the apple and tried to move the builtins into the library, 
> > and rename handle_internal_command into execv_git_builtin(), moving it 
> > into exec-cmd.c.
> >
> > Big mistake.
> 
> I really feel this should not go in.
Hmm.

My rationale was not only avoiding an exec() (which I would find worth it on its own), but because this would be a non-verbal step towards libification.

Ciao, Dscho

Nguyen Thai Ngoc Duy· Nov 30, 2007, 12:19 UTC · re: Eyvind Bernhardsen · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Nov 30, 2007 6:28 PM, Eyvind Bernhardsen <eyvind-git-list@orakel.ntnu.no> wrote:

Show 23 quoted lines
> On 29. nov. 2007, at 23.36, Junio C Hamano wrote:
>
> [...]
>
> > If people are really serious about reducing the number of commands in
> > the path, I would expect fixes and bugreports saying "I am setting
> > gitexecdir different from bindir in _my_ installation when I build
> > git,
> > and here are the things that does not work if I do so".  Within the
> > span
> > of more than 20 months (77cb17e9 introduced gitexecdir in Jan 2006), I
> > do not think there was a single such report or patch, other than the
> > message from Nguyen that started this thread.
>
> I'm setting gitexecdir different from bindir in my installation, and
> here are the things that don't work:
>
> - When pushing to my system over ssh, git-receive-pack and git-upload-
> pack are expected to be in $PATH.  I resolved the problem by putting
> symlinks in /usr/local/bin.
>
> I haven't seen any other problems, but then again, I only use git
> plumbing commands and my own scripts.

You remind me my experience of making every external C-based command builtin. There is another case: git-merge. It calls something like git-merge-$strategy.

-- 
Duy
Johannes Schindelin· Nov 30, 2007, 13:35 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Hi,
On Fri, 30 Nov 2007, Nguyen Thai Ngoc Duy wrote:
Show 29 quoted lines
> On Nov 30, 2007 6:28 PM, Eyvind Bernhardsen
> <eyvind-git-list@orakel.ntnu.no> wrote:
> > On 29. nov. 2007, at 23.36, Junio C Hamano wrote:
> >
> > [...]
> >
> > > If people are really serious about reducing the number of commands in
> > > the path, I would expect fixes and bugreports saying "I am setting
> > > gitexecdir different from bindir in _my_ installation when I build
> > > git,
> > > and here are the things that does not work if I do so".  Within the
> > > span
> > > of more than 20 months (77cb17e9 introduced gitexecdir in Jan 2006), I
> > > do not think there was a single such report or patch, other than the
> > > message from Nguyen that started this thread.
> >
> > I'm setting gitexecdir different from bindir in my installation, and
> > here are the things that don't work:
> >
> > - When pushing to my system over ssh, git-receive-pack and git-upload-
> > pack are expected to be in $PATH.  I resolved the problem by putting
> > symlinks in /usr/local/bin.
> >
> > I haven't seen any other problems, but then again, I only use git
> > plumbing commands and my own scripts.
> 
> You remind me my experience of making every external C-based command
> builtin. There is another case: git-merge. It calls  something like
> git-merge-$strategy.
Good catch!

Ciao, Dscho

Jeff King· Nov 29, 2007, 15:08 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Wed, Nov 28, 2007 at 03:14:56PM -0800, Junio C Hamano wrote:
>  - Post v1.5.5, start cooking the change that does not install hardlinks
>    for built-in commands, aiming for inclusion in v1.6.0, by the end of
>    2008.

I am against this, unless it is configurable. I think the goal of reducing user-visible commands is fine, and moving things to $(libexecdir) is a good way of doing that.

However, I personally still think the 'git-foo' forms are valuable (because fingers have already been trained, and because non-bash-programmable completions understand them). And I don't mind putting $(libexecdir)/git-core in my PATH to retain this behavior; it's a one-time configuration tweak, and it helps new users with the overwhelming command set.

But I don't see a point to removing the links entirely. The annoyance factor for people who want git-* is much higher, and I don't see that it actually buys us any help for new users (who will no longer care after everything is hidden in $(libexecdir) anyway).

-Peff
Nguyen Thai Ngoc Duy· Nov 29, 2007, 20:05 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Nov 29, 2007 10:08 PM, Jeff King <peff@peff.net> wrote:
Show 21 quoted lines
> On Wed, Nov 28, 2007 at 03:14:56PM -0800, Junio C Hamano wrote:
>
> >  - Post v1.5.5, start cooking the change that does not install hardlinks
> >    for built-in commands, aiming for inclusion in v1.6.0, by the end of
> >    2008.
>
> I am against this, unless it is configurable. I think the goal of
> reducing user-visible commands is fine, and moving things to
> $(libexecdir) is a good way of doing that.
>
> However, I personally still think the 'git-foo' forms are valuable
> (because fingers have already been trained, and because
> non-bash-programmable completions understand them). And I don't mind
> putting $(libexecdir)/git-core in my PATH to retain this behavior; it's
> a one-time configuration tweak, and it helps new users with the
> overwhelming command set.
>
> But I don't see a point to removing the links entirely. The annoyance
> factor for people who want git-* is much higher, and I don't see that it
> actually buys us any help for new users (who will no longer care after
> everything is hidden in $(libexecdir) anyway).

Maybe only not install hardlinks on systems that do not support it like Windows? git.exe duplication takes a lot of space.

-- 
Duy
Jeff King· Nov 29, 2007, 21:14 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Fri, Nov 30, 2007 at 03:05:05AM +0700, Nguyen Thai Ngoc Duy wrote:
Show 7 quoted lines
> > But I don't see a point to removing the links entirely. The annoyance
> > factor for people who want git-* is much higher, and I don't see that it
> > actually buys us any help for new users (who will no longer care after
> > everything is hidden in $(libexecdir) anyway).
> 
> Maybe only not install hardlinks on systems that do not support it
> like Windows? git.exe duplication takes a lot of space.

I think that is totally reasonable, as on those platforms there is actually something to be gained from removing those hardlinks (you could also of course make a very thin wrapper for "git-foo" that called "git foo"; it would still be wasteful, but not as much as copying the whole git.exe. But that is not worth doing unless people on Windows really want the dash forms).

-Peff
Johannes Schindelin· Nov 29, 2007, 22:19 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Hi,
On Thu, 29 Nov 2007, Jeff King wrote:
Show 16 quoted lines
> On Fri, Nov 30, 2007 at 03:05:05AM +0700, Nguyen Thai Ngoc Duy wrote:
> 
> > > But I don't see a point to removing the links entirely. The annoyance
> > > factor for people who want git-* is much higher, and I don't see that it
> > > actually buys us any help for new users (who will no longer care after
> > > everything is hidden in $(libexecdir) anyway).
> > 
> > Maybe only not install hardlinks on systems that do not support it
> > like Windows? git.exe duplication takes a lot of space.
> 
> I think that is totally reasonable, as on those platforms there is
> actually something to be gained from removing those hardlinks (you could
> also of course make a very thin wrapper for "git-foo" that called "git
> foo"; it would still be wasteful, but not as much as copying the whole
> git.exe. But that is not worth doing unless people on Windows really
> want the dash forms).

Note that one big problem with a few platforms having dash forms and others not is that you _will_ get scripts and aliases that do not work everywhere.

Consistency is good.

Ciao, Dscho

Jeff King· Nov 29, 2007, 23:14 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, Nov 29, 2007 at 10:19:16PM +0000, Johannes Schindelin wrote:
Show 8 quoted lines
> > I think that is totally reasonable, as on those platforms there is
> > actually something to be gained from removing those hardlinks (you could
> 
> Note that one big problem with a few platforms having dash forms and 
> others not is that you _will_ get scripts and aliases that do not work 
> everywhere.
> 
> Consistency is good.

Yes, I am fine with the user having to go to extra lengths to use the dash forms (like adding $(libexecdir) to their path), which I think should address your consistency concern.

-Peff
Linus Torvalds· Nov 29, 2007, 23:30 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, 29 Nov 2007, Jeff King wrote:
> 
> Yes, I am fine with the user having to go to extra lengths to use the
> dash forms (like adding $(libexecdir) to their path), which I think
> should address your consistency concern.

I agree. If we actually start moving the subcommands into a separate directory, I suspect scripts will be fixed up soon enough. Of course people *can* do it by just adding the path, but more likely, we'll just see people start doign "git xyz" instead of "git-xyz".

And from a consistency standpoint, that would be a *good* thing. There are many reasons why the git-xyz format *cannot* be the "consistent" form (ranging from the flags like --bare and -p to just aliases), so encouraging people to move to "git xyz" is just a good idea.

Yeah, yeah, the man-pages need the "git-xyz" form, but on the other hand, rather than "man git-xyz", you can just do "git help xyz" instead, and now you're consistently avoiding the dash again!

			Linus
Junio C Hamano· Nov 30, 2007, 00:13 UTC · re: Linus Torvalds · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 8 quoted lines
> And from a consistency standpoint, that would be a *good* thing. There are 
> many reasons why the git-xyz format *cannot* be the "consistent" form
> (ranging from the flags like --bare and -p to just aliases), so 
> encouraging people to move to "git xyz" is just a good idea.
>
> Yeah, yeah, the man-pages need the "git-xyz" form, but on the other hand, 
> rather than "man git-xyz", you can just do "git help xyz" instead, and now 
> you're consistently avoiding the dash again!

Ok. So here is a revised roadmap that a panda brain (that is not so well working today due to fever) came up.

 - v1.5.4 will ship with gitexecdir=$(bindir) in Makefile.  But the
   release notes for the version will warn users that:
   (1) using git-foo from the command line, and
   (2) using git-foo from your scripts without first prepending the
       return value of "git --exec-path" to the PATH
   is now officially deprecated (it has been deprecated for a long time
   since January 2006, v1.2.0~149) and upcoming v1.5.5 will ship with
   the default configuration that does not install git-foo form in
   user's PATH.
   If further will warn users that git-foo form will be removed in
   v1.5.6 for many commands and it will be merely an accident if some of
   them still work after that.
 - Post v1.5.4, start cooking gitexecdir=$(libexecdir)/git-core, aiming
   for inclusion in v1.5.5, perhaps in Feb-Mar 2008 timeframe.  This
   will also affect the sample RPM spec and resulting RPM binary
   packages I will place on k.org, and I'll ask Gerrit to do the same on
   Debian side.  The official binary packaging of individual distros are
   not under my control, but if there is a handy list of people I can
   send this notice to for other distros, that would help this process.
 - The release notes for v1.5.5 will warn users again that git-foo will
   be removed in v1.5.6 for many commands and it will be merely an
   accident if some of them still work.
 - Post v1.5.5, start cooking the change that does not install hardlinks
   for built-in commands, aiming for inclusion in v1.5.6, in May-Jun
   2008 timeframe.
Jeff King· Nov 30, 2007, 00:35 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, Nov 29, 2007 at 04:13:29PM -0800, Junio C Hamano wrote:
>  - Post v1.5.5, start cooking the change that does not install hardlinks
>    for built-in commands, aiming for inclusion in v1.5.6, in May-Jun
>    2008 timeframe.

I am still against this step, for the reasons mentioned in the mails leading up to the one you just quoted. I am fine with "does not install hardlinks for builtin-commands on systems that don't support hardlinks" (and of course all such hardlinks are in $(libexecdir)/git-core at this point).

-Peff
Junio C Hamano· Nov 30, 2007, 00:49 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> On Thu, Nov 29, 2007 at 04:13:29PM -0800, Junio C Hamano wrote:
>
>>  - Post v1.5.5, start cooking the change that does not install hardlinks
>>    for built-in commands, aiming for inclusion in v1.5.6, in May-Jun
>>    2008 timeframe.
>
> I am still against this step, for the reasons mentioned in the mails
> leading up to the one you just quoted. I am fine with "does not install
> hardlinks for builtin-commands on systems that don't support hardlinks"
> (and of course all such hardlinks are in $(libexecdir)/git-core at this
> point).

I understand your point was primarily "git-a<tab>". I think it has been solved for bash and zsh but not for other shells. I think possible and sensible avenues are (1) punt -- cvs, svn nor hg people do not seem to have problem with it, or (2) implement completion in your other favorite shells.

And I think the following from Linus makes sense.
Show 8 quoted lines
> And from a consistency standpoint, that would be a *good* thing. There are 
> many reasons why the git-xyz format *cannot* be the "consistent" form
> (ranging from the flags like --bare and -p to just aliases), so 
> encouraging people to move to "git xyz" is just a good idea.
>
> Yeah, yeah, the man-pages need the "git-xyz" form, but on the other hand, 
> rather than "man git-xyz", you can just do "git help xyz" instead, and now 
> you're consistently avoiding the dash again!

but I am feeling quite feverish today so I may be missing something obvious.

Jeff King· Nov 30, 2007, 00:58 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, Nov 29, 2007 at 04:49:26PM -0800, Junio C Hamano wrote:
Show 5 quoted lines
> I understand your point was primarily "git-a<tab>".  I think it has been
> solved for bash and zsh but not for other shells.  I think possible and
> sensible avenues are (1) punt -- cvs, svn nor hg people do not seem to
> have problem with it, or (2) implement completion in your other favorite
> shells.

My point is that (2) is already implemented for every program (shell or no) which understands filename completion, and there is a proposal for taking it away. I would consider that, except I haven't see any claimed advantages except that the hardlinks are awful under Windows.

I am proposing to have those dashed forms on platforms where it makes sense, but to treat them as second-class citizens (by putting them in $(libexecdir), which will address consistency problems.

Show 13 quoted lines
> And I think the following from Linus makes sense.
> 
> > And from a consistency standpoint, that would be a *good* thing. There are 
> > many reasons why the git-xyz format *cannot* be the "consistent" form
> > (ranging from the flags like --bare and -p to just aliases), so 
> > encouraging people to move to "git xyz" is just a good idea.
> >
> > Yeah, yeah, the man-pages need the "git-xyz" form, but on the other hand, 
> > rather than "man git-xyz", you can just do "git help xyz" instead, and now 
> > you're consistently avoiding the dash again!
> 
> but I am feeling quite feverish today so I may be missing something
> obvious.

I thought Linus' point was that moving the subcommands was sufficient for dealing with the consistency issue (i.e., all scripts would move to "git foo" and only those people who really wanted to would put the dashed forms in their path). From the same email you quoted, but just above:

Show 10 quoted lines
> On Thu, 29 Nov 2007, Jeff King wrote:
> >
> > Yes, I am fine with the user having to go to extra lengths to use the
> > dash forms (like adding $(libexecdir) to their path), which I think
> > should address your consistency concern.
> 
> I agree. If we actually start moving the subcommands into a separate
> directory, I suspect scripts will be fixed up soon enough. Of course
> people *can* do it by just adding the path, but more likely, we'll just
> see people start doign "git xyz" instead of "git-xyz".
But now we are arguing about what Linus meant like it is scripture. ;)
-Peff
Nicolas Pitre· Nov 30, 2007, 01:13 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, 29 Nov 2007, Jeff King wrote:
Show 12 quoted lines
> On Thu, Nov 29, 2007 at 04:49:26PM -0800, Junio C Hamano wrote:
> 
> > I understand your point was primarily "git-a<tab>".  I think it has been
> > solved for bash and zsh but not for other shells.  I think possible and
> > sensible avenues are (1) punt -- cvs, svn nor hg people do not seem to
> > have problem with it, or (2) implement completion in your other favorite
> > shells.
> 
> My point is that (2) is already implemented for every program (shell or
> no) which understands filename completion, and there is a proposal for
> taking it away. I would consider that, except I haven't see any claimed
> advantages except that the hardlinks are awful under Windows.

Weren't enough complaints about Git having waaaaaaaaaaay too many commands? Didn't those complaints come about often enough already?

	$ git-[tab]
	Display all 135 possibilities? (y or n)
Nicolas
Jeff King· Nov 30, 2007, 01:17 UTC · re: Nicolas Pitre · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, Nov 29, 2007 at 08:13:04PM -0500, Nicolas Pitre wrote:
Show 10 quoted lines
> > My point is that (2) is already implemented for every program (shell or
> > no) which understands filename completion, and there is a proposal for
> > taking it away. I would consider that, except I haven't see any claimed
> > advantages except that the hardlinks are awful under Windows.
> 
> Weren't enough complaints about Git having waaaaaaaaaaay too many 
> commands?  Didn't those complaints come about often enough already?
> 
> 	$ git-[tab]
> 	Display all 135 possibilities? (y or n)

Go back and read the thread to which you are responding. I am _not_ arguing against moving those commands to $(libexecdir) where no sane user will ever see them. That change addresses the issue you are talking about.

I _am_ arguing against removing them entirely, for those of us who want to go to the trouble of enabling this (by putting a non-standard entry into our PATH). Because the issue you are talking about will already have been dealt with, it is no longer a compelling reason to remove the hardlinks entirely.

The only reason I have heard to remove them entirely is that Windows doesn't properly support hardlinks, which I addressed in my other mails (and to which I have seen no rebuttal).

-Peff
Steffen Prohaska· Nov 30, 2007, 05:42 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Nov 30, 2007, at 2:17 AM, Jeff King wrote:
Show 5 quoted lines
>
> The only reason I have heard to remove them entirely is that Windows
> doesn't properly support hardlinks, which I addressed in my other  
> mails
> (and to which I have seen no rebuttal).

We don't have a problem with hardlinks for git-* commands in msysgit. The msysgit installer already creates hardlinks if installing on NTFS. On non-NTFS partitions, though, it needs to copy the files.

Note, this doesn't mean we love hardlinks in msysgit. Actually, msys does _not_ support them in its Unix emulation layer. So, for daily work git does not have hardlinks. For examples, the test script skip all hardlink related tests. The installer handles hardlinks using the Windows API directly and not using the Unix emulation layer.

We already have a setup that supports exporting only git and gitk to the a Windows Command Prompt (not the bash that comes with msysgit). If you choose this setup, a directory that contains only these two commands will be added to the system wide PATH. We use this indirection to hide all the the Unix tools that are included in msysgit (and needed by git) from the Windows Command Prompt.

	Steffen
Andreas Ericsson· Nov 30, 2007, 07:18 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Jeff King wrote:
Show 27 quoted lines
> On Thu, Nov 29, 2007 at 08:13:04PM -0500, Nicolas Pitre wrote:
> 
>>> My point is that (2) is already implemented for every program (shell or
>>> no) which understands filename completion, and there is a proposal for
>>> taking it away. I would consider that, except I haven't see any claimed
>>> advantages except that the hardlinks are awful under Windows.
>> Weren't enough complaints about Git having waaaaaaaaaaay too many 
>> commands?  Didn't those complaints come about often enough already?
>>
>> 	$ git-[tab]
>> 	Display all 135 possibilities? (y or n)
> 
> Go back and read the thread to which you are responding. I am _not_
> arguing against moving those commands to $(libexecdir) where no sane
> user will ever see them. That change addresses the issue you are talking
> about.
> 
> I _am_ arguing against removing them entirely, for those of us who want
> to go to the trouble of enabling this (by putting a non-standard entry
> into our PATH). Because the issue you are talking about will already
> have been dealt with, it is no longer a compelling reason to remove the
> hardlinks entirely.
> 
> The only reason I have heard to remove them entirely is that Windows
> doesn't properly support hardlinks, which I addressed in my other mails
> (and to which I have seen no rebuttal).
> 

It would provide a ui inconsistency between platforms. Several people pointed that out. It's decidedly a Bad Thing.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231
Jeff King· Nov 30, 2007, 15:09 UTC · re: Andreas Ericsson · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Fri, Nov 30, 2007 at 08:18:16AM +0100, Andreas Ericsson wrote:
Show 6 quoted lines
>> The only reason I have heard to remove them entirely is that Windows
>> doesn't properly support hardlinks, which I addressed in my other mails
>> (and to which I have seen no rebuttal).
>>
> It would provide a ui inconsistency between platforms. Several people
> pointed that out. It's decidedly a Bad Thing.

Which, as I said, I have already addressed (and which Linus has also expanded upon in this thread). Since those hardlinks would be hidden from users who did not go to some trouble to find them, there will not be inconsistency problems. Scripts will have to either go to some effort to change their PATH, or simply use the correct form, and I expect them to do the latter.

-Peff
Junio C Hamano· Nov 30, 2007, 20:01 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> On Fri, Nov 30, 2007 at 08:18:16AM +0100, Andreas Ericsson wrote:
> ...
>> It would provide a ui inconsistency between platforms. Several people
>> pointed that out. It's decidedly a Bad Thing.
>
> Which, as I said, I have already addressed (and which Linus has also
> expanded upon in this thread). Since those hardlinks would be hidden
> from users who did not go to some trouble to find them, there will not
> be inconsistency problems.

I already can see exchanges in the user community after such a change you propose would happen:

 Newbie: Ay! why doesn't git-commit work anymore?
 Jeff: Stupid Junio and Linus decided that you should not use dash form
       but say "git commit" instead.
 Newbie: But my fingers are trained and I like the "git-<tab>"
         completion.
 Jeff: If you really like that, here is a hidden trick.  Add
       /usr/libexec/git-core/ to your PATH.
 Newbie: Ah, that worked, thanks.
 A few days later...
 Newbie: Jeff, your trick does not work for my coworker.  He also has
         the latest git.  His installation does not even have that
         directory!  What gives?
 Jeff: Ah, sorry, that trick works for some platforms but not others.
 Newbie: Stupid inconsistency.  Who suggested that?
Jeff King· Nov 30, 2007, 21:25 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Fri, Nov 30, 2007 at 12:01:13PM -0800, Junio C Hamano wrote:
Show 5 quoted lines
> I already can see exchanges in the user community after such a change
> you propose would happen:
> [...]
>  Jeff: If you really like that, here is a hidden trick.  Add
>        /usr/libexec/git-core/ to your PATH.
What if I promise not to tell anyone? :)

Anyway, I don't think it will be a problem. You think it might. But I suspect neither of us has anything more than a gut feeling to argue with. And now I have registered my complaint, so you can do what you think is best.

I can, of course, always make my own hardlinks (which is really the same thing, except the "trick" is slightly harder to perform and perhaps less socially acceptable; OTOH, if such a trick is common, perhaps it means taking away the dash forms wasn't such a good idea after all).

>  Newbie: Stupid inconsistency.  Who suggested that?
Jeff [runs git-blame]: It must have been Junio! :)
-Peff
Johannes Schindelin· Nov 30, 2007, 23:10 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Hi,
On Fri, 30 Nov 2007, Jeff King wrote:
Show 9 quoted lines
> On Fri, Nov 30, 2007 at 12:01:13PM -0800, Junio C Hamano wrote:
> 
> > I already can see exchanges in the user community after such a change
> > you propose would happen:
> > [...]
> >  Jeff: If you really like that, here is a hidden trick.  Add
> >        /usr/libexec/git-core/ to your PATH.
> 
> What if I promise not to tell anyone? :)

By the same reasoning you can invade an unsuspecting country, saying that everybody will be better off afterwards. But the risk is high, not because of the probability, but because of the cost to pay if it does not work out.

Really, I'd rather have this be done right. So I am quite happy with Junio being "girly" (which I would have called cautious and nice-to-users, as well as considerate, though).

In the end I would be so much happier not to have hard links at all, and it seems that all the "easy" SCMs out there are quite well off without hard links, too.

To me, it is mighty annoying anybody brings up that "144 commands" argument Linus was referring to, and if there is _any_ way to shut up those bikeshedders, I am all for it.

But if that is not possible, so be it.

Ciao, Dscho

Wincent Colaiuta· Dec 2, 2007, 15:02 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

El 1/12/2007, a las 0:10, Johannes Schindelin escribió:
> To me, it is mighty annoying anybody brings up that "144 commands"
> argument Linus was referring to, and if there is _any_ way to shut up
> those bikeshedders, I am all for it.

This is not a bikeshed argument and it is not an "idiotic complaint" (to use Linus' phrase). It is a legitimate concern and a *real* UI problem.

You and Linus don't care that there are 140+ Git commands and I imagine that you know exactly what each of them does.

I don't really care either because although I don't know what every single command does, I know what the 20 or 30 commands I personally need for my own workflow do.

The problem is for *newcomers* to Git who sit down for the first time and ask themselves, "Now, how do I...?". This is not an idiotic complaint but a legitimate concern about a real UI problem.

Honestly, Johannes, do you think the following is a good UI?

$ git Display all 148 possibilities? (y or n) git* git-cvsimport* git-local- fetch* git-peek-remote* git-show-index* git-add* git-cvsserver* git- log* git-prune* git-show-ref* git-add--interactive* git-daemon* git-lost- found* git-prune-packed* git-ssh-fetch* git-am* git-describe* git-ls- files* git-pull* git-ssh-pull* git-annotate* git-diff* git-ls- remote* git-push* git-ssh-push* git-apply* git-diff-files* git-ls- tree* git-quiltimport* git-ssh-upload* git-applymbox git-diff-index* git- mailinfo* git-read-tree* git-stash* git-applypatch git-diff-tree* git- mailsplit* git-rebase* git-status* git-archimport* git-fast-import* git- merge* git-rebase--interactive* git-stripspace* git-archive* git-fetch* git-merge- base* git-receive-pack* git-submodule* git-bisect* git-fetch--tool* git-merge- file* git-reflog* git-svn* git-blame* git-fetch-pack* git-merge- index* git-relink* git-svnimport git-branch* git-filter-branch* git-merge- octopus* git-remote* git-symbolic-ref* git-bundle* git-fmt-merge-msg* git-merge-one- file* git-repack* git-tag* git-cat-file* git-for-each-ref* git-merge- ours* git-repo-config* git-tar-tree* git-check-attr* git-format-patch* git-merge- recursive* git-request-pull* git-unpack-file* git-check-ref-format* git-fsck* git-merge- resolve* git-rerere* git-unpack-objects* git-checkout* git-fsck-objects* git-merge- stupid* git-reset* git-update-index* git-checkout-index* git-gc* git-merge- subtree* git-rev-list* git-update-ref* git-cherry* git-get-tar-commit-id* git-merge- tree* git-rev-parse* git-update-server-info* git-cherry-pick* git-grep* git- mergetool* git-revert* git-upload-archive* git-citool git-gui/ git- mktag* git-rm* git-upload-pack* git-clean* git-hash-object* git- mktree* git-runstatus* git-var* git-clone* git-http-fetch* git- mv* git-send-email* git-verify-pack* git-commit* git-http-push* git-name- rev* git-send-pack* git-verify-tag* git-commit-tree* git-imap-send* git-pack- objects* git-sh-setup* git-whatchanged* git-config* git-index-pack* git-pack- redundant* git-shell* git-write-tree* git-convert-objects git-init* git-pack- refs* git-shortlog* gitk git-count-objects* git-init-db* git-parse- remote* git-show* git-cvsexportcommit* git-instaweb* git-patch- id* git-show-branch*

Would you argue that this screenshot shows a sane UI?
<http://wincent.com/tmp/git-ui.png>
Note: I am not complaining about the *number* of commands; I myself  
find them highly useful for scripting. I am saying that the  
*visibility* of those commands is the problem. That's why I support  
the efforts to move most of this stuff out of the default PATH. We're  
not talking about getting rid of it, just about putting it somewhere  
more appropriate in order to correct what is basically a *hideous*  
user interface for the beginner.

Cheers, Wincent

Johannes Schindelin· Dec 2, 2007, 16:39 UTC · re: Wincent Colaiuta · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Hi,
On Sun, 2 Dec 2007, Wincent Colaiuta wrote:
Show 11 quoted lines
> El 1/12/2007, a las 0:10, Johannes Schindelin escribi?:
> 
> > To me, it is mighty annoying anybody brings up that "144 commands" 
> > argument Linus was referring to, and if there is _any_ way to shut up 
> > those bikeshedders, I am all for it.
> 
> This is not a bikeshed argument and it is not an "idiotic complaint" (to 
> use Linus' phrase). It is a legitimate concern and a *real* UI problem.
> 
> You and Linus don't care that there are 140+ Git commands and I imagine 
> that you know exactly what each of them does.

Okay, how many executables are there in your /usr/bin/? Here there are 2973.

Guess what.  I am not intimidated by that number.

Ciao, Dscho

Pascal Obry· Dec 2, 2007, 16:56 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Johannes Schindelin a écrit :
> Okay, how many executables are there in your /usr/bin/?  Here there are 
> 2973.
> Guess what.  I am not intimidated by that number.
Good, and look in /usr/bin, all those 2973 binary are all disconnected.
Here we are speaking about a tool as a whole : Git.

And I agree that hiding some of them will probably help new comers. We can also argue that a new comers should read some documentation :)

After all I'm not sure what's the right move !

At least let me say something constructive :) I'm a new comer to Git. I've read many documentations before grabbing the system and I've not been impressed by the number of binaries in /usr/bin... Because I've almost never looked there. Most of the time I'm using "git <tab>" and the bash completion feature is just right for me.

Pascal.
-- 
--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595
Johannes Schindelin· Dec 2, 2007, 17:23 UTC · re: Pascal Obry · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Hi,
On Sun, 2 Dec 2007, Pascal Obry wrote:
Show 7 quoted lines
> Johannes Schindelin a ?crit :
> > Okay, how many executables are there in your /usr/bin/?  Here there 
> > are 2973. Guess what.  I am not intimidated by that number.
> 
> Good, and look in /usr/bin, all those 2973 binary are all disconnected.
> 
> Here we are speaking about a tool as a whole : Git.

No, we are speaking about different commands, such as commit, fetch, push, etc.

I refuse to believe that you cannot see the equivalence.
> I've read many documentations before grabbing the system and I've not
> been impressed by the number of binaries in /usr/bin... Because I've
> almost never looked there.
Exactly my point.
> Most of the time I'm using "git <tab>" and the bash completion feature 
> is just right for me.
Bash completion is really something fine.

But even without, I do not see a problem: many cvs users used only three out of 32 commands (most CVS users I personally know/knew only called add, commit and update). You could even see all 32 commands when calling the clunky command line "cvs --help-commands". I am convinced we're already more user-friendly than that.

Ciao, Dscho

Junio C Hamano· Dec 1, 2007, 02:37 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> I can, of course, always make my own hardlinks (which is really the same
> thing, except the "trick" is slightly harder to perform and perhaps less
> socially acceptable; OTOH, if such a trick is common, perhaps it means
> taking away the dash forms wasn't such a good idea after all).
>
>>  Newbie: Stupid inconsistency.  Who suggested that?
>
> Jeff [runs git-blame]: It must have been Junio! :)
You found a bug in git-blame, then ;-).  I think it should report Jeff.

As Windows ports need to have their own difference _anyway_, I personally do not think it is a big deal if the Makefile I ship continues to install the dashed form in gitexecdir, and Windows ports omit the hardlinks if they feel copies are wasteful.

However, that would introduce hard-to-track platform dependent bugs (e.g. "git-receive-pack" is asked for by "git-send-pack", but the other side does not have such a program anywhere). So my preference at this point is to move things out of PATH first (without removing the hardlinks), deal with possible fallout from that move.

And after things stablize, discuss to either remove the hardlinks from all installations, or to keep them in all installations. I do not think "it's this way here but that way there" is a good thing in general.

We do have "git-foo.exe" vs "git-foo" difference and there are some existing code (most notably, help.c::list_commands_in_dir()) that need to be aware of it. Let's try not to make things any worse.

Jeff King· Dec 1, 2007, 04:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Fri, Nov 30, 2007 at 06:37:12PM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> side does not have such a program anywhere).  So my preference at this
> point is to move things out of PATH first (without removing the
> hardlinks), deal with possible fallout from that move.
> 
> And after things stablize, discuss to either remove the hardlinks from
> all installations, or to keep them in all installations.  I do not think
> "it's this way here but that way there" is a good thing in general.
I think that it is a sensible course of action.
-Peff
Linus Torvalds· Nov 30, 2007, 02:29 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, 29 Nov 2007, Jeff King wrote:
Show 5 quoted lines
> 
> I thought Linus' point was that moving the subcommands was sufficient
> for dealing with the consistency issue (i.e., all scripts would move to
> "git foo" and only those people who really wanted to would put the
> dashed forms in their path). 

Yes. I meant that we might as well keep all the git-xyz forms around, but _only_ in the libexec directory (and make sure that the libexec directory by default is *not* a binary directory), so that they'd normally not be visible.

So then, people who want to use the old-fashioned git-xyz forms, because 
they just really hate white-space or whatever, could choose to do one of 
two things:
 - either just change the default libexec directory to be the same as the 
   binary install directory, and have all the git-xyz things in the same 
   place they've always been.
 - or just add $(gitlibexec) into their path.

but the default (which is what 99% of all people use) would be to not show them. I also think that it makes sense to avoid wasting diskspace with duplicate files, so in situations where you don't have hardlinks, just don't install the git-xyz files at all by default (and again, maybe we can have a option to the installer to do it for people who really are very attached to the git-xyz format, and prefer to waste even a lot of disk on it)

So I just think that the whole idiotic complaint that some people have (that whole "git-<tab><tab>" shows "Display all 144 possibilities?" and people are somehow using that as an argument that git is "complex") should be something we strive to undo. I think the complaint is insane (because the answer is "well, nobody forces you to _use_ all the power and scripts we give you!"), but still, it's a complaint, so let's just assume the user is right, and try to fix it.

So when you do "git-<tab><tab>" it should just beep at you and not show anything at all by default. And when you do "git <tab><tab>", we should make sure that the bash expansion (or zsh or whatever) shows a nice collection of common plumbing, not soemthing really scary.

			Linus
Nicolas Pitre· Nov 30, 2007, 02:55 UTC · re: Linus Torvalds · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, 29 Nov 2007, Linus Torvalds wrote:
Show 28 quoted lines
> Yes. I meant that we might as well keep all the git-xyz forms around, but 
> _only_ in the libexec directory (and make sure that the libexec directory 
> by default is *not* a binary directory), so that they'd normally not be 
> visible.
> 
> So then, people who want to use the old-fashioned git-xyz forms, because 
> they just really hate white-space or whatever, could choose to do one of 
> two things:
>  - either just change the default libexec directory to be the same as the 
>    binary install directory, and have all the git-xyz things in the same 
>    place they've always been.
>  - or just add $(gitlibexec) into their path.
> 
> but the default (which is what 99% of all people use) would be to not 
> show them. I also think that it makes sense to avoid wasting diskspace 
> with duplicate files, so in situations where you don't have hardlinks, 
> just don't install the git-xyz files at all by default (and again, maybe 
> we can have a option to the installer to do it for people who really are 
> very attached to the git-xyz format, and prefer to waste even a lot of 
> disk on it)
> 
> So I just think that the whole idiotic complaint that some people have 
> (that whole "git-<tab><tab>" shows "Display all 144 possibilities?" and 
> people are somehow using that as an argument that git is "complex") should 
> be something we strive to undo. I think the complaint is insane (because 
> the answer is "well, nobody forces you to _use_ all the power and scripts 
> we give you!"), but still, it's a complaint, so let's just assume the user 
> is right, and try to fix it.
Absolutely!

And despite Junio's appearance of some cowardliness on this issue :-) I think we should have this right now instead of later. Like Junio said himself, this was planned for a while already, and poorly maintained external scripts are already failing due to other reasons anyway.

Nicolas
Steffen Prohaska· Nov 30, 2007, 05:51 UTC · re: Linus Torvalds · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Nov 30, 2007, at 3:29 AM, Linus Torvalds wrote:
Show 19 quoted lines
> So I just think that the whole idiotic complaint that some people have
> (that whole "git-<tab><tab>" shows "Display all 144 possibilities?"  
> and
> people are somehow using that as an argument that git is "complex")  
> should
> be something we strive to undo. I think the complaint is insane  
> (because
> the answer is "well, nobody forces you to _use_ all the power and  
> scripts
> we give you!"), but still, it's a complaint, so let's just assume  
> the user
> is right, and try to fix it.
>
> So when you do "git-<tab><tab>" it should just beep at you and not  
> show
> anything at all by default. And when you do "git <tab><tab>", we  
> should
> make sure that the bash expansion (or zsh or whatever) shows a nice
> collection of common plumbing, not soemthing really scary.
What will happen to gitk?

Shouldn't it be included in the nice collection? gitk is an essential command. Then, following your reasoning, "git <tab><tab>" should recommend it, no?

Note, "git gui" already works. gitk would really be the last git "command" that can't be accessed through "git <command>"

	Steffen
Jeff King· Nov 30, 2007, 15:12 UTC · re: Steffen Prohaska · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Fri, Nov 30, 2007 at 06:51:35AM +0100, Steffen Prohaska wrote:
Show 8 quoted lines
> What will happen to gitk?
>
> Shouldn't it be included in the nice collection?  gitk is
> an essential command.  Then, following your reasoning,
> "git <tab><tab>" should recommend it, no?
>
> Note, "git gui" already works.  gitk would really be the last
> git "command" that can't be accessed through "git <command>"

Except for qgit, tig, etc. The only difference being that gitk actually ships with git.

But I am not opposed to having some "git foo" form for gitk.
-Peff
Santi Béjar· Nov 30, 2007, 15:28 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Nov 30, 2007 4:12 PM, Jeff King <peff@peff.net> wrote:
Show 15 quoted lines
> On Fri, Nov 30, 2007 at 06:51:35AM +0100, Steffen Prohaska wrote:
>
> > What will happen to gitk?
> >
> > Shouldn't it be included in the nice collection?  gitk is
> > an essential command.  Then, following your reasoning,
> > "git <tab><tab>" should recommend it, no?
> >
> > Note, "git gui" already works.  gitk would really be the last
> > git "command" that can't be accessed through "git <command>"
>
> Except for qgit, tig, etc. The only difference being that gitk actually
> ships with git.
>
> But I am not opposed to having some "git foo" form for gitk.
In mercurial "hg view" is actually an old version of gitk modified for hg.
And as "git view" it could be added to the "git help" list.
Santi
Jeff King· Nov 30, 2007, 15:29 UTC · re: Santi Béjar · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Fri, Nov 30, 2007 at 04:28:21PM +0100, Santi Béjar wrote:
Show 5 quoted lines
> > But I am not opposed to having some "git foo" form for gitk.
> 
> In mercurial "hg view" is actually an old version of gitk modified for hg.
> 
> And as "git view" it could be added to the "git help" list.

Unfortunately, there is already a "gitview" program similar to gitk, although it never made it out of contrib/.

-Peff
Linus Torvalds· Nov 30, 2007, 15:50 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Fri, 30 Nov 2007, Jeff King wrote:
Show 10 quoted lines
>
> On Fri, Nov 30, 2007 at 04:28:21PM +0100, Santi Béjar wrote:
> > > But I am not opposed to having some "git foo" form for gitk.
> > 
> > In mercurial "hg view" is actually an old version of gitk modified for hg.
> > 
> > And as "git view" it could be added to the "git help" list.
> 
> Unfortunately, there is already a "gitview" program similar to gitk,
> although it never made it out of contrib/.

Well, different people will want different viewers *anyway* (ie some will prefer qgit etc), so how about making "git view" be something that literally acts as a built-in alias that just defaults to running gitk (if for no other reason than the fact that gitk is the one that ships with git, and simply has most users).

There's a few other things that I think we could consider to be good built-in aliases: things like "git cat" being an alias for "git -p cat-file -p" etc.

The only difference between a "built-in alias" and a "built-in command" 
would be:
 - the alias has never even had the "git-xyz" format, and never will
 - the alias can be overridden by user aliases unlike "real" git commands
Hmm?

That way we can hide away gitk too (although I do suspect that we might as well just leave gitk in the path - it's different in naming from all the git-xyz commands anyway)

		Linus
Jeff King· Nov 30, 2007, 16:22 UTC · re: Linus Torvalds · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Fri, Nov 30, 2007 at 07:50:47AM -0800, Linus Torvalds wrote:
Show 5 quoted lines
> Well, different people will want different viewers *anyway* (ie some will 
> prefer qgit etc), so how about making "git view" be something that 
> literally acts as a built-in alias that just defaults to running gitk (if 
> for no other reason than the fact that gitk is the one that ships with 
> git, and simply has most users).
I think that is a good idea, and here's a patch.

-- >8 -- Support builtin aliases

Builtin aliases are "default" alias values that can be overridden by user-configured aliases.

For example, the first such alias is "view", an alias for
gitk. A user with no further configuration can run
"git view" to use gitk. However, they can also set the
config option "alias.view" to "!tig" to run tig.
---
 git.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
Show changes to git.c +9 −0
diff --git a/git.c b/git.c
index f220284..95296aa 100644
--- a/git.c
+++ b/git.c
@@ -151,6 +151,13 @@ static int split_cmdline(char *cmdline, const char ***argv)
 	return count;
 }
 
+static char *builtin_alias(const char *cmd)
+{
+	if (!strcmp(cmd, "view"))
+		return xstrdup("!gitk");
+	return NULL;
+}
+
 static int handle_alias(int *argcp, const char ***argv)
 {
 	int nongit = 0, envchanged = 0, ret = 0, saved_errno = errno;
@@ -162,6 +169,8 @@ static int handle_alias(int *argcp, const char ***argv)
 
 	alias_command = (*argv)[0];
 	git_config(git_alias_config);
+	if (!alias_string)
+		alias_string = builtin_alias(alias_command);
 	if (alias_string) {
 		if (alias_string[0] == '!') {
 			if (*argcp > 1) {
-- 
1.5.3.6.2064.g2e22f-dirty
Johannes Schindelin· Nov 30, 2007, 18:28 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Hi,
On Fri, 30 Nov 2007, Jeff King wrote:
Show 40 quoted lines
> Support builtin aliases
> 
> Builtin aliases are "default" alias values that can be
> overridden by user-configured aliases.
> 
> For example, the first such alias is "view", an alias for
> gitk. A user with no further configuration can run
> "git view" to use gitk. However, they can also set the
> config option "alias.view" to "!tig" to run tig.
> ---
>  git.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/git.c b/git.c
> index f220284..95296aa 100644
> --- a/git.c
> +++ b/git.c
> @@ -151,6 +151,13 @@ static int split_cmdline(char *cmdline, const char ***argv)
>  	return count;
>  }
>  
> +static char *builtin_alias(const char *cmd)
> +{
> +	if (!strcmp(cmd, "view"))
> +		return xstrdup("!gitk");
> +	return NULL;
> +}
> +
>  static int handle_alias(int *argcp, const char ***argv)
>  {
>  	int nongit = 0, envchanged = 0, ret = 0, saved_errno = errno;
> @@ -162,6 +169,8 @@ static int handle_alias(int *argcp, const char ***argv)
>  
>  	alias_command = (*argv)[0];
>  	git_config(git_alias_config);
> +	if (!alias_string)
> +		alias_string = builtin_alias(alias_command);
>  	if (alias_string) {
>  		if (alias_string[0] == '!') {
>  			if (*argcp > 1) {

Didn't you mean to put this _before_ the git_config() call? As you wrote it, the "soft" alias overrides the user-specified one.

Ciao, Dscho

Jeff King· Nov 30, 2007, 18:37 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Fri, Nov 30, 2007 at 06:28:50PM +0000, Johannes Schindelin wrote:
Show 12 quoted lines
> > @@ -162,6 +169,8 @@ static int handle_alias(int *argcp, const char ***argv)
> >  
> >  	alias_command = (*argv)[0];
> >  	git_config(git_alias_config);
> > +	if (!alias_string)
> > +		alias_string = builtin_alias(alias_command);
> >  	if (alias_string) {
> >  		if (alias_string[0] == '!') {
> >  			if (*argcp > 1) {
> 
> Didn't you mean to put this _before_ the git_config() call?  As you wrote 
> it, the "soft" alias overrides the user-specified one.

No. The "if (!alias_string)" means we only do the lookup if no user alias was found. Try it.

-Peff
Johannes Schindelin· Nov 30, 2007, 23:05 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Hi,
On Fri, 30 Nov 2007, Jeff King wrote:
Show 17 quoted lines
> On Fri, Nov 30, 2007 at 06:28:50PM +0000, Johannes Schindelin wrote:
> 
> > > @@ -162,6 +169,8 @@ static int handle_alias(int *argcp, const char ***argv)
> > >  
> > >  	alias_command = (*argv)[0];
> > >  	git_config(git_alias_config);
> > > +	if (!alias_string)
> > > +		alias_string = builtin_alias(alias_command);
> > >  	if (alias_string) {
> > >  		if (alias_string[0] == '!') {
> > >  			if (*argcp > 1) {
> > 
> > Didn't you mean to put this _before_ the git_config() call?  As you wrote 
> > it, the "soft" alias overrides the user-specified one.
> 
> No. The "if (!alias_string)" means we only do the lookup if no user
> alias was found. Try it.
Ah.  To me, that was rather easy to miss, though...

Ciao, Dscho

Jeff King· Nov 30, 2007, 23:21 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Fri, Nov 30, 2007 at 11:05:50PM +0000, Johannes Schindelin wrote:
Show 15 quoted lines
> > > >  	alias_command = (*argv)[0];
> > > >  	git_config(git_alias_config);
> > > > +	if (!alias_string)
> > > > +		alias_string = builtin_alias(alias_command);
> > > >  	if (alias_string) {
> > > >  		if (alias_string[0] == '!') {
> > > >  			if (*argcp > 1) {
> > > 
> > > Didn't you mean to put this _before_ the git_config() call?  As you wrote 
> > > it, the "soft" alias overrides the user-specified one.
> > 
> > No. The "if (!alias_string)" means we only do the lookup if no user
> > alias was found. Try it.
> 
> Ah.  To me, that was rather easy to miss, though...
I don't particularly care if it is re-written as:
  alias_string = builtin_alias(alias_command);
  git_config(git_alias_config);

which should be equivalent. I wrote it the original way to avoid doing the O(n) search through builtin aliases when it was unnecessary, but obviously this isn't a performance critical code path.

-Peff
Johannes Schindelin· Nov 30, 2007, 23:38 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Hi,
On Fri, 30 Nov 2007, Jeff King wrote:
Show 26 quoted lines
> On Fri, Nov 30, 2007 at 11:05:50PM +0000, Johannes Schindelin wrote:
> 
> > > > >  	alias_command = (*argv)[0];
> > > > >  	git_config(git_alias_config);
> > > > > +	if (!alias_string)
> > > > > +		alias_string = builtin_alias(alias_command);
> > > > >  	if (alias_string) {
> > > > >  		if (alias_string[0] == '!') {
> > > > >  			if (*argcp > 1) {
> > > > 
> > > > Didn't you mean to put this _before_ the git_config() call?  As you wrote 
> > > > it, the "soft" alias overrides the user-specified one.
> > > 
> > > No. The "if (!alias_string)" means we only do the lookup if no user
> > > alias was found. Try it.
> > 
> > Ah.  To me, that was rather easy to miss, though...
> 
> I don't particularly care if it is re-written as:
> 
>   alias_string = builtin_alias(alias_command);
>   git_config(git_alias_config);
> 
> which should be equivalent.  I wrote it the original way to avoid doing
> the O(n) search through builtin aliases when it was unnecessary, but
> obviously this isn't a performance critical code path.
Actually, I felt/feel quite dumb missing it.

Ciao, Dscho

Nicolas Pitre· Nov 30, 2007, 00:52 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, 29 Nov 2007, Jeff King wrote:
Show 11 quoted lines
> On Thu, Nov 29, 2007 at 04:13:29PM -0800, Junio C Hamano wrote:
> 
> >  - Post v1.5.5, start cooking the change that does not install hardlinks
> >    for built-in commands, aiming for inclusion in v1.5.6, in May-Jun
> >    2008 timeframe.
> 
> I am still against this step, for the reasons mentioned in the mails
> leading up to the one you just quoted. I am fine with "does not install
> hardlinks for builtin-commands on systems that don't support hardlinks"
> (and of course all such hardlinks are in $(libexecdir)/git-core at this
> point).

But only for porcelain, right? You certainly don't need the dashed form of plumbing commands?

Nicolas
Jeff King· Nov 30, 2007, 01:00 UTC · re: Nicolas Pitre · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, Nov 29, 2007 at 07:52:01PM -0500, Nicolas Pitre wrote:
Show 8 quoted lines
> > I am still against this step, for the reasons mentioned in the mails
> > leading up to the one you just quoted. I am fine with "does not install
> > hardlinks for builtin-commands on systems that don't support hardlinks"
> > (and of course all such hardlinks are in $(libexecdir)/git-core at this
> > point).
> 
> But only for porcelain, right?  You certainly don't need the dashed form 
> of plumbing commands?

In principle, yes, though one man's porcelain is another man's plumbing, so determining the correct set is hard (and why bother if they are all hidden from mere mortals, anyway?).

Perhaps because I am actively working on git, I tend to use a fair number of plumbing commands for under-the-hood inspection and experimentation.

-Peff
Nicolas Pitre· Nov 30, 2007, 01:19 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, 29 Nov 2007, Jeff King wrote:
Show 14 quoted lines
> On Thu, Nov 29, 2007 at 07:52:01PM -0500, Nicolas Pitre wrote:
> 
> > > I am still against this step, for the reasons mentioned in the mails
> > > leading up to the one you just quoted. I am fine with "does not install
> > > hardlinks for builtin-commands on systems that don't support hardlinks"
> > > (and of course all such hardlinks are in $(libexecdir)/git-core at this
> > > point).
> > 
> > But only for porcelain, right?  You certainly don't need the dashed form 
> > of plumbing commands?
> 
> In principle, yes, though one man's porcelain is another man's plumbing,
> so determining the correct set is hard (and why bother if they are all
> hidden from mere mortals, anyway?).

That would be a good reason not to bother determining which set to preserve and remove them all then.

Sure you'll miss the dashed form for, say, one week? After that your fingers should be retrained.

Nicolas
Jeff King· Nov 30, 2007, 01:25 UTC · re: Nicolas Pitre · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, Nov 29, 2007 at 08:19:38PM -0500, Nicolas Pitre wrote:
Show 6 quoted lines
> > In principle, yes, though one man's porcelain is another man's plumbing,
> > so determining the correct set is hard (and why bother if they are all
> > hidden from mere mortals, anyway?).
> 
> That would be a good reason not to bother determining which set to 
> preserve and remove them all then.

It clearly argues for putting all in the same boat, yes (but obviously we disagree on which boat).

> Sure you'll miss the dashed form for, say, one week? After that your 
> fingers should be retrained.

Perhaps, although that doesn't address my other point, about non-bash program in the world which already does filename completion (in my case, I am specifically thinking about vim's ":r!", but surely emacs users must have a similar issue).

But that is just talking about the disadvantages; you can argue that they are small, but they are clearly non-zero. More importantly, what are the _advantages_ of removing the hardlinks (and if you haven't read the other message I just sent you, I am talking not about putting hardlinks into a non-PATH directory, but about removing them entirely once they are already in that alternate directory)? If there aren't any advantages, or they are also small, then it makes sense to keep the hardlinks.

-Peff
Nicolas Pitre· Nov 30, 2007, 01:33 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, 29 Nov 2007, Jeff King wrote:
Show 28 quoted lines
> On Thu, Nov 29, 2007 at 08:19:38PM -0500, Nicolas Pitre wrote:
> 
> > > In principle, yes, though one man's porcelain is another man's plumbing,
> > > so determining the correct set is hard (and why bother if they are all
> > > hidden from mere mortals, anyway?).
> > 
> > That would be a good reason not to bother determining which set to 
> > preserve and remove them all then.
> 
> It clearly argues for putting all in the same boat, yes (but obviously
> we disagree on which boat).
> 
> > Sure you'll miss the dashed form for, say, one week? After that your 
> > fingers should be retrained.
> 
> Perhaps, although that doesn't address my other point, about non-bash
> program in the world which already does filename completion (in my case,
> I am specifically thinking about vim's ":r!", but surely emacs users
> must have a similar issue).
> 
> But that is just talking about the disadvantages; you can argue that
> they are small, but they are clearly non-zero. More importantly, what
> are the _advantages_ of removing the hardlinks (and if you haven't read
> the other message I just sent you, I am talking not about putting
> hardlinks into a non-PATH directory, but about removing them entirely
> once they are already in that alternate directory)? If there aren't any
> advantages, or they are also small, then it makes sense to keep the
> hardlinks.

So what you want is for the dashed hardlinks to exist _inside_ the libexec directory, even if most people won't "see" them due to that libexec directory not being in the shell path, right?

If that is what you mean then I personally don't care at all.
Nicolas
Jeff King· Nov 30, 2007, 01:53 UTC · re: Nicolas Pitre · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, Nov 29, 2007 at 08:33:39PM -0500, Nicolas Pitre wrote:
Show 5 quoted lines
> So what you want is for the dashed hardlinks to exist _inside_ the 
> libexec directory, even if most people won't "see" them due to that 
> libexec directory not being in the shell path, right?
> 
> If that is what you mean then I personally don't care at all.
That is exactly what I mean.
-Peff
A Large Angry SCM· Nov 30, 2007, 02:23 UTC · re: Jeff King · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Jeff King wrote:
Show 9 quoted lines
> On Thu, Nov 29, 2007 at 08:33:39PM -0500, Nicolas Pitre wrote:
> 
>> So what you want is for the dashed hardlinks to exist _inside_ the 
>> libexec directory, even if most people won't "see" them due to that 
>> libexec directory not being in the shell path, right?
>>
>> If that is what you mean then I personally don't care at all.
> 
> That is exactly what I mean.
And that also works for me when I set libexec=$(bindir).
Nguyen Thai Ngoc Duy· Nov 30, 2007, 00:40 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Nov 30, 2007 7:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
>  - Post v1.5.4, start cooking gitexecdir=$(libexecdir)/git-core, aiming
>    for inclusion in v1.5.5, perhaps in Feb-Mar 2008 timeframe.  This
>    will also affect the sample RPM spec and resulting RPM binary
>    packages I will place on k.org, and I'll ask Gerrit to do the same on
>    Debian side.  The official binary packaging of individual distros are
>    not under my control, but if there is a handy list of people I can
>    send this notice to for other distros, that would help this process.
You can find Gentoo maintainers here:
http://sources.gentoo.org/viewcvs.py/gentoo-x86/dev-util/git/metadata.xml?rev=1.6&view=markup
-- 
Duy
A Large Angry SCM· Nov 30, 2007, 00:51 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Junio C Hamano wrote: [...]

Show 35 quoted lines
> Ok.  So here is a revised roadmap that a panda brain (that is not so
> well working today due to fever) came up.
> 
>  - v1.5.4 will ship with gitexecdir=$(bindir) in Makefile.  But the
>    release notes for the version will warn users that:
> 
>    (1) using git-foo from the command line, and
> 
>    (2) using git-foo from your scripts without first prepending the
>        return value of "git --exec-path" to the PATH
> 
>    is now officially deprecated (it has been deprecated for a long time
>    since January 2006, v1.2.0~149) and upcoming v1.5.5 will ship with
>    the default configuration that does not install git-foo form in
>    user's PATH.
> 
>    If further will warn users that git-foo form will be removed in
>    v1.5.6 for many commands and it will be merely an accident if some of
>    them still work after that.
> 
>  - Post v1.5.4, start cooking gitexecdir=$(libexecdir)/git-core, aiming
>    for inclusion in v1.5.5, perhaps in Feb-Mar 2008 timeframe.  This
>    will also affect the sample RPM spec and resulting RPM binary
>    packages I will place on k.org, and I'll ask Gerrit to do the same on
>    Debian side.  The official binary packaging of individual distros are
>    not under my control, but if there is a handy list of people I can
>    send this notice to for other distros, that would help this process.
> 
>  - The release notes for v1.5.5 will warn users again that git-foo will
>    be removed in v1.5.6 for many commands and it will be merely an
>    accident if some of them still work.
> 
>  - Post v1.5.5, start cooking the change that does not install hardlinks
>    for built-in commands, aiming for inclusion in v1.5.6, in May-Jun
>    2008 timeframe.

Again, there needs to remain support in the Makefile to install the "dashed" versions of the commands for those that want it; and be able to set gitexecdir=$(binder) without editing the Makefile.

Johannes Schindelin· Nov 30, 2007, 00:54 UTC · re: A Large Angry SCM · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Hi,
On Thu, 29 Nov 2007, A Large Angry SCM wrote:
Show 41 quoted lines
> Junio C Hamano wrote:
> [...]
> > Ok.  So here is a revised roadmap that a panda brain (that is not so
> > well working today due to fever) came up.
> > 
> >  - v1.5.4 will ship with gitexecdir=$(bindir) in Makefile.  But the
> >    release notes for the version will warn users that:
> > 
> >    (1) using git-foo from the command line, and
> > 
> >    (2) using git-foo from your scripts without first prepending the
> >        return value of "git --exec-path" to the PATH
> > 
> >    is now officially deprecated (it has been deprecated for a long time
> >    since January 2006, v1.2.0~149) and upcoming v1.5.5 will ship with
> >    the default configuration that does not install git-foo form in
> >    user's PATH.
> > 
> >    If further will warn users that git-foo form will be removed in
> >    v1.5.6 for many commands and it will be merely an accident if some of
> >    them still work after that.
> > 
> >  - Post v1.5.4, start cooking gitexecdir=$(libexecdir)/git-core, aiming
> >    for inclusion in v1.5.5, perhaps in Feb-Mar 2008 timeframe.  This
> >    will also affect the sample RPM spec and resulting RPM binary
> >    packages I will place on k.org, and I'll ask Gerrit to do the same on
> >    Debian side.  The official binary packaging of individual distros are
> >    not under my control, but if there is a handy list of people I can
> >    send this notice to for other distros, that would help this process.
> > 
> >  - The release notes for v1.5.5 will warn users again that git-foo will
> >    be removed in v1.5.6 for many commands and it will be merely an
> >    accident if some of them still work.
> > 
> >  - Post v1.5.5, start cooking the change that does not install hardlinks
> >    for built-in commands, aiming for inclusion in v1.5.6, in May-Jun
> >    2008 timeframe.
> 
> Again, there needs to remain support in the Makefile to install the "dashed"
> versions of the commands for those that want it; and be able to set
> gitexecdir=$(binder) without editing the Makefile.

Umm. Why? If there is no compelling reason (and you named none, but there were quite a few reasons against it), why should the Makefile support the dash form after 1.5.6?

Ciao, Dscho

A Large Angry SCM· Nov 30, 2007, 02:03 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 29 Nov 2007, A Large Angry SCM wrote:
[...]
Show 7 quoted lines
>> Again, there needs to remain support in the Makefile to install the "dashed"
>> versions of the commands for those that want it; and be able to set
>> gitexecdir=$(binder) without editing the Makefile.
> 
> Umm.  Why?  If there is no compelling reason (and you named none, but 
> there were quite a few reasons against it), why should the Makefile 
> support the dash form after 1.5.6?

Not all shells support customized command completion (check the archives); "bang" history (check the archives); etc.

Nicolas Pitre· Nov 30, 2007, 01:01 UTC · re: A Large Angry SCM · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, 29 Nov 2007, A Large Angry SCM wrote:
> Again, there needs to remain support in the Makefile to install the "dashed"
> versions of the commands for those that want it; and be able to set
> gitexecdir=$(binder) without editing the Makefile.

Well, if you want a "non standard" installation, maybe you just can edit a line or two in the Makefile?

Nicolas
A Large Angry SCM· Nov 30, 2007, 02:17 UTC · re: Nicolas Pitre · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Nicolas Pitre wrote:
Show 8 quoted lines
> On Thu, 29 Nov 2007, A Large Angry SCM wrote:
> 
>> Again, there needs to remain support in the Makefile to install the "dashed"
>> versions of the commands for those that want it; and be able to set
>> gitexecdir=$(binder) without editing the Makefile.
> 
> Well, if you want a "non standard" installation, maybe you just can edit 
> a line or two in the Makefile?
Why do you object to a "install-dashed" Makefile target?
Nicolas Pitre· Nov 30, 2007, 02:27 UTC · re: A Large Angry SCM · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Thu, 29 Nov 2007, A Large Angry SCM wrote:
Show 12 quoted lines
> Nicolas Pitre wrote:
> > On Thu, 29 Nov 2007, A Large Angry SCM wrote:
> > 
> > > Again, there needs to remain support in the Makefile to install the
> > > "dashed"
> > > versions of the commands for those that want it; and be able to set
> > > gitexecdir=$(binder) without editing the Makefile.
> > 
> > Well, if you want a "non standard" installation, maybe you just can edit a
> > line or two in the Makefile?
> 
> Why do you object to a "install-dashed" Makefile target?
As long as it is not the default I don't object.
Nicolas
Jakub Narebski· Nov 29, 2007, 00:14 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

Nguyen Thai Ngoc Duy wrote:
Show 12 quoted lines
> On Nov 28, 2007 8:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> In case somebody is thinking about 36e5e70e0f40 (Start deprecating
>> "git-command" in favor of "git command"), that is a somewhat different
>> issue.  What Linus suggested is not installing git-foo link for built-in
>> commands _anywhere_ on the filesystem.  Not just "out of user's PATH".
>> That is not deprecating dash form but removing the support for it.  We
>> need to give ample time for users to adjust to such a change.
> 
> A little note on this one. I've been using git without builtin links
> for a while with my git-box port. There are still some builtin fixups
> needed. And because execv_git_cmd() always uses dash form, so it's
> impossible to use vanilla git without builtin links.
By the way, what is the status of your git-box port?
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Junio C Hamano· Dec 1, 2007, 19:32 UTC · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

"Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
Show 12 quoted lines
> On Nov 30, 2007 10:50 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> Well, different people will want different viewers *anyway* (ie some will
>> prefer qgit etc), so how about making "git view" be something that
>> literally acts as a built-in alias that just defaults to running gitk (if
>> for no other reason than the fact that gitk is the one that ships with
>> git, and simply has most users).
>
> We already have "git show", now we gonna get "git view", git trainers
> may have hard time explaining this one shows you a particular object
> while the other one shows you history. How about "git lshistory" (from
> clearcase land) or git show --history?
Heh, we have "bisect visualize".  How about "git visualize"?
Jeff King· Dec 1, 2007, 21:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Sat, Dec 01, 2007 at 11:32:27AM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> > We already have "git show", now we gonna get "git view", git trainers
> > may have hard time explaining this one shows you a particular object
> > while the other one shows you history. How about "git lshistory" (from
> > clearcase land) or git show --history?
> 
> Heh, we have "bisect visualize".  How about "git visualize"?

Yes, in retrospect "view" is probably not the best. "lshistory" just looks awful, and I think it's wrong for an option to "git show" to change it from a terminal application into a GUI application.

"visualize" is actually pretty good, except that it would be painful to type. On the other hand, I will probably still just type "gitk". I actually think these sorts of aliases may be most useful for user-facing scripts to say "and now show the dataset in the user's graphical history browser of choice."

-Peff
Nguyen Thai Ngoc Duy· Dec 2, 2007, 05:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] Move all dashed form git commands to libexecdir

On Dec 2, 2007 2:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
>
> > On Nov 30, 2007 10:50 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >
> >> Well, different people will want different viewers *anyway* (ie some will
> >> prefer qgit etc), so how about making "git view" be something that
> >> literally acts as a built-in alias that just defaults to running gitk (if
> >> for no other reason than the fact that gitk is the one that ships with
> >> git, and simply has most users).
> >
> > We already have "git show", now we gonna get "git view", git trainers
> > may have hard time explaining this one shows you a particular object
> > while the other one shows you history. How about "git lshistory" (from
> > clearcase land) or git show --history?
>
> Heh, we have "bisect visualize".  How about "git visualize"?
>
"git visualize"++
-- 
Duy

← back to recent threads