Subject: Re: [PATCH 08/19] completion: use $__git_dir instead of $(__gitdir)

From: SZEDER Gábor <[email-masked]>
Date: Wed, 9 May 2012 23:36:36 +0200

On Wed, May 09, 2012 at 01:56:02PM -0700, Junio C Hamano wrote:
> SZEDER Gábor <[email-masked]> writes:
> 
> >> > @@ -962,7 +967,8 @@ __git_aliases ()
> >> >  # __git_aliased_command requires 1 argument
> >> >  __git_aliased_command ()
> >> >  {
> >> > -	local word cmdline=$(git --git-dir="$(__gitdir)" \
> >> > +	__gitdir >/dev/null
> >> > +	local word cmdline=$(git --git-dir="$__git_dir" \
> >> >  		config --get "alias.$1")
> >> >  	for word in $cmdline; do
> >> >  		case "$word" in
> >> 
> >> Now this worries me.  The way I read 07/19 was that the local __git_dir=""
> >> declarations in __git_ps1 and __git were what protected this whole
> >> machinery to protect us against surprises from user doing "cd" between
> >> interactive commands, but you have the same __gitdir call to set up the
> >> global $__git_dir variable there, without the initialization to "".
> >> 
> >> Having to have a call to __gitdir seems to indicate to me that you cannot
> >> assume that the other initialization sites may not have been called before
> >> we get to this point.  Then why is 'local __git_dir=""' unneeded here?
> >
> > Your comments to the previous patch apply here.
> 
> Not really.  __git_ps1 and __gitk seems to do __gitdir very early to make
> sure anybody that use $__git_dir can rely on it

No, __git_ps1() and __gitk() do __gitdir() early, because they need
the path to the repository very early.

> but having to sprinkle
> "set up $__git_dir variable" everywhere means anybody who wants to update
> need to know if it is already called, which defeats the point of "we can
> use $__git_dir instead of calling $(__gitdir)" from maintainability's
> point of view.

Well, the point is better explained in the body of the commit message:

  just call __gitdir() directly and then use $__git_dir instead of
  doing 'dir="$(__gitdir)"' command substitution

Unfortunately, I just couldn't manage to squeeze all this into a
one-line short description.  So it really is about performance, and
not maintainability.