bug#23769: 25.0.95; Mode Line breakage in vc-git

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

I am seeing a recurrent problem with vc, on Emacs-25. I am running emacs
via cask, which is running a test set.

The error looks like this:


  vc-call-backend(Git mode-line-string "/home/phillord/emacs/lentic/de
  vc-mode-line("/home/phillord/emacs/lentic/dev-resources/chunk-commen
  vc-refresh-state()
  run-hooks(find-file-hook)
  after-find-file(nil t)
  find-file-noselect-1(#<buffer chunk-comment.clj> "~/emacs/lentic/dev
  find-file-noselect("/home/phillord/emacs/lentic/dev-resources/chunk-
  (setq this (find-file-noselect filename))
  (set-buffer (setq this (find-file-noselect filename)))
  (save-current-buffer (set-buffer (setq this (find-file-noselect file

Initially, I only saw it when using Emacs in my git commit-hook -- the
same "make" command in the shell caused no problems. However, it is
freely reproducible by launching with "GIT_DIR=.git make -k" which
suggests this is why it is failing during the git commit.

I've tried "instrumenting" (i.e. putting lots of logging) into vc.
The immediate cause of the error appears to be in vc-git--call when it
runs the git command "symbolic-ref".

vc-git--call: about to apply: (git nil (t nil) nil symbolic-ref (HEAD))
vc-git--call: return 128

If GIT_DIR is not set this has a zero return value.

I haven't worked out yet, how to find what the error is (128 is "any
other error").

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Glenn Morris]

AFAIK Emacs doesn't support GIT_DIR, see eg https://debbugs.gnu.org/5344 .

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

Glenn Morris <rgm@gnu.org> writes:

> AFAIK Emacs doesn't support GIT_DIR, see eg https://debbugs.gnu.org/5344 .

Glenn

Thanks for the pointer.

The curious thing in this case is that GIT_DIR is not saying anything at
all, other than the default. Emacs should not error under such
circumstances. In my specific case, even a silent failure would be
acceptable (cause I am using Emacs in batch -- I can't even see the mode
line!).

There's a patch attached that Bug report which never seems to have been
applied.

Phil

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/14/2016 08:03 PM, Phillip Lord wrote:

>> AFAIK Emacs doesn't support GIT_DIR, see eg https://debbugs.gnu.org/5344 .

> There's a patch attached that Bug report which never seems to have been
> applied.

Have you tried it? Does it fix your problem?

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

On Tue, June 14, 2016 10:52 pm, Dmitry Gutov wrote:
> On 06/14/2016 08:03 PM, Phillip Lord wrote:
>
>
>>> AFAIK Emacs doesn't support GIT_DIR, see eg
>>> https://debbugs.gnu.org/5344 .
>>>
>
>> There's a patch attached that Bug report which never seems to have been
>>  applied.
>
> Have you tried it? Does it fix your problem?

Don't think it will, no. That patch is worth considering anyway,
irrespective of this bug.

Think I have a solution to this one though -- will try tomorrow.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 06/14/2016 08:03 PM, Phillip Lord wrote:
>
>>> AFAIK Emacs doesn't support GIT_DIR, see eg https://debbugs.gnu.org/5344 .
>
>> There's a patch attached that Bug report which never seems to have been
>> applied.
>
> Have you tried it? Does it fix your problem?


The following patch addresses this bug.

John/Eli would you want this for Emacs-25.


I've noticed from instrumenting vc-git that it's not just the
symbolic-ref command that returns 128, but several others. So, it might
be that the best long term solution would be to make change vc-git--call
to set the default directory to the root, which would make GIT_DIR=.git
always correct.

At the moment, though, I've noticed some git commands are called with
simple file names, and some full, so this would probably require more
changes. No sense in making this sort of change for Emacs-25 now.




[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-error-from-vc-git-when-GIT_DIR-is-set.patch --]
[-- Type: text/x-diff, Size: 3577 bytes --]

From 0419734fd39f8cd67472c0294295ef1508e39d85 Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Wed, 15 Jun 2016 17:36:42 +0100
Subject: [PATCH] Fix error from vc-git when GIT_DIR is set

* lisp/vc/vc-git.el (vc-git-working-revision): Set default-directory to
  repository root to prevent an error when GIT_DIR is relative to it.
* test/automated/vc-git-tests.el: New file.

Addresses Bug#23769
---
 lisp/vc/vc-git.el              |  8 ++++--
 test/automated/vc-git-tests.el | 60 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 2 deletions(-)
 create mode 100644 test/automated/vc-git-tests.el

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index f35c84d..ea26a7d 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -260,9 +260,13 @@ vc-git-state
             (vc-git--state-code diff-letter)))
       (if (vc-git--empty-db-p) 'added 'up-to-date))))
 
-(defun vc-git-working-revision (_file)
+(defun vc-git-working-revision (file)
   "Git-specific version of `vc-working-revision'."
-  (let (process-file-side-effects)
+  ;; Call this in the root directory, or Emacs may error when setting
+  ;; the modeline when called from a git pre-commit hook, which sets
+  ;; GIT_DIR. (Bug #23769)
+  (let ((default-directory (vc-git-root file))
+        (process-file-side-effects))
     (vc-git--rev-parse "HEAD")))
 
 (defun vc-git--symbolic-ref (file)
diff --git a/test/automated/vc-git-tests.el b/test/automated/vc-git-tests.el
new file mode 100644
index 0000000..8001bcb
--- /dev/null
+++ b/test/automated/vc-git-tests.el
@@ -0,0 +1,60 @@
+;;; vc-git-tests.el --- tests for vc/vc-git.el
+
+;; Copyright (C) 2016 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'vc-git)
+
+;; This will break when merged from the emacs-25 branch to master,
+;; because of the change of directory structure. The actual file is
+;; not important at all, so long as it is checked into git.
+(defvar vc-git-tests-file
+  (concat
+   (expand-file-name
+    "data/xref/"
+    (file-name-directory (or load-file-name (buffer-file-name))))
+   "file1.txt"))
+
+
+(ert-deftest open-this-file ()
+  "Test for Bug #23769."
+  (should
+   (let ((git-dir (getenv "GIT_DIR")))
+     (unwind-protect
+         (progn
+           (setenv "GIT_DIR" nil)
+           (find-file vc-git-tests-file))
+       (when (get-file-buffer vc-git-tests-file)
+         (setenv "GIT_DIR" git-dir)
+         (kill-buffer (get-file-buffer vc-git-tests-file))))))
+  (should
+   (let ((git-dir (getenv "GIT_DIR")))
+     (unwind-protect
+         (progn
+           (setenv "GIT_DIR" ".git")
+           (find-file vc-git-tests-file))
+       (when (get-file-buffer vc-git-tests-file)
+         (setenv "GIT_DIR" git-dir)
+         (kill-buffer (get-file-buffer vc-git-tests-file)))))))
+
+
+;;; vc-git-tests.el ends here
-- 
2.8.4

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/15/2016 11:48 PM, Phillip Lord wrote:

> I've noticed from instrumenting vc-git that it's not just the
> symbolic-ref command that returns 128, but several others. So, it might
> be that the best long term solution would be to make change vc-git--call
> to set the default directory to the root, which would make GIT_DIR=.git
> always correct.

The patch looks wrong. Why does the problem script set GIT_DIR to '.git'?

In all examples I've found, this variable is set to an absolute value. 
In general, its purpose, it seems, is to point to the '.git' directory 
when it's named otherwise and/or is situated somewhere outside of the 
current directory tree.

The latter situation will break vc-git-root. As such, the submitted 
patch is only likely to work in the tautological case you've descried. 
And it will add some performance penalty to each call, because 
vc-git-root, though usually fast, is not free.

> At the moment, though, I've noticed some git commands are called with
> simple file names, and some full, so this would probably require more
> changes.

Indeed.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 06/15/2016 11:48 PM, Phillip Lord wrote:
>
>> I've noticed from instrumenting vc-git that it's not just the
>> symbolic-ref command that returns 128, but several others. So, it might
>> be that the best long term solution would be to make change vc-git--call
>> to set the default directory to the root, which would make GIT_DIR=.git
>> always correct.
>
> The patch looks wrong. Why does the problem script set GIT_DIR to '.git'?

Try it on a pre-commit hook. CWD is set to the root, and GIT_DIR is set
to .git. Other people have found this:

http://longair.net/blog/2011/04/09/missing-git-hooks-documentation/

AFAICT, it's not actually documented by git. Or it is but the
documentation is incomprehensible (hey, it's git, it could happen!).


> In all examples I've found, this variable is set to an absolute value. In
> general, its purpose, it seems, is to point to the '.git' directory when it's
> named otherwise and/or is situated somewhere outside of the current directory
> tree.
>
> The latter situation will break vc-git-root.

That situation does indeed happen. For example, I normally checkout
emacs into worktrees where you get this when commiting on emacs-25
branch which is a worktree off master.

/home/phillord/src/git/emacs-git/master/.git/worktrees/emacs-25

But in this case, I think vc-git-root will still work.

(defun vc-git-root (file)
  (or (vc-file-getprop file 'git-root)
      (vc-file-setprop file 'git-root (vc-find-root file ".git"))))

We still find ".git" because it is a *file* (not a directory) at the top
level of a worktree.

> As such, the submitted patch is only likely to work in the
> tautological case you've descried.

It's definately a risk. Git does many things, and people use it in many
ways.


> And it will add some performance penalty to each call, because
> vc-git-root, though usually fast, is not free.

Not free, but it is a constant time look up after the first. 


There is a simpler option. I am trying to solve the root cause of the
problem but, as you say, that might be fraught. Since the problem only
seems to cause an error with vc-git-mode-line-string, we could just
discard the error from vc-git-working-revision in this case.

Failing that, now I know what the problem is, at least I have a
workaround (unset GIT_DIR in the pre-commit hook).


Phil

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Noam Postavsky]

On Wed, Jun 15, 2016 at 6:09 PM, Phillip Lord
<phillip.lord@russet.org.uk> wrote:
>
> Failing that, now I know what the problem is, at least I have a
> workaround (unset GIT_DIR in the pre-commit hook).

Apparently this "_the_ right way to write hooks that touch the files
in the work tree."

http://permalink.gmane.org/gmane.comp.version-control.git/136276

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/16/2016 01:27 AM, Noam Postavsky wrote:

> Apparently this "_the_ right way to write hooks that touch the files
> in the work tree."
>
> http://permalink.gmane.org/gmane.comp.version-control.git/136276

Is it the same behavior we're talking about?

The thread says that GIT_DIR is set to '.', not to '.git'. I doubt the 
former setting would mesh well with binding default-directory to 
repository's root like the patch proposes.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 06/16/2016 01:27 AM, Noam Postavsky wrote:
>
>> Apparently this "_the_ right way to write hooks that touch the files
>> in the work tree."
>>
>> http://permalink.gmane.org/gmane.comp.version-control.git/136276
>
> Is it the same behavior we're talking about?
>
> The thread says that GIT_DIR is set to '.', not to '.git'. I doubt the former
> setting would mesh well with binding default-directory to repository's root
> like the patch proposes.

According to this:

http://longair.net/blog/2011/04/09/missing-git-hooks-documentation/

that's for the post-recieve hook. In this case CWD is the git directory.
The logic of this is (I guess) that it works also in a bare repo.
pre-commit can only ever run in a non-bare repo since you cannot commit
if you don't have a worktree.

I suspect emacs will not be affected by this because if anyone did
run emacs in post-recieve they would only touch files *inside* the git
repo which Emacs would identify as not version-controlled.

But, again, it illustrates the point that GIT_DIR can (and is often) set
to a relative directory.

Phil

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> On Wed, Jun 15, 2016 at 6:09 PM, Phillip Lord
> <phillip.lord@russet.org.uk> wrote:
>>
>> Failing that, now I know what the problem is, at least I have a
>> workaround (unset GIT_DIR in the pre-commit hook).
>
> Apparently this "_the_ right way to write hooks that touch the files
> in the work tree."
>
> http://permalink.gmane.org/gmane.comp.version-control.git/136276

I agree that this will work. But it worth reading the justification --
"in earlier versions of git" -- so that will be git 1.6 latest, as the
post was in 2010.

So, I don't think that Emacs should *require* this to be the case. git
doesn't appear to any more.

Phil

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/16/2016 01:09 AM, Phillip Lord wrote:

> Try it on a pre-commit hook. CWD is set to the root, and GIT_DIR is set
> to .git. Other people have found this:
>
> http://longair.net/blog/2011/04/09/missing-git-hooks-documentation/

OK, so hooks set it themselves.

> AFAICT, it's not actually documented by git. Or it is but the
> documentation is incomprehensible (hey, it's git, it could happen!).

Yup.

> That situation does indeed happen. For example, I normally checkout
> emacs into worktrees where you get this when commiting on emacs-25
> branch which is a worktree off master.

Does git-worktree use GIT_DIR in some way?

> There is a simpler option. I am trying to solve the root cause of the
> problem but, as you say, that might be fraught. Since the problem only
> seems to cause an error with vc-git-mode-line-string, we could just
> discard the error from vc-git-working-revision in this case.

That sounds like a last-resort option.

> Failing that, now I know what the problem is, at least I have a
> workaround (unset GIT_DIR in the pre-commit hook).

We could also unset GIT_DIR locally inside vc-git--call. At least when 
it's set to a known value such as '.git'.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 06/16/2016 01:09 AM, Phillip Lord wrote:
>> That situation does indeed happen. For example, I normally checkout
>> emacs into worktrees where you get this when commiting on emacs-25
>> branch which is a worktree off master.
>
> Does git-worktree use GIT_DIR in some way?

Well, it uses the directory that GIT_DIR points to (so
master/.git/worktrees/emacs-25 contains COMMIT_MSG, HEAD, index and so
on).

Whether is uses GIT_DIR, who knows. I pressume that all git commands
obey GIT_DIR.


>> There is a simpler option. I am trying to solve the root cause of the
>> problem but, as you say, that might be fraught. Since the problem only
>> seems to cause an error with vc-git-mode-line-string, we could just
>> discard the error from vc-git-working-revision in this case.
>
> That sounds like a last-resort option.

It's certainly true that it would nice to fix it elsewhere, but I am
unconvinced that setting the mode-line should ever result in an error as
a normal part of it's operation.


>> Failing that, now I know what the problem is, at least I have a
>> workaround (unset GIT_DIR in the pre-commit hook).
>
> We could also unset GIT_DIR locally inside vc-git--call. At least when
> it's set to a known value such as '.git'.

Yep; I guess, we know exactly what vc-git--call does with git, and if it
never depends on GIT_DIR that should work. Although setting and
unsetting GIT_DIR seems a bit of a pain.

Personally, I prefer my first option of calling git consistently --
always with CWD equal to root, and always with file paths relative to
the root. This way, GIT_DIR should not matter.

I presume the vc.el facade allows this, but if so, as this is going onto
master now, there is time.

Phil

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/16/2016 10:41 AM, Phillip Lord wrote:

> Well, it uses the directory that GIT_DIR points to (so
> master/.git/worktrees/emacs-25 contains COMMIT_MSG, HEAD, index and so
> on).
>
> Whether is uses GIT_DIR, who knows. I pressume that all git commands
> obey GIT_DIR.

Well, maybe it's used internally. It's of no concern to us here.

> It's certainly true that it would nice to fix it elsewhere, but I am
> unconvinced that setting the mode-line should ever result in an error as
> a normal part of it's operation.

It shouldn't. But it should let the user know that something's going 
wrong, so they can file a bug report.

> Yep; I guess, we know exactly what vc-git--call does with git, and if it
> never depends on GIT_DIR that should work. Although setting and
> unsetting GIT_DIR seems a bit of a pain.

How come? It'll be about as short as your current patch, if not shorter.

> Personally, I prefer my first option of calling git consistently --
> always with CWD equal to root, and always with file paths relative to
> the root. This way, GIT_DIR should not matter.

Let's not be hasty about it. If new arguments arise in favor of this, we 
can do it, but so far unsetting GIT_DIR seems like the cheapest option. 
Like this:

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index f35c84d..4e495a5 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1450,7 +1450,9 @@ 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 (cons
+                              "GIT_DIR="
+                              (cons "PAGER=" process-environment))))
      (apply 'process-file vc-git-program nil buffer nil command args)))

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

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/16/2016 02:25 PM, Dmitry Gutov wrote:
> If new arguments arise in favor of this, we
> can do it, but so far unsetting GIT_DIR seems like the cheapest option.
> Like this:

Actually, this is better. The previous one broke vc-git. :)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index f35c84d..2b827a3 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1451,6 +1451,7 @@ vc-git--call
  	(coding-system-for-write
           (or coding-system-for-write vc-git-commits-coding-system))
  	(process-environment (cons "PAGER=" process-environment)))
+    (setenv "GIT_DIR" nil)
      (apply 'process-file vc-git-program nil buffer nil command args)))

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

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Noam Postavsky]

On Thu, Jun 16, 2016 at 12:06 PM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> Actually, this is better. The previous one broke vc-git. :)
>
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index f35c84d..2b827a3 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1451,6 +1451,7 @@ vc-git--call
>         (coding-system-for-write
>           (or coding-system-for-write vc-git-commits-coding-system))
>         (process-environment (cons "PAGER=" process-environment)))
> +    (setenv "GIT_DIR" nil)
>

I think you want (cons "GIT_DIR" ...) [without "="], using setenv can
modify the process-environment list by side-effect.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/16/2016 07:54 PM, Noam Postavsky wrote:

> I think you want (cons "GIT_DIR" ...) [without "="], using setenv can
> modify the process-environment list by side-effect.

You're probably right.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> On Thu, Jun 16, 2016 at 12:06 PM, Dmitry Gutov <dgutov@yandex.ru> wrote:
>> Actually, this is better. The previous one broke vc-git. :)
>>
>> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
>> index f35c84d..2b827a3 100644
>> --- a/lisp/vc/vc-git.el
>> +++ b/lisp/vc/vc-git.el
>> @@ -1451,6 +1451,7 @@ vc-git--call
>>         (coding-system-for-write
>>           (or coding-system-for-write vc-git-commits-coding-system))
>>         (process-environment (cons "PAGER=" process-environment)))
>> +    (setenv "GIT_DIR" nil)
>>
>
> I think you want (cons "GIT_DIR" ...) [without "="], using setenv can
> modify the process-environment list by side-effect.

Confused. Can you right a full patch?

Phil

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/17/2016 12:47 AM, Phillip Lord wrote:

> Confused. Can you right a full patch?

Here's a version:

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index f35c84d..74004d9 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1451,6 +1451,7 @@ vc-git--call
  	(coding-system-for-write
           (or coding-system-for-write vc-git-commits-coding-system))
  	(process-environment (cons "PAGER=" process-environment)))
+    (push "GIT_DIR" process-environment)
      (apply 'process-file vc-git-program nil buffer nil command args)))

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

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 06/17/2016 12:47 AM, Phillip Lord wrote:
>
>> Confused. Can you right a full patch?
>
> Here's a version:
>
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index f35c84d..74004d9 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1451,6 +1451,7 @@ vc-git--call
>  	(coding-system-for-write
>           (or coding-system-for-write vc-git-commits-coding-system))
>  	(process-environment (cons "PAGER=" process-environment)))
> +    (push "GIT_DIR" process-environment)
>      (apply 'process-file vc-git-program nil buffer nil command args)))
>
>  (defun vc-git--out-ok (command &rest args)

In my hands this needs to be "GIT_DIR=", at least for my tests to pass!

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/17/2016 01:23 AM, Phillip Lord wrote:

> In my hands this needs to be "GIT_DIR=", at least for my tests to pass!

"GIT_DIR=" turns the value into an empty string instead of nil. That 
breaks vc-git here in normal usage.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 06/17/2016 01:23 AM, Phillip Lord wrote:
>
>> In my hands this needs to be "GIT_DIR=", at least for my tests to pass!
>
> "GIT_DIR=" turns the value into an empty string instead of nil. That breaks
> vc-git here in normal usage.

A problem as "GIT_DIR" on it's own, still breaks with my originally
reported error (at least when running my ert test I sent with my patch).

Are you not getting this failure also?

Phil

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/17/2016 01:42 AM, Phillip Lord wrote:

> A problem as "GIT_DIR" on it's own, still breaks with my originally
> reported error (at least when running my ert test I sent with my patch).
>
> Are you not getting this failure also?

Sorry, I was only testing that the addition doesn't break anything in 
the normal case, otherwise relying on process-environment's docstring.

There was a problem with the patch that not all vc-git code goes through 
vc-git--call (in fact, most don't), but even patching vc-git-command in 
a similar fashion doesn't change the behavior if Emacs was started with 
GIT_DIR=.git.

Could this feature be actually broken? Or how does Git ignore the 
modification?

"To use ‘process-environment’ to
remove an environment variable, include only its name in the list,
without "=VALUE"."

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index f35c84d..a544a2e 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1423,7 +1423,9 @@ vc-git-command
    (let ((coding-system-for-read
           (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)))
+         (or coding-system-for-write vc-git-commits-coding-system))
+        (process-environment process-environment))
+    (push "GIT_DIR" process-environment)
      (apply 'vc-do-command (or buffer "*vc*") okstatus vc-git-program
  	   ;; http://debbugs.gnu.org/16897
  	   (unless (and (not (cdr-safe file-or-list))
@@ -1451,6 +1453,7 @@ vc-git--call
  	(coding-system-for-write
           (or coding-system-for-write vc-git-commits-coding-system))
  	(process-environment (cons "PAGER=" process-environment)))
+    (push "GIT_DIR" process-environment)
      (apply 'process-file vc-git-program nil buffer nil command args)))

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

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Noam Postavsky]

On Thu, Jun 16, 2016 at 10:41 PM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> Could this feature be actually broken? Or how does Git ignore the
> modification?
>
> "To use ‘process-environment’ to
> remove an environment variable, include only its name in the list,
> without "=VALUE"."

Turns out this feature is broken, see
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23779

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Thu, 16 Jun 2016 23:37:06 -0400
> Cc: 23769@debbugs.gnu.org, Phillip Lord <phillip.lord@russet.org.uk>
> 
> On Thu, Jun 16, 2016 at 10:41 PM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> > Could this feature be actually broken? Or how does Git ignore the
> > modification?
> >
> > "To use ‘process-environment’ to
> > remove an environment variable, include only its name in the list,
> > without "=VALUE"."
> 
> Turns out this feature is broken, see
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23779

(setenv "FOO") works for me.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/17/2016 09:54 AM, Eli Zaretskii wrote:

> (setenv "FOO") works for me.

setenv works destructively. To use it and remain good citizens, we'd 
have to `(copy-sequence process-environment)` every time we call a Git 
command.

Maybe that's not huge in the grand scheme of things, but it certainly 
looks wasteful.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Eli Zaretskii]

> Cc: 23769@debbugs.gnu.org, phillip.lord@russet.org.uk
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 17 Jun 2016 15:06:10 +0300
> 
> On 06/17/2016 09:54 AM, Eli Zaretskii wrote:
> 
> > (setenv "FOO") works for me.
> 
> setenv works destructively. To use it and remain good citizens, we'd 
> have to `(copy-sequence process-environment)` every time we call a Git 
> command.
> 
> Maybe that's not huge in the grand scheme of things, but it certainly 
> looks wasteful.

If you want to manually remove the variable before adding it with no
value, that will also work.  Otherwise, I see no magic that would do
anything beyond the expected effect on any list.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

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

> If you want to manually remove the variable before adding it with no
> value, that will also work.

That would also be destructive.

> Otherwise, I see no magic that would do
> anything beyond the expected effect on any list.

process-environment just needs to be fixed.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 06/17/2016 09:54 AM, Eli Zaretskii wrote:
>
>> (setenv "FOO") works for me.
>
> setenv works destructively. To use it and remain good citizens, we'd have to
> `(copy-sequence process-environment)` every time we call a Git command.
>
> Maybe that's not huge in the grand scheme of things, but it certainly looks
> wasteful.

I used this in my tests....

(let ((foo (getenv "FOO")))
  (unwind-protect
      (progn    
        (setenv "FOO")
        (apply 'git-program args))
    (setenv "FOO" foo)))

It's why I that setting and unsetting GIT_DIR seemed a bit of a pain
earlier. It should work though.

Phil

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 06/16/2016 02:25 PM, Dmitry Gutov wrote:
>> If new arguments arise in favor of this, we
>> can do it, but so far unsetting GIT_DIR seems like the cheapest option.
>> Like this:
>
> Actually, this is better. The previous one broke vc-git. :)
>
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index f35c84d..2b827a3 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1451,6 +1451,7 @@ vc-git--call
>  	(coding-system-for-write
>           (or coding-system-for-write vc-git-commits-coding-system))
>  	(process-environment (cons "PAGER=" process-environment)))
> +    (setenv "GIT_DIR" nil)
>      (apply 'process-file vc-git-program nil buffer nil command args)))
>
>  (defun vc-git--out-ok (command &rest args)

This just sets GIT_DIR nil globally right? It does appear to work, but
is it not a bit blunt.

Phil

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/17/2016 12:47 AM, Phillip Lord wrote:

> This just sets GIT_DIR nil globally right? It does appear to work, but
> is it not a bit blunt.

It would be a fine solution because we have a local binding for 
process-environment just a little bit above. But setenv changes it 
destructively, as Noam pointed out, so back to refining the other patch.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

Hi Philip,

Could you please pull the latest changes from emacs-25, rebuild, and 
check that applying the patch from 
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23769#86 fixes this problem?

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Philip,

It's "Phillip" actually, but "Phil" is easier.

>
> Could you please pull the latest changes from emacs-25, rebuild, and
> check that applying the patch from
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23769#86 fixes this
> problem?

Yes, this appears to work on the emacs-25 head.

Are you going to add the test case that I sent in also? If not, I'll
just add it to master (I don't think it will merge cleanly, so this
might be the best solution anyway).

Phil

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Dmitry Gutov]

On 06/21/2016 11:45 PM, Phillip Lord wrote:

> It's "Phillip" actually, but "Phil" is easier.

Oh. Sorry, Phillip.

>> Could you please pull the latest changes from emacs-25, rebuild, and
>> check that applying the patch from
>> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23769#86 fixes this
>> problem?
>
> Yes, this appears to work on the emacs-25 head.

Thanks. Installed, with Eli's blessing.

> Are you going to add the test case that I sent in also? If not, I'll
> just add it to master (I don't think it will merge cleanly, so this
> might be the best solution anyway).

Yes, please go ahead with it on master.

bug#23769: 25.0.95; Mode Line breakage in vc-git

[John Wiegley]

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

>>>>> Phillip Lord <phillip.lord@russet.org.uk> writes:

> The following patch addresses this bug.

> John/Eli would you want this for Emacs-25.

This can wait until 25.2, since no one has had a chance to use this in the
pretests.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 629 bytes --]

bug#23769: 25.0.95; Mode Line breakage in vc-git

[Phillip Lord]

Glenn Morris <rgm@gnu.org> writes:

> AFAIK Emacs doesn't support GIT_DIR, see eg https://debbugs.gnu.org/5344 .

I think I have tracked the cause of the problem.

It happens if a test opens a file in a subdirectory of the git root.
When the file is opened Emacs checks to see whether the file is
versioned. Somewhere down the line Emacs calls vc-git-working-revision,
which eventually results in invocation of `git symbolic-ref HEAD`.
All is good.

Unless, GIT_DIR is set to ".git". Now, `git symbolic-ref HEAD` fails in
subdirectories because GIT_DIR is still set to .git, when the .git
directory is in the root. git symbolic-ref fails with 128 "Fatal: Not a
git repository".

Unfortunately, this is exactly the situation on a pre-commit hook. Git
changes working directory to the root of the git repository, and then
sets GIT_DIR to be .git.

Potential solutions:

1) Ignore return values from git (fail silently)
2) Unset GIT_DIR (but I don't know why it is set by git, and I assume it
   is for some reason)
3) Run all vc-git commands from the root dir, and use fully qualified
   names.

Will think further.

Phil