Magit should always specify --git-dir or GIT_DIR when cd-ing to a gitdir to run some Git subcommand. about magit HOT 19 CLOSED

In my situation that prompted me to report this, I have a worktree/.git symlink to the /path/to/actual.git.

Same here. I'm using repo to work on Android. Anyone using repo will hit this bug, I think, since repo consolidates Git directories under .repo/, and points each checked-out repository to it with a .git symlink.

magit-status works, and I can stage hunks, but committing within Magit fails with *ERROR*: Not inside Git repository. I have to git commit from a shell.

from magit.

The problem is that git assumes that any $GIT_EDITOR is dumb. I.e., all that the editor does is open the file and once the user indicates that they are done, it exits with status 0.

By itself emacs or (a emacsclient that connects to a running emacs instance) is dumb. It only does the things mentioned above.

However if git-commit-setup.el has been loaded in an emacs instance, then the resulting $GIT_EDITOR is no longer dumb. It now does things like showing a diff in a separate buffer, and it allows invoking any Magit command.

Until recently this worked just fine, because git could be invoked in the working tree or in the git_dir. Now that git may refuse to operate in the git_dir, it becomes a problem that the only information that the $GIT_EDITOR has access to is /path/to/.git/COMMIT_EDITMSG.

This affects every "smart" GIT_EDITOR (one that does more than let the user edit the file and then exits with 0). Because of that, I think this should be fixed in git.

A) git should be careful to set $GIT_DIR when invoking $EDITOR (and hope that anything else happening after $EDITOR is invoked preserves that envvar)

This would be one way of trying to fix it on git's end. That might work for many $GIT_EDITORs that are new processes that actually use their environment.

However, when using emacsclient, that's not how it works. emacsclient is a subprocess, but it connects to a separate process, the already running emacs. emacs would not see the GIT_DIR environment variable.

As I have already said, emacsclient does not somehow pass along its environment to the emacs server that it connects to. Theoretically it could do that, but not until Emacs 30 is releases, and Magit is going to support Emacs releases all the way back to 27 for a few more years.

B) git-commit-setup should be careful about the context it's running git commands in (e.g. this bug belongs filed against emacs itself, not against magit)

All information that "Emacs" / git-commit-setup has access to is:

Hey there EDITOR! Please make the user edit the file /path/to/COMMIT_EDITMSG.

We could look for core.worktree in the file /path/to/config and cd there first. (By setting the buffer-local default-directory to that directory.)

But if we took this approach, I fear that we might actually be by-passing the security feature that was enabled by safe.bareRepository=explicit.

from magit.

[ +cc some participants from related issues: @soppera @jonathantanmy @darkfeline ]

Any time that magit decides to cd to a gitdir and run some operation, it should provide either GIT_DIR or --git-dir.

Over at gh-5074, @tarsius has two comments that give some details about why doing that isn't straightforward in the context of Magit.

In the first comment, he also says this:

Of course when git-commit-setup calls git rev-parse --show-toplevel, it could add --git-dir=/tmp/git-repo-test/.git/, but I fear that would just reintroduce the security issue that you are trying to avoid by using safe.barerepository = explicit.

I haven't yet read up on this topic, but I agree with that concern. We of course want to make sure any solution on our end doesn't do that, so any thoughts you or others that are closer to the security issue have on that would be valuable.

Following Git's 45bb916248 (setup: allow cwd=.git w/ bareRepository=explicit, 2024-01-20), here are the spots that I'm aware of where committing with Magit will fail when safe.bareRepository=explicit:

• absorbed submodules (mentioned in gh-5074 and gh-5091)
• worktrees
• the --separated-git-dir scenario you mention here

#!/bin/sh

set -eu

tdir=$(mktemp -d "${TMPDIR:-/tmp}"/magit-5100-sub-XXXXXXX)
git init "$tdir"
cd "$tdir"

git commit --allow-empty -mc1
git init sub
git -C sub commit --allow-empty -mc1

git submodule add ./sub
git commit -m 'add sub'

git submodule absorbgitdirs
# try to commit in sub

#!/bin/sh

set -eu

tdir=$(mktemp -d "${TMPDIR:-/tmp}"/magit-5100-wtree-XXXXXXX)
cd "$tdir"

git init main
git -C main commit --allow-empty -mc1
git -C main worktree add --detach ../wtree
# try to commit in wtree

#!/bin/sh

set -eu

tdir=$(mktemp -d "${TMPDIR:-/tmp}"/magit-5100-sep-XXXXXXX)
cd "$tdir"
git init --separate-git-dir=foo.git foo
git -C foo commit --allow-empty -mc1
# try to commit in foo

I think the --separated-git-dir case is notable for the following reason. I could imagine the submodule and worktree cases being resolved by changes in Git similar to the .git exception added by 45bb916248. But even then the --separated-git-dir would remain, so we'd need to do something on our end to make that work.

Jonas, one idea I've played around with a bit is whether we can use the --separated-git-dir kludge added in 439ed76 (Record the working tree for separated git directories, 2017-02-08).

As a quick-and-dirty test (not thinking through things like symlinks or TRAMP), I can commit in all of the above cases with this change:

diff --git a/lisp/magit-git.el b/lisp/magit-git.el
index bb84b1d0..1c69de80 100644
--- a/lisp/magit-git.el
+++ b/lisp/magit-git.el
@@ -957,7 +957,8 @@ (defun magit-toplevel (&optional directory)
                 ((car (rassoc gitdir magit--separated-gitdirs)))
                 (;; Step outside the control directory to enter the
                  ;; working tree.
-                 (file-name-directory (directory-file-name gitdir))))))))))))
+                 (file-name-directory (directory-file-name gitdir)))))))
+         (car (rassoc default-directory magit--separated-gitdirs)))))))
 
 (defun magit--toplevel-safe ()
   (or (magit-toplevel)

Then there's still the problem of git-invoking commands the user may call from the $GIT_DIR file. For example, one mentioned in gh-5074 is magit-diff-while-committing, and that could be fixed by putting a magit-with-toplevel at the top of magit-commit-diff-1, but auditing and adjusting every command that might be called from a $GIT_DIR file isn't an appealing direction to go.

Anyway, at this point I'm just mentioning the current --separated-git-dir handling as something that might be worth considering as part of the solution.

from magit.

#5074 (comment) has a workaround for the .git/ case now fixed in the latest Git release. Here's a similar workaround for the remaining cases (assuming the latest Git is present for the .git/ fix):

(defun my/git-commit-setup--chdir ()
  (when-let ((wtree (car (rassoc default-directory magit--separated-gitdirs))))
    (setq default-directory wtree)))
(advice-add 'git-commit-setup :before #'my/git-commit-setup--chdir)

from magit.

I'm the original reporter on whose behalf @nasamuffin reported this issue. I'm happy to provide more details and am a long-time emacs user, so reasonably comfortable with trying things out.

The separated-gitdirs is related but slightly different from my immediate issue, which unfortunately falls under the "symlinks (or TRAMP)" category. With --separate-git-dir I end up with a .git file containing gitdir: /path/to/actual.git. In my situation that prompted me to report this, I have a worktree/.git symlink to the /path/to/actual.git. I don't know if that's a significant difference.

I'm happy to provide additional information, including the bug template information (backtraces, emacs versions, etc.) in case that's helpful. I can confirm that magit--separated-gitdirs has entries for the separated directories: specifically

(("/home/xof/studio-main/tools/adt/idea/" . "/home/xof/studio-main/.repo/projects/tools/adt/idea.git/"))

(but not the symlink /home/xof/studio-main/tools/adt/idea/.git)

from magit.

I guess I'm confused why magit is cding to the gitdir to perform a commit operation, rather than using the cwd.

And if it's able to cd to the gitdir first, then that means the gitdir is known to magit (to get there from the cwd in the first place), right? So I'm having trouble understanding which piece of info is missing.

(Alternatively, could magit stop cding to the gitdir to perform most operations? I really don't see why it would be necessary for a commit.)

from magit.

(Alternatively, could magit stop cding to the gitdir to perform most operations? I really don't see why it would be necessary for a commit.)

That would work for me. I'm not sure if this would break other special cases.

from magit.

Magit should always specify --git-dir or GIT_DIR when cd-ing to a gitdir to run some Git subcommand.

Magit is not cd-ing to a gitdir.

1. Magit calls git commit (without -m ...).
   Edit: Just in case: Magit does no cd-ing before doing that.
   Edit 2: From here on Magit isn't even involved anymore (if we ignore that the stand-alone git-commit.el is maintained inside Magit's repository). Think of it as a "monorepo".
2. git calls $EDITOR /path/to/.git/COMMIT_EDITMSG.
3. $EDITOR happens to be emacsclient ....
4. emacsclient tells the running emacs server to open /path/to/.git/COMMIT_EDITMSG.
5. emacs opens /path/to/.git/COMMIT_EDITMSG.
6. emacs calls the function git-commit-setup because the file is named COMMIT_EDITMSG.
7. git-commit-setup runs git ... inside /path/to/.git/ because the file that is being edited is located in that directory.

Technically there is no connect between (1) and (7). While git is a subprocess of emacs and emacsclient is a subprocess of git, emacs is not a subprocess of git. Also while emacsclient could forward its environment to emacs somehow, it currently does not do that.

Edit: Right, I already explained all that in the message @kyleam linked to above. Well, this part is worth repeating one more time:

Magit is NOT cd-ing to a gitdir.

It never ever does that for any git command whatsoever, including but not limited to git commit.

from magit.

Oh interesting! So it sounds like either:

A) git should be careful to set $GIT_DIR when invoking $EDITOR (and hope that anything else happening after $EDITOR is invoked preserves that envvar)
or
B) git-commit-setup should be careful about the context it's running git commands in (e.g. this bug belongs filed against emacs itself, not against magit)

Thanks, the explanation in tarsius's comment is very clear.

from magit.

I'm not sure where we are with this: is there a path forward?

One thing I did wonder about was whether the $EDITOR for a running magit session could be a wrapper around emacsclient. (Unlike emacs itself, emacsclient doesn't support a mixture of forms-for-eval and files-to-visit). Something schematically like

#! /bin/sh

emacsclient -e \(magit--edit-for-editor\ "$1"\ "$GIT_DIR"\)

though I appreciate that that's painful to get right in all circumstances across all supported platforms. (I have no expectation that I even got the quoting right, let alone anything else.)

from magit.

I'm not sure where we are with this: is there a path forward?

Currently your best option is to disable global-git-commit-mode. Disabling a global mode, which happens to be enabled by default, is tricky business. Put the following early in your init file, before anything that may load git-commit and magit (because loading the latter causes the former to be loaded).

(use-package git-commit
  :defer t
  :custom ((global-git-commit-mode . nil)))

(

Note that until a minute ago, git-commit.el's font-locking was enabled even when global-git-commit-mode was successfully turned off. That's wrong, so now the mode has to be enabled to get the font-locking. However, this part of git-commit can be used even if the rest can not, so you might want to use this instead:

(use-package git-commit
  :defer t
  :custom ((global-git-commit-mode . nil))
  :config (add-hook 'after-change-major-mode-hook
                    #'git-commit-setup-font-lock-in-buffer))

This causes git config core.commentchar to be run in the git_dir, but if that fails, that is silently ignored (but that of course means that this setting will be ignored, so you shouldn't use it or else git commit will treat all your comments as part of the message).

)

Now you won't be able to use git/Magit in that buffer, but you (optionally) get font-locking and you can actually finish writing the message.

To tell Git that you are done, you now have to first safe the buffer (as usual with C-x C-s), and tell Git about it using C-x #. (Not sure why the buffer has to be explicitly saved first. You could of course implement a command that does both these things.)

To instead abort, use M-x server-edit-abort. (I believe there is no default binding for the that, but that might also just be my setup).

If you want to use git/Magit in this buffer, while writing the current message, you have to M-x cd and manually navigate to the location of the working tree. Once you have done that, you can also enable the mode locally using M-: eval-expresson RET (git-commit-setup) RET. If you do that, you can use C-c C-c and C-c C-k again; or C-c C-d to show the diff, and a bunch of other things.

(Of course it would be easier to just switch to another buffer which already happens to be visiting a file or magit buffer, associated with the working tree; and then do all your git things there.)

The above instructions also provide some guidance on how someone not familiar with Magit/git-commit/emacsclient... (but familiar with Git and the details of safe.bareRepository=explicit), could help. Once the below function were implemented, it would only have to be modified lightly for git-commit-setup to call it, instead of the user having to do it manually.

(defun git-commit-setup-after-all ()
  (interactive)
  (when-let ((workdir (safely-determine-workdir))) ; TODO implement me
    (setq default-directory workdir)
    (add-hook 'after-change-major-mode-hook
              #'git-commit-setup-font-lock-in-buffer)
    (git-commit-setup)))

from magit.

Or, we could just use the kludge that @kyleam posted way up in #5100 (comment).

#5074 (comment) has a workaround for the .git/ case now fixed in the latest Git release. Here's a similar workaround for the remaining cases (assuming the latest Git is present for the .git/ fix):

(defun my/git-commit-setup--chdir ()
  (when-let ((wtree (car (rassoc default-directory magit--separated-gitdirs))))
    (setq default-directory wtree)))
(advice-add 'git-commit-setup :before #'my/git-commit-setup--chdir)

Given that this was just ignored so far, I think my best option is to just do it automatically and be done with this. This also would be safe. (Well it might be a good idea to think about it once more, instead of me pushing such a change right now, but it seems like a good idea. @kyleam, what do you thing?)

I have in fact considered automatically changing the default-directory in the message buffer to the working directory a few times before -- regardless of this issue. I sometimes want to quickly open a file from the repository, while I am in this buffer (using something as basic as find-file-other-window). Doing that is not convenient if the git_dir is detached.

I don't recall why I haven't done this already. Might be that I thought of some downside, or feared that while I did not see any, some users might. Or maybe I did not realize that the existing magit--separated-gitdirs could be used for this purpose, or maybe that didn't exist yet, when I last seriously considered it. In any case, there should probably be an option that controls whether this is done or not.

from magit.

(safely-determine-workdir))) ; TODO implement me

So if I understand correctly, the hope is that Git would provide a command that would let us figure out the working tree from inside $GIT_DIR regardless of the safe.barerepository=explicit setting.

Conceptually that sounds great, but I'm not too optimistic given related previous discussion around --separate-git-dir and setting core.worktree. For the --separate-git-dir kludge in 439ed76 (Record the working tree for separated git directories, 2017-02-08), we had a similar thought back then (#2981 (comment)), hoping that Git would set core.worktree when --separate-git-dir is passed (similar to what's done for submodules). Even more so when we realized that Git used to set that variable when --separate-git-dir was passed.

$ git --version
git version 2.11.0
$ git init --separate-git-dir=foo.git foo
Initialized empty Git repository in /tmp/magit-5100-j1rmvGg/foo.git/
$ grep worktree foo.git/config
######################################################################
$ git --version
git version 2.10.5
$ git init --separate-git-dir=foo.git foo
Initialized empty Git repository in /tmp/magit-5100-GMVp0xo/foo.git/
$ grep worktree foo.git/config
        worktree = /tmp/magit-5100-GMVp0xo/foo

But when I asked about the behavior change on the Git list, Duy answered that it being set by --separate-git-dir was unintentional, and that he didn't think it was safe to assume a single $GIT_DIR to working tree mapping. I think I was convinced by that at the time, but revisiting the thread now I haven't given it too much more thought.

I think my best option is to just do it automatically and be done with this. This also would be safe.

That matches my current thinking.

I played around with the patch below a bit (not sure its changes are quite what you have in mind, but, at any rate, I think it captures the core idea). And it worked for cases I tried (basically the test cases in my comment above, the test case below, and a test from TRAMP using /sudo::). As above, that assumes the latest Git for the fix for the .git/ case.

diff --git a/lisp/git-commit.el b/lisp/git-commit.el
index 40563455..87b44db2 100644
--- a/lisp/git-commit.el
+++ b/lisp/git-commit.el
@@ -334,6 +334,12 @@ (defcustom git-commit-use-local-message-ring nil
   :safe 'booleanp
   :type 'boolean)
 
+(defcustom git-commit-cd-to-toplevel t
+  "TODO"
+  :group 'git-commit
+  :safe 'booleanp
+  :type 'boolean)
+
 ;;;; Faces
 
 (defgroup git-commit-faces nil
@@ -547,6 +553,9 @@ (defvar git-commit-header-line-format nil
 
 (defun git-commit-setup ()
   (when (fboundp 'magit-toplevel)
+    (when-let ((top (and git-commit-cd-to-toplevel
+                         (magit-toplevel))))
+      (setq default-directory top))
     ;; `magit-toplevel' is autoloaded and defined in magit-git.el,
     ;; That library declares this functions without loading
     ;; magit-process.el, which defines it.
@@ -554,7 +563,8 @@ (defun git-commit-setup ()
   ;; Pretend that git-commit-mode is a major-mode,
   ;; so that directory-local settings can be used.
   (let ((default-directory
-         (or (and (not (file-exists-p ".dir-locals.el"))
+         (or (and (not git-commit-cd-to-toplevel)
+                  (not (file-exists-p ".dir-locals.el"))
                   ;; When $GIT_DIR/.dir-locals.el doesn't exist,
                   ;; fallback to $GIT_WORK_TREE/.dir-locals.el,
                   ;; because the maintainer can use the latter
diff --git a/lisp/magit-git.el b/lisp/magit-git.el
index bb84b1d0..6336d6f1 100644
--- a/lisp/magit-git.el
+++ b/lisp/magit-git.el
@@ -957,7 +957,9 @@ (defun magit-toplevel (&optional directory)
                 ((car (rassoc gitdir magit--separated-gitdirs)))
                 (;; Step outside the control directory to enter the
                  ;; working tree.
-                 (file-name-directory (directory-file-name gitdir))))))))))))
+                 (file-name-directory (directory-file-name gitdir)))))))
+         ;; TODO
+         (car (rassoc default-directory magit--separated-gitdirs)))))))
 
 (defun magit--toplevel-safe ()
   (or (magit-toplevel)

#!/bin/sh

set -eu

tdir=$(mktemp -d "${TMPDIR:-/tmp}"/magit-5100-link-XXXXXXX)
cd "$tdir"

prefix=.repo/a/b/c
mkdir -p "$prefix"
git init --separate-git-dir="$prefix/foo.git" foo
ln -sf "$tdir/$prefix/foo.git" foo/.git
git -C foo commit --allow-empty -mc1
# try to commit from foo

@csrhodes, for your link scenario you describe above, did you confirm that the git-commit-setup advice didn't work? You show that magit--separated-gitdirs has the resolved path $GIT_DIR, but that's okay as long as it matches what where Git places you when $EDITOR starts up. And if it does fail, could you tweak the above script so that it triggers the issue?

I have in fact considered automatically changing the default-directory in the message buffer to the working directory a few times before -- regardless of this issue. I sometimes want to quickly open a file from the repository, while I am in this buffer (using something as basic as find-file-other-window). Doing that is not convenient if the git_dir is detached.

Yeah, I hit into that a good amount too, and especially when using worktrees and submodules.

Might be that I thought of some downside, or feared that while I did not see any, some users might.

I don't doubt there's some lurking, but fwiw I can't think of anything at the moment.

from magit.

Sorry! The git-commit-setup advice from this comment does work for my case.

from magit.

I played around with the patch below a bit (not sure its changes are quite what you have in mind, but, at any rate, I think it captures the core idea).

I think it would be better to not modify magit-toplevel and do it all within git-commit-setup. I.e., essentially do what the advice does (modulo the .dir-locals.el complication).

Something like (completely untested):

diff --git a/lisp/git-commit.el b/lisp/git-commit.el
@@ -334,6 +334,12 @@ (defcustom git-commit-use-local-message-ring nil
   :safe 'booleanp
   :type 'boolean)
 
+(defcustom git-commit-cd-to-toplevel nil ; Make it opt-in.
+  "TODO"
+  :group 'git-commit
+  ;; :safe 'booleanp ; I wouldn't go that far.
+  :type 'boolean)
+
 ;;;; Faces
 
 (defgroup git-commit-faces nil
@@ -546,27 +552,34 @@ (defvar git-commit-header-line-format nil
   (setq git-commit-usage-message nil) ; show a shorter message")
 
 (defun git-commit-setup ()
-  (when (fboundp 'magit-toplevel)
-    ;; `magit-toplevel' is autoloaded and defined in magit-git.el,
-    ;; That library declares this functions without loading
-    ;; magit-process.el, which defines it.
-    (require 'magit-process nil t))
-  ;; Pretend that git-commit-mode is a major-mode,
-  ;; so that directory-local settings can be used.
-  (let ((default-directory
-         (or (and (not (file-exists-p ".dir-locals.el"))
-                  ;; When $GIT_DIR/.dir-locals.el doesn't exist,
-                  ;; fallback to $GIT_WORK_TREE/.dir-locals.el,
-                  ;; because the maintainer can use the latter
-                  ;; to enforce conventions, while s/he has no
-                  ;; control over the former.
-                  (fboundp 'magit-toplevel)  ; silence byte-compiler
-                  (magit-toplevel))
-             default-directory)))
-    (let ((buffer-file-name nil)         ; trick hack-dir-local-variables
-          (major-mode 'git-commit-mode)) ; trick dir-locals-collect-variables
-      (hack-dir-local-variables)
-      (hack-local-variables-apply)))
+  (let ((gitdir default-directory)
+        (cd nil))
+    (when (and (fboundp 'magit-toplevel) (boundp 'magit--separated-gitdirs))
+      ;; `magit-toplevel' is autoloaded and defined in magit-git.el.  That
+      ;; library declares this function without loading magit-process.el,
+      ;; which defines it.
+      (require 'magit-process nil t)
+      (when git-commit-cd-to-toplevel
+        (setq cd (or (car (rassoc default-directory magit--separated-gitdirs))
+                     (magit-toplevel)))))
+    ;; Pretend that git-commit-mode is a major-mode,
+    ;; so that directory-local settings can be used.
+    (let ((default-directory
+           (or (and (not (file-exists-p (expand-file-name ".dir-locals.el" gitdir)))
+                    ;; When $GIT_DIR/.dir-locals.el doesn't exist,
+                    ;; fallback to $GIT_WORK_TREE/.dir-locals.el,
+                    ;; because the maintainer can use the latter
+                    ;; to enforce conventions, while s/he has no
+                    ;; control over the former.
+                    (fboundp 'magit-toplevel)
+                    (or cd (magit-toplevel)))
+               gitdir)))
+      (let ((buffer-file-name nil)         ; trick hack-dir-local-variables
+            (major-mode 'git-commit-mode)) ; trick dir-locals-collect-variables
+        (hack-dir-local-variables)
+        (hack-local-variables-apply)))
+    (when cd
+      (setq default-directory cd)))
   (when git-commit-major-mode
     (let ((auto-mode-alist
            ;; `set-auto-mode--apply-alist' removes the remote part from

from magit.

I think it would be better to not modify magit-toplevel

Okay.

Something like (completely untested):

Fwiw I tested your patch in the four cases I posted above and was able to create a commit under safe.barerepository=explicit.

from magit.

My workaround for now is to have EDITOR point to "emacsclient.sh" that contains something like:

emacsclient -e "(progn (setenv \"GIT_DIR\" \"$GIT_DIR\") (setenv \"GIT_INDEX_FILE\" \"$GIT_INDEX_FILE\") (find-file \"$1\"))"

(but need to unset the env variables later or otherwise all the other buffers are affected)

from magit.

Thanks @kyleam. I've now tested those three cases too; still works.

I've merged this now. To enable this workaround/feature users have to (setq git-commit-cd-to-toplevel t).

from magit.

Thank you @tarsius!

By the way, above I said

I think the --separated-git-dir case is notable for the following reason. I could imagine the submodule and worktree cases being resolved by changes in Git similar to the .git exception added by 45bb916248. But even then the --separated-git-dir would remain, so we'd need to do something on our end to make that work.

The submodule and worktree commands should be resolved in Git itself with the next release: https://lore.kernel.org/git/[email protected]/T/#u

from magit.