"set GIT_DIR and ..." brakes git gui tools invokation on windows

[Heiko Voigt]

Hallo Giuseppe and Shawn,

i tracked down a bug my git gui users experienced today to a9fa11:
"git-gui: set GIT_DIR and GIT_WORK_TREE after setup"

It appears on msysgit (windows) and on osx so I guess it will be
reproduceable on other platforms as well.

I am still not sure why but if you add and invoke

git submodule update

from the tools menu it errors out. I first thought it would be
setup of GIT_DIR and GIT_WORK_TREE which is now globally known but on
the commandline "submodule update" works with these variables set.

Before I dive more into debugging maybe you have an idea what is going
wrong here?

cheers Heiko

P.S.: This revert fixes the issue for me:

-- &< ---
From c10682f7022190f0dca7075bab157e09da9d30a7 Mon Sep 17 00:00:00 2001
From: Heiko Voigt <heiko...@mahr.de>
Date: Wed, 17 Feb 2010 16:20:06 +0100
Subject: [PATCH] Revert "git-gui: set GIT_DIR and GIT_WORK_TREE after setup"

This reverts commit a9fa11fe5bd5978bb175b3b5663f6477a345d428.
---
git-gui/git-gui.sh | 43 +++++++++++++++++++++++--------------------
1 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 7d54511..05193a0 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -1192,9 +1192,6 @@ if {[lindex $_reponame end] eq {.git}} {
set _reponame [lindex $_reponame end]
}

-set env(GIT_DIR) $_gitdir
-set env(GIT_WORK_TREE) $_gitworktree
-
######################################################################
##
## global init
@@ -1994,7 +1991,7 @@ set starting_gitk_msg [mc "Starting gitk... please wait..."]

proc do_gitk {revs {is_submodule false}} {
global current_diff_path file_states current_diff_side ui_index
- global _gitdir _gitworktree
+ global _gitworktree

# -- Always start gitk through whatever we were loaded with. This
# lets us bypass using shell process on Windows systems.
@@ -2006,12 +2003,19 @@ proc do_gitk {revs {is_submodule false}} {
} else {
global env

+ if {[info exists env(GIT_DIR)]} {
+ set old_GIT_DIR $env(GIT_DIR)
+ } else {
+ set old_GIT_DIR {}
+ }
+
set pwd [pwd]

if {!$is_submodule} {
if {![is_bare]} {
cd $_gitworktree
}
+ set env(GIT_DIR) [file normalize [gitdir]]
} else {
cd $current_diff_path
if {$revs eq {--}} {
@@ -2032,18 +2036,15 @@ proc do_gitk {revs {is_submodule false}} {
}
set revs $old_sha1...$new_sha1
}
- # GIT_DIR and GIT_WORK_TREE for the submodule are not the ones
- # we've been using for the main repository, so unset them.
- # TODO we could make life easier (start up faster?) for gitk
- # by setting these to the appropriate values to allow gitk
- # to skip the heuristics to find their proper value
- unset env(GIT_DIR)
- unset env(GIT_WORK_TREE)
+ if {[info exists env(GIT_DIR)]} {
+ unset env(GIT_DIR)
+ }
}
eval exec $cmd $revs "--" "--" &

- set env(GIT_DIR) $_gitdir
- set env(GIT_WORK_TREE) $_gitworktree
+ if {$old_GIT_DIR ne {}} {
+ set env(GIT_DIR) $old_GIT_DIR
+ }
cd $pwd

ui_status $::starting_gitk_msg
@@ -2064,20 +2065,22 @@ proc do_git_gui {} {
error_popup [mc "Couldn't find git gui in PATH"]
} else {
global env
- global _gitdir _gitworktree

- # see note in do_gitk about unsetting these vars when
- # running tools in a submodule
- unset env(GIT_DIR)
- unset env(GIT_WORK_TREE)
+ if {[info exists env(GIT_DIR)]} {
+ set old_GIT_DIR $env(GIT_DIR)
+ unset env(GIT_DIR)
+ } else {
+ set old_GIT_DIR {}
+ }

set pwd [pwd]
cd $current_diff_path

eval exec $exe gui &

- set env(GIT_DIR) $_gitdir
- set env(GIT_WORK_TREE) $_gitworktree
+ if {$old_GIT_DIR ne {}} {
+ set env(GIT_DIR) $old_GIT_DIR
+ }
cd $pwd

ui_status $::starting_gitk_msg
--
1.7.0.rc1.7.gc0da5

[Johannes Schindelin]

Hi,

On Wed, 17 Feb 2010, Heiko Voigt wrote:

> Hallo Giuseppe and Shawn,
>
> i tracked down a bug my git gui users experienced today to a9fa11:
> "git-gui: set GIT_DIR and GIT_WORK_TREE after setup"
>

> [...]

>
> P.S.: This revert fixes the issue for me:

> [...]

Do you think this would be a necessary temporary workaround for Git for
Windows 1.7.0?

Ciao,
Dscho

[Heiko Voigt]

culled the CC list a little to save Guiseppe and Shawn some reading
time.

Yes, I already have it in my private tree. My users interact with
submodules using the tools menu. That would probably save us some bug
reports. I am currently test driving your devel branch with some of
them.

BTW, as I looked through my fixes I remembered this one: How about
merging the unlink workaround in case files are still openend from my
hv/automatic_retry branch on 4msysgit?

cheers Heiko

[Johannes Schindelin]

Hi,

On Wed, 17 Feb 2010, Heiko Voigt wrote:

> On Wed, Feb 17, 2010 at 09:52:01PM +0100, Johannes Schindelin wrote:
> > On Wed, 17 Feb 2010, Heiko Voigt wrote:
> >
> > > i tracked down a bug my git gui users experienced today to a9fa11:
> > > "git-gui: set GIT_DIR and GIT_WORK_TREE after setup"
> > >
> > > [...]
> > >
> > > P.S.: This revert fixes the issue for me:
> > > [...]
> >
> > Do you think this would be a necessary temporary workaround for Git
> > for Windows 1.7.0?
>
> Yes, I already have it in my private tree. My users interact with
> submodules using the tools menu. That would probably save us some bug
> reports. I am currently test driving your devel branch with some of
> them.

Okay, can you please push to devel?

> BTW, as I looked through my fixes I remembered this one: How about
> merging the unlink workaround in case files are still openend from my
> hv/automatic_retry branch on 4msysgit?

Good point. IIRC Hannes was not opposed to it, and I think it makes sense
to have it in Git for Windows. Please push that to devel, too.

Thank you!
Dscho

[Pat Thoyts]

On 18 February 2010 09:08, Johannes Schindelin

<Johannes....@gmx.de> wrote:
> Hi,
>
> On Wed, 17 Feb 2010, Heiko Voigt wrote:
>
>> On Wed, Feb 17, 2010 at 09:52:01PM +0100, Johannes Schindelin wrote:
>> > On Wed, 17 Feb 2010, Heiko Voigt wrote:
>> >
>> > > i tracked down a bug my git gui users experienced today to a9fa11:
>> > > "git-gui: set GIT_DIR and GIT_WORK_TREE after setup"
>> > >
>> > > [...]
>> > >
>> > > P.S.: This revert fixes the issue for me:
>> > > [...]
>> >
>> > Do you think this would be a necessary temporary workaround for Git
>> > for Windows 1.7.0?
>>
>> Yes, I already have it in my private tree. My users interact with
>> submodules using the tools menu. That would probably save us some bug
>> reports. I am currently test driving your devel branch with some of
>> them.
>
> Okay, can you please push to devel?
>

I'm not entirely sure if it is the same issue but someone posted a bug
to the git mailing list last night and I posted a patch and it touched
the setup of the _gitworkdir variable.

You didn't say what the problem was that this fixes.

Mailing list reference:
http://article.gmane.org/gmane.comp.version-control.git/140288

[Giuseppe Bilotta]

Hello Heiko,

thanks for the bug report.

On Wed, Feb 17, 2010 at 6:43 PM, Heiko Voigt <hvo...@hvoigt.net> wrote:
> Hallo Giuseppe and Shawn,
>
> i tracked down a bug my git gui users experienced today to a9fa11:
> "git-gui: set GIT_DIR and GIT_WORK_TREE after setup"
>
> It appears on msysgit (windows) and on osx so I guess it will be
> reproduceable on other platforms as well.
>
> I am still not sure why but if you add and invoke
>
>  git submodule update
>
> from the tools menu it errors out. I first thought it would be
> setup of GIT_DIR and GIT_WORK_TREE which is now globally known but on
> the commandline "submodule update" works with these variables set.

Could you be a little more specific about the error you are getting?
Also, are you running the git submodule update tool in a submodule
dir, or in the main repo? This can make a significant difference
because when running from within a submodule we wipe the GIT_DIR and
GIT_WORK_TREE definitions.

> Before I dive more into debugging maybe you have an idea what is going
> wrong here?

I don't have a Windows setup so I cannot test msysgit , but I'm
wondering if it might be some problem with Unix vs Windows path
separators.

--
Giuseppe "Oblomov" Bilotta

[Heiko Voigt]

On Thu, Feb 18, 2010 at 10:08:44AM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Wed, 17 Feb 2010, Heiko Voigt wrote:
>
> > On Wed, Feb 17, 2010 at 09:52:01PM +0100, Johannes Schindelin wrote:
> > > On Wed, 17 Feb 2010, Heiko Voigt wrote:
> > >
> > > > i tracked down a bug my git gui users experienced today to a9fa11:
> > > > "git-gui: set GIT_DIR and GIT_WORK_TREE after setup"
> > > >
> > > > [...]
> > > >
> > > > P.S.: This revert fixes the issue for me:
> > > > [...]
> > >
> > > Do you think this would be a necessary temporary workaround for Git
> > > for Windows 1.7.0?
> >
> > Yes, I already have it in my private tree. My users interact with
> > submodules using the tools menu. That would probably save us some bug
> > reports. I am currently test driving your devel branch with some of
> > them.
>
> Okay, can you please push to devel?

Ok, its there.

>
> > BTW, as I looked through my fixes I remembered this one: How about
> > merging the unlink workaround in case files are still openend from my
> > hv/automatic_retry branch on 4msysgit?
>
> Good point. IIRC Hannes was not opposed to it, and I think it makes sense
> to have it in Git for Windows. Please push that to devel, too.

This also.

[Johannes Schindelin]

Hi,

Thanks!

Ciao,
Dscho

[Heiko Voigt]

It was about git submodule commands launched from the tools menu in git
gui. From the commit message and the change it seems that it could be
releated. I will have a look into it and publish the results. In case it
does work with this fix I would revert the revert and add the patch from
the mailing list on top.

cheers Heiko

[Heiko Voigt]

Unfortunately, this does not fix it. I can reproduce the error on Mac OS
X and on Windows so its probably not Windows related:

To reproduce take this sequence of commands:

git clone git://repo.or.cz/msysgit.git
cd msysgit
git submodule update --init
git gui
# the following instructions are for clicking
* Tools->Add...
Enter Name: submodule update
Enter Command: git submodule update
* Add
* Tools->submodule update

Error message is:
"fatal: reference is not a tree: 543f8d61eb235820aecefde6e46ec90e9803a6d8
"Unable to checkout '543f8d61eb235820aecefde6e46ec90e9803a6d8' in submodule path 'doc/git/html'"

cheers Heiko

[Heiko Voigt]

On Thu, Feb 18, 2010 at 01:39:50PM +0100, Giuseppe Bilotta wrote:
> Hello Heiko,
>
> thanks for the bug report.
>
> On Wed, Feb 17, 2010 at 6:43 PM, Heiko Voigt <hvo...@hvoigt.net> wrote:
> > Hallo Giuseppe and Shawn,
> >
> > i tracked down a bug my git gui users experienced today to a9fa11:
> > "git-gui: set GIT_DIR and GIT_WORK_TREE after setup"
> >
> > It appears on msysgit (windows) and on osx so I guess it will be
> > reproduceable on other platforms as well.
> >
> > I am still not sure why but if you add and invoke
> >
> > �git submodule update
> >
> > from the tools menu it errors out. I first thought it would be
> > setup of GIT_DIR and GIT_WORK_TREE which is now globally known but on
> > the commandline "submodule update" works with these variables set.
>
> Could you be a little more specific about the error you are getting?
> Also, are you running the git submodule update tool in a submodule
> dir, or in the main repo? This can make a significant difference
> because when running from within a submodule we wipe the GIT_DIR and
> GIT_WORK_TREE definitions.

Here is a complete recipe how to reproduce the bug:

http://article.gmane.org/gmane.comp.version-control.msysgit/8755

> > Before I dive more into debugging maybe you have an idea what is going
> > wrong here?
>
> I don't have a Windows setup so I cannot test msysgit , but I'm
> wondering if it might be some problem with Unix vs Windows path
> separators.

Its very likely not Windows path stuff. As you can read above and in my
recipe I can also reproduce this on Mac OS X.

Wait I'll even go further and try it on my Linux virtual machine ...
and yes I can reproduce it there as well (git version 1.7.0.31.g1df487)

cheers Heiko

[Heiko Voigt]

On Thu, Feb 18, 2010 at 10:27:08AM +0000, Pat Thoyts wrote:

As this fixes an unrelated issue which is also there after the revert of
a9fa11 (just tested it) I would suggest to also pick this fix onto devel.

cheers Heiko

[Johannes Schindelin]

Hi,

Sure, if it fixes things, go ahead!

Ciao,
Dscho

[Heiko Voigt]

Hi,

Done. I also pushed another trivial fix which fixes an error when
cloning using git-gui which I just send to the list and you in copy[1].

cheers Heiko

[1] http://groups.google.com/group/msysgit/msg/23485726fdfbd858

[Johannes Schindelin]

Hi,

Thanks, thanks, and thanks!

Ciao,
Dscho

[Giuseppe Bilotta]

On Thu, Feb 18, 2010 at 9:37 PM, Heiko Voigt <hvo...@hvoigt.net> wrote:
>
> Here is a complete recipe how to reproduce the bug:
>
> http://article.gmane.org/gmane.comp.version-control.msysgit/8755
>

Sorry for not getting back at you earlier. With that recipe I was able
to reproduce the bug locally, I'll let you know as soon as I have a
fix.

--
Giuseppe "Oblomov" Bilotta

[Giuseppe Bilotta]

I've done some testing and the problem is actually in git-submodule,
although it is exposed by my patch.

oblomov@oblomov:~/src/msysgit$ GIT_WORK_TREE=/home/oblomov/src/msysgit
git submodule update

fatal: reference is not a tree: 543f8d61eb235820aecefde6e46ec90e9803a6d8
Unable to checkout '543f8d61eb235820aecefde6e46ec90e9803a6d8' in
submodule path 'doc/git/html'

I've prepared a patch which I'm proposing for git maint.

--
Giuseppe "Oblomov" Bilotta

[Junio C Hamano]

Giuseppe Bilotta <giuseppe...@gmail.com> writes:

> I'm pretty confident fixing this on the submodules side is the more correct
> approach, since otherwise even a simple
> $ GIT_WORK_TREE=. git submodule update
> on the command-line can fail.

True; while I didn't bother to check what the codepaths after these
unsetting do, I suspect you should also think about what effect it has to
have other GIT_* environment variables seep through to them (GIT_INDEX_FILE,
GIT_CONFIG and GIT_OBJECT_DIRECTORY come to mind). You would probably
want to have a single shell helper function to unset even if you end up
deciding that it is sufficient to clear GIT_DIR and GIT_WORK_TREE and
nothing else.

[Giuseppe Bilotta]

git-submodule takes care of clearing GIT_DIR whenever it operates
on a submodule index or configuration, but forgot to unset GIT_WORK_TREE
before operating on the submodule worktree, which would lead to failures
when GIT_WORK_TREE was set.

This only happened in very unusual contexts such as operating on the
main worktree from outside of it, but since "git-gui: set GIT_DIR and
GIT_WORK_TREE after setup" (a9fa11fe5bd5978bb) such failures could also
be provoked by invoking an external tool such as "git submodule update"
from the Git GUI in a standard setup.

Signed-off-by: Giuseppe Bilotta <giuseppe...@gmail.com>
---
git-submodule.sh | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)

The bug (git submodules not working in Git GUI since my patch) was spotted by
Heiko Voigt working on/with msysgit, and he kindly provided a recipe to
replicate it:
http://article.gmane.org/gmane.comp.version-control.msysgit/8755

I'm pretty confident fixing this on the submodules side is the more correct
approach, since otherwise even a simple
$ GIT_WORK_TREE=. git submodule update
on the command-line can fail.

I also believe this is material for git maint.

diff --git a/git-submodule.sh b/git-submodule.sh
index 5869c00..69afc84 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -223,6 +223,7 @@ cmd_add()
module_clone "$path" "$realrepo" "$reference" || exit
(
unset GIT_DIR
+ unset GIT_WORK_TREE
cd "$path" &&
# ash fails to wordsplit ${branch:+-b "$branch"...}
case "$branch" in
@@ -279,6 +280,7 @@ cmd_foreach()
(
prefix="$prefix$path/"
unset GIT_DIR
+ unset GIT_WORK_TREE
cd "$path" &&
eval "$@" &&
if test -n "$recursive"
@@ -477,7 +479,7 @@ cmd_update()
;;
esac

- (unset GIT_DIR; cd "$path" && $command "$sha1") ||
+ (unset GIT_DIR; unset GIT_WORK_TREE; cd "$path" && $command "$sha1") ||
die "Unable to $action '$sha1' in submodule path '$path'"
say "Submodule path '$path': $msg '$sha1'"
fi
@@ -771,6 +773,7 @@ cmd_status()
(
prefix="$displaypath/"
unset GIT_DIR
+ unset GIT_WORK_TREE
cd "$path" &&
cmd_status $orig_args
) ||
--
1.7.0.199.g49ef3.dirty

[Junio C Hamano]

Giuseppe Bilotta <giuseppe...@gmail.com> writes:

> Good point. All GIT_* env variables should be resent when descending
> into a submodule. Is there a way to loop over them, or do I have to do
> something horrible like env | grep ^GIT_ | cut -f1 -d= to get the
> list?

I would suggest to enumerate only what you care about explicitly, without
consulting the user's environment. The user may have variables you do not
know about and not related to git at all.

IOW, don't try to be too clever for your own good ;-)

[Giuseppe Bilotta]

Good point. All GIT_* env variables should be resent when descending

into a submodule. Is there a way to loop over them, or do I have to do
something horrible like env | grep ^GIT_ | cut -f1 -d= to get the
list?

--
Giuseppe "Oblomov" Bilotta

[Giuseppe Bilotta]

git-submodule used to take care of clearing GIT_DIR whenever it operated

on a submodule index or configuration, but forgot to unset GIT_WORK_TREE

or other repo-local variables. This which would lead to failures e.g.
when GIT_WORK_TREE was set.

This only happened in very unusual contexts such as operating on the
main worktree from outside of it, but since "git-gui: set GIT_DIR and
GIT_WORK_TREE after setup" (a9fa11fe5bd5978bb) such failures could also
be provoked by invoking an external tool such as "git submodule update"

from the Git Gui in a standard setup.

Solve by using the newly introduced clear_local_git_env() shell function
to ensure that all repo-local environment variables are unset.

Signed-off-by: Giuseppe Bilotta <giuseppe...@gmail.com>
---

git-submodule.sh | 20 ++++++++++----------
1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 5869c00..1ea4143 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -222,7 +222,7 @@ cmd_add()

module_clone "$path" "$realrepo" "$reference" || exit
(

- unset GIT_DIR
+ clear_local_git_env

cd "$path" &&
# ash fails to wordsplit ${branch:+-b "$branch"...}
case "$branch" in

@@ -278,7 +278,7 @@ cmd_foreach()
name=$(module_name "$path")
(
prefix="$prefix$path/"
- unset GIT_DIR
+ clear_local_git_env

cd "$path" &&
eval "$@" &&
if test -n "$recursive"

@@ -434,7 +434,7 @@ cmd_update()
module_clone "$path" "$url" "$reference"|| exit
subsha1=
else
- subsha1=$(unset GIT_DIR; cd "$path" &&
+ subsha1=$(clear_local_git_env; cd "$path" &&
git rev-parse --verify HEAD) ||
die "Unable to find current revision in submodule path '$path'"
fi
@@ -454,7 +454,7 @@ cmd_update()

if test -z "$nofetch"
then

- (unset GIT_DIR; cd "$path" &&

+ (clear_local_git_env; cd "$path" &&
git-fetch) ||
die "Unable to fetch in submodule path '$path'"
fi
@@ -477,14 +477,14 @@ cmd_update()

;;
esac

- (unset GIT_DIR; cd "$path" && $command "$sha1") ||

+ (clear_local_git_env; cd "$path" && $command "$sha1") ||

die "Unable to $action '$sha1' in submodule path '$path'"
say "Submodule path '$path': $msg '$sha1'"
fi

if test -n "$recursive"
then
- (unset GIT_DIR; cd "$path" && cmd_update $orig_args) ||
+ (clear_local_git_env; cd "$path" && cmd_update $orig_args) ||
die "Failed to recurse into submodule path '$path'"
fi
done
@@ -492,7 +492,7 @@ cmd_update()

set_name_rev () {
revname=$( (
- unset GIT_DIR
+ clear_local_git_env
cd "$1" && {
git describe "$2" 2>/dev/null ||
git describe --tags "$2" 2>/dev/null ||
@@ -760,7 +760,7 @@ cmd_status()
else
if test -z "$cached"
then
- sha1=$(unset GIT_DIR; cd "$path" && git rev-parse --verify HEAD)
+ sha1=$(clear_local_git_env; cd "$path" && git rev-parse --verify HEAD)
set_name_rev "$path" "$sha1"
fi
say "+$sha1 $displaypath$revname"
@@ -770,7 +770,7 @@ cmd_status()
then
(
prefix="$displaypath/"
- unset GIT_DIR
+ clear_local_git_env

cd "$path" &&
cmd_status $orig_args
) ||

@@ -821,7 +821,7 @@ cmd_sync()
if test -e "$path"/.git
then
(
- unset GIT_DIR
+ clear_local_git_env
cd "$path"
remote=$(get_default_remote)
say "Synchronizing submodule url for '$name'"
--
1.7.0.200.g5ba36.dirty

[Giuseppe Bilotta]

Introduce an auxiliary function to clear all repo-local environment
variables. This should be invoked by any shell script that switches
repository during execution, to ensure that the environment is clean
and that things such as the git dir and worktree are set up correctly.

Signed-off-by: Giuseppe Bilotta <giuseppe...@gmail.com>
---

git-sh-setup.sh | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 7a09566..d382879 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -172,6 +172,15 @@ get_author_ident_from_commit () {
LANG=C LC_ALL=C sed -ne "$pick_author_script"
}

+# Clear repo-local GIT_* environment variables. Useful when switching to
+# another repository (e.g. when entering a submodule)
+clear_local_git_env() {
+ unset GIT_DIR GIT_WORKTREE GIT_OBJECT_DIRECTORY \
+ GIT_INDEX_FILE GIT_GRAFT_FILE GIT_CONFIG \
+ GIT_NO_REPLACE_OBJECTS
+
+}
+
# Make sure we are in a valid repository of a vintage we understand,
# if we require to be in a git repository.
if test -z "$NONGIT_OK"
--
1.7.0.200.g5ba36.dirty

[Johannes Schindelin]

Hi,

On Mon, 22 Feb 2010, Giuseppe Bilotta wrote:

> git-submodule takes care of clearing GIT_DIR whenever it operates
> on a submodule index or configuration, but forgot to unset GIT_WORK_TREE
> before operating on the submodule worktree, which would lead to failures
> when GIT_WORK_TREE was set.
>
> This only happened in very unusual contexts such as operating on the
> main worktree from outside of it, but since "git-gui: set GIT_DIR and
> GIT_WORK_TREE after setup" (a9fa11fe5bd5978bb) such failures could also
> be provoked by invoking an external tool such as "git submodule update"
> from the Git GUI in a standard setup.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe...@gmail.com>
> ---

Heiko, would you say that this patch is an elegant solution to the problem
you reported? If so, would you please pull it to 4msysgit.git's devel?

Thanks,
Dscho

[Johannes Sixt]

Giuseppe Bilotta schrieb:

> +# Clear repo-local GIT_* environment variables. Useful when switching to
> +# another repository (e.g. when entering a submodule)
> +clear_local_git_env() {
> + unset GIT_DIR GIT_WORKTREE GIT_OBJECT_DIRECTORY \
> + GIT_INDEX_FILE GIT_GRAFT_FILE GIT_CONFIG \
> + GIT_NO_REPLACE_OBJECTS

IMO, this list should be in sync with the one you find in
connect.c:git_connect() around line 611. They have the same purpose.

(And, BTW, a vertical list would be more readable than a mixed
horizontal+vertical list, IMVHO.)

-- Hannes

[Giuseppe Bilotta]

On Tue, Feb 23, 2010 at 7:49 AM, Johannes Sixt <j.s...@viscovery.net> wrote:
> Giuseppe Bilotta schrieb:
>> +# Clear repo-local GIT_* environment variables. Useful when switching to
>> +# another repository (e.g. when entering a submodule)
>> +clear_local_git_env() {
>> +     unset   GIT_DIR GIT_WORKTREE GIT_OBJECT_DIRECTORY \
>> +             GIT_INDEX_FILE GIT_GRAFT_FILE GIT_CONFIG \
>> +             GIT_NO_REPLACE_OBJECTS
>
> IMO, this list should be in sync with the one you find in
> connect.c:git_connect() around line 611. They have the same purpose.

Ah, interesting, I was looking for such a list but only found the more
generic one in cache.h
By comparing them it would seem they serve the same purpose, indeed. I
notice that the connect.c is missing GIT_CONFIG (which _must_ be unset
for us). I also notice that the connect.c one unsets the alternate DB;
I had doubts about it when preparing the list in this case.

I will resend a new patch to replace this one, syncing the two lists.

> (And, BTW, a vertical list would be more readable than a mixed
> horizontal+vertical list, IMVHO.)

I tend to conserve vertical space, but I have no particular objection to that.

--
Giuseppe "Oblomov" Bilotta

[Giuseppe Bilotta]

git-submodule used to take care of clearing GIT_DIR whenever it operated

on a submodule index or configuration, but forgot to unset GIT_WORK_TREE

or other repo-local variables. This which would lead to failures e.g.
when GIT_WORK_TREE was set.

This only happened in very unusual contexts such as operating on the
main worktree from outside of it, but since "git-gui: set GIT_DIR and
GIT_WORK_TREE after setup" (a9fa11fe5bd5978bb) such failures could also
be provoked by invoking an external tool such as "git submodule update"

from the Git Gui in a standard setup.

Solve by using the newly introduced clear_local_git_env() shell function
to ensure that all repo-local environment variables are unset.

Signed-off-by: Giuseppe Bilotta <giuseppe...@gmail.com>
---

[Giuseppe Bilotta]

Introduce an auxiliary function to clear all repo-local environment
variables. This should be invoked by any shell script that switches
repository during execution, to ensure that the environment is clean
and that things such as the git dir and worktree are set up correctly.

The list matches the one in git_connect(), so bring them in sync by adding
the missing CONFIG_ENVIRONMENT. Also add a note about the fact that they
should be kept that way.

Signed-off-by: Giuseppe Bilotta <giuseppe...@gmail.com>
---

connect.c | 2 ++
git-sh-setup.sh | 15 +++++++++++++++
2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/connect.c b/connect.c
index 9f39038..12dd0b5 100644
--- a/connect.c
+++ b/connect.c
@@ -583,8 +583,10 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
}
else {
/* remove these from the environment */
+ /* see also clear_local_git_env() in git-sh-setup.sh */
const char *env[] = {
ALTERNATE_DB_ENVIRONMENT,
+ CONFIG_ENVIRONMENT,
DB_ENVIRONMENT,
GIT_DIR_ENVIRONMENT,
GIT_WORK_TREE_ENVIRONMENT,
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 7a09566..f1be832 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -172,6 +172,21 @@ get_author_ident_from_commit () {

LANG=C LC_ALL=C sed -ne "$pick_author_script"
}

+# Clear repo-local GIT_* environment variables. Useful when switching to

+# another repository (e.g. when entering a submodule). See also the env
+# list in git_connect()
+clear_local_git_env() {
+ unset GIT_ALTERNATE_OBJECT_DIRECTORIES \
+ GIT_CONFIG \
+ GIT_DIR \
+ GIT_GRAFT_FILE \
+ GIT_INDEX_FILE \
+ GIT_NO_REPLACE_OBJECTS \
+ GIT_OBJECT_DIRECTORY \
+ GIT_WORKTREE

[Giuseppe Bilotta]

Second (or maybe actually third) iteration of the series to ensure that
submodules work correctly when repo-local environment is set. The only
thing changed from the previous iteration is an extra variable in
clear_local_git_env() to bring it up in sync with the list in git_connect(),
and also the addition of CONFIG_ENVIRONMENT in git_connect() itself.

I had considered making a static list of these variables somewhere, but
making it accessible to both the built-ins and shell scripts (something à
la git rev-parse --local-git-env maybe?) is a little over my head at the
moment (not much free time to dedicate to git, sadly). Also I'm not sure
the extra git call needed to get the list would be worth it.

Giuseppe Bilotta (2):
shell setup: clear_local_git_env() function
submodules: ensure clean environment when operating in a submodule

connect.c | 2 ++
git-sh-setup.sh | 15 +++++++++++++++

git-submodule.sh | 20 ++++++++++----------
3 files changed, 27 insertions(+), 10 deletions(-)

[Jens Lehmann]

Am 23.02.2010 09:30, schrieb Giuseppe Bilotta:
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 7a09566..f1be832 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -172,6 +172,21 @@ get_author_ident_from_commit () {
> LANG=C LC_ALL=C sed -ne "$pick_author_script"
> }
>
> +# Clear repo-local GIT_* environment variables. Useful when switching to
> +# another repository (e.g. when entering a submodule). See also the env
> +# list in git_connect()
> +clear_local_git_env() {
> + unset GIT_ALTERNATE_OBJECT_DIRECTORIES \
> + GIT_CONFIG \
> + GIT_DIR \
> + GIT_GRAFT_FILE \
> + GIT_INDEX_FILE \
> + GIT_NO_REPLACE_OBJECTS \
> + GIT_OBJECT_DIRECTORY \
> + GIT_WORKTREE

Shouldn't that last one be GIT_WORK_TREE?

[Heiko Voigt]

I would like to leave the patch cooking a little more. For example see
the objection of Jens[1]. Just to be sure that we do not pull in other
issues with this. Sorry I do not have the time for extensive testing
currently. So my suggestion would be to leave msysgit in the current
state as I did not discover any downsides with just the revert with my
current users tests.

As far as I understand the patch fixes issues when submodules itself
contain submodules and it seems that not many users are doing this
currently. Once everybody is happy (and tried) the patches I do not have
any objections to pushing it into devel.

What do you think?

cheers Heiko

[1] http://groups.google.com/group/msysgit/msg/481e252ba4f49ede

[Johannes Schindelin]

Hi,

On Tue, 23 Feb 2010, Heiko Voigt wrote:

> On Tue, Feb 23, 2010 at 01:04:00AM +0100, Johannes Schindelin wrote:
>
> > On Mon, 22 Feb 2010, Giuseppe Bilotta wrote:
> >
> > > git-submodule takes care of clearing GIT_DIR whenever it operates on
> > > a submodule index or configuration, but forgot to unset
> > > GIT_WORK_TREE before operating on the submodule worktree, which
> > > would lead to failures when GIT_WORK_TREE was set.
> > >
> > > This only happened in very unusual contexts such as operating on the
> > > main worktree from outside of it, but since "git-gui: set GIT_DIR
> > > and GIT_WORK_TREE after setup" (a9fa11fe5bd5978bb) such failures
> > > could also be provoked by invoking an external tool such as "git
> > > submodule update" from the Git GUI in a standard setup.
> >

> > Heiko, would you say that this patch is an elegant solution to the
> > problem you reported? If so, would you please pull it to
> > 4msysgit.git's devel?
>
> I would like to leave the patch cooking a little more.

Yes, that matches my feeling.

Thanks,
Dscho

[Giuseppe Bilotta]

Introduce an auxiliary function to clear all repo-local environment
variables. This should be invoked by any shell script that switches
repository during execution, to ensure that the environment is clean
and that things such as the git dir and worktree are set up correctly.

Signed-off-by: Giuseppe Bilotta <giuseppe...@gmail.com>
---
git-sh-setup.sh | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 7a09566..6131670 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -172,6 +172,13 @@ get_author_ident_from_commit () {

LANG=C LC_ALL=C sed -ne "$pick_author_script"
}

+# Clear repo-local GIT_* environment variables. Useful when switching to
+# another repository (e.g. when entering a submodule). See also the env
+# list in git_connect()
+clear_local_git_env() {

+ unset $(git rev-parse --local-env-vars)

+}
+
# Make sure we are in a valid repository of a vintage we understand,
# if we require to be in a git repository.
if test -z "$NONGIT_OK"

--
1.7.0.200.g5ba36.dirty