[PATCH] Invoke git-repo-config directly.

[PATCH] Invoke git-repo-config directly.

[Qingning Huo]

The system have GNU git installed at /usr/bin/git.  I installed git-core
to ~/opt/bin.  ~/opt/bin is in my PATH, but is after /usr/bin.  I have
set alias git="$HOME/opt/bin/git".

git-push and git-pull behaves strangely, because they call "git
repo-config", which runs /usr/bin/git.  Using "git-repo-config" directly
fixed the problem.

Signed-off-by: Qingning Huo <qhuo@mayhq.co.uk>

---

 git-pull.sh     |    4 ++--
 git-sh-setup.sh |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

a0194fff002cb12ac58b202201d387f8ea55b225
diff --git a/git-pull.sh b/git-pull.sh
index 6caf1aa..e32e2b0 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -70,7 +70,7 @@ case "$merge_head" in
 	exit 0
 	;;
 ?*' '?*)
-	var=`git repo-config --get pull.octopus`
+	var=`git-repo-config --get pull.octopus`
 	if test '' = "$var"
 	then
 		strategy_default_args='-s octopus'
@@ -79,7 +79,7 @@ case "$merge_head" in
 	fi
 	;;
 *)
-	var=`git repo-config --get pull.twohead`
+	var=`git-repo-config --get pull.twohead`
 	if test '' = "$var"
 	then
 		strategy_default_args='-s recursive'
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 025ef2d..12f5ede 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -41,7 +41,7 @@ then
 	: ${GIT_OBJECT_DIRECTORY="$GIT_DIR/objects"}
 
 	# Make sure we are in a valid repository of a vintage we understand.
-	GIT_DIR="$GIT_DIR" git repo-config --get core.nosuch >/dev/null
+	GIT_DIR="$GIT_DIR" git-repo-config --get core.nosuch >/dev/null
 	if test $? = 128
 	then
 	    exit
-- 
1.2.4.ga019-dirty

Re: [PATCH] Invoke git-repo-config directly.

[Johannes Schindelin]

Hi,

On Tue, 14 Mar 2006, Qingning Huo wrote:

> -	var=`git repo-config --get pull.octopus`
> +	var=`git-repo-config --get pull.octopus`

This is unlikely to be applied; there are plans to have a "libexec" path 
in which all git executables are stored, and just the "git" wrapper in the 
path. Your patch would break git in those setups.

Ciao,
Dscho

P.S.: BTW there are quite a few discussions of this in the mailing list 
archives...

Re: [PATCH] Invoke git-repo-config directly.

[Qingning Huo]

On Tue, Mar 14, 2006 at 10:20:53PM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 14 Mar 2006, Qingning Huo wrote:
> 
> > -	var=`git repo-config --get pull.octopus`
> > +	var=`git-repo-config --get pull.octopus`
> 
> This is unlikely to be applied; there are plans to have a "libexec" path 
> in which all git executables are stored, and just the "git" wrapper in the 
> path. Your patch would break git in those setups.
> 

I do not mind whether this patch is applied.  What I want is git calls
its helper programs, instead of any random git program in my PATH.  If
git-programs are installed to libexec path, how about calling them
with absolute path?

Regards,
Qingning

Re: [PATCH] Invoke git-repo-config directly.

[Linus Torvalds]

On Tue, 14 Mar 2006, Qingning Huo wrote:
>
> The system have GNU git installed at /usr/bin/git.  I installed git-core
> to ~/opt/bin.  ~/opt/bin is in my PATH, but is after /usr/bin.  I have
> set alias git="$HOME/opt/bin/git".

This should not be a problem with the modern "git.c" wrapper. It 
_should_, if you call it with the full path, automatically prepend that 
path to the PATH when executing sub-commands. 

So if you run git as "$HOME/opt/bin/git", the PATH _should_ be 
 - first the "PREFIX/bin" path as defined by the build
 - second the "$HOME/opt/bin/" path as defined by the fact that you ran 
   git from that path
 - finally the normal $PATH.

To check this out, do this:

	ln -s /usr/bin/printenv ~/opt/bin/git-printenv
	git printenv

and you should see the proper PATH that git ends up using internally that 
way.

So your problem seems to be that you do "git-pull", when you really should 
do "git pull" (where that wrapper will set up PATH for you). Since you 
don't use the wrapper, the scripts end up doing the wrong thing.

		Linus

Re: [PATCH] Invoke git-repo-config directly.

[Qingning Huo]

On Tue, Mar 14, 2006 at 01:58:09PM -0800, Linus Torvalds wrote:
> 
> 
> On Tue, 14 Mar 2006, Qingning Huo wrote:
> >
> > The system have GNU git installed at /usr/bin/git.  I installed git-core
> > to ~/opt/bin.  ~/opt/bin is in my PATH, but is after /usr/bin.  I have
> > set alias git="$HOME/opt/bin/git".
> 
> This should not be a problem with the modern "git.c" wrapper. It 
> _should_, if you call it with the full path, automatically prepend that 
> path to the PATH when executing sub-commands. 
> 
> So if you run git as "$HOME/opt/bin/git", the PATH _should_ be 
>  - first the "PREFIX/bin" path as defined by the build
>  - second the "$HOME/opt/bin/" path as defined by the fact that you ran 
>    git from that path
>  - finally the normal $PATH.
> 
> To check this out, do this:
> 
> 	ln -s /usr/bin/printenv ~/opt/bin/git-printenv
> 	git printenv
> 
> and you should see the proper PATH that git ends up using internally that 
> way.
> 
> So your problem seems to be that you do "git-pull", when you really should 
> do "git pull" (where that wrapper will set up PATH for you). Since you 
> don't use the wrapper, the scripts end up doing the wrong thing.
> 

Thanks for your detailed explanation.  Yes, "git push" and "git pull"
both work fine out of the box.  That is the good thing.  But,

$ grep git git-pull.sh

. git-sh-setup
orig_head=$(git-rev-parse --verify HEAD) || die "Pulling into a black hole?"
git-fetch --update-head-ok "$@" || exit 1
curr_head=$(git-rev-parse --verify HEAD)
        git-read-tree -u -m "$orig_head" "$curr_head" ||
        var=`git repo-config --get pull.octopus`
        var=`git repo-config --get pull.twohead`
merge_name=$(git-fmt-merge-msg <"$GIT_DIR/FETCH_HEAD")
git-merge $no_summary $no_commit $strategy_args "$merge_name" HEAD $merge_head

We have "git-read-tree" and "git repo-config" at the same time.  Are
there any rules saying which form should be preferred?  How about pick
one form and stick to it?

If we uniformly call git helper programs/scripts with "git helper"
style, would git(1) append two paths to PATH everytime it is being
invoked?  For example, "git pull" -> "git repo-config" would prepend
~/opt/bin four times to PATH.  This wouldn't be very effecient.

Regards,
Qingning

Re: [PATCH] Invoke git-repo-config directly.

[Linus Torvalds]

On Tue, 14 Mar 2006, Qingning Huo wrote:
> 
> Thanks for your detailed explanation.  Yes, "git push" and "git pull"
> both work fine out of the box.  That is the good thing.  But,
> 
> $ grep git git-pull.sh
> 
> . git-sh-setup
> orig_head=$(git-rev-parse --verify HEAD) || die "Pulling into a black hole?"
> git-fetch --update-head-ok "$@" || exit 1
> curr_head=$(git-rev-parse --verify HEAD)
>         git-read-tree -u -m "$orig_head" "$curr_head" ||
>         var=`git repo-config --get pull.octopus`
>         var=`git repo-config --get pull.twohead`
> merge_name=$(git-fmt-merge-msg <"$GIT_DIR/FETCH_HEAD")
> git-merge $no_summary $no_commit $strategy_args "$merge_name" HEAD $merge_head
> 
> We have "git-read-tree" and "git repo-config" at the same time.  Are
> there any rules saying which form should be preferred?  How about pick
> one form and stick to it?

I agree that it is inconsistent as-is. So a patch to make it use the 
"git-repo-config" form (the argument being that internally, we use the 
full names) might be good if just for consistency.

		Linus

Re: [PATCH] Invoke git-repo-config directly.

[Qingning Huo]

On Tue, Mar 14, 2006 at 03:07:39PM -0800, Linus Torvalds wrote:
> On Tue, 14 Mar 2006, Qingning Huo wrote:
> > We have "git-read-tree" and "git repo-config" at the same time.  Are
> > there any rules saying which form should be preferred?  How about pick
> > one form and stick to it?
> 
> I agree that it is inconsistent as-is. So a patch to make it use the 
> "git-repo-config" form (the argument being that internally, we use the 
> full names) might be good if just for consistency.
> 

Can these two patches be accepted then?  What do others think?

Qingning

Re: [PATCH] Invoke git-repo-config directly.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> I agree that it is inconsistent as-is. So a patch to make it use the 
> "git-repo-config" form (the argument being that internally, we use the 
> full names) might be good if just for consistency.

If we do the dash-form for consistency's sake, we should do
PATH="`git --exec-path`:$PATH" in git-setup-sh when/before we do
so.  For scripts that do not use git-setup-sh they must have
their own.

This would make difference if/when we switch to
/usr/lib/git/exec, but until then it is not.

Re: [PATCH] Invoke git-repo-config directly.

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

(replying to myself ...)

> Linus Torvalds <torvalds@osdl.org> writes:
>
>> I agree that it is inconsistent as-is. So a patch to make it use the 
>> "git-repo-config" form (the argument being that internally, we use the 
>> full names) might be good if just for consistency.
>
> If we do the dash-form for consistency's sake,...

Probably I should add that personally my preference is to standardize on
the dashless form.

Re: [PATCH] Invoke git-repo-config directly.

[Qingning Huo]

On Wed, Mar 15, 2006 at 01:35:22PM -0800, Junio C Hamano wrote:
> Junio C Hamano <junkio@cox.net> writes:
> 
> > Linus Torvalds <torvalds@osdl.org> writes:
> >
> >> I agree that it is inconsistent as-is. So a patch to make it use the 
> >> "git-repo-config" form (the argument being that internally, we use the 
> >> full names) might be good if just for consistency.
> >
> > If we do the dash-form for consistency's sake,...
> 
> Probably I should add that personally my preference is to standardize on
> the dashless form.
> 

That would remove the possibility to run "git-push" in my scenario.  And
what is the benefit?  Wasting CPU cycles to an extra execve?
Duplicating git --exec-path to PATH?

Personally I recommend to always run git helper programs with absolute
path, this would remove all confusions, not play with users' PATH
environment, and save CPU cycles.

Qingning

Re: [PATCH] Invoke git-repo-config directly.

[Linus Torvalds]

On Wed, 15 Mar 2006, Junio C Hamano wrote:
> 
> If we do the dash-form for consistency's sake, we should do
> PATH="`git --exec-path`:$PATH" in git-setup-sh when/before we do
> so.

Yes. That would make sense too. Then git-setup-sh would look more like 
what the builtin git.c does.

		Linus

Re: [PATCH] Invoke git-repo-config directly.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Wed, 15 Mar 2006, Junio C Hamano wrote:
>> 
>> If we do the dash-form for consistency's sake, we should do
>> PATH="`git --exec-path`:$PATH" in git-setup-sh when/before we do
>> so.
>
> Yes. That would make sense too. Then git-setup-sh would look more like 
> what the builtin git.c does.

I think some historical background is in order.

We started without bindir vs execdir distinction but we wanted a
way to someday migrate out of putting everything in bindir.  As
one part of the solution, "git" wrapper was invented, and as the
result of that effort, some parts of the scripts, and lot of
documentation pages and sample scripts, lost dashes.

Historically, git tools have always wanted everything git-* to
be found on user's PATH, and we were alarmed to see 100+ git-*
commands in /usr/bin.  That's why "git" wrapper and
GIT_EXEC_PATH environment were invented.  People can have
/usr/bin/git and no other git-* on their PATH, because that
"git" knows where to find the rest of git-* commands.  For that
to work, the scripts should know where to find the rest -- and
cleanest way is to run others via "git foo" form.

Consistency via s/git-foo/git foo/g _is_ the goal, but that kind
of change interferes with the other patches that do the real
work, and it is kind of boring, so nobody has done wholesale
clean-up of all the scripts.

Invoking with the full path is not an option -- it makes
building and testing-before-installing process too cumbersome.
You are welcome to try and send in a patch to do that if (and
only if) you volunteer to go the whole nine yards, but I have to
warn you that that approach is something we have already
considered and discarded, one reason why is because it makes the
testsuite unworkable (testing needs to be done before
installing).

If you want to use the other "git" (GNU interactive tools, which
I once heard is changing its name to gitfm or something like
that -- how nice of them -- but has it ever happened?), and if
you want to have /usr/bin (which has that "git") and then
/other/bin (which has on-topic "git") on your PATH, in that
order, that would be a problem.  Saying just "git" would invoke
other "git" that does not know what to do.

If you can solve that without hardcoding the full path in our
scripts, that would be ideal.  But otherwise, especially with
changing things back to "git-foo" form without making sure going
backward in that way would not hinder the migration out of
/usr/bin, then that creates more problems than it solves.

So that is why I said I would prefer dashless form.

I _think_ the simplest fix is to change the order of directories
you list on your PATH so that "git" is found before GNU
interactive tool, which is my impression that most people seem
to do (many in fact do not have GNU interactive tool on their
PATH anywhere).

Re: [PATCH] Invoke git-repo-config directly.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Wed, 15 Mar 2006, Junio C Hamano wrote:
>> 
>> If we do the dash-form for consistency's sake, we should do
>> PATH="`git --exec-path`:$PATH" in git-setup-sh when/before we do
>> so.
>
> Yes. That would make sense too. Then git-setup-sh would look more like 
> what the builtin git.c does.

I actually think what git.c does is _wrong_.

Believe it or not, there are people with bizarre configurations
just like Qingning's is bizarre in different sense [*1*].  They
have diff they do not like (either broken or inadequete)
installed under /usr/bin, and they install GNU diff under
/home/$u/bin, and have their PATH set so that /home/$u/bin/diff
is found before /usr/bin/diff (up to here, there is nothing
bizarre).  However, somehow they have the latest "git" installed
under /usr/bin (this _is_ the bizarre part).

Earlier, we prepended GIT_EXEC_PATH to user-supplied PATH when
C-level routines needed to exec git programs.  This breaks that
"bizarre" setup -- for such a setup, GIT_EXEC_PATH is currently
set to /usr/bin and when we try to exec "diff", we ended up
running /usr/bin/diff.  So in order to work this around, we
introduced exec[vl]_git_cmd() wrappers which use --exec-path
(if the command supports per invocation override), GIT_EXEC_PATH
environment, and then gitexecdir in Makefile when it was built,
in this order.  "git" wrapper should know about this, but the
older code was never removed when we introduced exec_cmd.c.

I think we need to do something like the attached patch.

[Footnote]

*1* I first wrote "configuration even more bizarre than
Qingning's", but before sending the message out I came to
senses.  We have to admit that that configuration is quite
unusual, and I would say it is the one more bizarre than the one
in question here.  When people have standard system executable
directories such as /usr/bin and /bin and private directories
like /home/$u/bin and /usr/local/bin on their PATH, private
directories always come before the system directories, so that
things in the system directories (often you do not have write
permission to) can be overriden by private directories (which is
under your control).

-- >8 --

diff --git a/git.c b/git.c
index 0b40e30..25e6a4e 100644
--- a/git.c
+++ b/git.c
@@ -219,25 +219,6 @@ static void cmd_usage(int show_all, cons
 	exit(1);
 }
 
-static void prepend_to_path(const char *dir, int len)
-{
-	char *path, *old_path = getenv("PATH");
-	int path_len = len;
-
-	if (!old_path)
-		old_path = "/usr/local/bin:/usr/bin:/bin";
-
-	path_len = len + strlen(old_path) + 1;
-
-	path = malloc(path_len + 1);
-
-	memcpy(path, dir, len);
-	path[len] = ':';
-	memcpy(path + len + 1, old_path, path_len - len);
-
-	setenv("PATH", path, 1);
-}
-
 static void show_man_page(const char *git_cmd)
 {
 	const char *page;
@@ -447,18 +428,6 @@ int main(int argc, const char **argv, ch
 	}
 	argv[0] = cmd;
 
-	/*
-	 * We search for git commands in the following order:
-	 *  - git_exec_path()
-	 *  - the path of the "git" command if we could find it
-	 *    in $0
-	 *  - the regular PATH.
-	 */
-	if (exec_path)
-		prepend_to_path(exec_path, strlen(exec_path));
-	exec_path = git_exec_path();
-	prepend_to_path(exec_path, strlen(exec_path));
-
 	/* See if it's an internal command */
 	handle_internal_command(argc, argv, envp);
 

Re: [PATCH] Invoke git-repo-config directly.

[Qingning Huo]

On Wed, Mar 15, 2006 at 03:35:36PM -0800, Junio C Hamano wrote:
> Linus Torvalds <torvalds@osdl.org> writes:
> 
> > On Wed, 15 Mar 2006, Junio C Hamano wrote:
> >> 
> >> If we do the dash-form for consistency's sake, we should do
> >> PATH="`git --exec-path`:$PATH" in git-setup-sh when/before we do
> >> so.
> >
> > Yes. That would make sense too. Then git-setup-sh would look more like 
> > what the builtin git.c does.
> 
> I think some historical background is in order.
> 
> We started without bindir vs execdir distinction but we wanted a
> way to someday migrate out of putting everything in bindir.  As
> one part of the solution, "git" wrapper was invented, and as the
> result of that effort, some parts of the scripts, and lot of
> documentation pages and sample scripts, lost dashes.
> 
> Historically, git tools have always wanted everything git-* to
> be found on user's PATH, and we were alarmed to see 100+ git-*
> commands in /usr/bin.  That's why "git" wrapper and
> GIT_EXEC_PATH environment were invented.  People can have
> /usr/bin/git and no other git-* on their PATH, because that
> "git" knows where to find the rest of git-* commands.  For that
> to work, the scripts should know where to find the rest -- and
> cleanest way is to run others via "git foo" form.
> 
> Consistency via s/git-foo/git foo/g _is_ the goal, but that kind
> of change interferes with the other patches that do the real
> work, and it is kind of boring, so nobody has done wholesale
> clean-up of all the scripts.

Thanks for the historical background.

> Invoking with the full path is not an option -- it makes
> building and testing-before-installing process too cumbersome.
> You are welcome to try and send in a patch to do that if (and
> only if) you volunteer to go the whole nine yards, but I have to
> warn you that that approach is something we have already
> considered and discarded, one reason why is because it makes the
> testsuite unworkable (testing needs to be done before
> installing).

So we recognize the full path approach is desired, but because of
technique reasons (building and testing), it is not applied.  I would
like to have a look of the said patch.  Can you give me some pointers?

> If you want to use the other "git" (GNU interactive tools, which
> I once heard is changing its name to gitfm or something like
> that -- how nice of them -- but has it ever happened?), and if
> you want to have /usr/bin (which has that "git") and then
> /other/bin (which has on-topic "git") on your PATH, in that
> order, that would be a problem.  Saying just "git" would invoke
> other "git" that does not know what to do.

This is exactly the problem I want to solve.  In this case, I alias git
to use our git program.

> If you can solve that without hardcoding the full path in our
> scripts, that would be ideal.  But otherwise, especially with
> changing things back to "git-foo" form without making sure going
> backward in that way would not hinder the migration out of
> /usr/bin, then that creates more problems than it solves.

We do not hard code the full path in the script.  We can use a function
gitexec() (in git-sh-setup) similiar to execv_git_cmd().  gitexec
searches git-command in GIT_EXEC_PATH, and then libexedir.  libexedir is
hardcoded in git-sh-setup.  But can be override by the environment
GIT_EXEC_PATH.  The make test target should set GIT_EXEC_PATH to the
build directory.

The remaining job is search and replace.  If we call gitexec() git(), we
can even keep the "git command" form unchanged.  But I prefer explicit
gitexec().

> I _think_ the simplest fix is to change the order of directories
> you list on your PATH so that "git" is found before GNU
> interactive tool, which is my impression that most people seem
> to do (many in fact do not have GNU interactive tool on their
> PATH anywhere).

I cannot uninstall /usr/bin/git.  It is not under my control.  I am
wondering whether I should move my home dir to the front of the PATH.
But a good tool should not force users to change their settings.

Qingning

Re: [PATCH] Invoke git-repo-config directly.

[Junio C Hamano]

Qingning Huo <qhuo@mayhq.org> writes:

> I cannot uninstall /usr/bin/git.  It is not under my control.  I am
> wondering whether I should move my home dir to the front of the PATH.

That's what I found your configuration quite unusual.

When people have standard system executable directories such as
/usr/bin and /bin and private directories like /home/$u/bin and
/usr/local/bin on their PATH, private directories always come
before the system directories, so that things in the system
directories (often you do not have write permission to) can be
overriden by private directories (which is under your control).
I've never seen anybody sane to have system path before private
ones.

Re: [PATCH] Invoke git-repo-config directly.

[Junio C Hamano]

Qingning Huo <qhuo@mayhq.org> writes:

> So we recognize the full path approach is desired,

I said "if you can work problems your environment has _without_
doing the full path thing, then it would be ideal".

I never said full path is desired -- I despise full path, in
fact.  It makes certain things very inconvenient.

> ...but because of
> technique reasons (building and testing), it is not applied.

There were not even a patch.  I suspect people involved in the
discussion realized that approach was unworkably cumbersome.

In our Makefile, we have sed script mechanism to replace tokens
so we _could_ change our sources to do something like this:

        diff --git a/git-commit.sh b/git-commit.sh
        index 330a434..10835c6 100755
        --- a/git-commit.sh
        +++ b/git-commit.sh
	...
        @@ -115,7 +115,7 @@ run_status () {
                    echo '#
         # Initial commit
         #'
        -	    git-ls-files |
        +	    @@GIT_PATH@@git-ls-files |
                    sed -e '
                            s/\\/\\\\/g
                            s/ /\\ /g
        @@ -126,7 +126,7 @@ run_status () {
                    committable="$?"
                fi

        -	git-diff-files --name-status |
        +	@@GIT_PATH@@git-diff-files --name-status |
                sed -e '
                        s/\\/\\\\/g
                        s/ /\\ /g
	...

and sed it out with 's/@@GIT_PATH@@/$(gitexecdir_SQ)/g'.

However, you have to realize that I often want to try things out
*without* running "make", let alone installing.  The current way
things are set up lets me say:

	$ GIT_EXEC_PATH=$my_git_source \
          sh -x $my_git_source/git-commit.sh

to see where things break.  Changing things the way I quoted
above would make things _extremely_ inconvenient for me.

And it is ugly.  "Making things ugly and inconvenient for what
purpose?" is the question I have to ask myself at this point.

And if the answer is "to support unusual configuration which,
quite frankly, I think is broken", then...

We could probably define a shell function that looks like:

        git_exec () {
                cmd="$1"
                shift
                case "${GIT_EXEC_PATH+set}" in
                set) ;;
                *) GIT_EXEC_PATH='@@GIT_EXEC_PATH@@' ;;
                esac
                "$GIT_EXEC_PATH/git-$cmd" "$@"
        }

in git-sh-setup [*1*], and then rewrite the above to something
like this instead:

        diff --git a/git-commit.sh b/git-commit.sh
        index 330a434..8a73420 100755
        --- a/git-commit.sh
        +++ b/git-commit.sh
        ...
        @@ -115,7 +115,7 @@ run_status () {
                    echo '#
         # Initial commit
         #'
        -	    git-ls-files |
        +	    git_exec ls-files |
                    sed -e '
                            s/\\/\\\\/g
                            s/ /\\ /g
        @@ -126,7 +126,7 @@ run_status () {
                    committable="$?"
                fi

        -	git-diff-files  --name-status |
        +	git_exec diff-files  --name-status |
                sed -e '
                        s/\\/\\\\/g
                        s/ /\\ /g
        ...

But that does not cover Perl nor Python scripts, and does not
address the ugliness either.


[Footnote]

*1* BTW, I just noticed that git-sh-setup needs to be on user's
PATH, so we probably have to inline and duplicate the git_exec()
shell function definition at the beginning of each script after
all, when we make the initial ". git-sh-setup" inclusion to
honor GIT_EXEC_PATH without munging the user's PATH.

Which is not a big deal by itself, since we preprocess
*.{sh,perl,py} files anyway, but still it leaves a _big_
ugliness factor.

Re: [PATCH] Invoke git-repo-config directly.

[Junio C Hamano]

Qingning Huo <qhuo@mayhq.org> writes:

> On Wed, Mar 15, 2006 at 03:35:36PM -0800, Junio C Hamano wrote:
>> I think some historical background is in order.
>> ...
> Thanks for the historical background.

Sorry, it just occurred to me that I was totally confused, and I
apologize for giving only half of the historical background,
without giving the "end of the story -- conclusion", which I did
not remember until now.

What we ended up deciding after that "My /usr/bin/git is sane
but /usr/bin/diff is wrong so I have /home/$u/bin/diff and
/home/$u/bin is listed earlier in my $PATH" discussion, as I
remember, was this:

 * We expect users to invoke things on the command line via the
   "git" wrapper.  Either having the directory that has our
   "git" in earlier on the PATH, or giving the full path to it
   as the command (your use of alias qualifies as the latter).

 * One of the important things the "git" wrapper does is to
   prefix GIT_EXEC_PATH to the $PATH environment before invoking
   the "git-foo" commands [*1*].  

 * "git-foo" command, if it is written in a scripting language,
   can safely assume "git-bar" is found on the PATH while it is
   executing.  If it is C-level and uses exec[vl]_git_cmd() to
   run the subcommand, it would also work fine (it will use the
   right GIT_EXEC_PATH).

This has a few implications:

 * By doing the above setup, we are effectively punting on the
   original issue.  First of all, it is very inconceivable that
   /usr/bin/git is sane, and /usr/bin/diff is not (git wants a
   working diff after all).  If the user is overriding
   /usr/bin/diff by having a better diff under /home/$u/bin/, he
   could have his own copy of git under /home/$u/bin/ as well
   (more likely, the person with correct privilege to installed
   /usr/bin/git would have replaced faulty /usr/bin/diff).  More
   importantly, when bindir and gitexecdir are split, gitexecdir
   will not be /usr/bin (bindir will be).  It will more likely
   to be /usr/lib/git/exec, and it would contain only git-*
   things; prefixing that directory to PATH would not mask the
   /home/$u/bin/diff or anything else non-git the user wanted to
   mask stuff from /usr/bin with, so the original issue becomes
   moot at that point.

 * Typing "git-foo" on the command line would work most of the
   time, if git is built with gitexecdir set to standard bin
   directory (the way our Makefile is shipped).  git-foo will be
   found on your PATH, and it will find "git-bar" on your PATH.
   However, when we split bindir and gitexecdir, typing
   "git-foo" on the command line without having GIT_EXEC_PATH on
   your PATH would stop working, so users have been encouraged
   to train their fingers to use dashless form ever since
   GIT_EXEC_PATH was introduced [*2*].

 * If "git-foo" invoked from the command line (without necessary
   GIT_EXEC_PATH prefixing done by the "git" wrapper) tries to
   run "git", the right git needs to be found on your PATH,
   otherwise things may not necessarily work (your setup
   violates this).  But people have been encouraged to say "git
   foo" to begin with, so this is probably not a big
   deal. Retraining people to say "git foo" instead of "git-foo"
   is a bigger issue compared to this.

 * If "git foo" is typed on the command line, underlying
   "git-foo" is run with GIT_EXEC_PATH prefixed, so it will find
   "git-bar".  If the script uses "git bar", "git" needs to be
   found either in GIT_EXEC_PATH or on your PATH, since our
   Makefile does not install "git" in gitexecdir.  Right now, we
   have bindir == gitexecdir, so "git bar" in "git-foo" script
   will work fine and this is a non-issue, but when we split
   them, it will break your setup, so we need to address this
   before that happens.

So in short, there are two real issues with your original
"git-push misbehaves".  One is that it broke because it was
spelled with dash.  I think "git push" shouldn't misbehave even
with your setup, because the "git" wrapper, before it calls
"git-push", should set up the PATH so that its exec-path is in
front of anything you have in your original PATH (either
/usr/bin or ~/opt/bin), and should find the right "git".

And the second issue is the last point in the "implications"
list above.  You are right, and I stand corrected.  Our scripts
should consistently use dash form.

One thing that bothers me is that we need to keep encouraging
users to use dashless form from the command line, while we
update our scripts to use dash form.  What a contradicting and
confusing situation X-<.


[Footnote]

*1* The "remove prefixing from git.c" patch I sent out earlier
tonight is wrong.  That's the crucial piece to make this whole
setup work.

*2* I think I've been careful enough to use dashless form in the
examples I give in my list postings, but I am not sure how
successful I have been.

Re: [PATCH] Invoke git-repo-config directly.

[Andreas Ericsson]

Junio C Hamano wrote:
> 
> And the second issue is the last point in the "implications"
> list above.  You are right, and I stand corrected.  Our scripts
> should consistently use dash form.
> 
> One thing that bothers me is that we need to keep encouraging
> users to use dashless form from the command line, while we
> update our scripts to use dash form.  What a contradicting and
> confusing situation X-<.
> 

One of the two reasons for rewriting the git wrapper in C was the 
performance penalty that came from having it as a shell-script while it 
was desirable from a porcelainish standpoint to use the dash-less form 
since we thought even then that "git" would always be in PATH while 
"git-foo" was to be moved to the still-imaginary GIT_EXEC_PATH.

The prepending of the GIT_EXEC_PATH to PATH was a laziness workaround 
for scripts that use the dashed form until we'd had time to change 
those, although I see from the commit-message that it's a far cry from 
abundantly clear (reading it now I even think it's clear I meant the 
other way around, although I remember I didn't). Anyways, the relative 
parts are these, from commit 8e49d50388211a0f3e7286f6ee600bf7736f4814

---8<---8<---8<---
     The location of the GIT_EXEC_PATH (name discussion's closed,
     thank gods) can be obtained by running

     	git --exec-path

     which will hopefully give porcelainistas ample time to adapt their
     heavy-duty loops to call the core programs directly and thus save
     the extra fork() / execve() overhead, although that's not really
     necessary any more.

     The --exec-path value is prepended to $PATH, so the git-* programs
     should Just Work without ever requiring any changes to how they call
     other programs in the suite.

     Some timing values for 10000 invocations of git-var >&/dev/null:
     	git.sh: 24.194s
     	git.c:   9.044s
     	git-var: 7.377s

---8<---8<---8<---

 From the timing values there I think the performance issues of using 
the dash-less form can just be ignored. Very rarely will a porcelainish 
wrapper do 10000 invocations of git commands where less than 2 seconds 
will be a large percentage of the overall runtime.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Invoke git-repo-config directly.

[Mark Wooding]

Junio C Hamano <junkio@cox.net> wrote:

> *1* BTW, I just noticed that git-sh-setup needs to be on user's
> PATH, so we probably have to inline and duplicate the git_exec()
> shell function definition at the beginning of each script after
> all, when we make the initial ". git-sh-setup" inclusion to
> honor GIT_EXEC_PATH without munging the user's PATH.

. ${GIT_EXEC_PATH-'@@@GIT_EXEC_PATH@@@'}/git-sh-setup

isn't too grim, and shows how the git_exec shell function can be made
somewhat terser.

By the way, am I the only person who /likes/ having all the git-*
programs on his path?  It makes shell completion work fairly well
without having to install strange completion scripts which get out of
date for one thing.

-- [mdw]

Re: [PATCH] Invoke git-repo-config directly.

[Andreas Ericsson]

Mark Wooding wrote:
> Junio C Hamano <junkio@cox.net> wrote:
> 
> 
>>*1* BTW, I just noticed that git-sh-setup needs to be on user's
>>PATH, so we probably have to inline and duplicate the git_exec()
>>shell function definition at the beginning of each script after
>>all, when we make the initial ". git-sh-setup" inclusion to
>>honor GIT_EXEC_PATH without munging the user's PATH.
> 
> 
> . ${GIT_EXEC_PATH-'@@@GIT_EXEC_PATH@@@'}/git-sh-setup
> 
> isn't too grim, and shows how the git_exec shell function can be made
> somewhat terser.
> 

But it breaks the convenience when testing.

> By the way, am I the only person who /likes/ having all the git-*
> programs on his path?  It makes shell completion work fairly well
> without having to install strange completion scripts which get out of
> date for one thing.
> 

I like it too, but I don't use it unless I can't remember what the 
command was named (finger-training). It shouldn't be too difficult to 
make git.c write its own auto-generated bash-completion rules. If 
someone would care to teach me the syntax I'd gladly hack up a patch for 
it. This is a Good Thing, since it means it would also work for the 
internal commands, which bash's path-completion doesn't.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Invoke git-repo-config directly.

[Timo Hirvonen]

On Thu, 16 Mar 2006 12:53:20 +0000 (UTC)
Mark Wooding <mdw@distorted.org.uk> wrote:

> By the way, am I the only person who /likes/ having all the git-*
> programs on his path?  It makes shell completion work fairly well
> without having to install strange completion scripts which get out of
> date for one thing.

I like git-* for the same reason.  But if git potty had aliases for long
commands then git-* commands would become irrelevant.  Especially
"git co" would be nice.  It even would be faster to type than
git-ch<tab>c<tab>o<tab> ;)

-- 
http://onion.dynserv.net/~timo/

Re: [PATCH] Invoke git-repo-config directly.

[Andreas Ericsson]

Timo Hirvonen wrote:
> On Thu, 16 Mar 2006 12:53:20 +0000 (UTC)
> Mark Wooding <mdw@distorted.org.uk> wrote:
> 
> 
>>By the way, am I the only person who /likes/ having all the git-*
>>programs on his path?  It makes shell completion work fairly well
>>without having to install strange completion scripts which get out of
>>date for one thing.
> 
> 
> I like git-* for the same reason.  But if git potty had aliases for long
> commands then git-* commands would become irrelevant.  Especially
> "git co" would be nice.  It even would be faster to type than
> git-ch<tab>c<tab>o<tab> ;)
> 

It would indeed, and it should also be fairly trivial. However, adding 
short-hands that are identical with cvs and svn but does a totally 
different thing (well, not really different, but cvs users will be 
surprised) is not necessarily a good thing.

It would be better, imo, to add ambiguity detection for commands that 
lacks an exact match. That way "git br" and "git branch" would be 
identical and the logic only needs doing once. I'm not terribly excited 
about it though, so...

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Invoke git-repo-config directly.

[Jon Loeliger]

On Thu, 2006-03-16 at 04:14, Junio C Hamano wrote:

> And the second issue is the last point in the "implications"
> list above.  You are right, and I stand corrected.  Our scripts
> should consistently use dash form.
> 
> One thing that bothers me is that we need to keep encouraging
> users to use dashless form from the command line, while we
> update our scripts to use dash form.  What a contradicting and
> confusing situation X-<.

I think it has a philosophical rationale, though.

While we want to proved an abstraction layer to the
user through the "git" wrapper, we still want to be
precise internally within the scripts themselves.

Or so.

jdl

Re: [PATCH] Invoke git-repo-config directly.

[Jon Loeliger]

On Thu, 2006-03-16 at 13:27, Jon Loeliger wrote:

> While we want to proved an abstraction layer to the
> user through the "git" wrapper, we still want to be
> precise internally within the scripts themselves.

Hey!  Nice English there, Jon!

s/proved/provide

jdl

Re: [PATCH] Invoke git-repo-config directly.

[Qingning Huo]

On Thu, Mar 16, 2006 at 12:26:09AM -0800, Junio C Hamano wrote:
> 
> We could probably define a shell function that looks like:
> 
>         git_exec () {
>                 cmd="$1"
>                 shift
>                 case "${GIT_EXEC_PATH+set}" in
>                 set) ;;
>                 *) GIT_EXEC_PATH='@@GIT_EXEC_PATH@@' ;;
>                 esac
>                 "$GIT_EXEC_PATH/git-$cmd" "$@"
>         }
> 
> in git-sh-setup [*1*], and then rewrite the above to something
> like this instead:
> 
>         diff --git a/git-commit.sh b/git-commit.sh
>         index 330a434..8a73420 100755
>         --- a/git-commit.sh
>         +++ b/git-commit.sh
>         ...
>         @@ -115,7 +115,7 @@ run_status () {
>                     echo '#
>          # Initial commit
>          #'
>         -	    git-ls-files |
>         +	    git_exec ls-files |
>                     sed -e '
>                             s/\\/\\\\/g
>                             s/ /\\ /g
>         @@ -126,7 +126,7 @@ run_status () {
>                     committable="$?"
>                 fi
> 
>         -	git-diff-files  --name-status |
>         +	git_exec diff-files  --name-status |
>                 sed -e '
>                         s/\\/\\\\/g
>                         s/ /\\ /g
>         ...
> 
> But that does not cover Perl nor Python scripts, and does not
> address the ugliness either.

This is similiar to what I had in mind when I recommended the full path
approach.  Perl or Python should be able to do the similiar.  I have no
comment on the ugliness.  The functionality and effeciency of the
program is more important to me.  But I do recognize the difficulties of
changing all scripts overnight.

Anyway, there are at least other two ways to solve my problem.  (a)
setup PATH in git-sh-setup, or (b) consistently use git-command form in
scripts.  Even before their implementation, I can still use "git push".  

Qingning

Re: [PATCH] Invoke git-repo-config directly.

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

>> . ${GIT_EXEC_PATH-'@@@GIT_EXEC_PATH@@@'}/git-sh-setup
>> isn't too grim, and shows how the git_exec shell function can be made
>> somewhat terser.
>
> But it breaks the convenience when testing.

I think that's OK.  When I test things in the build directory
without installing I would usually do:

	$ GIT_EXEC_PATH=`pwd` PATH=`pwd`:/usr/bin:/bin ./git-cmd-to-test

and the above would not break things.

Re: [PATCH] Invoke git-repo-config directly.

[Mark Wooding]

Junio C Hamano <junkio@cox.net> wrote:

> and the above would not break things.

Indeed.  I just translated Junio's `git_exec' shell function into a
one-liner.

-- [mdw]