On Sun, 1 Nov 2026, Jonas Bernoulli wrote:
> I'm adding Steve to the discussion. > > > Augusto Stoffel <[email protected]> writes: > >> On Fri, 30 Oct 2026, Jonas Bernoulli wrote: > >>>>> (declare (indent 0) (debug (body))) > >>> (declare (indent 0) (debug (body))) >>> (let ((p (gensym))) >>> `(let ((,p process-environment)) >>> (with-temp-buffer >>> (let ((process-environment ,p)) >>> ,@body))))) >> >> A.k.a. >> >> (defmacro magit--with-temp-process-buffer (&rest body) >> (declare (indent 0) (debug (body))) >> (let ((process-environment process-environment)) >> (with-temp-buffer >> ,@body))) > > Not really. > > [ I am sorry, this ended up stream-of-though style, because I am in a > hurry. A family event is coming up. So please bear with me. I will > get to a (probably sufficient) solution at the end. But the detour is > interesting too. (I think I found an Emacs bug.) ] > > `process-environment' *seems to* behave differently, so let's use > another variable: > > (defvar *var* :default) > > (with-temp-buffer > (let ((*var* :outer)) > (with-temp-buffer > *var*))) => :outer (since not buffer-local) > > (with-temp-buffer > (setq-local *var* :outer) > (with-temp-buffer > *var*)) => :default (as anyone would expect) > > (with-temp-buffer > (make-local-variable '*var*) > (let ((*var* :outer)) > (with-temp-buffer > *var*))) => :default (surprising when first encountered, but ...) > > makes sense and documented in (info "(elisp)Intro to Buffer-Local"): > > *Warning:* When a variable has buffer-local bindings in one or more > buffers, ‘let’ rebinds the binding that’s currently in effect. For > instance, if the current buffer has a buffer-local value, ‘let’ > temporarily rebinds that. If no buffer-local bindings are in effect, > ‘let’ rebinds the default value. If inside the ‘let’ you then change to > a different current buffer in which a different binding is in effect, > you won’t see the ‘let’ binding any more. And if you exit the ‘let’ > while still in the other buffer, you won’t see the unbinding occur > (though it will occur properly). Here is an example to illustrate: Oh my, you're right. I get surprised by this every time I come across it (which is not the first). > That is what I try to counter with: > > (defmacro with-temp-buffer-transplant-local-*var* (&rest body) > `(let ((-var- *var*)) > (with-temp-buffer > (let ((*var* -var-)) > *var*)))) > > (with-temp-buffer > (make-local-variable '*var*) > (let ((*var* :outer)) > (with-temp-buffer-transplant-local-*var* > *var*))) => :outer (as needed in this case) > > But `process-environment' *seems to* behave differently: > > (setenv "FOO" ":default") > > (with-temp-buffer > (let ((process-environment (copy-sequence process-environment))) > (setenv "FOO" ":outer") > (with-temp-buffer > (getenv "FOO")))) => :outer (since not buffer-local) > > (with-temp-buffer > (setq-local process-environment (copy-sequence process-environment)) > (setenv "FOO" ":outer") > (with-temp-buffer > (getenv "FOO"))) => :outer (why?) > > The reason is that `setenv' modifies the strings in > `process-environment'. This seems questionable. It means that > > (let ((process-environment (copy-sequence process-environment))) > (setenv ...) > ...) setenv also says As a special case, setting variable `TZ' calls `set-time-zone-rule' as a side-effect. so I'm not quite sure what's the purpose of that function, other than interactive use or init. But I wouldn't use it in Lisp. I suggest to just cons the new variables on top of process-environment. > might end up changing the outer `process-environment'. The fix is > to use > > (let ((process-environment (copy-tree process-environment))) > (setenv ...) ;^^^^ > ...) > > instead. I haven't checked whether that is documented anywhere, but > even the official macro for scoped envvar bindings fails to do this, > so it must be a common bug: > > (defmacro with-environment-variables (variables &rest body) > "Set VARIABLES in the environment and execute BODY. > VARIABLES is a list of variable settings of the form (VAR VALUE), > where VAR is the name of the variable (a string) and VALUE > is its value (also a string). > > The previous values will be restored upon exit." > (declare (indent 1) (debug (sexp body))) > (unless (consp variables) > (error "Invalid VARIABLES: %s" variables)) > `(let ((process-environment (copy-sequence process-environment))) > ,@(mapcar (lambda (elem) > `(setenv ,(car elem) ,(cadr elem))) > variables) > ,@body)) > > I'll switch to using `copy-tree' myself. But I think that's something > that needs to be fixed in Emacs. > >> But this changes Tramp's behavior in that if you do >> >> (setq-local process-environment >> (cons "SECRETIVE_SECRET=123" process-environment)) >> >> then `make-process' will not propagate SECRETIVE_SECRET to the remote >> machine, while (magit--with-temp-process-buffer (make-process ...)) will >> propagate it. > > Indeed, and that's of course a big issue. > >> If your goal is integrating with packages that set process-environment >> locally (thanks for that!) > > "Locally" here means "using a let-binding", right? (As opposed to > "using setq-local".) No, I meant buffer-locally. I'm talking about packages like envrc or buffer-env. > Well, the goal was to fix "Stashes are not saved when using envrc" > (https://github.com/magit/magit/issues/4160). Steve, author of envrc.el > proposed this macro. I am unsure how that package used to "set it > locally" back then. Now doing so it involves: > > ;;; Propagate local environment to commands that use temp buffers > > (defun envrc-propagate-environment (orig &rest args) > "Advice function to wrap a command ORIG and make it use our local env. > This can be used to force compliance where ORIG starts processes > in a temp buffer. ARGS is as for ORIG." > (if envrc-mode > (inheritenv (apply orig args)) > (apply orig args))) > > (defun envrc-propagate-tramp-environment (buf) > "Advice function to propagate tramp vars into local values in BUF. > `tramp-remote-path' and `tramp-remote-process-environment' are propagated." > (when envrc-mode > (let ((cur-path envrc--remote-path) > (cur-env tramp-remote-process-environment)) > (with-current-buffer buf > (setq-local tramp-remote-process-environment cur-env) > (setq-local envrc--remote-path cur-path)))) > buf) > > (defun envrc-get-remote-path (fn vec) > "Advice function to wrap FN (`tramp-get-remote-path') with its argument > VEC. > Shortcuts tramp caching direnv sets the variable `exec-path'." > (with-current-buffer (tramp-get-connection-buffer vec) > (or envrc--remote-path > (apply fn vec nil)))) > > (advice-add 'async-shell-command :around #'envrc-propagate-environment) > (advice-add 'shell-command :around #'envrc-propagate-environment) > (advice-add 'shell-command-to-string :around #'envrc-propagate-environment) > (advice-add 'dired-shell-command :around #'envrc-propagate-environment) > (advice-add 'org-babel-eval :around #'envrc-propagate-environment) > (advice-add 'org-export-file :around #'envrc-propagate-environment) > (advice-add 'vc-do-command :around #'envrc-propagate-environment) > (advice-add 'vc-call-backend :around #'envrc-propagate-environment) > (advice-add 'vc-dir :around #'envrc-propagate-environment) > (advice-add 'tramp-get-connection-buffer :filter-return > #'envrc-propagate-tramp-environment) > (advice-add 'tramp-get-remote-path :around #'envrc-get-remote-path) > > I do not know whether > 1. that is the full story > 2. this makes the kludge in Magit unnecessary (at least for this package) > 3. that by itself causes secret leakage. > > I do know that I want > A. Magit to not cause leakage > B. Magit to not lose data because $GIT_INDEX_FILE is unset > C. create a Magit release today, because I already skipped last months > monthly release and this also holds up monthly releases of other > packages > > So I think what I will do is > I. make magit--with-temp-process-buffer an alias for with-temp-file, > for the time being > II. add a safety-net > > (defmacro magit-with-temp-index (tree arg &rest body) > (declare (indent 2) (debug (form form body))) > (let ((file (gensym "file"))) > `(let ((magit--refresh-cache nil) > (,file (magit-convert-filename-for-git > (make-temp-name > (expand-file-name "index.magit." (magit-gitdir)))))) > (unwind-protect > (magit-with-toplevel > (when-let* ((tree ,tree) > (_(not (magit-git-success > "read-tree" ,arg tree > (concat "--index-output=" ,file))))) > (error "Cannot read tree %s" tree)) > + (with-environment-variables (("GIT_INDEX_FILE" ,file)) > + (unless (equal (magit-git-string > + "-c" "alias.GIF=!echo \"$GIT_INDEX_FILE\"" > "GIF") > + ,file) > + (error "BUG: $GIT_INDEX_FILE ends up unset"))) > (with-environment-variables (("GIT_INDEX_FILE" ,file)) > ,@body)) > (ignore-errors > (delete-file (concat (file-remote-p default-directory) > ,file))))))) > > III. maybe (conceptually) replace (copy-sequence process-environment) > with (copy-tree process-environment) > >> while at the same time being compatible with >> Tramp's trickery around environment variables >> (cf. tramp-local-environment-variable-p), then the right thing to do is >> to call buffer-local-toplevel-value in the original buffer and then >> set-buffer-local-toplevel-value in the temp buffer. > > I think I now understand what you are getting at, but this accomplishes > the opposite of what we want. > > We must ensure that the let-binding in the temporary buffer only binds > the value that are let-bound in the previous buffer. What you propose > ensures that we only let-bind the values that are *not* let-bound in > the previous buffer. Hum, that's confusing. Of course I'm suggesting to only let-bind in the temp buffer what was originally let-bound. That's what buffer-local-toplevel-value and set-buffer-local-toplevel-value allow you to do... > But if we actually do the same in magit--with-temp-process-buffer as > is being done in tramp-sh-handle-make-process, then we > (unsurprisingly) get the behavior we want: > > (with-temp-buffer ;; I.e., in some magit buffer. > ;; Third-party makes process-environment local > (make-local-variable 'process-environment) > ;; Avoid Emacs bug. > (setq process-environment (copy-tree process-environment)) > ;; Forth-party sets a secret > (setenv "SECRET" "12345") > ;; We bind an envvar > (let ((process-environment (cons "GIT_INDEX_FILE=temp" > process-environment))) > (list > ;; Using the naive approach, both value are missing. > (with-temp-buffer > (list (getenv "SECRET") > (getenv "GIT_INDEX_FILE"))) > ;; Using Magit's old approach, both value are visible. > (let ((p process-environment)) > (with-temp-buffer > (let ((process-environment p)) > (list (getenv "SECRET") > (getenv "GIT_INDEX_FILE"))))) > ;; Using proposed approch, we hide value we don't want > ;; to hide and, don't hide value we want to hide. > (let ((p (or (ignore-error void-variable > (buffer-local-toplevel-value 'process-environment)) > (default-toplevel-value 'process-environment)))) > (with-temp-buffer > (let ((process-environment p)) > (list (getenv "SECRET") > (getenv "GIT_INDEX_FILE"))))) > > ;;; ^ BAD > ;;; v GOOD > > ;; So do the oppsite. I.e., REALLY do the same thing as Tramp. > (list (cons :orig-bindings (*tramp-env-diff*)) > (let ((p (*tramp-env-diff*))) > (with-temp-buffer > (let ((process-environment p)) > (cons :copied-bindings (*tramp-env-diff*))))))))) > > (defun *tramp-env-diff* () > ;; Copied from tramp-handle-make-process. > (let (env) > (dolist (elt process-environment env) > (when (and > (string-search "=" elt) > (not (tramp-local-environment-variable-p elt))) > (setq env (cons elt env)))))) > > So, instead of a placeholder alias as I said above, I might actually > go with > > (defmacro magit--with-temp-process-buffer (&rest body) > (let ((p (gensym))) > `(let ((,p (seq-filter > (lambda (elt) > (and (string-search "=" elt) > (not (tramp-local-environment-variable-p elt)))) > process-environment))) > (with-temp-buffer > (let ((process-environment ,p)) > ,@body))))) > > Unfortunately I now have to run. > > Cheers, > Jonas Okay, I actually have a suggestion: (defmacro magit--with-temp-process-buffer (&rest body) (declare (indent 0) (debug (body))) (cl-letf* (((default-value 'process-environment) process-environment)) (with-temp-buffer ,@body))) what this does is ensuring that: 1. without Tramp involved, processes in the temp buffer are started with the same environment as the original buffer; 2. with Tramp involved, only new let-bindings made inside BODY are propagated to processes started inside BODY. [ PS: Perhaps (with-temp-buffer (make-process ...)) is in general a pattern to be avoided. There's always the alternative of calling (make-process :buffer (generate-new-buffer " temp") ...) directly in the "original" buffer (the one holding the desired environment) and then killing the process buffer in its own sentinel function... ]
