[PATCH] allow commands to be executed in submodules

[PATCH] allow commands to be executed in submodules

[Martin Waitz]

Add an extra "submodule" field to struct child_process to be able to
easily start commands which are to be executed in a submodule
repository.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---

 run-command.c     |   13 ++++++
 run-command.h     |    1 +
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/run-command.c b/run-command.c
index eff523e..c2475e4 100644
--- a/run-command.c
+++ b/run-command.c
@@ -73,6 +73,19 @@ int start_command(struct child_process *cmd)
 			close(cmd->out);
 		}
 
+		if (cmd->submodule) {
+			int err = chdir(cmd->submodule);
+			if (err) {
+				die("cannot exec %s in %s.",
+					cmd->argv[0], cmd->submodule);
+			}
+			/* don't inherit supermodule environment */
+			unsetenv(GIT_DIR_ENVIRONMENT);
+			unsetenv(DB_ENVIRONMENT);
+			unsetenv(INDEX_ENVIRONMENT);
+			unsetenv(GRAFT_ENVIRONMENT);
+		}
+
 		if (cmd->git_cmd) {
 			execv_git_cmd(cmd->argv);
 		} else {
diff --git a/run-command.h b/run-command.h
index 3680ef9..2940186 100644
--- a/run-command.h
+++ b/run-command.h
@@ -16,6 +16,7 @@ struct child_process {
 	pid_t pid;
 	int in;
 	int out;
+	const char *submodule;
 	unsigned close_in:1;
 	unsigned close_out:1;
 	unsigned no_stdin:1;
-- 
1.5.2.2.g081e


-- 
Martin Waitz

Re: [PATCH] allow commands to be executed in submodules

[Alex Riesen]

Martin Waitz, Sun, May 20, 2007 17:39:08 +0200:
> Add an extra "submodule" field to struct child_process to be able to
> easily start commands which are to be executed in a submodule
> repository.

How about making it more generic by allowing to specify the directory
to change to and environment for subprocess? You probably will be able
to convert even some of existing code to your new run_command then
(merge_recursive in builtin-revert.c, for example).

Something like this, perhaps:

diff --git a/run-command.c b/run-command.c
index eff523e..605aa1e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -73,6 +73,13 @@ int start_command(struct child_process *cmd)
 			close(cmd->out);
 		}
 
+		if (cmd->dir && chdir(cmd->dir))
+			die("exec %s: cd to %s failed (%s)", cmd->argv[0],
+			    cmd->dir, strerror(errno));
+		if (cmd->env) {
+			for (; *cmd->env; cmd->env++)
+				putenv((char*)*cmd->env);
+		}
 		if (cmd->git_cmd) {
 			execv_git_cmd(cmd->argv);
 		} else {
@@ -133,13 +140,38 @@ int run_command(struct child_process *cmd)
 	return finish_command(cmd);
 }
 
+static void prepare_run_command_v_opt(struct child_process *cmd,
+				      const char **argv,
+				      int opt)
+{
+	memset(cmd, 0, sizeof(*cmd));
+	cmd->argv = argv;
+	cmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
+	cmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
+	cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
+}
+
 int run_command_v_opt(const char **argv, int opt)
 {
 	struct child_process cmd;
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.argv = argv;
-	cmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
-	cmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
-	cmd.stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
+	prepare_run_command_v_opt(&cmd, argv, opt);
 	return run_command(&cmd);
 }
+
+int run_command_v_opt_cd(const char **argv, int opt, const char *dir)
+{
+	struct child_process cmd;
+	prepare_run_command_v_opt(&cmd, argv, opt);
+	cmd.dir = dir;
+	return run_command(&cmd);
+}
+
+int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env)
+{
+	struct child_process cmd;
+	prepare_run_command_v_opt(&cmd, argv, opt);
+	cmd.dir = dir;
+	cmd.env = env;
+	return run_command(&cmd);
+}
+
diff --git a/run-command.h b/run-command.h
index 3680ef9..af1e0bf 100644
--- a/run-command.h
+++ b/run-command.h
@@ -16,6 +16,8 @@ struct child_process {
 	pid_t pid;
 	int in;
 	int out;
+	const char *dir;
+	const char *const *env;
 	unsigned close_in:1;
 	unsigned close_out:1;
 	unsigned no_stdin:1;
@@ -32,5 +34,7 @@ int run_command(struct child_process *);
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
 #define RUN_COMMAND_STDOUT_TO_STDERR 4
 int run_command_v_opt(const char **argv, int opt);
+int run_command_v_opt_cd(const char **argv, int opt, const char *dir);
+int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);
 
 #endif

Re: [PATCH] allow commands to be executed in submodules

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Martin Waitz, Sun, May 20, 2007 17:39:08 +0200:
>> Add an extra "submodule" field to struct child_process to be able to
>> easily start commands which are to be executed in a submodule
>> repository.
>
> How about making it more generic by allowing to specify the directory
> to change to and environment for subprocess? You probably will be able
> to convert even some of existing code to your new run_command then
> (merge_recursive in builtin-revert.c, for example).

Sounds useful and more generic.

Re: [PATCH] allow commands to be executed in submodules

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

hoi :)

On Sun, May 20, 2007 at 11:25:24AM -0700, Junio C Hamano wrote:
> Sounds useful and more generic.

I explicitly wanted to have a method to execute one command in
the environment of a submodule.  That way we can update it in
one place if we later add more environment variables which
influence the repository.

Do we really have so many places where we want to execute commands
in a different directory or with different environment?  Is it worth
keeping run-command generic and having to introduce knowledge about
how to run submodule commands in multiple places?

That said I don't have any strong feeling about it, as long as one
or the other patch is applied.

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] allow commands to be executed in submodules

[Alex Riesen]

Martin Waitz, Sun, May 20, 2007 22:48:02 +0200:
> Do we really have so many places where we want to execute commands
> in a different directory or with different environment?  Is it worth
> keeping run-command generic and having to introduce knowledge about
> how to run submodule commands in multiple places?

Is there multiple places? Is it hard to create a specific function out
of a generic one? (which can be used from other places and your
specific can't and we would need the generic one anyway).

"Generic" is not about "multiple places". Generic is about "general"
as opposite to "specific". Gives you flexibility and wider application
range.

Re: [PATCH] allow commands to be executed in submodules

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 935 bytes --]

hoi :)

On Sun, May 20, 2007 at 10:59:33PM +0200, Alex Riesen wrote:
> Is there multiple places? Is it hard to create a specific function out
> of a generic one? (which can be used from other places and your
> specific can't and we would need the generic one anyway).

you can add a specific new function for submodules, dropping the
nice property of child_process that you only have to initialize a few
fields and then can run the command.

> "Generic" is not about "multiple places". Generic is about "general"
> as opposite to "specific". Gives you flexibility and wider application
> range.

Generic code and abstractions only make sense when they are _useful_.
Lets not overengineer it.  If we later see that we need more, then
so be it.  KISS.

But now lets go on and don't discuss about such details.
I'm happy if I can run commands in submodules and don't care about the
actual code.

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] Add ability to specify environment extension to run_command

[Alex Riesen]

There is no way to specify and override for the environment: there is
no visible user for it (yet, something in git-daemon could need it).

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Sven Verdoolaege, Mon, May 21, 2007 11:03:39 +0200:
> Could you sign-off on this for me so I can use it my patch set?
> 

So here it is. On top of the previos patch regarding chdir before
exec. Junio, if needed, I can resend that first patch about chdir.

 run-command.c |   17 ++++++++++++++++-
 run-command.h |    2 ++
 2 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/run-command.c b/run-command.c
index 043b570..605aa1e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -76,6 +76,10 @@ int start_command(struct child_process *cmd)
 		if (cmd->dir && chdir(cmd->dir))
 			die("exec %s: cd to %s failed (%s)", cmd->argv[0],
 			    cmd->dir, strerror(errno));
+		if (cmd->env) {
+			for (; *cmd->env; cmd->env++)
+				putenv((char*)*cmd->env);
+		}
 		if (cmd->git_cmd) {
 			execv_git_cmd(cmd->argv);
 		} else {
@@ -137,7 +141,8 @@ int run_command(struct child_process *cmd)
 }
 
 static void prepare_run_command_v_opt(struct child_process *cmd,
-				      const char **argv, int opt)
+				      const char **argv,
+				      int opt)
 {
 	memset(cmd, 0, sizeof(*cmd));
 	cmd->argv = argv;
@@ -160,3 +165,13 @@ int run_command_v_opt_cd(const char **argv, int opt, const char *dir)
 	cmd.dir = dir;
 	return run_command(&cmd);
 }
+
+int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env)
+{
+	struct child_process cmd;
+	prepare_run_command_v_opt(&cmd, argv, opt);
+	cmd.dir = dir;
+	cmd.env = env;
+	return run_command(&cmd);
+}
+
diff --git a/run-command.h b/run-command.h
index cbd7484..af1e0bf 100644
--- a/run-command.h
+++ b/run-command.h
@@ -17,6 +17,7 @@ struct child_process {
 	int in;
 	int out;
 	const char *dir;
+	const char *const *env;
 	unsigned close_in:1;
 	unsigned close_out:1;
 	unsigned no_stdin:1;
@@ -34,5 +35,6 @@ int run_command(struct child_process *);
 #define RUN_COMMAND_STDOUT_TO_STDERR 4
 int run_command_v_opt(const char **argv, int opt);
 int run_command_v_opt_cd(const char **argv, int opt, const char *dir);
+int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);
 
 #endif
-- 
1.5.2.rc3.112.gc1e43

Re: [PATCH] Add ability to specify environment extension to run_command

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> There is no way to specify and override for the environment: there is
> no visible user for it (yet, something in git-daemon could need it).
>
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
>
> Sven Verdoolaege, Mon, May 21, 2007 11:03:39 +0200:
>> Could you sign-off on this for me so I can use it my patch set?
>> 
>
> So here it is. On top of the previos patch regarding chdir before
> exec. Junio, if needed, I can resend that first patch about chdir.

Both of them in a row would be good, so yes, resend is
appreciated.

> @@ -76,6 +76,10 @@ int start_command(struct child_process *cmd)
>  		if (cmd->dir && chdir(cmd->dir))
>  			die("exec %s: cd to %s failed (%s)", cmd->argv[0],
>  			    cmd->dir, strerror(errno));
> +		if (cmd->env) {
> +			for (; *cmd->env; cmd->env++)
> +				putenv((char*)*cmd->env);
> +		}
>  		if (cmd->git_cmd) {
>  			execv_git_cmd(cmd->argv);
>  		} else {

I had a feeling that some callers needed to be able to unsetenv
some.  How would this patch help them, or are they outside of
the scope?

Re: [PATCH] Add ability to specify environment extension to run_command

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

hoi :)

On Mon, May 21, 2007 at 04:02:42PM -0700, Junio C Hamano wrote:
> I had a feeling that some callers needed to be able to unsetenv
> some.  How would this patch help them, or are they outside of
> the scope?

At first I had the same objection but the putenv documentation
told me that at least in glibc you can unsetenv by providing
the variable name without a "=".

But perhaps we should check for other systems?

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add ability to specify environment extension to run_command

[Junio C Hamano]

Martin Waitz <tali@admingilde.org> writes:

> On Mon, May 21, 2007 at 04:02:42PM -0700, Junio C Hamano wrote:
>> I had a feeling that some callers needed to be able to unsetenv
>> some.  How would this patch help them, or are they outside of
>> the scope?
>
> At first I had the same objection but the putenv documentation
> told me that at least in glibc you can unsetenv by providing
> the variable name without a "=".

I recall SysV putenv() does not remove "ENVNAME" without '=', and
http://www.opengroup.org/onlinepubs/000095399/functions/putenv.html
seems to say that as well.

> But perhaps we should check for other systems?

Probably.

At least we have setenv/unsetenv calls already (with emulation
where they aren't available), we could do something like this:

	struct child_process {
        	...
                const struct {
                        const char *name;
                        const char *value; /* NULL to unsetenv */
                } *env;
		...
	};

and in start_command():

	if (cmd->env) {
		int i;
                for (i = 0; cmd->env[i].name; i++) {
			if (cmd->env[i].value)
				setenv(cmd->env[i].name, cmd->env[i].value, 1);
			else
				unsetenv(cmd->env[i].name);
                }
        }

Re: [PATCH] Add ability to specify environment extension to run_command

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> Martin Waitz <tali@admingilde.org> writes:
> 
> > On Mon, May 21, 2007 at 04:02:42PM -0700, Junio C Hamano wrote:
> >> I had a feeling that some callers needed to be able to unsetenv
> >> some.  How would this patch help them, or are they outside of
> >> the scope?
> >
> > At first I had the same objection but the putenv documentation
> > told me that at least in glibc you can unsetenv by providing
> > the variable name without a "=".
> 
> I recall SysV putenv() does not remove "ENVNAME" without '=', and
> http://www.opengroup.org/onlinepubs/000095399/functions/putenv.html
> seems to say that as well.

Are we overbuilding this thing?

I thought this thread all started because we wanted to run a
command in a subproject, and did not want the parent's GIT_*
environment variables to confuse the subproject process when
it started.  That's a pretty simple concept: clear any GIT_*
environment variable that can change behavior in the subproject.
And almost everyone who is trying to use this API and alter the
env wants exactly that - a subproject command.

-- 
Shawn.

Re: [PATCH] Add ability to specify environment extension to run_command

[Sven Verdoolaege]

On Tue, May 22, 2007 at 02:38:22AM -0400, Shawn O. Pearce wrote:
> I thought this thread all started because we wanted to run a
> command in a subproject, and did not want the parent's GIT_*
> environment variables to confuse the subproject process when
> it started.  That's a pretty simple concept: clear any GIT_*
> environment variable that can change behavior in the subproject.
> And almost everyone who is trying to use this API and alter the
> env wants exactly that - a subproject command.

This would work for me, I suppose.
Right now, I sometimes set GIT_DIR explicitly, but I could just chdir
(or use --git-dir=).

skimo

Re: [PATCH] Add ability to specify environment extension to run_command

[Alex Riesen]

Junio C Hamano, Tue, May 22, 2007 08:33:44 +0200:
> 	struct child_process {
>         	...
>                 const struct {
>                         const char *name;
>                         const char *value; /* NULL to unsetenv */
>                 } *env;
> 		...
> 	};

I actually like how the environment is organized. And it is simple to
define in the source. And there are well-known routines for
environment-like array manipulation.

Re: [PATCH] Add ability to specify environment extension to run_command

[Alex Riesen]

Junio C Hamano, Tue, May 22, 2007 01:02:42 +0200:
> >
> > So here it is. On top of the previos patch regarding chdir before
> > exec. Junio, if needed, I can resend that first patch about chdir.
> 
> Both of them in a row would be good, so yes, resend is
> appreciated.

Will be resent.

> > @@ -76,6 +76,10 @@ int start_command(struct child_process *cmd)
> >  		if (cmd->dir && chdir(cmd->dir))
> >  			die("exec %s: cd to %s failed (%s)", cmd->argv[0],
> >  			    cmd->dir, strerror(errno));
> > +		if (cmd->env) {
> > +			for (; *cmd->env; cmd->env++)
> > +				putenv((char*)*cmd->env);
> > +		}
> >  		if (cmd->git_cmd) {
> >  			execv_git_cmd(cmd->argv);
> >  		} else {
> 
> I had a feeling that some callers needed to be able to unsetenv
> some.  How would this patch help them, or are they outside of
> the scope?
> 

Others already discussed the issue. Just to be sure, I reimplemented
that comfortable putenv with unsetenv: if an environment entry ends
with a "=" it will be unset.

[PATCH] Add run_command_v_opt_cd: chdir into a directory before exec

[Alex Riesen]

It can make code simplier (no need to preserve cwd) and safer
(no chance the cwd of the current process is accidentally forgotten).

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 run-command.c |   27 ++++++++++++++++++++++-----
 run-command.h |    2 ++
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/run-command.c b/run-command.c
index eff523e..043b570 100644
--- a/run-command.c
+++ b/run-command.c
@@ -73,6 +73,9 @@ int start_command(struct child_process *cmd)
 			close(cmd->out);
 		}
 
+		if (cmd->dir && chdir(cmd->dir))
+			die("exec %s: cd to %s failed (%s)", cmd->argv[0],
+			    cmd->dir, strerror(errno));
 		if (cmd->git_cmd) {
 			execv_git_cmd(cmd->argv);
 		} else {
@@ -133,13 +136,27 @@ int run_command(struct child_process *cmd)
 	return finish_command(cmd);
 }
 
+static void prepare_run_command_v_opt(struct child_process *cmd,
+				      const char **argv, int opt)
+{
+	memset(cmd, 0, sizeof(*cmd));
+	cmd->argv = argv;
+	cmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
+	cmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
+	cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
+}
+
 int run_command_v_opt(const char **argv, int opt)
 {
 	struct child_process cmd;
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.argv = argv;
-	cmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
-	cmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
-	cmd.stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
+	prepare_run_command_v_opt(&cmd, argv, opt);
+	return run_command(&cmd);
+}
+
+int run_command_v_opt_cd(const char **argv, int opt, const char *dir)
+{
+	struct child_process cmd;
+	prepare_run_command_v_opt(&cmd, argv, opt);
+	cmd.dir = dir;
 	return run_command(&cmd);
 }
diff --git a/run-command.h b/run-command.h
index 3680ef9..cbd7484 100644
--- a/run-command.h
+++ b/run-command.h
@@ -16,6 +16,7 @@ struct child_process {
 	pid_t pid;
 	int in;
 	int out;
+	const char *dir;
 	unsigned close_in:1;
 	unsigned close_out:1;
 	unsigned no_stdin:1;
@@ -32,5 +33,6 @@ int run_command(struct child_process *);
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
 #define RUN_COMMAND_STDOUT_TO_STDERR 4
 int run_command_v_opt(const char **argv, int opt);
+int run_command_v_opt_cd(const char **argv, int opt, const char *dir);
 
 #endif
-- 
1.5.2.51.g16099

[PATCH] Add ability to specify environment extension to run_command

[Alex Riesen]

There is no way to specify and override for the environment:
there'd be no user for it yet.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 run-command.c |   17 ++++++++++++++++-
 run-command.h |    2 ++
 2 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/run-command.c b/run-command.c
index 043b570..605aa1e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -76,6 +76,10 @@ int start_command(struct child_process *cmd)
 		if (cmd->dir && chdir(cmd->dir))
 			die("exec %s: cd to %s failed (%s)", cmd->argv[0],
 			    cmd->dir, strerror(errno));
+		if (cmd->env) {
+			for (; *cmd->env; cmd->env++)
+				putenv((char*)*cmd->env);
+		}
 		if (cmd->git_cmd) {
 			execv_git_cmd(cmd->argv);
 		} else {
@@ -137,7 +141,8 @@ int run_command(struct child_process *cmd)
 }
 
 static void prepare_run_command_v_opt(struct child_process *cmd,
-				      const char **argv, int opt)
+				      const char **argv,
+				      int opt)
 {
 	memset(cmd, 0, sizeof(*cmd));
 	cmd->argv = argv;
@@ -160,3 +165,13 @@ int run_command_v_opt_cd(const char **argv, int opt, const char *dir)
 	cmd.dir = dir;
 	return run_command(&cmd);
 }
+
+int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env)
+{
+	struct child_process cmd;
+	prepare_run_command_v_opt(&cmd, argv, opt);
+	cmd.dir = dir;
+	cmd.env = env;
+	return run_command(&cmd);
+}
+
diff --git a/run-command.h b/run-command.h
index cbd7484..af1e0bf 100644
--- a/run-command.h
+++ b/run-command.h
@@ -17,6 +17,7 @@ struct child_process {
 	int in;
 	int out;
 	const char *dir;
+	const char *const *env;
 	unsigned close_in:1;
 	unsigned close_out:1;
 	unsigned no_stdin:1;
@@ -34,5 +35,6 @@ int run_command(struct child_process *);
 #define RUN_COMMAND_STDOUT_TO_STDERR 4
 int run_command_v_opt(const char **argv, int opt);
 int run_command_v_opt_cd(const char **argv, int opt, const char *dir);
+int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);
 
 #endif
-- 
1.5.2.51.g16099

[PATCH] Allow environment variables to be unset in the processes started by run_command

[Alex Riesen]

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 run-command.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/run-command.c b/run-command.c
index 605aa1e..3a5f737 100644
--- a/run-command.c
+++ b/run-command.c
@@ -77,8 +77,18 @@ int start_command(struct child_process *cmd)
 			die("exec %s: cd to %s failed (%s)", cmd->argv[0],
 			    cmd->dir, strerror(errno));
 		if (cmd->env) {
-			for (; *cmd->env; cmd->env++)
-				putenv((char*)*cmd->env);
+			char *unsetbuf = NULL;
+			for (; *cmd->env; cmd->env++) {
+				size_t n = strlen(*cmd->env);
+				if (n && (*cmd->env)[n-1] == '=') {
+					unsetbuf = xrealloc(unsetbuf, n);
+					memcpy(unsetbuf, *cmd->env, --n);
+					unsetbuf[n] = '\0';
+					unsetenv(unsetbuf);
+				} else
+					putenv((char*)*cmd->env);
+			}
+			free(unsetbuf);
 		}
 		if (cmd->git_cmd) {
 			execv_git_cmd(cmd->argv);
-- 
1.5.2.51.g16099

Re: [PATCH] Add ability to specify environment extension to run_command

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Others already discussed the issue. Just to be sure, I reimplemented
> that comfortable putenv with unsetenv: if an environment entry ends
> with a "=" it will be unset.

Although combination of putenv and unsetenv gives a somewhat
queasy feeling for obvious reasons, I'll let it pass.  As we
are coming up with an interface that uses only one string per
environment element, that is probably a sensible thing to do,
rather than trying to do the "historically correct" pairing of
setenv/unsetenv.

However, I do not think "VAR=" to unset it is a good interface.
Having an environment variable whose value happens to be an
empty string and not having the variable at all are two
different things.

Because you _scan_ the whole string in your patch to see if it
ends with = anyway, a trivial improvement would be to do:

	if (strchr(cmd->env, '='))
                putenv(cmd->env);
	else
        	unsetenv(cmd->env);

If you do not mind such a special syntax (e.g. "VAR="), I would
suggest doing that as a prefix (e.g. "!VAR") and do:

	if (cmd->env[0] != '!')
        	putenv(cmd->env);
	else
		unsetenv(cmd->env + 1);

The former look cleaner but less efficient; we are going to exec
so I do not think micro-optimization would matter at all, so my
suggestion would be to do the strchr().

Re: [PATCH] Add ability to specify environment extension to run_command

[Alex Riesen]

Junio C Hamano, Wed, May 23, 2007 00:19:47 +0200:
> > Others already discussed the issue. Just to be sure, I reimplemented
> > that comfortable putenv with unsetenv: if an environment entry ends
> > with a "=" it will be unset.
> 
> Although combination of putenv and unsetenv gives a somewhat
> queasy feeling for obvious reasons, I'll let it pass.  As we
> are coming up with an interface that uses only one string per
> environment element, that is probably a sensible thing to do,
> rather than trying to do the "historically correct" pairing of
> setenv/unsetenv.
> 
> However, I do not think "VAR=" to unset it is a good interface.
> Having an environment variable whose value happens to be an
> empty string and not having the variable at all are two
> different things.

Right

> Because you _scan_ the whole string in your patch to see if it
> ends with = anyway, a trivial improvement would be to do:
> 
> 	if (strchr(cmd->env, '='))
>                 putenv(cmd->env);
> 	else
>         	unsetenv(cmd->env);

I like this one. The env field in struct child_process and run_command
will have to mention it in comments (in run-command.h), it's kind of
special.

> If you do not mind such a special syntax (e.g. "VAR="), I would
> suggest doing that as a prefix (e.g. "!VAR") and do:

Nah, !VAR is a _working_ environment variable name.

    int main(int argc, char *argv[], char *envp[])
    {
	    const char *argv1[] = {"/usr/bin/perl", "-e", "print $ENV{'!VAR'}", NULL};
	    const char *envp1[] = {"!VAR=value", NULL};
	    execve(*argv, (char**)argv1, (char**)envp1);
	    return 0;
    }

    $ gcc ... && ./a.out
    value

Someone could want it. We surely could use "=", though :)

[PATCH] Allow environment variables to be unset in the processes started by run_command

[Alex Riesen]

To unset a variable, just specify its name, without "=". For example:

    const char *env[] = {"GIT_DIR=.git", "PWD", NULL};
    const char *argv[] = {"git-ls-files", "-s", NULL};
    int err = run_command_v_opt_cd_env(argv, RUN_GIT_CMD, ".", env);

The PWD will be unset before executing git-ls-files.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Alex Riesen, Wed, May 23, 2007 01:14:42 +0200:
> > Because you _scan_ the whole string in your patch to see if it
> > ends with = anyway, a trivial improvement would be to do:
> > 
> > 	if (strchr(cmd->env, '='))
> >                 putenv(cmd->env);
> > 	else
> >         	unsetenv(cmd->env);
> 
> I like this one. The env field in struct child_process and run_command
> will have to mention it in comments (in run-command.h), it's kind of
> special.
> 

 run-command.c |    8 ++++++--
 run-command.h |    5 +++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/run-command.c b/run-command.c
index 605aa1e..3b1899e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -77,8 +77,12 @@ int start_command(struct child_process *cmd)
 			die("exec %s: cd to %s failed (%s)", cmd->argv[0],
 			    cmd->dir, strerror(errno));
 		if (cmd->env) {
-			for (; *cmd->env; cmd->env++)
-				putenv((char*)*cmd->env);
+			for (; *cmd->env; cmd->env++) {
+				if (strchr(*cmd->env, '='))
+					putenv((char*)*cmd->env);
+				else
+					unsetenv(*cmd->env);
+			}
 		}
 		if (cmd->git_cmd) {
 			execv_git_cmd(cmd->argv);
diff --git a/run-command.h b/run-command.h
index af1e0bf..7958eb1 100644
--- a/run-command.h
+++ b/run-command.h
@@ -35,6 +35,11 @@ int run_command(struct child_process *);
 #define RUN_COMMAND_STDOUT_TO_STDERR 4
 int run_command_v_opt(const char **argv, int opt);
 int run_command_v_opt_cd(const char **argv, int opt, const char *dir);
+
+/*
+ * env (the environment) is to be formatted like environ: "VAR=VALUE".
+ * To unset an environment variable use just "VAR".
+ */
 int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);
 
 #endif
-- 
1.5.2.67.gbd3c2