GNU bug report logs - #26066
vc-git-status gives wrong result when called from outside repository

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 26066 in the body.
You can then email your comments to 26066 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.0.50; vc-git-status gives wrong result
Date: Sat, 11 Mar 2017 21:42:46 -0500

**To reproduce:**

First, we create a simple git repository in a shell:
  $ mkdir -p ~/Desktop/temp
  $ cd ~/Desktop/temp
  $ git init
  $ echo abc > file.txt
  $ git add . && git commit -a -m "Init"

You can verify that the repository is up to date by running, e.g. 'git 
status'.

Next, we load emacs -Q and run the following:
(progn
  (setq default-directory "/")
  (require 'vc-git)
  (vc-git-state "/home/ganc/Desktop/temp/file.txt"))

The response is `added'. It should be `up-to-date'.

**Notes**


I get this same behavior on emacs 24.5, 25.1. I have some ideas about
how to fix the problems but I want to make sure this isn't somehow the
expected behavior.


In GNU Emacs 26.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.20.9)
 of 2017-03-10 built on lcy01-22
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
System Description:    Ubuntu 16.10

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --build=x86_64-linux-gnu --prefix=/usr
 '--includedir=${prefix}/include' '--mandir=${prefix}/share/man'
 '--infodir=${prefix}/share/info' --sysconfdir=/etc --localstatedir=/var
 --disable-silent-rules '--libdir=${prefix}/lib/x86_64-linux-gnu'
 '--libexecdir=${prefix}/lib/x86_64-linux-gnu' --disable-maintainer-mode
 --disable-dependency-tracking --prefix=/usr --sharedstatedir=/var/lib
 --program-suffix=-snapshot --with-modules=yes --with-x=yes
 --with-x-toolkit=gtk3 --with-xwidgets=yes 'CFLAGS=-g -O2
 -fdebug-prefix-map=/build/emacs-snapshot-rvTCtk/emacs-snapshot-93192-26848af-emacs=. -fstack-protector-strong
 -Wformat -Werror=format-security' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' 'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 MODULES XWIDGETS LIBSYSTEMD

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message puny seq byte-opt subr-x gv
bytecomp byte-compile cl-extra help-mode cconv cl-loaddefs pcase cl-lib
dired dired-loaddefs format-spec rfc822 mml easymenu mml-sec
password-cache epa derived epg epg-config gnus-util rmail rmail-loaddefs
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr
mail-utils time-date mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript case-table epa-hook jka-cmpr-hook help
simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs button
faces cus-face macroexp files text-properties overlay sha1 md5 base64
format env code-pages mule custom widget hashtable-print-readable
backquote dbusbind inotify dynamic-setting system-font-setting
font-render-setting xwidget-internal move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 96617 5698)
 (symbols 48 20192 1)
 (miscs 40 48 115)
 (strings 32 17613 4552)
 (string-bytes 1 565163)
 (vectors 16 14650)
 (vector-slots 8 482994 4091)
 (floats 8 48 68)
 (intervals 56 241 0)
 (buffers 976 11))

Message #8 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Jonathan Ganc <jonganc <at> gmail.com>
Cc: 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 14 Mar 2017 09:45:36 -0400

retitle 26066 vc-git-status gives wrong result when called from outside repository
severity 26066 minor
found 26066 24.5
found 26066 25.2
quit

(list 
 (let ((default-directory "/"))
   (require 'vc-git)
   (vc-git-state "/home/npostavs/src/emacs/emacs-master/README"))
 (let ((default-directory "/home/npostavs/src/emacs/emacs-master/"))
   (require 'vc-git)
   (vc-git-state "README"))) ;=> (added up-to-date)

Jonathan Ganc <jonganc <at> gmail.com> writes:

> I get this same behavior on emacs 24.5, 25.1. I have some ideas about
> how to fix the problems but I want to make sure this isn't somehow the
> expected behavior.

I doubt this particular behaviour is expected, but it's probably
unexpected that you would call vc-git-status from outside the repo.  If
you can fix things to handle this case without harming the normal case,
that sounds fine to me.

Message #19 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: 26066 <at> debbugs.gnu.org
Cc: Jonathan Ganc <jonganc <at> gmail.com>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Wed, 15 Mar 2017 20:42:54 -0400

[Message part 1 (text/plain, inline)]

[Please use Reply All to keep 26066 <at> debbugs.gnu.org on Cc]

[Message part 2 (message/rfc822, inline)]

From: Jonathan Ganc <jonganc <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 14 Mar 2017 20:25:57 -0400

The actual problem is with the function vc-git--empty-db-p, which acts 
in the directory default-directory (implicitly, through vc-git--call and 
then process-file). Thus, I think replacing (in vc-git-state)
(vc-git--empty-db-p)
with
(let ((default-directory (file-name-directory file)))
   (vc-git--empty-db-p))
should resolve the issue without side effect.

As an aside, is there a reason that vc-git-status does not use "git 
status -z --porcelain --ignored"? It seems more natural/straightfoward 
and it would also deal with ignored and removed files. It could also 
work with directories, which the current function doesn't.

[Message part 3 (text/plain, inline)]

Let-binding default-directory sounds okay.

Regarding 'status -z --porcelain', I think it might not have been
available at the time te code was written.  I'm not sure how far back we
need to maintain compatibility.

Message #22 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: npostavs <at> users.sourceforge.net, 26066 <at> debbugs.gnu.org
Cc: Jonathan Ganc <jonganc <at> gmail.com>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Thu, 16 Mar 2017 02:48:24 +0200

On 16.03.2017 02:42, npostavs <at> users.sourceforge.net wrote:
> As an aside, is there a reason that vc-git-status does not use "git
> status -z --porcelain --ignored"? It seems more natural/straightfoward
> and it would also deal with ignored and removed files. It could also
> work with directories, which the current function doesn't.

Like mentioned in a discussion on a different bug, we'll accept a patch 
like that. You can even reuse some code from `vc-git-conflicted-files`.

Message #25 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Wed, 15 Mar 2017 22:40:40 -0400

First of all, I realized that the let binding needs to surround the 
entire vc-git-state function, not just (vc-git--empty-db-p) (because 
vc-git--run-command-string, vc-git--call have the same issue with 
default-directory). E.g., change

(let ((diff (vc-git--run-command-string
               file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))

to

(let* ((default-directory (file-name-directory file))
       (diff (vc-git--run-command-string
             file "diff-index" "-p" "--raw" "-z" "HEAD" "--")))

But yes, I will work on an improved version using git status and try 
incorporate/improve on vc-git-conflicted-files.

(My ultimate goal is to try to get something like atom's status for git 
files into emacs, which is how I noticed the problem to begin with).


On 03/15/2017 08:48 PM, Dmitry Gutov wrote:
> On 16.03.2017 02:42, npostavs <at> users.sourceforge.net wrote:
>> As an aside, is there a reason that vc-git-status does not use "git
>> status -z --porcelain --ignored"? It seems more natural/straightfoward
>> and it would also deal with ignored and removed files. It could also
>> work with directories, which the current function doesn't.
>
> Like mentioned in a discussion on a different bug, we'll accept a 
> patch like that. You can even reuse some code from 
> `vc-git-conflicted-files`.

Message #28 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Fri, 17 Mar 2017 22:38:07 -0400

[Message part 1 (text/plain, inline)]

Hi.

I have never submitted an emacs patch before so I don't know if I am 
doing this correctly (or if one of you has already submitted a fix). I 
have attached a patch file.

Later, I will submit the more elegant fix.


On 03/15/2017 08:48 PM, Dmitry Gutov wrote:
> On 16.03.2017 02:42, npostavs <at> users.sourceforge.net wrote:
>> As an aside, is there a reason that vc-git-status does not use "git
>> status -z --porcelain --ignored"? It seems more natural/straightfoward
>> and it would also deal with ignored and removed files. It could also
>> work with directories, which the current function doesn't.
>
> Like mentioned in a discussion on a different bug, we'll accept a 
> patch like that. You can even reuse some code from 
> `vc-git-conflicted-files`.

[0001-Fix-default-directory-for-vc-git-Bug-2606.patch (text/x-patch, attachment)]

Message #31 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jonathan Ganc <jonganc <at> gmail.com>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 21 Mar 2017 11:19:29 +0200

Hi Jonathan,

On 16.03.2017 04:40, Jonathan Ganc wrote:

> (My ultimate goal is to try to get something like atom's status for git 
> files into emacs, which is how I noticed the problem to begin with).

Could you expand on what you are trying to do, and how this bug stops 
you from doing it?

Message #34 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 21 Mar 2017 12:10:33 -0400

Hi,

Well, the ideal situation is if could get a listing like the one atom 
has for projects, where you get a visual view of directories and can see 
folders with changed files as well as the status of files (orangish 
means modified, green means new, greyed means ignored):

http://imgur.com/a/PiiGF

Actually, emacs has something not so dissimilar in the Neotree package. 
(Install it and use `(setq neo-vc-integration '(face char))` ). It has a 
few shortcomings:

1. Because of this bug, it was giving me the wrong status for files, 
i.e. they showed up wrong. That is how I noticed the bug in the first place.

2. I would like to be able to color directories as well as files. I 
don't know if that is something that would have a component in vc-git.el 
/ vc-....el or would go entirely in some package (e.g. Neotree)

3. It would be nice to be able to show mutiple directory trees at once 
in Neotree, though this is not as important.


The first, big problem is that, because of this bug,


On 03/21/2017 05:19 AM, Dmitry Gutov wrote:
> Hi Jonathan,
>
> On 16.03.2017 04:40, Jonathan Ganc wrote:
>
>> (My ultimate goal is to try to get something like atom's status for 
>> git files into emacs, which is how I noticed the problem to begin with).
>
> Could you expand on what you are trying to do, and how this bug stops 
> you from doing it?

Message #37 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Jonathan Ganc <jonganc <at> gmail.com>
Cc: npostavs <at> users.sourceforge.net, 26066 <at> debbugs.gnu.org,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Wed, 22 Mar 2017 13:11:01 +0100

Jonathan Ganc <jonganc <at> gmail.com> writes:

> http://imgur.com/a/PiiGF

I didn't quite follow the whole thread, but you might also want to try
`diff-hl-dired-mode' implemented by the diff-hl package in Gnu Elpa.


Michael.

Message #40 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jonathan Ganc <jonganc <at> gmail.com>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Wed, 22 Mar 2017 18:20:04 +0200

On 21.03.2017 18:10, Jonathan Ganc wrote:

> Well, the ideal situation is if could get a listing like the one atom 
> has for projects, where you get a visual view of directories and can see 
> folders with changed files as well as the status of files (orangish 
> means modified, green means new, greyed means ignored):
> 
> http://imgur.com/a/PiiGF
> 
> Actually, emacs has something not so dissimilar in the Neotree package. 
> (Install it and use `(setq neo-vc-integration '(face char))` ). It has a 
> few shortcomings:
> 
> 1. Because of this bug, it was giving me the wrong status for files, 
> i.e. they showed up wrong. That is how I noticed the bug in the first 
> place.

I see, thanks. Neotree does have an optional feature that colors the 
tree leaves according to their VC statuses.

The problem might have gone unnoticed until now because file's status is 
cached, and because it can only be apparent if several projects are 
opened at the same time.

While the problem is fairly obvious, a proper fix would most likely 
touch other backends and commands, to the point that the 
default-directory binding might have to be done inside vc-call-backend.

There is a very simple workaround on the caller's side, though: bind 
default-directory inside the Neotree code, to the respective project 
root (or just the file's parent directory). That will be necessary 
anyway for it to work in the released Emacs versions.

> 2. I would like to be able to color directories as well as files. I 
> don't know if that is something that would have a component in vc-git.el 
> / vc-....el or would go entirely in some package (e.g. Neotree)

This differs from one version control system to another, but Git doesn't 
actually track directories. So the notion of "directory status" is 
poorly defined. But see the previously mentioned diff-hl-dired-mode.

> 3. It would be nice to be able to show mutiple directory trees at once 
> in Neotree, though this is not as important.

If it's unable to show the non-current projects, how is the bug 
triggered? default-directory would have to be outside of the project.

Message #43 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Wed, 22 Mar 2017 22:18:25 -0400

Hi,

Thanks for both responses.


>> 3. It would be nice to be able to show mutiple directory trees at 
>> once in Neotree, though this is not as important.
>
> If it's unable to show the non-current projects, how is the bug 
> triggered? default-directory would have to be outside of the project.

I'm not actually sure why it is causing me problems. I just tried using 
Neotree and encountered this problem. I didn't try anything special 
besides opening the window. I'm not sure how/why Neotree sets 
default-directory. I need to look into this.


> The problem might have gone unnoticed until now because file's status 
> is cached, and because it can only be apparent if several projects are 
> opened at the same time.
>
> While the problem is fairly obvious, a proper fix would most likely 
> touch other backends and commands, to the point that the 
> default-directory binding might have to be done inside vc-call-backend.

Yeah, I'm not really sure what the workflow should be; I'm just getting 
started with the vc functions in emacs. One issue, though, is that 
default-directory can only be set for functions that identify a 
filename, because we need to have a directory to set things to. For this 
reason, I don't know that one could generally set default-directory in 
vc-call-backend. It could make sense to set default-directory for 
vc-git--run-command-string (although to get vc-git-state to work, one 
would still need to set it for vc-git--empty-db-p).

It's worth nothing there seems to be inconsistency within vc-git. Some 
commands like vc-git-checkin, vc-git-next-revision, do set 
default-directory to the directory of the input file; other comands like 
vc-git-merge-branch seem to do the opposite and assume the root is 
already given by default-directory.

At the very least, the documentation for vc-git-state should note that 
default-directory needs to be set.

>
> There is a very simple workaround on the caller's side, though: bind 
> default-directory inside the Neotree code, to the respective project 
> root (or just the file's parent directory). That will be necessary 
> anyway for it to work in the released Emacs versions.
>

That may be the best answer. I am not so familiar with the Neotree code 
but it should be doable.

>> 2. I would like to be able to color directories as well as files. I 
>> don't know if that is something that would have a component in 
>> vc-git.el / vc-....el or would go entirely in some package (e.g. 
>> Neotree)
>
> This differs from one version control system to another, but Git 
> doesn't actually track directories. So the notion of "directory 
> status" is poorly defined. But see the previously mentioned 
> diff-hl-dired-mode.
>

You're right, in principle, but atom handles it by coloring directories 
based on if they have modified files within. I don't know if there is a 
"canonical" way to do this, although one idea would be to return a list 
of all file status in the directory, e.g. `(up-to-date modified ignored)`.

I started looking through diff-hl. It looks like it has a bunch of neat 
and useful features, though I admit I was a bit confused by the workflow.

Message #46 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Thu, 30 Mar 2017 23:16:02 -0400

[Message part 1 (text/plain, inline)]

I have attached my proposed patch to use 'git status' for finding the 
git status of files.

I have also attached git-test.sh, which generates a subdirectory 
git-test with files in all the relevant git states I could think of (if 
you use it, look at the mods, repo2, repo3 directories for interesting 
files).

I then verified that 1) my function gives the same result as 
vc-git-dir-status-files where vc-git-dir-status-files shows the file, 
with one exception: if a file has a merge conflict (i.e. the status is 
"UU"), my function returns conflict; 2) it is the same speed as the 
current vc-git-state.

I may try eventually try rewriting vc-git-dir-status-files using this 
since it appears to be about an order of magnitude faster than the 
current implementation.

On 03/22/2017 10:18 PM, Jonathan Ganc wrote:
> Hi,
>
> Thanks for both responses.
>
>
>>> 3. It would be nice to be able to show mutiple directory trees at 
>>> once in Neotree, though this is not as important.
>>
>> If it's unable to show the non-current projects, how is the bug 
>> triggered? default-directory would have to be outside of the project.
>
> I'm not actually sure why it is causing me problems. I just tried 
> using Neotree and encountered this problem. I didn't try anything 
> special besides opening the window. I'm not sure how/why Neotree sets 
> default-directory. I need to look into this.
>
>
>> The problem might have gone unnoticed until now because file's status 
>> is cached, and because it can only be apparent if several projects 
>> are opened at the same time.
>>
>> While the problem is fairly obvious, a proper fix would most likely 
>> touch other backends and commands, to the point that the 
>> default-directory binding might have to be done inside vc-call-backend.
>
> Yeah, I'm not really sure what the workflow should be; I'm just 
> getting started with the vc functions in emacs. One issue, though, is 
> that default-directory can only be set for functions that identify a 
> filename, because we need to have a directory to set things to. For 
> this reason, I don't know that one could generally set 
> default-directory in vc-call-backend. It could make sense to set 
> default-directory for vc-git--run-command-string (although to get 
> vc-git-state to work, one would still need to set it for 
> vc-git--empty-db-p).
>
> It's worth nothing there seems to be inconsistency within vc-git. Some 
> commands like vc-git-checkin, vc-git-next-revision, do set 
> default-directory to the directory of the input file; other comands 
> like vc-git-merge-branch seem to do the opposite and assume the root 
> is already given by default-directory.
>
> At the very least, the documentation for vc-git-state should note that 
> default-directory needs to be set.
>
>>
>> There is a very simple workaround on the caller's side, though: bind 
>> default-directory inside the Neotree code, to the respective project 
>> root (or just the file's parent directory). That will be necessary 
>> anyway for it to work in the released Emacs versions.
>>
>
> That may be the best answer. I am not so familiar with the Neotree 
> code but it should be doable.
>
>>> 2. I would like to be able to color directories as well as files. I 
>>> don't know if that is something that would have a component in 
>>> vc-git.el / vc-....el or would go entirely in some package (e.g. 
>>> Neotree)
>>
>> This differs from one version control system to another, but Git 
>> doesn't actually track directories. So the notion of "directory 
>> status" is poorly defined. But see the previously mentioned 
>> diff-hl-dired-mode.
>>
>
> You're right, in principle, but atom handles it by coloring 
> directories based on if they have modified files within. I don't know 
> if there is a "canonical" way to do this, although one idea would be 
> to return a list of all file status in the directory, e.g. 
> `(up-to-date modified ignored)`.
>
> I started looking through diff-hl. It looks like it has a bunch of 
> neat and useful features, though I admit I was a bit confused by the 
> workflow.
>

[git-test.sh (application/x-shellscript, attachment)]

[0001-Use-git-status-in-vc-git-status.patch (text/x-patch, attachment)]

Message #49 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Sun, 9 Apr 2017 22:16:24 -0400

Hi,

I am just checking if anyone has had a chance to review my proposed 
improvement to vc-git-status? I have been using it without problem but I 
would love to get feedback.

Jonathan


On 03/30/2017 11:16 PM, Jonathan Ganc wrote:
> I have attached my proposed patch to use 'git status' for finding the 
> git status of files.
>
> I have also attached git-test.sh, which generates a subdirectory 
> git-test with files in all the relevant git states I could think of 
> (if you use it, look at the mods, repo2, repo3 directories for 
> interesting files).
>
> I then verified that 1) my function gives the same result as 
> vc-git-dir-status-files where vc-git-dir-status-files shows the file, 
> with one exception: if a file has a merge conflict (i.e. the status is 
> "UU"), my function returns conflict; 2) it is the same speed as the 
> current vc-git-state.
>
> I may try eventually try rewriting vc-git-dir-status-files using this 
> since it appears to be about an order of magnitude faster than the 
> current implementation.
>
> On 03/22/2017 10:18 PM, Jonathan Ganc wrote:
>> Hi,
>>
>> Thanks for both responses.
>>
>>
>>>> 3. It would be nice to be able to show mutiple directory trees at 
>>>> once in Neotree, though this is not as important.
>>>
>>> If it's unable to show the non-current projects, how is the bug 
>>> triggered? default-directory would have to be outside of the project.
>>
>> I'm not actually sure why it is causing me problems. I just tried 
>> using Neotree and encountered this problem. I didn't try anything 
>> special besides opening the window. I'm not sure how/why Neotree sets 
>> default-directory. I need to look into this.
>>
>>
>>> The problem might have gone unnoticed until now because file's 
>>> status is cached, and because it can only be apparent if several 
>>> projects are opened at the same time.
>>>
>>> While the problem is fairly obvious, a proper fix would most likely 
>>> touch other backends and commands, to the point that the 
>>> default-directory binding might have to be done inside vc-call-backend.
>>
>> Yeah, I'm not really sure what the workflow should be; I'm just 
>> getting started with the vc functions in emacs. One issue, though, is 
>> that default-directory can only be set for functions that identify a 
>> filename, because we need to have a directory to set things to. For 
>> this reason, I don't know that one could generally set 
>> default-directory in vc-call-backend. It could make sense to set 
>> default-directory for vc-git--run-command-string (although to get 
>> vc-git-state to work, one would still need to set it for 
>> vc-git--empty-db-p).
>>
>> It's worth nothing there seems to be inconsistency within vc-git. 
>> Some commands like vc-git-checkin, vc-git-next-revision, do set 
>> default-directory to the directory of the input file; other comands 
>> like vc-git-merge-branch seem to do the opposite and assume the root 
>> is already given by default-directory.
>>
>> At the very least, the documentation for vc-git-state should note 
>> that default-directory needs to be set.
>>
>>>
>>> There is a very simple workaround on the caller's side, though: bind 
>>> default-directory inside the Neotree code, to the respective project 
>>> root (or just the file's parent directory). That will be necessary 
>>> anyway for it to work in the released Emacs versions.
>>>
>>
>> That may be the best answer. I am not so familiar with the Neotree 
>> code but it should be doable.
>>
>>>> 2. I would like to be able to color directories as well as files. I 
>>>> don't know if that is something that would have a component in 
>>>> vc-git.el / vc-....el or would go entirely in some package (e.g. 
>>>> Neotree)
>>>
>>> This differs from one version control system to another, but Git 
>>> doesn't actually track directories. So the notion of "directory 
>>> status" is poorly defined. But see the previously mentioned 
>>> diff-hl-dired-mode.
>>>
>>
>> You're right, in principle, but atom handles it by coloring 
>> directories based on if they have modified files within. I don't know 
>> if there is a "canonical" way to do this, although one idea would be 
>> to return a list of all file status in the directory, e.g. 
>> `(up-to-date modified ignored)`.
>>
>> I started looking through diff-hl. It looks like it has a bunch of 
>> neat and useful features, though I admit I was a bit confused by the 
>> workflow.
>>
>

Message #52 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Jonathan Ganc <jonganc <at> gmail.com>
Cc: 26066 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Sun, 09 Apr 2017 22:58:11 -0400

Jonathan Ganc <jonganc <at> gmail.com> writes:

>  
> +(defun vc-git--git-status-to-vc-state (code-list)

> +  (setq code-list (remq nil code-list))
> +  (pcase code-list
> +    ('nil 'up-to-date)

You seem to be handling nil in 2 different ways.

> +       ;; have only seen this with a file that is only present in the
> +       ;; index. let us call this `removed'

Comments should be full sentences (start with uppercase, end with
period), and sentences should be separated with double spaces.

>  (defun vc-git-state (file)
>    "Git-specific version of `vc-state'."
> +  
> +  (save-match-data
> +    (let* ((default-directory (file-name-directory (expand-file-name file)))
> +           (status
> +            (vc-git--run-command-string file "status" "--porcelain" "-z"
> +                                        "--untracked-files" "--ignored" "--"))
> +           code-list)

> +      (while (string-match "^\\(..\\)[^\0]+\0\\(\\(?:a\\|[^a]\\)*\\)$" status)

I think you're missing a space after the first 2 characters.

    alternate -z format recommended for machine parsing. [...] Second, a
    NUL (ASCII 0) follows each filename, [...] (but a space still
    separates the status field from the first filename).

> +        (add-to-list 'code-list (match-string 1 status) t 'ignore)

Don't use `add-to-list' on local variables.  The usual idiom for
collecting a series of items in a list would be

    (let ((code-list nil))
      (while ...
        (push ... code-list))
      (setq code-list (nreverse code-list)))

> +        (setq status (match-string 2 status)))

Instead of matching the rest of the string in the regexp, just set a
position variable to (match-end 0), and then pass that position as the
string-match's START parameter.

Message #55 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jonathan Ganc <jonganc <at> gmail.com>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Mon, 10 Apr 2017 06:26:16 +0300

On 31.03.2017 06:16, Jonathan Ganc wrote:
> +  (save-match-data
> +    (let* ((default-directory (file-name-directory (expand-file-name file)))

We don't require or expect that any particular function keeps the match 
data intact. I don't think that save-match-data is needed here.

Aside from this an Noam's comments, the patch is long enough to require 
a copyright assignment. I didn't find yours on file.

Will you sign one?

Message #58 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Mon, 10 Apr 2017 00:41:15 -0400

[Message part 1 (text/plain, inline)]

I agree with the suggestions from both of you and have made the 
corrections. I have attached a new patch. Please let me know if you have 
further thoughts.

I will gladly sign a copyright assignment, if you point me to where I 
can do that.


On 04/09/2017 11:26 PM, Dmitry Gutov wrote:
> On 31.03.2017 06:16, Jonathan Ganc wrote:
>> +  (save-match-data
>> +    (let* ((default-directory (file-name-directory (expand-file-name 
>> file)))
>
> We don't require or expect that any particular function keeps the 
> match data intact. I don't think that save-match-data is needed here.
>
> Aside from this an Noam's comments, the patch is long enough to 
> require a copyright assignment. I didn't find yours on file.
>
> Will you sign one?

[0001-update-vc-git.patch (text/x-patch, attachment)]

Message #61 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Mon, 10 Apr 2017 00:43:35 -0400

BTW, Dmitry, I've started using Diff-Hl and it's really nice!


On 04/09/2017 11:26 PM, Dmitry Gutov wrote:
> On 31.03.2017 06:16, Jonathan Ganc wrote:
>> +  (save-match-data
>> +    (let* ((default-directory (file-name-directory (expand-file-name 
>> file)))
>
> We don't require or expect that any particular function keeps the 
> match data intact. I don't think that save-match-data is needed here.
>
> Aside from this an Noam's comments, the patch is long enough to 
> require a copyright assignment. I didn't find yours on file.
>
> Will you sign one?

Message #64 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jonathan Ganc <jonganc <at> gmail.com>
Cc: npostavs <at> users.sourceforge.net, 26066 <at> debbugs.gnu.org, dgutov <at> yandex.ru
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Mon, 10 Apr 2017 09:05:25 +0300

> From: Jonathan Ganc <jonganc <at> gmail.com>
> Date: Mon, 10 Apr 2017 00:41:15 -0400
> 
> I will gladly sign a copyright assignment, if you point me to where I 
> can do that.

I sent the form off-list.

Thanks.

Message #67 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Thien-Thi Nguyen <ttn <at> gnu.org>
To: 26066 <at> debbugs.gnu.org
Cc: Jonathan Ganc <jonganc <at> gmail.com>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Mon, 10 Apr 2017 09:35:28 +0200

[Message part 1 (text/plain, inline)]

() Jonathan Ganc <jonganc <at> gmail.com>
() Mon, 10 Apr 2017 00:41:15 -0400

   +      (while (string-match "\\(..\\) [^\0]+\0" status next-match)
   +        (push (match-string 1 status) code-list)
   +        (setq next-match (match-end 0)))
   +      (setq code-list (nreverse code-list))

You can also use something like:

 (mapcar (lambda (s)
           (substring s 0 2))
         (split-string status "\0"))

to compute the value of ‘code-list’.  Might be quicker, too.

-- 
Thien-Thi Nguyen -----------------------------------------------
 (defun responsep (query)
   (pcase (context query)
     (`(technical ,ml) (correctp ml))
     ...))                              748E A0E8 1CB8 A748 9BFA
--------------------------------------- 6CE4 6703 2224 4C80 7502

[signature.asc (application/pgp-signature, inline)]

Message #70 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jonathan Ganc <jonganc <at> gmail.com>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 11 Apr 2017 02:26:39 +0300

On 10.04.2017 07:43, Jonathan Ganc wrote:
> BTW, Dmitry, I've started using Diff-Hl and it's really nice!

I'm glad you like it :)

Message #73 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jonathan Ganc <jonganc <at> gmail.com>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 11 Apr 2017 02:46:40 +0300

On 10.04.2017 07:41, Jonathan Ganc wrote:
> I agree with the suggestions from both of you and have made the 
> corrections. I have attached a new patch. Please let me know if you have 
> further thoughts.

Thanks. I guess the main thing left is to decide whether vc-git-state 
should bind default-directory. For example, vc-bzr-state doesn't, while 
vc-hg-state does.

Does this binding affect the command output, in this particular case?

Message #76 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jonathan Ganc <jonganc <at> gmail.com>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 11 Apr 2017 03:07:21 +0300

On 23.03.2017 04:18, Jonathan Ganc wrote:

> Yeah, I'm not really sure what the workflow should be; I'm just getting 
> started with the vc functions in emacs. One issue, though, is that 
> default-directory can only be set for functions that identify a 
> filename, because we need to have a directory to set things to. For this 
> reason, I don't know that one could generally set default-directory in 
> vc-call-backend.

You are right. But we could set it in vc-state-refresh.

> It could make sense to set default-directory for 
> vc-git--run-command-string (although to get vc-git-state to work, one 
> would still need to set it for vc-git--empty-db-p).

That's also an option, yes. But binding it on the higher level would be 
better, I think.

> It's worth nothing there seems to be inconsistency within vc-git. Some 
> commands like vc-git-checkin, vc-git-next-revision, do set 
> default-directory to the directory of the input file; other comands like 
> vc-git-merge-branch seem to do the opposite and assume the root is 
> already given by default-directory.

True. Some of those occurrences are most likely there just so the DIR 
argument is not left unused. Some have less obvious reasons.

vc-git-checkin, for instance, does it to call 'git commit' from the 
repository root, instead of an arbitrary directory in it. I'm sure it's 
really necessary, though.

> At the very least, the documentation for vc-git-state should note that 
> default-directory needs to be set.

The backend-specific functions are largely internal, we don't expect 
clients to call them directly. If anything, the documentation can be 
clarified on whether an arbitrary backend's 'state' command should set 
default-directory, or use the current one.

> You're right, in principle, but atom handles it by coloring directories 
> based on if they have modified files within. I don't know if there is a 
> "canonical" way to do this, although one idea would be to return a list 
> of all file status in the directory, e.g. `(up-to-date modified ignored)`.

Yeah, diff-hl-dired does something like that. If all the "interesting" 
statuses in a directory are the same, we assign it that status. If 
they're mixed, we mark it as "changed".

Message #79 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Mon, 10 Apr 2017 23:52:24 -0400

[Message part 1 (text/plain, inline)]

I've incorporated Thien-Thi's suggestion, as well as an if to check 
status for nil (which indicates unregistered).


On 04/10/2017 08:07 PM, Dmitry Gutov wrote:
> You are right. But we could set it in vc-state-refresh.
>

On 04/10/2017 07:46 PM, Dmitry Gutov wrote:
>
> Thanks. I guess the main thing left is to decide whether vc-git-state 
> should bind default-directory. For example, vc-bzr-state doesn't, 
> while vc-hg-state does.
>
> Does this binding affect the command output, in this particular case?

The binding affects the output if default-directory is not set inside 
the file's repository.

Since, in principle, the vc functions should be agnostic to the choice 
of vcs, either a) vc-state documentation should state that 
default-directory should be set to get generally correct responses or b) 
it should be set in some function (and I agree that vc-state-refresh 
makes sense).

I think the overhead of setting the directory is rather low. In some 
admittedly rudimentary benchmarks, there is almost no difference in 
performance setting default-directory.

There's also the question of how to handle default-directory. You cannot 
simply do (file-name-directory file), because that fails if FILE is 
given without a directory. I think the correct one is 
(file-name-directory (expand-file-name file)) (which, surprisingly, is 
slighly faster than (file-name-directory (concat default-directory file)) ).

[0001-update-vc-git.patch (text/x-patch, attachment)]

Message #82 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 11 Apr 2017 09:08:35 -0400

I think I've changed my mind about where/whether to bind 
default-directory. Since it is not universally bound by all vcs engines, 
it should probably be considered "backend specific", which would 
therefore suggest that it not be bound before the backend specific 
functions, e.g. vc-git-state. On the other hand, since it sometimes is 
necessary for correct output, I do think it should be bound in vc-git-state.


On 04/10/2017 11:52 PM, Jonathan Ganc wrote:
>
> The binding affects the output if default-directory is not set inside 
> the file's repository.
>
> Since, in principle, the vc functions should be agnostic to the choice 
> of vcs, either a) vc-state documentation should state that 
> default-directory should be set to get generally correct responses or 
> b) it should be set in some function (and I agree that 
> vc-state-refresh makes sense).
>
> I think the overhead of setting the directory is rather low. In some 
> admittedly rudimentary benchmarks, there is almost no difference in 
> performance setting default-directory.
>
> There's also the question of how to handle default-directory. You 
> cannot simply do (file-name-directory file), because that fails if 
> FILE is given without a directory. I think the correct one is 
> (file-name-directory (expand-file-name file)) (which, surprisingly, is 
> slighly faster than (file-name-directory (concat default-directory 
> file)) ).

Message #85 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jonathan Ganc <jonganc <at> gmail.com>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Wed, 12 Apr 2017 02:27:41 +0300

On 11.04.2017 16:08, Jonathan Ganc wrote:
> I think I've changed my mind about where/whether to bind 
> default-directory. Since it is not universally bound by all vcs engines, 

That doesn't necessarily imply that they shouldn't.

> On the other hand, since it sometimes is 
> necessary for correct output, I do think it should be bound in 
> vc-git-state.

Is it necessary for correct output in other backends, in similar scenarios?

I agree that fixing this makes sense, but it should be done in an 
organized fashion, and separately from this patch.

If vc-git supports calling vc-git-state from outside of the repository, 
but not some other commands, or if vc-git-state does but some other 
backends' vc-xx-state does not, this will increase inconsistency and 
make it harder for the programmers to write VCS-agnostic code, which is 
one of the main goals of VC.

vc-state is also among the first functions a piece of third-party code 
would call. So it becomes the choke point which forces the caller to get 
the value of default-directory right, before any other backend commands 
are called. Until we take inventory of them, it's better to keep the bug 
in vc-state, I think.

>> Since, in principle, the vc functions should be agnostic to the choice 
>> of vcs, either a) vc-state documentation should state that 
>> default-directory should be set to get generally correct responses or

If we decide that, that should be the rule we declare for all VC backend 
commands, not just vc-state.

>> b) it should be set in some function (and I agree that 
>> vc-state-refresh makes sense).
>>
>> I think the overhead of setting the directory is rather low. In some 
>> admittedly rudimentary benchmarks, there is almost no difference in 
>> performance setting default-directory.

Yeah, I'm not worried about performance here either.

>> There's also the question of how to handle default-directory. You 
>> cannot simply do (file-name-directory file), because that fails if 
>> FILE is given without a directory. I think the correct one is 
>> (file-name-directory (expand-file-name file))

Yup, that should work. I'm not sure any of these commands are ever 
called with relative names, though.

> (which, surprisingly, is 
>> slighly faster than (file-name-directory (concat default-directory 
>> file)) ).

That probably depends on the file. The first option was faster in the 
example I've tried.

Message #88 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jonathan Ganc <jonganc <at> gmail.com>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Wed, 12 Apr 2017 02:36:19 +0300

On 12.04.2017 02:27, Dmitry Gutov wrote:

>> (which, surprisingly, is
>>> slighly faster than (file-name-directory (concat default-directory 
>>> file)) ).
> 
> That probably depends on the file. The first option was faster in the 
> example I've tried.

I meant the second one, sorry.

Message #91 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Thu, 13 Apr 2017 20:42:42 -0400

[Message part 1 (text/plain, inline)]

First, I'm sorry for this delayed response. I've had a busy few days 
(and I needed to think about the issue)!

I'm fine with decoupling the question about default-directory from the 
patch for git status. I have attached a patch that does not bind 
default-directory. (And I now have turned in the assignment paperwork to 
the FSF!).

I continue to think that binding default-directory in vc-git-state makes 
the most sense.

> If vc-git supports calling vc-git-state from outside of the 
> repository, but not some other commands, or if vc-git-state does but 
> some other backends' vc-xx-state does not, this will increase 
> inconsistency and make it harder for the programmers to write 
> VCS-agnostic code, which is one of the main goals of VC.

But being vcs-agnostic is more for people who use vc-state or 
vc-state-refresh, rather than people actually writing the vc-specific 
functions, no? If this bug is unchanged, people using vc-state have to 
ask themselves: "if the backend is git, I have to bind default-directory 
but if it's not, I don't." That is inconsistent with vc agnosticism.

> Is it necessary for correct output in other backends, in similar 
> scenarios?
>
> I agree that fixing this makes sense, but it should be done in an 
> organized fashion

That's a valid question and a valid point. But I actually think that 
adding the binding in vc-git-state furthers the goal of systematically 
answering them. I doubt we'll be able to find, at one time, people with 
knowledge of more than 4 or 5 of the vc's (e.g. I know only git) who 
have the time to address this issue. What we can do instead, though, is 
to encourage people to properly write the backend specific functions. 
If, at some point, we notice that every vc-BACKEND-state binds 
default-directory, then we can move the binding to vc-state.

However, this question reinforces my opinion that binding 
default-directory can be treated satisfactorily as a backend-specific 
question. The fact that I can't argue in general that an arbitrary vc 
system needs default-directory set suggests to me that one should leave 
it to the vc system to decide. For example, I could imagine a backend 
that wants the current directory to be the root directory of the 
repository, rather than the directory containing the file.

[0001-update-vc-git.patch (text/x-patch, attachment)]

Message #94 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Sun, 23 Apr 2017 14:21:59 -0400

Since there have been no more objections to my patch (the one in my 
previous note does not bind default-directory), could someone merge it in?

Message #97 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Jonathan Ganc <jonganc <at> gmail.com>
Cc: 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Sun, 23 Apr 2017 18:28:04 -0400

> Subject: [PATCH] update vc-git
>
> ---

Could you add a meaningful commit message please?  See CONTRIBUTE (under
"Commit messages") for details of Emacs' standard format.

I think the patch is basically okay now, just a few minor nitpicks below.

> +(defun vc-git--git-status-to-vc-state (code-list)
> +  "Convert a list CODE-LIST of two-letter git status strings to a vc status.

This line is too long, I think it should be fine to shorten to just
"Convert CODE-LIST to a vc status".  You explain the format of
CODE-LIST in the next paragraph anyway.

> +Each element of CODE-LIST comes from the first two characters of
> +a line returned by 'git status' and should be passed in the order given by 'git status'.

This paragraph looks unfilled, hit M-q on it.

> +       ;; I have only seen this with a file that is only present in the
> +       ;; index.  Let us call this `removed'

Missing period.

> +      (setq code-list
> +            (mapcar (lambda (s)
> +                      (substring s 0 2))
> +                    (delete "" (split-string status "\0"))))

If you pass a non-nil OMIT-NULLS parameter to split-string, the
(delete ""...) should become unnecessary.

> +      (vc-git--git-status-to-vc-state code-list))))

I would suggest dropping the temporary code-list variable here, and
just do

      (vc-git--git-status-to-vc-state
       (mapcar (lambda (s) (substring s 0 2))
               (split-string status "\0" t)))

Message #100 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: Jonathan Ganc <jonganc <at> gmail.com>, 26066 <at> debbugs.gnu.org,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Sun, 23 Apr 2017 18:45:18 -0400

>> +                    (delete "" (split-string status "\0"))))
> If you pass a non-nil OMIT-NULLS parameter to split-string, the
> (delete ""...) should become unnecessary.

Or use "\0+".


        Stefan

Message #103 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Sun, 23 Apr 2017 20:50:38 -0400

[Message part 1 (text/plain, inline)]

Thanks for the feedback!

Updated with the suggested changes.


On 04/23/2017 06:28 PM, Noam Postavsky wrote:
>> Subject: [PATCH] update vc-git
>>
>> ---
> Could you add a meaningful commit message please?  See CONTRIBUTE (under
> "Commit messages") for details of Emacs' standard format.
>
> I think the patch is basically okay now, just a few minor nitpicks below.
>
>> +(defun vc-git--git-status-to-vc-state (code-list)
>> +  "Convert a list CODE-LIST of two-letter git status strings to a vc status.
> This line is too long, I think it should be fine to shorten to just
> "Convert CODE-LIST to a vc status".  You explain the format of
> CODE-LIST in the next paragraph anyway.
>
>> +Each element of CODE-LIST comes from the first two characters of
>> +a line returned by 'git status' and should be passed in the order given by 'git status'.
> This paragraph looks unfilled, hit M-q on it.
>
>> +       ;; I have only seen this with a file that is only present in the
>> +       ;; index.  Let us call this `removed'
> Missing period.
>
>> +      (setq code-list
>> +            (mapcar (lambda (s)
>> +                      (substring s 0 2))
>> +                    (delete "" (split-string status "\0"))))
> If you pass a non-nil OMIT-NULLS parameter to split-string, the
> (delete ""...) should become unnecessary.
>
>> +      (vc-git--git-status-to-vc-state code-list))))
> I would suggest dropping the temporary code-list variable here, and
> just do
>
>        (vc-git--git-status-to-vc-state
>         (mapcar (lambda (s) (substring s 0 2))
>                 (split-string status "\0" t)))

[0001-Update-vc-git-state-to-use-git-status.patch (text/x-patch, attachment)]

Message #108 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Jonathan Ganc <jonganc <at> gmail.com>
Cc: 26066 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Noam Postavsky <npostavs <at> users.sourceforge.net>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Mon, 24 Apr 2017 11:20:57 +0200

On Apr 23 2017, Jonathan Ganc <jonganc <at> gmail.com> wrote:

> +(defun vc-git--git-status-to-vc-state (code-list)
> +  "Convert CODE-LIST to a vc status.
> +
> +Each element of CODE-LIST comes from the first two characters of
> +a line returned by 'git status' and should be passed in the order
> +given by 'git status'.
> +
> +\(It is necessary to allow CODE-LIST to be a list because
> +sometimes git status returns multiple lines, e.g. for a file that
> +is removed from the index but is present in the HEAD and working
> +tree.) "

I think the last paragraph could just be a code comment, given that you
even put it in parens to de-emphasize it.  Also, extra space at the end.

Andreas.

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

Message #111 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: 26066 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Noam Postavsky <npostavs <at> users.sourceforge.net>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 25 Apr 2017 11:38:39 -0400

[Message part 1 (text/plain, inline)]

Good point. Changed.


On 04/24/2017 05:20 AM, Andreas Schwab wrote:
> On Apr 23 2017, Jonathan Ganc <jonganc <at> gmail.com> wrote:
>
>> +(defun vc-git--git-status-to-vc-state (code-list)
>> +  "Convert CODE-LIST to a vc status.
>> +
>> +Each element of CODE-LIST comes from the first two characters of
>> +a line returned by 'git status' and should be passed in the order
>> +given by 'git status'.
>> +
>> +\(It is necessary to allow CODE-LIST to be a list because
>> +sometimes git status returns multiple lines, e.g. for a file that
>> +is removed from the index but is present in the HEAD and working
>> +tree.) "
> I think the last paragraph could just be a code comment, given that you
> even put it in parens to de-emphasize it.  Also, extra space at the end.
>
> Andreas.
>

[0001-Update-vc-git-state-to-use-git-status.patch (text/x-patch, attachment)]

Message #114 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jonathan Ganc <jonganc <at> gmail.com>, Andreas Schwab <schwab <at> linux-m68k.org>
Cc: 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Noam Postavsky <npostavs <at> users.sourceforge.net>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Wed, 26 Apr 2017 02:49:23 +0300

On 25.04.2017 18:38, Jonathan Ganc wrote:
> Good point. Changed.

Thank you very much, Jonathan, and thanks to the other reviewers!

I'll install the patch shortly.

Message #117 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, Andreas Schwab <schwab <at> linux-m68k.org>
Cc: 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Noam Postavsky <npostavs <at> users.sourceforge.net>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 25 Apr 2017 23:18:15 -0400

Thanks Dmitry! And to all the reviewers, I very much appreciated the 
feedback!


On 04/25/2017 07:49 PM, Dmitry Gutov wrote:
> On 25.04.2017 18:38, Jonathan Ganc wrote:
>> Good point. Changed.
>
> Thank you very much, Jonathan, and thanks to the other reviewers!
>
> I'll install the patch shortly.

Message #120 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jonathan Ganc <jonganc <at> gmail.com>, Andreas Schwab <schwab <at> linux-m68k.org>
Cc: 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Noam Postavsky <npostavs <at> users.sourceforge.net>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Mon, 1 May 2017 04:43:15 +0300

On 26.04.2017 6:18, Jonathan Ganc wrote:
> Thanks Dmitry! And to all the reviewers, I very much appreciated the 
> feedback!

Patch installed, with some minor copy edits. Keeping the bug report open 
because it's actually about the default-directory binding.

Message #123 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jonathan Ganc <jonganc <at> gmail.com>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Mon, 1 May 2017 04:57:33 +0300

On 14.04.2017 3:42, Jonathan Ganc wrote:
> First, I'm sorry for this delayed response. I've had a busy few days 
> (and I needed to think about the issue)!

I'm sorry about the delay as well.

> I'm fine with decoupling the question about default-directory from the 
> patch for git status. I have attached a patch that does not bind 
> default-directory. (And I now have turned in the assignment paperwork to 
> the FSF!).

Thanks! That part is applied now.

>> If vc-git supports calling vc-git-state from outside of the 
>> repository, but not some other commands, or if vc-git-state does but 
>> some other backends' vc-xx-state does not, this will increase 
>> inconsistency and make it harder for the programmers to write 
>> VCS-agnostic code, which is one of the main goals of VC.
> 
> But being vcs-agnostic is more for people who use vc-state or 
> vc-state-refresh, rather than people actually writing the vc-specific 
> functions, no?

The vc-specific functions are supposed to conform to the common 
interface. To be as indistinguishable as feasible.

> If this bug is unchanged, people using vc-state have to 
> ask themselves: "if the backend is git, I have to bind default-directory 
> but if it's not, I don't." That is inconsistent with vc agnosticism.

If that was only the Git backend's problem, I might have agreed.

However, not all other backends bind default-directory. Bzr doesn't, as 
one example. Hg and SVN seem to do that, though. If you have a chance to 
experiment with some other popular ones, please do.

But what about the problems like "I don't have to bind default-directory 
when calling vc-state, but I still have to do that when calling other 
backend commands"?

> That's a valid question and a valid point. But I actually think that 
> adding the binding in vc-git-state furthers the goal of systematically 
> answering them.

Since we'll have to hunt down every other command that needs it (or, 
worse, wait for bug reports and thus spread that effort across several 
Emacs releases), I'm not so sure it would be a great step.

> I doubt we'll be able to find, at one time, people with 
> knowledge of more than 4 or 5 of the vc's (e.g. I know only git) who 
> have the time to address this issue.

I think we only have 4-5 backends which correspond to reasonably popular 
version control systems. The rest can simply follow their example.

> What we can do instead, though, is 
> to encourage people to properly write the backend specific functions.

How?

> The fact that I can't argue in general that an arbitrary vc 
> system needs default-directory set suggests to me that one should leave 
> it to the vc system to decide. For example, I could imagine a backend 
> that wants the current directory to be the root directory of the 
> repository, rather than the directory containing the file.

Such backend (I'm not aware of any, though) could still rebind 
default-directory inside its function.

Message #126 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Jonathan Ganc <jonganc <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Mon, 1 May 2017 15:22:39 -0400

On 04/30/2017 09:57 PM, Dmitry Gutov wrote:
>
> I'm sorry about the delay as well.
>
No worries.
>
>> If this bug is unchanged, people using vc-state have to ask 
>> themselves: "if the backend is git, I have to bind default-directory 
>> but if it's not, I don't." That is inconsistent with vc agnosticism.
>
> However, not all other backends bind default-directory. Bzr doesn't, 
> as one example. Hg and SVN seem to do that, though. If you have a 
> chance to experiment with some other popular ones, please do.
>
But it's not clear to me that bzr needs to bind it. In other words, git 
needs to bind it get a working answer and perhaps bzr doesn't. 
Unfortunately, I only know git. If I get a chance I'll look at some 
others but I've never used them.

Community message: If anyone else who uses other VC systems is reading 
this, it'd great to know if vc-state gives the correct answer if 
default-directory is bound to a random directory!

> But what about the problems like "I don't have to bind 
> default-directory when calling vc-state, but I still have to do that 
> when calling other backend commands"?
>
This is just about clarification: are you thinking that one possible 
solution is not to have any of the vc code bind default-directory, i.e. 
that the caller might always be responsible for that?

Message #129 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jonathan Ganc <jonganc <at> gmail.com>
Cc: npostavs <at> users.sourceforge.net, 26066 <at> debbugs.gnu.org,
 monnier <at> IRO.UMontreal.CA, dgutov <at> yandex.ru
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Mon, 01 May 2017 22:42:25 +0300

> From: Jonathan Ganc <jonganc <at> gmail.com>
> Date: Mon, 1 May 2017 15:22:39 -0400
> 
> Community message: If anyone else who uses other VC systems is reading 
> this, it'd great to know if vc-state gives the correct answer if 
> default-directory is bound to a random directory!

I tried that with bzr, and vc-state produces a wrong result if
default-directory is not set to the correct value.

Message #132 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jonathan Ganc <jonganc <at> gmail.com>, npostavs <at> users.sourceforge.net,
 26066 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 2 May 2017 01:13:40 +0300

On 01.05.2017 22:22, Jonathan Ganc wrote:

>> But what about the problems like "I don't have to bind 
>> default-directory when calling vc-state, but I still have to do that 
>> when calling other backend commands"?
>>
> This is just about clarification: are you thinking that one possible 
> solution is not to have any of the vc code bind default-directory, i.e. 
> that the caller might always be responsible for that?

Suppose so. I do not particularly like any of the obvious options.

Making the caller bind default-directory is not ideal, but sprinkling 
default-directory bindings over all VC command implementations doesn't 
sound great.

The former seems less error-prone in the end, though. And in that 
approach, the caller package might find just one place they have to put 
the default-directory binding at, so that it works for multiple calls.

Message #135 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Jonathan Ganc <jonganc <at> gmail.com>, 26066 <at> debbugs.gnu.org,
 Andreas Schwab <schwab <at> linux-m68k.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Noam Postavsky <npostavs <at> users.sourceforge.net>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Tue, 25 Jun 2019 01:01:27 +0200

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> On 26.04.2017 6:18, Jonathan Ganc wrote:
>> Thanks Dmitry! And to all the reviewers, I very much appreciated the
>> feedback!
>
> Patch installed, with some minor copy edits. Keeping the bug report
> open because it's actually about the default-directory binding.

The original problem here was fixed and the patch applied -- is the
default-directory binding still being considered?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #138 received at 26066 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Jonathan Ganc <jonganc <at> gmail.com>, 26066 <at> debbugs.gnu.org,
 Andreas Schwab <schwab <at> linux-m68k.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Noam Postavsky <npostavs <at> users.sourceforge.net>
Subject: Re: bug#26066: 26.0.50; vc-git-status gives wrong result
Date: Mon, 10 Aug 2020 17:05:34 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Dmitry Gutov <dgutov <at> yandex.ru> writes:
>
>> On 26.04.2017 6:18, Jonathan Ganc wrote:
>>> Thanks Dmitry! And to all the reviewers, I very much appreciated the
>>> feedback!
>>
>> Patch installed, with some minor copy edits. Keeping the bug report
>> open because it's actually about the default-directory binding.
>
> The original problem here was fixed and the patch applied -- is the
> default-directory binding still being considered?

I asked this a year ago, and there was no response, so I'm guessing not,
and I'm closing the bug report.  Please reopen (or refile a new report,
since this is rather long and involved) if this is still an issue.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.