threads / patch / 22760

patchsubmodules: ensure clean environment when operating in a submodule

Subject: [PATCH] submodules: ensure clean environment when operating in a submodule

## tl;dr

15 messages between Feb 22, 2010 and Feb 23, 2010. Diffs are folded; open one to read it.

replies: 14people: 6as markdown or json

Giuseppe Bilotta· Feb 22, 2010, 22:16 UTC · lore

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.bilotta@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.
Show changes to git-submodule.sh +4 −1
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· Feb 22, 2010, 22:43 UTC · re: Giuseppe Bilotta · lore

Re: [PATCH] submodules: ensure clean environment when operating in a submodule

Giuseppe Bilotta <giuseppe.bilotta@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· Feb 22, 2010, 22:56 UTC · re: Junio C Hamano · lore

Re: [PATCH] submodules: ensure clean environment when operating in a submodule

On Mon, Feb 22, 2010 at 11:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Giuseppe Bilotta <giuseppe.bilotta@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.

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
Junio C Hamano· Feb 22, 2010, 23:16 UTC · re: Giuseppe Bilotta · lore

Re: [PATCH] submodules: ensure clean environment when operating in a submodule

Giuseppe Bilotta <giuseppe.bilotta@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· Feb 22, 2010, 23:31 UTC · re: Junio C Hamano · lore

[PATCH 1/2] shell setup: clear_local_git_env() function

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.bilotta@gmail.com>
---
 git-sh-setup.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
Show changes to git-sh-setup.sh +9 −0
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 Sixt· Feb 23, 2010, 06:49 UTC · re: Giuseppe Bilotta · lore

Re: [PATCH 1/2] shell setup: clear_local_git_env() function

Giuseppe Bilotta schrieb:
Show 6 quoted lines
> +# 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· Feb 23, 2010, 07:55 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/2] shell setup: clear_local_git_env() function

On Tue, Feb 23, 2010 at 7:49 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 10 quoted lines
> 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· Feb 23, 2010, 08:30 UTC · re: Giuseppe Bilotta · lore

[PATCHv2 0/2] clear environment for submodules

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(-)
Giuseppe Bilotta· Feb 23, 2010, 08:30 UTC · re: Giuseppe Bilotta · lore

[PATCHv2 1/2] shell setup: clear_local_git_env() function

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.bilotta@gmail.com>
---
 connect.c       |    2 ++
 git-sh-setup.sh |   15 +++++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)
Show changes to 2 files +17 −0

connect.c, git-sh-setup.sh

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
+
+}
+
 # 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
Jens Lehmann· Feb 23, 2010, 20:25 UTC · re: Giuseppe Bilotta · lore

Re: [PATCHv2 1/2] shell setup: clear_local_git_env() function

Am 23.02.2010 09:30, schrieb Giuseppe Bilotta:
Show 20 quoted lines
> 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?
Show 6 quoted lines
> +
> +}
> +
>  # 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"
Giuseppe Bilotta· Feb 23, 2010, 08:30 UTC · re: Giuseppe Bilotta · lore

[PATCHv2 2/2] submodules: ensure clean environment when operating in a submodule

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.bilotta@gmail.com>
---
 git-submodule.sh |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)
Show changes to git-submodule.sh +10 −10
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· Feb 22, 2010, 23:31 UTC · re: Junio C Hamano · lore

[PATCH 2/2] submodules: ensure clean environment when operating in a submodule

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.bilotta@gmail.com>
---
 git-submodule.sh |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)
Show changes to git-submodule.sh +10 −10
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
Johannes Schindelin· Feb 23, 2010, 00:04 UTC · re: Giuseppe Bilotta · lore

Re: [msysGit] [PATCH] submodules: ensure clean environment when operating in a submodule

Hi,
On Mon, 22 Feb 2010, Giuseppe Bilotta wrote:
Show 13 quoted lines
> 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.bilotta@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

Heiko Voigt· Feb 23, 2010, 21:07 UTC · re: Johannes Schindelin · lore

Re: Re: [msysGit] [PATCH] submodules: ensure clean environment when operating in a submodule

On Tue, Feb 23, 2010 at 01:04:00AM +0100, Johannes Schindelin wrote:
Show 20 quoted lines
> 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.bilotta@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?

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· Feb 23, 2010, 21:47 UTC · re: Heiko Voigt · lore

Re: Re: [msysGit] [PATCH] submodules: ensure clean environment when operating in a submodule

Hi,
On Tue, 23 Feb 2010, Heiko Voigt wrote:
Show 20 quoted lines
> 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

← back to recent threads