Magit commit fails with `safe.barerepository=explicit`. about magit HOT 13 CLOSED

FYI there is a proposed Git patch to fix this issue: https://lore.kernel.org/git/[email protected]/T/#t.

from magit.

I tested with a recent version of Git included the fix and I can confirm I don't have the issue anymore.

from magit.

(I am a contributor to Git.) One potential way to solve this is to add --git-dir=/tmp/git-repo-test/.git/ as a CLI argument when Magit invokes Git here, as described in the documentation for git-config under safe.bareRepository.

from magit.

A possible work around:

(defun git-commit-setup-updir ()
  ;; This assumes that the the working tree belonging to the current
  ;; directory, is its parent directory.
  (setq default-directory (file-name-parent-directory default-directory)))
(advice-add 'git-commit-setup :before 'git-commit-setup-updir)

It works, but the assumption mentioned in the comment above, is a big caveat.

I don't think that explicitly passing --git-dir=/tmp/git-repo-test/.git/ to some git command is possible here. This is how committing works in Magit:

1. Magit calls EDITOR=emacsclient git commit.
2. Git invokes EDITOR.
3. emacsclient contacts the Emacs server, asking it to edit /tmp/git-repo-test/.git/COMMIT_EDITMSG.
4. Emacs opens that file and because of some configuration that was added when git-commit.el was first loaded, it knows that it also has to call git-commit-setup in the buffer visiting the opened file.
5. git-commit-setup knows nothing about the above, and has to figure it all out on its own. It knows in what directory it is being called and that is the git directory of some repository in which a commit is being created. All it really knows is that directory, everything else it has to deduce from that.

But because safe.barerepository = explicit it's not allowed to do that. Game over.

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.

The above kludge works by stepping out of the git directory, into the working tree; which of course only works if that's where it is located.

from magit.

@jonathantanmy

(I am a contributor to Git.)

Determining the top-level directory is a lot more involved in Magit than just calling git rev-parse --show-toplevel. Adding -git-dir= there would complicate matters a lot if we concluded that doing so was safe. Also, because the function that does that, magit-toplevel, is called from many other places we would have to make sure that really only gets added in this one case. It would get really ugly and potentially fragile.

But that reminds me of the primary reason that function is so complicated. git rev-parse --show-toplevel et al. insist on following symlinks. We don't want that when the user has configured Emacs to not follow symlinks. It would be very convenient, if there was a --no-follow-links argument that could be used together with these arguments.

Because that does not exist, Magit can end up having to call git rev-parse multiple times to determine the unresolved location of the working tree or git directory (which is a performance issue, at least on Windows). And in some edge-case it has to use heuristics and in the hairier edge-case, it cannot avoid following symlinks at all.

Do you think something like --no-follow-links could be added to Git?

from magit.

git-commit-setup knows nothing about the above, and has to figure it all out on its own. It knows in what directory it is being called and that is the git directory of some repository in which a commit is being created. All it really knows is that directory, everything else it has to deduce from that.

Would it work to only go up one directory if the current directory's name is .git?

Something like:

(defun git-commit-setup-updir ()
  (when (string= (file-name-nondirectory (directory-file-name default-directory))
                 ".git")
    (setq default-directory
          (file-name-parent-directory default-directory))))

As far as I know we can't have a .git inside the working directory of a Git repository (bare or not bare) so the only case where the current directory is .git is for a repository with a working tree.

Of course Git could do the same and not fail in that case as well. Maybe Git should be fixed.

from magit.

It works, but the assumption mentioned in the comment above, is a big caveat.

Correction, this allows the creation of a commit, but the "changes to be committed" diff is always empty. Apparently that code somehow depends on default-directory being inside the git directory.

Would it work to only go up one directory if the current directory's name is .git?

Maybe. There are a lot of special cases we have to deal with and they interact in strange ways, and I don't presently have them all in my head. It might work for you, but it might break things for others.

I recommend you use an advice for now, and keep refining it as you run into issues.

from magit.

Correction, this allows the creation of a commit, but the "changes to be committed" diff is always empty. Apparently that code somehow depends on default-directory being inside the git directory.

I am not sure to follow; with:

(defun git-commit-setup-updir ()
  (when (string= (file-name-nondirectory (directory-file-name default-directory))
                 ".git")
    (setq default-directory
          (file-name-parent-directory default-directory))))

(advice-add 'git-commit-setup :before 'git-commit-setup-updir)

I do see:

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# On branch main
#
# Initial commit
#
# Changes to be committed:
#	new file:   f
#

in the message buffer and:

Staged changes
1 file changed, 0 insertions(+), 0 deletions(-)
f | 0 

new file   f

[back]

in the magit-diff: git-repo-test buffer.

Am I missing something here?

from magit.

Correction, this allows the creation of a commit, but the "changes to be committed" diff is always empty. Apparently that code somehow depends on default-directory being inside the git directory.

Correction, again. As you noticed, it does work. However there are special cases where it does not work. For a submodule, the kludge would move from /outer/repo/.git/modules/amodule to /outer/repo/.git/modules, which would obviously not do what we want.

from magit.

For a submodule, the kludge would move from /outer/repo/.git/modules/amodule to /outer/repo/.git/modules, which would obviously not do what we want.

Indeed my "fix" does not deal with sub-modules at all as then the parent directory is not .git.

from magit.

This has been committed: git/git@e77b796.

I have not yet had a chance to test it though.

from magit.

To fix the submodule case, perhaps we should only check whether the working directory contains any directory named .git? That is the patch that has been proposed on the Git mailing list https://lore.kernel.org/git/[email protected]/T/#m9fcbcc499280bf4f01e2d477261c0e06050061e1

from magit.

I am rather busy with other things and intend to wait and see how this plays out on their end.

from magit.