[PATCHv3 0/5] repo-local env vars handling

[PATCHv3 0/5] repo-local env vars handling

[Giuseppe Bilotta]

Ok, the Git GUI + submodule fix is shaping up in a pretty much more
general thing. In this third iteration, the patch has been augmented by
two important patches:

* the first patch defines a static list (const char *const []) holding
  all the repo-local environment variables. The list is not used yet,
  just defined
* the third patch adds a rev-parse (I couldn't think of a better place)
  option to list these variables (just the names, not the values)

The connect and sh-setup patches have obviously been adapted to use
the new provided features. The last patch didn't change, which in
particular means that Jens Lehmann's C-side submodule fix is still
valid, although it might benefit from being reworked to exploit the
static list exposed by this patchset.

Giuseppe Bilotta (5):
  cache: static list of repo-local env vars
  connect: use static list of repo-local env vars
  rev-parse: --local-env-vars option
  shell setup: clear_local_git_env() function
  submodules: ensure clean environment when operating in a submodule

 Documentation/git-rev-parse.txt |    6 ++++++
 Makefile                        |    1 +
 builtin-rev-parse.c             |    8 ++++++++
 cache.c                         |   13 +++++++++++++
 cache.h                         |    2 ++
 connect.c                       |   13 +------------
 git-sh-setup.sh                 |    7 +++++++
 git-submodule.sh                |   20 ++++++++++----------
 8 files changed, 48 insertions(+), 22 deletions(-)
 create mode 100644 cache.c

[PATCHv3 1/5] cache: static list of repo-local env vars

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 Makefile |    1 +
 cache.c  |   13 +++++++++++++
 cache.h  |    2 ++
 3 files changed, 16 insertions(+), 0 deletions(-)
 create mode 100644 cache.c

diff --git a/Makefile b/Makefile
index 36611f4..9e9a858 100644
--- a/Makefile
+++ b/Makefile
@@ -524,6 +524,7 @@ LIB_OBJS += bisect.o
 LIB_OBJS += blob.o
 LIB_OBJS += branch.o
 LIB_OBJS += bundle.o
+LIB_OBJS += cache.o
 LIB_OBJS += cache-tree.o
 LIB_OBJS += color.o
 LIB_OBJS += combine-diff.o
diff --git a/cache.c b/cache.c
new file mode 100644
index 0000000..961ea56
--- /dev/null
+++ b/cache.c
@@ -0,0 +1,13 @@
+#include "cache.h"
+
+const char *const local_repo_env[] = {
+	ALTERNATE_DB_ENVIRONMENT,
+	CONFIG_ENVIRONMENT,
+	DB_ENVIRONMENT,
+	GIT_DIR_ENVIRONMENT,
+	GIT_WORK_TREE_ENVIRONMENT,
+	GRAFT_ENVIRONMENT,
+	INDEX_ENVIRONMENT,
+	NO_REPLACE_OBJECTS_ENVIRONMENT,
+	NULL
+};
diff --git a/cache.h b/cache.h
index c863085..7648a61 100644
--- a/cache.h
+++ b/cache.h
@@ -388,6 +388,8 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_NOTES_REF_ENVIRONMENT "GIT_NOTES_REF"
 #define GIT_NOTES_DEFAULT_REF "refs/notes/commits"
 
+extern const char *const local_repo_env[];
+
 extern int is_bare_repository_cfg;
 extern int is_bare_repository(void);
 extern int is_inside_git_dir(void);
-- 
1.7.0.200.g5ba36.dirty

[PATCHv3 2/5] connect: use static list of repo-local env vars

[Giuseppe Bilotta]

This adds the missing GIT_CONFIG to the list of env vars that
should be cleared.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 connect.c |   13 +------------
 1 files changed, 1 insertions(+), 12 deletions(-)

diff --git a/connect.c b/connect.c
index 9f39038..94b3707 100644
--- a/connect.c
+++ b/connect.c
@@ -582,18 +582,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 		*arg++ = host;
 	}
 	else {
-		/* remove these from the environment */
-		const char *env[] = {
-			ALTERNATE_DB_ENVIRONMENT,
-			DB_ENVIRONMENT,
-			GIT_DIR_ENVIRONMENT,
-			GIT_WORK_TREE_ENVIRONMENT,
-			GRAFT_ENVIRONMENT,
-			INDEX_ENVIRONMENT,
-			NO_REPLACE_OBJECTS_ENVIRONMENT,
-			NULL
-		};
-		conn->env = env;
+		conn->env = local_repo_env;
 		conn->use_shell = 1;
 	}
 	*arg++ = cmd.buf;
-- 
1.7.0.200.g5ba36.dirty

[PATCHv3 3/5] rev-parse: --local-env-vars option

[Giuseppe Bilotta]

This prints the list of repo-local environment variables.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 Documentation/git-rev-parse.txt |    6 ++++++
 builtin-rev-parse.c             |    8 ++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 1a613aa..8db600f 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -148,6 +148,12 @@ shown.  If the pattern does not contain a globbing character (`?`,
 --is-bare-repository::
 	When the repository is bare print "true", otherwise "false".
 
+--local-env-vars::
+	List the GIT_* environment variables that are local to the
+	repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
+	Only the names of the variables are listed, not their value,
+	even if they are set.
+
 --short::
 --short=number::
 	Instead of outputting the full SHA1 values of object names try to
diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index a8c5043..ab7b520 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -455,7 +455,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	if (argc > 1 && !strcmp("--sq-quote", argv[1]))
 		return cmd_sq_quote(argc - 2, argv + 2);
 
+	if (argc > 0 && !strcmp("--local-env-vars", argv[1])) {
+		unsigned int i = 0;
+		const char *var;
+		while (var = local_repo_env[i++])
+			printf("%s\n", var);
+		return 0;
+	}
 	if (argc > 1 && !strcmp("-h", argv[1]))
+
 		usage(builtin_rev_parse_usage);
 
 	prefix = setup_git_directory();
-- 
1.7.0.200.g5ba36.dirty

[PATCHv3 4/5] shell setup: clear_local_git_env() function

[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.bilotta@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

[PATCHv3 5/5] submodules: ensure clean environment when operating in a submodule

[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 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(-)

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

Re: [PATCHv3 2/5] connect: use static list of repo-local env vars

[Johannes Sixt]

Giuseppe Bilotta schrieb:
> -		/* remove these from the environment */

You shouldn't remove this comment.

> -		const char *env[] = {
> -			ALTERNATE_DB_ENVIRONMENT,
> -			DB_ENVIRONMENT,
> -			GIT_DIR_ENVIRONMENT,
> -			GIT_WORK_TREE_ENVIRONMENT,
> -			GRAFT_ENVIRONMENT,
> -			INDEX_ENVIRONMENT,
> -			NO_REPLACE_OBJECTS_ENVIRONMENT,
> -			NULL
> -		};
> -		conn->env = env;
> +		conn->env = local_repo_env;

IMO, you should squash this patch and the previous one under the subject
"Move list of repo-local environment variables to a public place", and
that public place could be environment.c instead of a new file cache.c
(the name "cache.c" is definitely wrong).

-- Hannes

Re: [PATCHv3 2/5] connect: use static list of repo-local env vars

[Giuseppe Bilotta]

Doh. I knew I should have waitied a little more before sending the new
series ...

On Wed, Feb 24, 2010 at 8:14 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Giuseppe Bilotta schrieb:
>> -             /* remove these from the environment */
>
> You shouldn't remove this comment.

Right. I should probably rephrase it a little, though.

>> -             const char *env[] = {
>> -                     ALTERNATE_DB_ENVIRONMENT,
>> -                     DB_ENVIRONMENT,
>> -                     GIT_DIR_ENVIRONMENT,
>> -                     GIT_WORK_TREE_ENVIRONMENT,
>> -                     GRAFT_ENVIRONMENT,
>> -                     INDEX_ENVIRONMENT,
>> -                     NO_REPLACE_OBJECTS_ENVIRONMENT,
>> -                     NULL
>> -             };
>> -             conn->env = env;
>> +             conn->env = local_repo_env;
>
> IMO, you should squash this patch and the previous one under the subject
> "Move list of repo-local environment variables to a public place", and
> that public place could be environment.c instead of a new file cache.c
> (the name "cache.c" is definitely wrong).

The list is now the right place (v4). The squashing is most definitely
a good idea, gives a better idea of the code movement.

-- 
Giuseppe "Oblomov" Bilotta