bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Noam Postavsky]

Start emacs -Q, evaluate the following lisp code (I wrote the return
values after ;=>)

(defun check-env-var (var)
  (catch 'ret
    (dolist (var=val (process-lines "env"))
      (when (string-prefix-p var var=val)
        (throw 'ret var=val)))))

(check-env-var "SHELL");=>"SHELL=/bin/bash"
(let ((process-environment (copy-sequence process-environment)))
  (setenv "SHELL" nil)
  (check-env-var "SHELL"));=>nil
(let ((process-environment (cons "SHELL" process-environment)))
  (check-env-var "SHELL"));=>"SHELL=/bin/bash"
(let ((process-environment (cons "SHELL=" process-environment)))
  (check-env-var "SHELL"));=>"SHELL="

As you can see from the 3rd expression, contrary to its docstring,
consing the env variable "SHELL" onto process-environment has no
effect at all.

process-environment is a variable defined in ‘C source code’.
Its value is
[...]
Documentation:
List of overridden environment variables for subprocesses to inherit.
Each element should be a string of the form ENVVARNAME=VALUE.

Entries in this list take precedence to those in the frame-local
environments.  Therefore, let-binding ‘process-environment’ is an easy
way to temporarily change the value of an environment variable,
irrespective of where it comes from.  To use ‘process-environment’ to
remove an environment variable, include only its name in the list,
without "=VALUE".

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Thu, 16 Jun 2016 23:33:02 -0400
> 
> (check-env-var "SHELL");=>"SHELL=/bin/bash"
> (let ((process-environment (copy-sequence process-environment)))
>   (setenv "SHELL" nil)
>   (check-env-var "SHELL"));=>nil
> (let ((process-environment (cons "SHELL" process-environment)))
>   (check-env-var "SHELL"));=>"SHELL=/bin/bash"
> (let ((process-environment (cons "SHELL=" process-environment)))
>   (check-env-var "SHELL"));=>"SHELL="
> 
> As you can see from the 3rd expression, contrary to its docstring,
> consing the env variable "SHELL" onto process-environment has no
> effect at all.
> 
> process-environment is a variable defined in ‘C source code’.
> Its value is
> [...]
> Documentation:
> List of overridden environment variables for subprocesses to inherit.
> Each element should be a string of the form ENVVARNAME=VALUE.
> 
> Entries in this list take precedence to those in the frame-local
> environments.  Therefore, let-binding ‘process-environment’ is an easy
> way to temporarily change the value of an environment variable,
> irrespective of where it comes from.  To use ‘process-environment’ to
> remove an environment variable, include only its name in the list,
> without "=VALUE".

Where does it say that you can use 'cons' or 'push', and only them, to
the effect of removing the variable from the environment passed to
child processes?  process-environment is just a simple list, so these
two functions just _add_ another member to the list, they don't remove
the old member.

My reading of the last sentence you cite is that you must manually
remove the old member "SHELL=whatever", and _then_ add a member that
has no value.

Am I missing something?

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Dmitry Gutov]

On 06/17/2016 10:11 AM, Eli Zaretskii wrote:

> Where does it say that you can use 'cons' or 'push', and only them, to
> the effect of removing the variable from the environment passed to
> child processes?

That works with other Emacs features, such as auto-mode-alist.

You can also override the values in process-environment using a cons 
(which strongly suggests the semantics of "first element wins"). Just 
not "remove" them, currently.

> process-environment is just a simple list, so these
> two functions just _add_ another member to the list, they don't remove
> the old member.

What would be the point of ever using the "only its name" form if you 
have to scrub process-environment of all other mentions of this variable?

> My reading of the last sentence you cite is that you must manually
> remove the old member "SHELL=whatever", and _then_ add a member that
> has no value.

If I've removed all mentions of "GIT_DIR" in there, it's already 
removed, and the subprocesses shouldn't see it. Why add the new member then?

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Eli Zaretskii]

> Cc: 23779@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 17 Jun 2016 15:17:38 +0300
> 
> On 06/17/2016 10:11 AM, Eli Zaretskii wrote:
> 
> > Where does it say that you can use 'cons' or 'push', and only them, to
> > the effect of removing the variable from the environment passed to
> > child processes?
> 
> That works with other Emacs features, such as auto-mode-alist.

There should be code to make it work, it won't work by itself.
(auto-mode-alist is different, because it's used entirely in Lisp.  By
contrast, here we must construct the C-level environment array we pass
to child programs so as to remove the variable from it.)

> You can also override the values in process-environment using a cons 
> (which strongly suggests the semantics of "first element wins"). Just 
> not "remove" them, currently.

Looks like this never worked as intended.  Does the patch below fix
this?

diff --git a/src/callproc.c b/src/callproc.c
index db602f5..2fb5b1d 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -1099,7 +1099,7 @@ add_env (char **env, char **new_env, char *string)
       char *p = *ep, *q = string;
       while (ok)
 	{
-	  if (*q != *p)
+	  if (*p && *q != *p)
 	    break;
 	  if (*q == 0)
 	    /* The string is a lone variable name; keep it for now, we

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Dmitry Gutov]

On 06/17/2016 05:01 PM, Eli Zaretskii wrote:

> There should be code to make it work, it won't work by itself.

No argument there.

> Looks like this never worked as intended.  Does the patch below fix
> this?

Looks like it does. Please push at your convenience.

I wonder if we should make setenv work non-destructively now.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Eli Zaretskii]

> Cc: npostavs@users.sourceforge.net, 23779@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 17 Jun 2016 17:12:52 +0300
> 
> On 06/17/2016 05:01 PM, Eli Zaretskii wrote:
> 
> > Looks like this never worked as intended.  Does the patch below fix
> > this?
> 
> Looks like it does. Please push at your convenience.

I'd like the patch to be eyeballed by a few more people.  Paul,
Andreas, do you see any problems with it?  If not, I'd like to push it
to the emacs-25 branch.

> I wonder if we should make setenv work non-destructively now.

Why should we do that?  We have initial-environment if we need the
original value.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Dmitry Gutov]

On 06/17/2016 05:19 PM, Eli Zaretskii wrote:

>> I wonder if we should make setenv work non-destructively now.
>
> Why should we do that?  We have initial-environment if we need the
> original value.

Normally, we only want to change the environment for the duration of a 
command. So, what are the downsides?

One the plus side:

- setenv-internal will become simpler.
- We won't have to cons manually anymore. The code will become a bit 
nicer, like:

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index f35c84d..5315e0a 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1450,7 +1450,8 @@ vc-git--call
           (or coding-system-for-read vc-git-log-output-coding-system))
  	(coding-system-for-write
           (or coding-system-for-write vc-git-commits-coding-system))
-	(process-environment (cons "PAGER=" process-environment)))
+	(process-environment process-environment))
+    (setenv "PAGER")
      (apply 'process-file vc-git-program nil buffer nil command args)))

  (defun vc-git--out-ok (command &rest args)

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Andreas Schwab]

Dmitry Gutov <dgutov@yandex.ru> writes:

> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index f35c84d..5315e0a 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1450,7 +1450,8 @@ vc-git--call
>           (or coding-system-for-read vc-git-log-output-coding-system))
>  	(coding-system-for-write
>           (or coding-system-for-write vc-git-commits-coding-system))
> -	(process-environment (cons "PAGER=" process-environment)))
> +	(process-environment process-environment))
> +    (setenv "PAGER")
>      (apply 'process-file vc-git-program nil buffer nil command args)))

Is that in any way different from git --no-pager?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Dmitry Gutov]

On 06/17/2016 07:52 PM, Andreas Schwab wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:

>>           (or coding-system-for-read vc-git-log-output-coding-system))
>>  	(coding-system-for-write
>>           (or coding-system-for-write vc-git-commits-coding-system))
>> -	(process-environment (cons "PAGER=" process-environment)))
>> +	(process-environment process-environment))
>> +    (setenv "PAGER")
>>      (apply 'process-file vc-git-program nil buffer nil command args)))
>
> Is that in any way different from git --no-pager?

The effect is likely the same. But that is beside the point: the example 
is not about PAGER but about setenv.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Eli Zaretskii]

> Cc: npostavs@users.sourceforge.net, 23779@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 17 Jun 2016 17:47:49 +0300
> 
> On 06/17/2016 05:19 PM, Eli Zaretskii wrote:
> 
> >> I wonder if we should make setenv work non-destructively now.
> >
> > Why should we do that?  We have initial-environment if we need the
> > original value.
> 
> Normally, we only want to change the environment for the duration of a 
> command.

Now that 'push' works, why do we need setenv for that?

> So, what are the downsides?

That there's no way of changing the environment permanently?

> - We won't have to cons manually anymore. The code will become a bit 
> nicer, like:
> 
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index f35c84d..5315e0a 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1450,7 +1450,8 @@ vc-git--call
>            (or coding-system-for-read vc-git-log-output-coding-system))
>   	(coding-system-for-write
>            (or coding-system-for-write vc-git-commits-coding-system))
> -	(process-environment (cons "PAGER=" process-environment)))
> +	(process-environment process-environment))
> +    (setenv "PAGER")

I'm not sure I see why that is nicer.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Dmitry Gutov]

On 06/17/2016 08:06 PM, Eli Zaretskii wrote:

> Now that 'push' works, why do we need setenv for that?

So that the user doesn't have to (push "VAR=value"), or (push "VAR="), 
or know the difference between the latter and (push "VAR").

setenv is a better abstraction. In fact, if you use it, you don't even 
have to know the format of process-environment.

>> So, what are the downsides?
>
> That there's no way of changing the environment permanently?

The environment is changes as a result. It just doesn't modify the 
original list, and, as such, contains duplicate values for the changed 
variables.

> I'm not sure I see why that is nicer.

Do you know what value "PAGER=" assigns to the variable PAGER?

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Eli Zaretskii]

> Cc: eggert@cs.ucla.edu, schwab@linux-m68k.org,
>  npostavs@users.sourceforge.net, 23779@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 17 Jun 2016 22:01:19 +0300
> 
> On 06/17/2016 08:06 PM, Eli Zaretskii wrote:
> 
> > Now that 'push' works, why do we need setenv for that?
> 
> So that the user doesn't have to (push "VAR=value"), or (push "VAR="), 
> or know the difference between the latter and (push "VAR").
> 
> setenv is a better abstraction. In fact, if you use it, you don't even 
> have to know the format of process-environment.

We will have to agree to disagree on this one.

> >> So, what are the downsides?
> >
> > That there's no way of changing the environment permanently?
> 
> The environment is changes as a result. It just doesn't modify the 
> original list, and, as such, contains duplicate values for the changed 
> variables.

What do I do if I do want to change the original one?

> > I'm not sure I see why that is nicer.
> 
> Do you know what value "PAGER=" assigns to the variable PAGER?

Not sure what that means, or why do you ask that.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Dmitry Gutov]

On 06/17/2016 11:10 PM, Eli Zaretskii wrote:

> What do I do if I do want to change the original one?

That's what I was asking: are there scenarios where someone would want 
to do that? If they're very exceptional, if might be fine if the caller 
has to manipulate the process-environment list on a lower level themselves.

> Not sure what that means, or why do you ask that.

Do you know which value

(setq process-environment (cons "PAGER=" ...))

assigns to the environment variable PAGER?

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Eli Zaretskii]

> Cc: npostavs@users.sourceforge.net, 23779@debbugs.gnu.org,
>  schwab@linux-m68k.org, eggert@cs.ucla.edu
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 18 Jun 2016 00:26:28 +0300
> 
> On 06/17/2016 11:10 PM, Eli Zaretskii wrote:
> 
> > What do I do if I do want to change the original one?
> 
> That's what I was asking: are there scenarios where someone would want 
> to do that? If they're very exceptional, if might be fine if the caller 
> has to manipulate the process-environment list on a lower level themselves.

I don't think we have good means of finding that out.  In general, a
use case that was possible (and for many years at that) should remain
possible, unless there's clear and hard evidence that it's no longer
valid, something I very much doubt we have in this case.

> > Not sure what that means, or why do you ask that.
> 
> Do you know which value
> 
> (setq process-environment (cons "PAGER=" ...))
> 
> assigns to the environment variable PAGER?

An empty value, AFAIK.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Noam Postavsky]

On Fri, Jun 17, 2016 at 10:47 AM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> On 06/17/2016 05:19 PM, Eli Zaretskii wrote:
>
>>> I wonder if we should make setenv work non-destructively now.
>>
>>
>> Why should we do that?  We have initial-environment if we need the
>> original value.
>
>
> Normally, we only want to change the environment for the duration of a
> command. So, what are the downsides?

process-environment could grow without bound. Only a serious problem
if some code calls setenv many times without let-binding
process-environment which is probably a mistake, but still there could
be code like that.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Dmitry Gutov]

On 06/18/2016 04:36 AM, Noam Postavsky wrote:

> process-environment could grow without bound. Only a serious problem
> if some code calls setenv many times without let-binding
> process-environment which is probably a mistake, but still there could
> be code like that.

Yes, it's easy to see an artificial scenario leading to that. But is it 
at all probable to see one in practice?

Well, anyway, I believe I've made my case about setenv. It would be 
nice, but far less important than getting Eli's patch in.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Paul Eggert]

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

On 06/17/2016 04:19 PM, Eli Zaretskii wrote:
>> Looks like it does. Please push at your convenience.
> I'd like the patch to be eyeballed by a few more people.  Paul,
> Andreas, do you see any problems with it?  If not, I'd like to push it
> to the emacs-25 branch
The patch is correct. The code is tricky so I'm not surprised you wanted 
another pair of eyes. The attached patch is a very minor tweak of your 
patch that made it a bit easier for me to follow. I resisted the 
temptation of cleaning up the surrounding code to make it more readable.

I like the idea of putting this into the emacs-25 branch. (Or your 
patch; doesn't really matter.)

[-- Attachment #2: 0001-Consing-V-into-process-environment-should-remove-V.patch --]
[-- Type: text/x-patch, Size: 940 bytes --]

From ed5dabd6bb61613649f619f6f210d070812895d7 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 19 Jun 2016 03:58:37 +0200
Subject: [PATCH] Consing "V" into process-environment should remove V

Problem reported by Noam Postavsky (Bug#23779).
* src/callproc.c (add_env): Consider new string "FOO=BAR" to match plain
"FOO" in the old proto-environment.  This is a slight tweak of the fix
suggested by Eli Zaretskii in http://bugs.gnu.org/23779#14
---
 src/callproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/callproc.c b/src/callproc.c
index db602f5..38f3f92 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -1099,7 +1099,7 @@ add_env (char **env, char **new_env, char *string)
       char *p = *ep, *q = string;
       while (ok)
 	{
-	  if (*q != *p)
+	  if (*q != *p && *p)
 	    break;
 	  if (*q == 0)
 	    /* The string is a lone variable name; keep it for now, we
-- 
2.5.5

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Eli Zaretskii]

> Cc: npostavs@users.sourceforge.net, 23779@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 19 Jun 2016 04:27:32 +0200
> 
> On 06/17/2016 04:19 PM, Eli Zaretskii wrote:
> >> Looks like it does. Please push at your convenience.
> > I'd like the patch to be eyeballed by a few more people.  Paul,
> > Andreas, do you see any problems with it?  If not, I'd like to push it
> > to the emacs-25 branch
> The patch is correct.

Thanks for the review.  I will push the patch soon to emacs-25.

> The code is tricky so I'm not surprised you wanted another pair of
> eyes.

Indeed.

> The attached patch is a very minor tweak of your 
> patch that made it a bit easier for me to follow.

I think I will go with my version, mainly because it will most
probably be short-lived, and so is not worth optimizing.

> I resisted the temptation of cleaning up the surrounding code to
> make it more readable.

Feel free to do that on master; I can mark the emacs-25 fix "not to be
merged".

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Paul Eggert]

On 06/19/2016 05:01 PM, Eli Zaretskii wrote:
>> >I resisted the temptation of cleaning up the surrounding code to
>> >make it more readable.
> Feel free to do that on master; I can mark the emacs-25 fix "not to be
> merged".

Please don't bother. I'll merge emacs-25 to master before fiddling with 
this stuff on master, if I ever get around to fiddling. That's my usual 
practice for this sort of thing.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Eli Zaretskii]

> Cc: dgutov@yandex.ru, schwab@linux-m68k.org, npostavs@users.sourceforge.net,
>  23779@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 20 Jun 2016 00:53:21 +0200
> 
> On 06/19/2016 05:01 PM, Eli Zaretskii wrote:
> >> >I resisted the temptation of cleaning up the surrounding code to
> >> >make it more readable.
> > Feel free to do that on master; I can mark the emacs-25 fix "not to be
> > merged".
> 
> Please don't bother.

OK, thanks.  Pushed as 5f37572 on the emacs-25 branch, and closing.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Dmitry Gutov]

On 06/20/2016 05:21 PM, Eli Zaretskii wrote:

> OK, thanks.  Pushed as 5f37572 on the emacs-25 branch, and closing.

Can we fix bug#23769 along with it?

The solution using process-environment looks obviously safe to me.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 21 Jun 2016 16:53:40 +0300
> 
> On 06/20/2016 05:21 PM, Eli Zaretskii wrote:
> 
> > OK, thanks.  Pushed as 5f37572 on the emacs-25 branch, and closing.
> 
> Can we fix bug#23769 along with it?
> 
> The solution using process-environment looks obviously safe to me.

Please show a patch you have in mind.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Dmitry Gutov]

On 06/21/2016 06:21 PM, Eli Zaretskii wrote:

> Please show a patch you have in mind.

The one in http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23769#86.

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Eli Zaretskii]

> Cc: 23779@debbugs.gnu.org, npostavs@users.sourceforge.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 21 Jun 2016 18:24:38 +0300
> 
> On 06/21/2016 06:21 PM, Eli Zaretskii wrote:
> 
> > Please show a patch you have in mind.
> 
> The one in http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23769#86.

Looks OK to me.  (For some reason, I was under the impression that it
doesn't necessarily fix the problem.)

bug#23779: 25.0.95; consing "SHELLVAR" onto process-environment doesn't remove it from subprocess env

[Dmitry Gutov]

On 06/21/2016 07:12 PM, Eli Zaretskii wrote:

>> The one in http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23769#86.
>
> Looks OK to me.  (For some reason, I was under the impression that it
> doesn't necessarily fix the problem.)

It did not when it was presented. Hence the filing of this bug.