bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Wolfgang Scherer]

Here is another per-tree/per-directory bug, I just found:

1. Prepare a Mercurial repository `check-hgs` with sub directories `sub1`
   and `sub2` and the files `sub1/a`, `sub2/x`,

   .. code-block:: sh

      mkdir -p check-hgs/sub1 check-hgs/sub2
      touch check-hgs/sub1/a check-hgs/sub2/x
      cd check-hgs && hg init

2. Find file `check-hgs/sub1/a`.

3. Press `C-x v d RET`, which will show a "Working dir" different from "Root":

   .. code-block:: text

      Working dir: check-hgs/sub1/
      Root       : check-hgs

                           ./
                           sub1/
      unregistered         sub1/a
                           sub2/
      unregistered         sub2/x

4. Go to line

   "unregistered         sub2/x"

   and press `f`.

5. Press `C-x d`.

   The default directory is "check-hgs/sub1/sub2/", which is wrong. It
   should be "check-hgs/sub2/".

The problem results from the call to `expand-file-name` in
`vc-dir-current-file`, which uses the default directory, but
`vc-dir-fileinfo->name` delivers a filename relative to the root
directory.

.. code-block:: elisp

   (defun vc-dir-current-file ()
     (let ((node (ewoc-locate vc-ewoc)))
       (unless node
     (error "No file available"))
       (expand-file-name (vc-dir-fileinfo->name (ewoc-data node)))))


However, modifying `vc-dir-current-file` does not solve the problem,
since all other invocations of `vc-dir-fileinfo->name` ultimately
suffer from the same deficiency.

It is IMHO better to remove the hybrid state of `vc-dir-mode`, where
the default directory is different from the root directory.
“

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Dmitry Gutov]

On 01.02.2020 3:13, Wolfgang Scherer wrote:
> 3. Press `C-x v d RET`, which will show a "Working dir" different from "Root":
> 
>     .. code-block:: text
> 
>        Working dir: check-hgs/sub1/
>        Root       : check-hgs
> 
>                             ./
>                             sub1/
>        unregistered         sub1/a
>                             sub2/
>        unregistered         sub2/x

Maybe this should be fixed in vc-hg-status-files by not showing 
"outside" files and using proper relative names.

Compare how VC-Dir works with Git: when you open it in a subdirectory, 
it only shows changed files in that directory, and with proper relative 
names.

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Dmitry Gutov]

On 01.02.2020 3:13, Wolfgang Scherer wrote:
> It is IMHO better to remove the hybrid state of `vc-dir-mode`, where
> the default directory is different from the root directory

If you mean to always open VC-Dir in repository roots, I'm not sure we 
can do that.

We had a discussion about that at one point (purely for reasons of 
usability), and turns out some people routinely open VC-Dir in 
subdirectories. To focus on a subset of a project, or whatever.

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Wolfgang Scherer]

Am 01.02.20 um 08:20 schrieb Dmitry Gutov:
> On 01.02.2020 3:13, Wolfgang Scherer wrote:
>> 3. Press `C-x v d RET`, which will show a "Working dir" different from "Root":
>>
>>     .. code-block:: text
>>
>>        Working dir: check-hgs/sub1/
>>        Root       : check-hgs
>>
>>                             ./
>>                             sub1/
>>        unregistered         sub1/a
>>                             sub2/
>>        unregistered         sub2/x
>
> Maybe this should be fixed in vc-hg-status-files by not showing "outside" files and using proper relative names.
>
> Compare how VC-Dir works with Git: when you open it in a subdirectory, it only shows changed files in that directory, and with proper relative names.

The behavior should be consistent. In Emacs 24/25 vc-dir-mode for Mercurial behaved exactly like Git. Since the behavior for Mercurial changed some time after Emacs 24, I just assumed (probably because of my bias towards `dvc`) that the new behavior was intended.

I really have no preference how the faulty behavior is fixed, since I have long since defined some advice before `vc-dir-prepare-status-buffer`. I have also not checked the documentation to find out what the intended behavior is.

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Dmitry Gutov]

On 03.02.2020 1:35, Wolfgang Scherer wrote:
> 
> Am 01.02.20 um 08:20 schrieb Dmitry Gutov:
>> On 01.02.2020 3:13, Wolfgang Scherer wrote:
>>> 3. Press `C-x v d RET`, which will show a "Working dir" different from "Root":
>>>
>>>      .. code-block:: text
>>>
>>>         Working dir: check-hgs/sub1/
>>>         Root       : check-hgs
>>>
>>>                              ./
>>>                              sub1/
>>>         unregistered         sub1/a
>>>                              sub2/
>>>         unregistered         sub2/x
>>
>> Maybe this should be fixed in vc-hg-status-files by not showing "outside" files and using proper relative names.
>>
>> Compare how VC-Dir works with Git: when you open it in a subdirectory, it only shows changed files in that directory, and with proper relative names.
> 
> The behavior should be consistent.

Right.

> In Emacs 24/25 vc-dir-mode for Mercurial behaved exactly like Git. Since the behavior for Mercurial changed some time after Emacs 24, I just assumed (probably because of my bias towards `dvc`) that the new behavior was intended.

In 25 or 26 Daniel added a Lisp-based faster implementation for some 
commands (de76a167dc09dc695a5acebabb7ab354a6bf556e) since apparently Hg 
itself was too slow.

So this bug must have arrived with that implementation.

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Wolfgang Scherer]

Am 03.02.20 um 14:11 schrieb Dmitry Gutov:
> On 03.02.2020 1:35, Wolfgang Scherer wrote:
>>
>> Am 01.02.20 um 08:20 schrieb Dmitry Gutov:
>>> On 01.02.2020 3:13, Wolfgang Scherer wrote:
>>>> 3. Press `C-x v d RET`, which will show a "Working dir" different from "Root":
>>>>
>>>>      .. code-block:: text
>>>>
>>>>         Working dir: check-hgs/sub1/
>>>>         Root       : check-hgs
>>>>
>>>>                              ./
>>>>                              sub1/
>>>>         unregistered         sub1/a
>>>>                              sub2/
>>>>         unregistered         sub2/x
>>>
>>> Maybe this should be fixed in vc-hg-status-files by not showing "outside" files and using proper relative names.
>>>
>>> Compare how VC-Dir works with Git: when you open it in a subdirectory, it only shows changed files in that directory, and with proper relative names.
>>
>> The behavior should be consistent.
>
> Right.
>
>> In Emacs 24/25 vc-dir-mode for Mercurial behaved exactly like Git. Since the behavior for Mercurial changed some time after Emacs 24, I just assumed (probably because of my bias towards `dvc`) that the new behavior was intended.
>
> In 25 or 26 Daniel added a Lisp-based faster implementation for some commands (de76a167dc09dc695a5acebabb7ab354a6bf556e) since apparently Hg itself was too slow.
>
> So this bug must have arrived with that implementation.


Reverting de76a167dc09dc695a5acebabb7ab354a6bf556e does not change anything for me.

I had to revert 06083cf41c473404d246de9b91a0116f38c5485f to restore the correct behavior:

Author: Dmitry Gutov <dgutov@yandex.ru>
Date:   Sat Jan 30 06:55:32 2016 +0300

    Don't pass DIR to 'hg status'
   
    * lisp/vc/vc-hg.el (vc-hg-dir-status-files):
    Don't pass DIR to 'hg status' (bug#22481).

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Dmitry Gutov]

On 04.02.2020 19:04, Wolfgang Scherer wrote:
> Reverting de76a167dc09dc695a5acebabb7ab354a6bf556e does not change anything for me.
> 
> I had to revert 06083cf41c473404d246de9b91a0116f38c5485f to restore the correct behavior:
> 
> Author: Dmitry Gutov<dgutov@yandex.ru>
> Date:   Sat Jan 30 06:55:32 2016 +0300
> 
>      Don't pass DIR to 'hg status'
>     
>      * lisp/vc/vc-hg.el (vc-hg-dir-status-files):
>      Don't pass DIR to 'hg status' (bug#22481).

Good catch, thank you. Could you try this patch?

diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
index c9a2cb412e..a91e1f5ad6 100644
--- a/lisp/vc/vc-hg.el
+++ b/lisp/vc/vc-hg.el
@@ -1352,14 +1352,14 @@ vc-hg-after-dir-status
  ;; Follows vc-exec-after.
  (declare-function vc-set-async-update "vc-dispatcher" (process-buffer))

-(defun vc-hg-dir-status-files (_dir files update-function)
+(defun vc-hg-dir-status-files (dir files update-function)
    ;; XXX: We can't pass DIR directly to 'hg status' because that
    ;; returns all ignored files if FILES is non-nil (bug#22481).
-  ;; If honoring DIR ever becomes important, try using '-I DIR/'.
-  (vc-hg-command (current-buffer) 'async files
-                 "status"
-                 (concat "-mardu" (if files "i"))
-                 "-C")
+  (let ((default-directory dir))
+    (vc-hg-command (current-buffer) 'async files
+                   "status" "re:" "-I" "."
+                   (concat "-mardu" (if files "i"))
+                   "-C"))
    (vc-run-delayed
      (vc-hg-after-dir-status update-function)))

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Wolfgang Scherer]

Am 05.02.20 um 16:56 schrieb Dmitry Gutov:
> On 04.02.2020 19:04, Wolfgang Scherer wrote:
>> Reverting de76a167dc09dc695a5acebabb7ab354a6bf556e does not change anything for me.
>>
>> I had to revert 06083cf41c473404d246de9b91a0116f38c5485f to restore the correct behavior:
>>
> Good catch, thank you. Could you try this patch?
>
Works fine. Directory listing in vc-dir-mode is truncated at sub-directory. Files are processed correctly.

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Dmitry Gutov]

On 06.02.2020 16:25, Wolfgang Scherer wrote:
> 
> Am 05.02.20 um 16:56 schrieb Dmitry Gutov:
>> On 04.02.2020 19:04, Wolfgang Scherer wrote:
>>> Reverting de76a167dc09dc695a5acebabb7ab354a6bf556e does not change anything for me.
>>>
>>> I had to revert 06083cf41c473404d246de9b91a0116f38c5485f to restore the correct behavior:
>>>
>> Good catch, thank you. Could you try this patch?
>>
> Works fine. Directory listing in vc-dir-mode is truncated at sub-directory. Files are processed correctly.

Very good.

Eli, I'd like to get it into emacs-27. OK?

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 6 Feb 2020 16:38:54 +0300
> 
> >> Good catch, thank you. Could you try this patch?
> >>
> > Works fine. Directory listing in vc-dir-mode is truncated at sub-directory. Files are processed correctly.
> 
> Very good.
> 
> Eli, I'd like to get it into emacs-27. OK?

I'll have to ask you to walk me through the change and explain how it
solves the original issue.

I tried to figure that out myself, but it's too hard: the patch has no
log message, the function being changed has no doc string which could
describe its contract, and the Mercurial documentation I've read seems
to contradict what you are doing (e.g., it says "re:" should be
immediately followed by a regexp).  Sorry, I cannot make up my mind
whether this is a safe change without understanding it better.

Thanks.

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Dmitry Gutov]

On 07.02.2020 12:53, Eli Zaretskii wrote:
> I'll have to ask you to walk me through the change and explain how it
> solves the original issue.

There are two changes:

1. Having the returned file names relative to the working dir instead of 
the repository root. For that, we change 'hg status' to 'hg status re:'.

2. Only including the files inside the working dir in the output. That 
happens by adding '-I .' to the arguments. Otherwise, Mercurial adds, in 
the presented scenario, an entry like '../sub2/x', which is extraneous.

> the Mercurial documentation I've read seems
> to contradict what you are doing (e.g., it says "re:" should be
> immediately followed by a regexp).

You can search the manual for "hg status re:".

I agree it's a bit unclear (it also mentions "hg help patterns" for some 
reason), but here's an SO answer that got me this far: 
https://stackoverflow.com/a/46743585/615245

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Eli Zaretskii]

> Cc: 39380@debbugs.gnu.org, Wolfgang.Scherer@gmx.de
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 7 Feb 2020 14:03:21 +0300
> 
> On 07.02.2020 12:53, Eli Zaretskii wrote:
> > I'll have to ask you to walk me through the change and explain how it
> > solves the original issue.
> 
> There are two changes:
> 
> 1. Having the returned file names relative to the working dir instead of 
> the repository root. For that, we change 'hg status' to 'hg status re:'.

Why do we want the file names to be relative?  If default-directory of
the buffer is set correctly, that shouldn't be necessary, and if the
file names are absolute, then the default-directory doesn't even
matter much.

> 2. Only including the files inside the working dir in the output. That 
> happens by adding '-I .' to the arguments. Otherwise, Mercurial adds, in 
> the presented scenario, an entry like '../sub2/x', which is extraneous.

This change should be in NEWS, IMO.

> > the Mercurial documentation I've read seems
> > to contradict what you are doing (e.g., it says "re:" should be
> > immediately followed by a regexp).
> 
> You can search the manual for "hg status re:".

Which only finds a single example, where this notation is not
explained at all (and which I already saw).

> I agree it's a bit unclear (it also mentions "hg help patterns" for some 
> reason), but here's an SO answer that got me this far: 
> https://stackoverflow.com/a/46743585/615245

Isn't it disturbing that we are using a feature that doesn't seem to
be documented?

But to get to the issue at hand: you are saying that "C-x v d" in a
subdirectory of the root of a Mercurial repository became broken in
Emacs 25.1?

Also, is DIR guaranteed to be the directory where FILES live?

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Dmitry Gutov]

On 07.02.2020 17:00, Eli Zaretskii wrote:
>> Cc: 39380@debbugs.gnu.org, Wolfgang.Scherer@gmx.de
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Fri, 7 Feb 2020 14:03:21 +0300
>>
>> On 07.02.2020 12:53, Eli Zaretskii wrote:
>>> I'll have to ask you to walk me through the change and explain how it
>>> solves the original issue.
>>
>> There are two changes:
>>
>> 1. Having the returned file names relative to the working dir instead of
>> the repository root. For that, we change 'hg status' to 'hg status re:'.
> 
> Why do we want the file names to be relative?  If default-directory of
> the buffer is set correctly, that shouldn't be necessary, and if the
> file names are absolute, then the default-directory doesn't even
> matter much.

They are already relative, but against the repository root.

And people want to open vc-dir buffers in any directory, not just in 
repository roots.

We could force the file names to be absolute, and then pipe them through 
file-relative-name, but that's just extra work.

>> 2. Only including the files inside the working dir in the output. That
>> happens by adding '-I .' to the arguments. Otherwise, Mercurial adds, in
>> the presented scenario, an entry like '../sub2/x', which is extraneous.
> 
> This change should be in NEWS, IMO.

It's a bugfix for an older, unintended change. It's not a new behavior.

>>> the Mercurial documentation I've read seems
>>> to contradict what you are doing (e.g., it says "re:" should be
>>> immediately followed by a regexp).
>>
>> You can search the manual for "hg status re:".
> 
> Which only finds a single example, where this notation is not
> explained at all (and which I already saw).

Not my fault.

>> I agree it's a bit unclear (it also mentions "hg help patterns" for some
>> reason), but here's an SO answer that got me this far:
>> https://stackoverflow.com/a/46743585/615245
> 
> Isn't it disturbing that we are using a feature that doesn't seem to
> be documented?

It's been there for enough years, and it's the best choice I've found so 
far.

> But to get to the issue at hand: you are saying that "C-x v d" in a
> subdirectory of the root of a Mercurial repository became broken in
> Emacs 25.1?
> 
> Also, is DIR guaranteed to be the directory where FILES live?

Yes, see the description of dir-status-files in the top commentary in vc.el.

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Eli Zaretskii]

> Cc: 39380@debbugs.gnu.org, Wolfgang.Scherer@gmx.de
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 8 Feb 2020 04:20:26 +0300
> 
> On 07.02.2020 17:00, Eli Zaretskii wrote:
> >> Cc: 39380@debbugs.gnu.org, Wolfgang.Scherer@gmx.de
> >> From: Dmitry Gutov <dgutov@yandex.ru>
> >> Date: Fri, 7 Feb 2020 14:03:21 +0300
> >>
> >> On 07.02.2020 12:53, Eli Zaretskii wrote:
> >>> I'll have to ask you to walk me through the change and explain how it
> >>> solves the original issue.
> >>
> >> There are two changes:
> >>
> >> 1. Having the returned file names relative to the working dir instead of
> >> the repository root. For that, we change 'hg status' to 'hg status re:'.
> > 
> > Why do we want the file names to be relative?  If default-directory of
> > the buffer is set correctly, that shouldn't be necessary, and if the
> > file names are absolute, then the default-directory doesn't even
> > matter much.
> 
> They are already relative, but against the repository root.
> 
> And people want to open vc-dir buffers in any directory, not just in 
> repository roots.

If I say "hg status" in a subdirectory, I by default get file names
relative to the root.  Wouldn't Mercurial users be surprised that
Emacs produces a different display?  (I understand that there's a
recent tendency to make it easier to produce relative file names, and
there's even a config option to make that the default, but I'm asking
whether we should force this behavior on users even if their
preference is not to.)

> We could force the file names to be absolute, and then pipe them through 
> file-relative-name, but that's just extra work.

I'm more bothered by the user-facing changes in this respect than by
algorithmic details.  Would it work to set the default-directory of
the buffer to the repository's root instead? would that solve the
original problem?

If setting the default-directory could be a solution, I'd prefer doing
that on the release branch, so that user-facing behavior is unchanged.
We could then make changes on master to offer a defcustom that is
parallel to the commands.status.relative config setting, and if that's
set to prefer relative names, use your modified code.  WDYT?

> > This change should be in NEWS, IMO.
> 
> It's a bugfix for an older, unintended change. It's not a new behavior.

It changes how files are presented to the user, so I think it has
user-visible effects that should be mentioned in NEWS.

> > Also, is DIR guaranteed to be the directory where FILES live?
> 
> Yes, see the description of dir-status-files in the top commentary in vc.el.

Thanks.

What about this question:

> you are saying that "C-x v d" in a subdirectory of the root of a
> Mercurial repository became broken in Emacs 25.1?

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Dmitry Gutov]

On 08.02.2020 11:04, Eli Zaretskii wrote:

>> And people want to open vc-dir buffers in any directory, not just in
>> repository roots.
> 
> If I say "hg status" in a subdirectory, I by default get file names
> relative to the root.  Wouldn't Mercurial users be surprised that
> Emacs produces a different display?  (I understand that there's a
> recent tendency to make it easier to produce relative file names, and
> there's even a config option to make that the default, but I'm asking
> whether we should force this behavior on users even if their
> preference is not to.)

VC is intended to provide a unified interface across backends, even if 
it's at times different from the "native" behaviors.

Anyway, it's very easy to get the names against the root anyway: just 
open VC-Dir there. And it's the default input: M-x vc-dir RET.

>> We could force the file names to be absolute, and then pipe them through
>> file-relative-name, but that's just extra work.
> 
> I'm more bothered by the user-facing changes in this respect than by
> algorithmic details.  Would it work to set the default-directory of
> the buffer to the repository's root instead? would that solve the
> original problem?

Again, it's a bugfix. VC-Dir expects this, other backends work like 
this, and vc-hg worked like this until Emacs 24 or 25.

And the present behavior makes VC-Dir have broken behavior.

There are only two possible reasons the buggy behavior wasn't noticed: 
a) Hg users are a minority among our users, b) most people open VC-Dir 
in repository root anyway. IOW, the difference wasn't triggered/seen.

>>> This change should be in NEWS, IMO.
>>
>> It's a bugfix for an older, unintended change. It's not a new behavior.
> 
> It changes how files are presented to the user, so I think it has
> user-visible effects that should be mentioned in NEWS.

I don't mind adding some NEWS entry, but, like said above, the users 
likely didn't see the change.

> What about this question:
> 
>> you are saying that "C-x v d" in a subdirectory of the root of a
>> Mercurial repository became broken in Emacs 25.1?

Yes. :-(

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Eli Zaretskii]

> Cc: 39380@debbugs.gnu.org, Wolfgang.Scherer@gmx.de
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 8 Feb 2020 12:36:46 +0300
> 
> > If I say "hg status" in a subdirectory, I by default get file names
> > relative to the root.  Wouldn't Mercurial users be surprised that
> > Emacs produces a different display?  (I understand that there's a
> > recent tendency to make it easier to produce relative file names, and
> > there's even a config option to make that the default, but I'm asking
> > whether we should force this behavior on users even if their
> > preference is not to.)
> 
> VC is intended to provide a unified interface across backends, even if 
> it's at times different from the "native" behaviors.
> 
> Anyway, it's very easy to get the names against the root anyway: just 
> open VC-Dir there. And it's the default input: M-x vc-dir RET.

OK.

bug#39380: 26.3: Opening files in vc-dir-mode with differing root and working dir fails

[Dmitry Gutov]

Version: 27.1

On 08.02.2020 12:11, Eli Zaretskii wrote:
>> Cc: 39380@debbugs.gnu.org, Wolfgang.Scherer@gmx.de
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Sat, 8 Feb 2020 12:36:46 +0300
>>
>>> If I say "hg status" in a subdirectory, I by default get file names
>>> relative to the root.  Wouldn't Mercurial users be surprised that
>>> Emacs produces a different display?  (I understand that there's a
>>> recent tendency to make it easier to produce relative file names, and
>>> there's even a config option to make that the default, but I'm asking
>>> whether we should force this behavior on users even if their
>>> preference is not to.)
>>
>> VC is intended to provide a unified interface across backends, even if
>> it's at times different from the "native" behaviors.
>>
>> Anyway, it's very easy to get the names against the root anyway: just
>> open VC-Dir there. And it's the default input: M-x vc-dir RET.
> 
> OK.

Taking this as the approval of the patch, pushed.

Using "re:" adds a bit of a performance overhead, but it's probably not 
really noticeable on small-to-medium projects (and on a big one it was 
~100ms where without it dir-status-files takes ~1s).