bug#33650: 27.0.50; Root diffs between revisions

bug#33650: 27.0.50; Root diffs between revisions

[Juri Linkov]

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

A VC command that still doesn't allow using diff-mode automatically
thus preventing proper fontification and all other related features
is `vc-root-diff' with a prefix argument HISTORIC.  It has such comment:

      ;; FIXME: this does not work right, `vc-version-diff' ends up
      ;; calling `vc-deduce-fileset' to find the files to diff, and
      ;; that's not what we want here, we want the diff for the VC root dir.

This patch fixes this omission by implementing `vc-root-version-diff'
that is able to compare two revisions even on separate branches from
their roots:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-root-version-diff.patch --]
[-- Type: text/x-diff, Size: 1911 bytes --]

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index de43544864..74979e7381 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1827,6 +1827,28 @@ vc-version-diff
   (vc-diff-internal t (vc-deduce-fileset t) rev1 rev2
 		    (called-interactively-p 'interactive)))
 
+;;;###autoload
+(defun vc-root-version-diff (_files rev1 rev2)
+  "Report diffs between revisions of the whole tree in the repository history."
+  (interactive (vc-diff-build-argument-list-internal))
+  ;; This is a mix of `vc-root-diff' and `vc-version-diff'
+  (when (and (not rev1) rev2)
+    (error "Not a valid revision range"))
+  (let ((backend (vc-deduce-backend))
+        (default-directory default-directory)
+        rootdir)
+    (if backend
+	(setq rootdir (vc-call-backend backend 'root default-directory))
+      (setq rootdir (read-directory-name "Directory for VC root-diff: "))
+      (setq backend (vc-responsible-backend rootdir))
+      (if backend
+	  (setq default-directory rootdir)
+	(error "Directory is not version controlled")))
+    (let ((default-directory rootdir))
+      (vc-diff-internal
+       t (list backend (list rootdir)) rev1 rev2
+       (called-interactively-p 'interactive)))))
+
 ;;;###autoload
 (defun vc-diff (&optional historic not-urgent)
   "Display diffs between file revisions.
@@ -1900,10 +1922,8 @@ vc-root-diff
 saving the buffer."
   (interactive (list current-prefix-arg t))
   (if historic
-      ;; FIXME: this does not work right, `vc-version-diff' ends up
-      ;; calling `vc-deduce-fileset' to find the files to diff, and
-      ;; that's not what we want here, we want the diff for the VC root dir.
-      (call-interactively 'vc-version-diff)
+      ;; We want the diff for the VC root dir.
+      (call-interactively 'vc-root-version-diff)
     (when buffer-file-name (vc-buffer-sync not-urgent))
     (let ((backend (vc-deduce-backend))
 	  (default-directory default-directory)

bug#33650: 27.0.50; Root diffs between revisions

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 06 Dec 2018 23:54:16 +0200
> 
> This patch fixes this omission by implementing `vc-root-version-diff'
> that is able to compare two revisions even on separate branches from
> their roots:

Thanks.  A few comments:

> +(defun vc-root-version-diff (_files rev1 rev2)
> +  "Report diffs between revisions of the whole tree in the repository history."

This doc string doesn't describe the arguments, and doesn't tell how
those arguments are populated in interactive invocations.

I also wonder what would this mean for a VCS such as CVS or RCS, where
there's no meaning of a "tree revision".  I'm guessing this is why the
old code deduced files to diff.  See also the description of vc-diff
in the Emacs manual.

Finally, we need documentation changes to go with this change of
user-visible behavior.

bug#33650: 27.0.50; Root diffs between revisions

[Juri Linkov]

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

>> +(defun vc-root-version-diff (_files rev1 rev2)
>> +  "Report diffs between revisions of the whole tree in the repository history."
>
> This doc string doesn't describe the arguments, and doesn't tell how
> those arguments are populated in interactive invocations.

I copied the doc string from `vc-version-diff' that doesn't describe its
arguments.  Now I fixed the doc string of `vc-version-diff' as well below.

> I also wonder what would this mean for a VCS such as CVS or RCS, where
> there's no meaning of a "tree revision".  I'm guessing this is why the
> old code deduced files to diff.  See also the description of vc-diff
> in the Emacs manual.

`vc-root-diff' is not applicable to VCS without tree revisions, and
the manual says nothing about this.  Maybe the manual should say this
explicitly for `vc-root-diff' as it says it for `vc-print-root-log'
in (info "(emacs) VC Change Log")

     ‘C-x v L’ (‘vc-print-root-log’) displays a ‘*vc-change-log*’ buffer
  showing the history of the entire version-controlled directory tree
  (RCS, SCCS, CVS, and SRC do not support this feature).

> Finally, we need documentation changes to go with this change of
> user-visible behavior.

This feature is already documented in (info "(emacs) Old Revisions")

  ‘C-x v D’
       Compare the entire working tree to the revision you started from
       (‘vc-root-diff’).  With a prefix argument, prompt for two revisions
       and compare their trees.

It didn't work as documented before properly implemented by the patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-root-version-diff.2.patch --]
[-- Type: text/x-diff, Size: 2442 bytes --]

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index dbbc3e2038..ba410dca0e 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1817,7 +1817,8 @@ vc-diff-build-argument-list-internal
 
 ;;;###autoload
 (defun vc-version-diff (_files rev1 rev2)
-  "Report diffs between revisions of the fileset in the repository history."
+  "Report diffs between revisions of the fileset in the repository history.
+REV1 is an older revision, REV2 is a newer revision."
   (interactive (vc-diff-build-argument-list-internal))
   ;; All that was just so we could do argument completion!
   (when (and (not rev1) rev2)
@@ -1827,6 +1828,29 @@ vc-version-diff
   (vc-diff-internal t (vc-deduce-fileset t) rev1 rev2
 		    (called-interactively-p 'interactive)))
 
+;;;###autoload
+(defun vc-root-version-diff (_files rev1 rev2)
+  "Report diffs between revisions of the whole tree in the repository history.
+REV1 is an older revision, REV2 is a newer revision."
+  (interactive (vc-diff-build-argument-list-internal))
+  ;; This is a mix of `vc-root-diff' and `vc-version-diff'
+  (when (and (not rev1) rev2)
+    (error "Not a valid revision range"))
+  (let ((backend (vc-deduce-backend))
+        (default-directory default-directory)
+        rootdir)
+    (if backend
+	(setq rootdir (vc-call-backend backend 'root default-directory))
+      (setq rootdir (read-directory-name "Directory for VC root-diff: "))
+      (setq backend (vc-responsible-backend rootdir))
+      (if backend
+	  (setq default-directory rootdir)
+	(error "Directory is not version controlled")))
+    (let ((default-directory rootdir))
+      (vc-diff-internal
+       t (list backend (list rootdir)) rev1 rev2
+       (called-interactively-p 'interactive)))))
+
 ;;;###autoload
 (defun vc-diff (&optional historic not-urgent)
   "Display diffs between file revisions.
@@ -1900,10 +1924,8 @@ vc-root-diff
 saving the buffer."
   (interactive (list current-prefix-arg t))
   (if historic
-      ;; FIXME: this does not work right, `vc-version-diff' ends up
-      ;; calling `vc-deduce-fileset' to find the files to diff, and
-      ;; that's not what we want here, we want the diff for the VC root dir.
-      (call-interactively 'vc-version-diff)
+      ;; We want the diff for the VC root dir.
+      (call-interactively 'vc-root-version-diff)
     (when buffer-file-name (vc-buffer-sync not-urgent))
     (let ((backend (vc-deduce-backend))
 	  (default-directory default-directory)

bug#33650: 27.0.50; Root diffs between revisions

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33650@debbugs.gnu.org
> Date: Sun, 09 Dec 2018 01:32:47 +0200
> 
> > I also wonder what would this mean for a VCS such as CVS or RCS, where
> > there's no meaning of a "tree revision".  I'm guessing this is why the
> > old code deduced files to diff.  See also the description of vc-diff
> > in the Emacs manual.
> 
> `vc-root-diff' is not applicable to VCS without tree revisions, and
> the manual says nothing about this.  Maybe the manual should say this
> explicitly for `vc-root-diff' as it says it for `vc-print-root-log'
> in (info "(emacs) VC Change Log")

Yes, we should say that.

> > Finally, we need documentation changes to go with this change of
> > user-visible behavior.
> 
> This feature is already documented in (info "(emacs) Old Revisions")
> 
>   ‘C-x v D’
>        Compare the entire working tree to the revision you started from
>        (‘vc-root-diff’).  With a prefix argument, prompt for two revisions
>        and compare their trees.
> 
> It didn't work as documented before properly implemented by the patch.

Isn't the result user-visible change in behavior?

>  (defun vc-version-diff (_files rev1 rev2)
> -  "Report diffs between revisions of the fileset in the repository history."
> +  "Report diffs between revisions of the fileset in the repository history.
> +REV1 is an older revision, REV2 is a newer revision."

We usually mention the arguments in the first line of the doc string.
Something like this:

  Report diffs between revisions REV1 and REV2 in the repository history.

I wonder whether "fileset" needs to be mentioned at all, given your
changes.  When does any "fileset" come into play here?

> +(defun vc-root-version-diff (_files rev1 rev2)
> +  "Report diffs between revisions of the whole tree in the repository history.
> +REV1 is an older revision, REV2 is a newer revision."

Same here: please mention the arguments in the first sentence.

> +    (if backend
> +	(setq rootdir (vc-call-backend backend 'root default-directory))

There's a TAB at the beginning of this line which should be spaces.

Thanks.

bug#33650: 27.0.50; Root diffs between revisions

[Juri Linkov]

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

>> > Finally, we need documentation changes to go with this change of
>> > user-visible behavior.
>>
>> This feature is already documented in (info "(emacs) Old Revisions")
>>
>>   ‘C-x v D’
>>        Compare the entire working tree to the revision you started from
>>        (‘vc-root-diff’).  With a prefix argument, prompt for two revisions
>>        and compare their trees.
>>
>> It didn't work as documented before properly implemented by the patch.
>
> Isn't the result user-visible change in behavior?

Yes, this fix changes user-visible behavior, so perhaps it should be
mentioned in NEWS.

>>  (defun vc-version-diff (_files rev1 rev2)
>> -  "Report diffs between revisions of the fileset in the repository history."
>> +  "Report diffs between revisions of the fileset in the repository history.
>> +REV1 is an older revision, REV2 is a newer revision."
>
> We usually mention the arguments in the first line of the doc string.
> Something like this:
>
>   Report diffs between revisions REV1 and REV2 in the repository history.
>
> I wonder whether "fileset" needs to be mentioned at all, given your
> changes.  When does any "fileset" come into play here?

"fileset" is essential to be mentioned in `vc-version-diff'
whereas the text "in the repository history" is unnecessary.

>> +(defun vc-root-version-diff (_files rev1 rev2)
>> +  "Report diffs between revisions of the whole tree in the repository history.
>> +REV1 is an older revision, REV2 is a newer revision."
>
> Same here: please mention the arguments in the first sentence.

As "fileset" is essential for `vc-version-diff',
"whole tree" is essential for `vc-root-version-diff':


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-root-version-diff.3.patch --]
[-- Type: text/x-diff, Size: 3653 bytes --]

diff --git a/doc/emacs/maintaining.texi b/doc/emacs/maintaining.texi
index 4527c23d9e..e92db9e419 100644
--- a/doc/emacs/maintaining.texi
+++ b/doc/emacs/maintaining.texi
@@ -774,8 +774,7 @@ Old Revisions
 
 @item C-x v D
 Compare the entire working tree to the revision you started from
-(@code{vc-root-diff}).  With a prefix argument, prompt for two
-revisions and compare their trees.
+(@code{vc-root-diff}).
 
 @item C-x v ~
 Prompt for a revision of the current file, and visit it in a separate
@@ -829,7 +828,9 @@ Old Revisions
 it displays the changes in the entire current working tree (i.e., the
 working tree containing the current VC fileset).  If you invoke this
 command from a Dired buffer, it applies to the working tree containing
-the directory.
+the directory.  With a prefix argument, prompt for two revisions and
+compare the entire version-controlled directory trees (RCS, SCCS, CVS,
+and SRC do not support this feature).
 
 @vindex vc-diff-switches
   You can customize the @command{diff} options that @kbd{C-x v =} and
@@ -963,6 +964,7 @@ VC Change Log
 Directory Mode}) or a Dired buffer (@pxref{Dired}), it applies to the
 file listed on the current line.
 
+@kindex C-x v L
 @findex vc-print-root-log
 @findex log-view-toggle-entry-display
   @kbd{C-x v L} (@code{vc-print-root-log}) displays a
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index dbbc3e2038..5ff9f4d5be 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1817,7 +1817,7 @@ vc-diff-build-argument-list-internal
 
 ;;;###autoload
 (defun vc-version-diff (_files rev1 rev2)
-  "Report diffs between revisions of the fileset in the repository history."
+  "Report diffs between REV1 and REV2 revisions of the fileset."
   (interactive (vc-diff-build-argument-list-internal))
   ;; All that was just so we could do argument completion!
   (when (and (not rev1) rev2)
@@ -1827,6 +1827,28 @@ vc-version-diff
   (vc-diff-internal t (vc-deduce-fileset t) rev1 rev2
 		    (called-interactively-p 'interactive)))
 
+;;;###autoload
+(defun vc-root-version-diff (_files rev1 rev2)
+  "Report diffs between REV1 and REV2 revisions of the whole tree."
+  (interactive (vc-diff-build-argument-list-internal))
+  ;; This is a mix of `vc-root-diff' and `vc-version-diff'
+  (when (and (not rev1) rev2)
+    (error "Not a valid revision range"))
+  (let ((backend (vc-deduce-backend))
+        (default-directory default-directory)
+        rootdir)
+    (if backend
+        (setq rootdir (vc-call-backend backend 'root default-directory))
+      (setq rootdir (read-directory-name "Directory for VC root-diff: "))
+      (setq backend (vc-responsible-backend rootdir))
+      (if backend
+          (setq default-directory rootdir)
+        (error "Directory is not version controlled")))
+    (let ((default-directory rootdir))
+      (vc-diff-internal
+       t (list backend (list rootdir)) rev1 rev2
+       (called-interactively-p 'interactive)))))
+
 ;;;###autoload
 (defun vc-diff (&optional historic not-urgent)
   "Display diffs between file revisions.
@@ -1900,10 +1922,8 @@ vc-root-diff
 saving the buffer."
   (interactive (list current-prefix-arg t))
   (if historic
-      ;; FIXME: this does not work right, `vc-version-diff' ends up
-      ;; calling `vc-deduce-fileset' to find the files to diff, and
-      ;; that's not what we want here, we want the diff for the VC root dir.
-      (call-interactively 'vc-version-diff)
+      ;; We want the diff for the VC root dir.
+      (call-interactively 'vc-root-version-diff)
     (when buffer-file-name (vc-buffer-sync not-urgent))
     (let ((backend (vc-deduce-backend))
 	  (default-directory default-directory)

bug#33650: 27.0.50; Root diffs between revisions

[Dmitry Gutov]

On 10.12.2018 2:04, Juri Linkov wrote:
> @item C-x v D
>   Compare the entire working tree to the revision you started from
> -(@code{vc-root-diff}).  With a prefix argument, prompt for two
> -revisions and compare their trees.
> +(@code{vc-root-diff}).

Why remove this part? Isn't it fixed in this patch?

bug#33650: 27.0.50; Root diffs between revisions

[Eli Zaretskii]

> Cc: 33650@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 10 Dec 2018 02:59:08 +0200
> 
> On 10.12.2018 2:04, Juri Linkov wrote:
> > @item C-x v D
> >   Compare the entire working tree to the revision you started from
> > -(@code{vc-root-diff}).  With a prefix argument, prompt for two
> > -revisions and compare their trees.
> > +(@code{vc-root-diff}).
> 
> Why remove this part? Isn't it fixed in this patch?

The above is the short summary of the command.  The longer description
does mention this feature.  So I think this is in line with our style
in describing commands in the manual.

bug#33650: 27.0.50; Root diffs between revisions

[Juri Linkov]

>> @item C-x v D
>>   Compare the entire working tree to the revision you started from
>> -(@code{vc-root-diff}).  With a prefix argument, prompt for two
>> -revisions and compare their trees.
>> +(@code{vc-root-diff}).
>
> Why remove this part? Isn't it fixed in this patch?

For consistency with ‘C-x v L’ (‘vc-print-root-log’) whose
prefix argument is described in the longer description
in (info "(emacs) VC Change Log")

But since you asked I looked closer and see that e.g. a prefix argument
of ‘C-x v =’ is described in the short summary in the same node
(info "(emacs) Old Revisions")

  ‘C-x v =’
       Compare the work files in the current VC fileset with the versions
       you started from (‘vc-diff’).  With a prefix argument, prompt for
       two revisions of the current VC fileset and compare them.

So I installed something better.

bug#33650: 27.0.50; Root diffs between revisions

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33650@debbugs.gnu.org
> Date: Mon, 10 Dec 2018 02:04:30 +0200
> 
> >> It didn't work as documented before properly implemented by the patch.
> >
> > Isn't the result user-visible change in behavior?
> 
> Yes, this fix changes user-visible behavior, so perhaps it should be
> mentioned in NEWS.

I think so too.

> > I wonder whether "fileset" needs to be mentioned at all, given your
> > changes.  When does any "fileset" come into play here?
> 
> "fileset" is essential to be mentioned in `vc-version-diff'
> whereas the text "in the repository history" is unnecessary.
> 
> >> +(defun vc-root-version-diff (_files rev1 rev2)
> >> +  "Report diffs between revisions of the whole tree in the repository history.
> >> +REV1 is an older revision, REV2 is a newer revision."
> >
> > Same here: please mention the arguments in the first sentence.
> 
> As "fileset" is essential for `vc-version-diff',
> "whole tree" is essential for `vc-root-version-diff':

OK, thanks.

bug#33650: 27.0.50; Root diffs between revisions

[Juri Linkov]

>> > Isn't the result user-visible change in behavior?
>>
>> Yes, this fix changes user-visible behavior, so perhaps it should be
>> mentioned in NEWS.
>
> I think so too.
>
>> As "fileset" is essential for `vc-version-diff',
>> "whole tree" is essential for `vc-root-version-diff':
>
> OK, thanks.

Pushed to master and closed.