Is vc-diff-mergebase broken on master?

Is vc-diff-mergebase broken on master?

[Matthias Meulien]

Hello,

I compiled from a clean Git repository with HEAD pointing to master
(a1887cc5e6) on both my personal computer and the one I use at work. And
vc-diff-mergebase is now unusable: Whatever I choose for the older and
the newer revisions, I get a "No changes between..." messages.

Am I missing something? I do lots of merge request review and use this
command on a daily basis, so I suspect a regression has been
introduced.

Am I the only one facing troubles with recent master and vc-git?
-- 
Matthias

Re: Is vc-diff-mergebase broken on master?

[Daniel Martín]

Matthias Meulien <orontee@gmail.com> writes:

> Hello,
>
> I compiled from a clean Git repository with HEAD pointing to master
> (a1887cc5e6) on both my personal computer and the one I use at work. And
> vc-diff-mergebase is now unusable: Whatever I choose for the older and
> the newer revisions, I get a "No changes between..." messages.
>
> Am I missing something? I do lots of merge request review and use this
> command on a daily basis, so I suspect a regression has been
> introduced.
>
> Am I the only one facing troubles with recent master and vc-git?

I suggest you M-x report-emacs-bug.  I also see the same anomalous
behavior you describe.

Re: Is vc-diff-mergebase broken on master?

[Dmitry Gutov]

Hi Matthias,

On 30.08.2021 23:39, Matthias Meulien wrote:
> I compiled from a clean Git repository with HEAD pointing to master
> (a1887cc5e6) on both my personal computer and the one I use at work. And
> vc-diff-mergebase is now unusable: Whatever I choose for the older and
> the newer revisions, I get a "No changes between..." messages.
> 
> Am I missing something? I do lots of merge request review and use this
> command on a daily basis, so I suspect a regression has been
> introduced.
> 
> Am I the only one facing troubles with recent master and vc-git?

Thanks for the email. It is unfortunately fallout from bug#39452.

I just pushed commit d2ad64b which seems to fix vc-diff-mergebase, as 
well similar problems that could still remain in any less frequently 
used commands, check it out.

That said, new contributions to vc-tests.el would be quite welcome.

If there was even one test scenario in there that called 
vc-diff-mergebase, we wouldn't have let this bug slip by.

Re: Is vc-diff-mergebase broken on master?

[Matthias Meulien]

Hi Dmitry,

Dmitry Gutov <dgutov@yandex.ru> writes:
> I just pushed commit d2ad64b which seems to fix vc-diff-mergebase, as
> well similar problems that could still remain in any less frequently
> used commands, check it out.

Done. I confirm vc-diff-mergebase works is back. As you said, there was
similar problems in other places: I saw missing tags or branch names in
*vc-change-log* buffers. I'll let you know if I still see any of those
problems.

Thanks a lot for your reactivity!

> That said, new contributions to vc-tests.el would be quite welcome.

Good point.
-- 
Matthias

Re: Is vc-diff-mergebase broken on master?

[Matthias Meulien]

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

Hi Dmitry,

Dmitry Gutov <dgutov@yandex.ru> writes:

> (...) I just pushed commit d2ad64b which seems to fix
> vc-diff-mergebase, as well similar problems that could still remain in
> any less frequently used commands, check it out.
>
> That said, new contributions to vc-tests.el would be quite welcome.
>
> If there was even one test scenario in there that called
> vc-diff-mergebase, we wouldn't have let this bug slip by.

I tried to write a test for with vc-version-diff to start with something
that looks easier to me. (Almost copy-pasta I'm afraid of.)

I installed RCS, CVS, SVN, Git, Bzr. Unfortunately the test fails with
the first two; Still no idea why...

Since it's the first time I use ERT and before I spend more time
investigating, can you confirm it's the kind of test you had in mind?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Extend-vc-tests.patch --]
[-- Type: text/x-diff, Size: 3521 bytes --]

From 6fc74be7a410233c0f71651ffdac49adbf82334e Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Fri, 3 Sep 2021 00:42:29 +0200
Subject: [PATCH] Extend vc tests

* test/lisp/vc/vc-tests.el (vc-test--version-diff): Test vc-version-diff
---
 test/lisp/vc/vc-tests.el | 71 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/test/lisp/vc/vc-tests.el b/test/lisp/vc/vc-tests.el
index f2807a3f06..beb79d1b36 100644
--- a/test/lisp/vc/vc-tests.el
+++ b/test/lisp/vc/vc-tests.el
@@ -547,6 +547,65 @@ vc-test--checkout-model
         (if tempdir (delete-directory tempdir t))
         (run-hooks 'vc-test--cleanup-hook)))))
 
+(defun vc-test--version-diff (backend)
+  "Check the diff version of a repository."
+
+  (let ((vc-handled-backends `(,backend))
+	(default-directory
+	  (file-name-as-directory
+	   (expand-file-name
+	    (make-temp-name "vc-test") temporary-file-directory)))
+	(process-environment process-environment)
+	tempdir
+	vc-test--cleanup-hook)
+    (when (eq backend 'Bzr)
+      (setq tempdir (make-temp-file "vc-test--version-diff" t)
+	    process-environment (cons (format "BZR_HOME=%s" tempdir)
+				      process-environment)))
+
+    (unwind-protect
+	(progn
+	  ;; Cleanup.
+	  (add-hook
+	   'vc-test--cleanup-hook
+	   `(lambda () (delete-directory ,default-directory 'recursive)))
+
+	  ;; Create empty repository.  Check repository checkout model.
+	  (make-directory default-directory)
+	  (vc-test--create-repo-function backend)
+
+	  (let* ((tmp-name (expand-file-name "foo" default-directory))
+                 (files (list (file-name-nondirectory tmp-name))))
+	    ;; Write and register a new file.
+	    (write-region "originaltext" nil tmp-name nil 'nomessage)
+	    (vc-register (list backend files))
+
+            ;; Checkin file.
+            (let ((buff (find-file tmp-name)))
+              (with-current-buffer buff
+                  (progn
+                    (vc-checkin files backend)
+                    (insert "Testing vc-version-diff")
+                    (log-edit-done))))
+
+            ;; Modify file content.
+            (write-region "updatedtext" nil tmp-name nil 'nomessage)
+
+	    ;; Check version diff.
+            (message "vc-version-diff")
+            (vc-version-diff files nil nil)
+            (with-current-buffer "*vc-diff*"
+              (progn
+                (let ((rawtext (buffer-substring-no-properties (point-min)
+                                                               (point-max))))
+                  (should (string-search "-originaltext" rawtext))
+                  (should (string-search "+updatedtext" rawtext)))))))
+
+      ;; Save exit.
+      (ignore-errors
+        (if tempdir (delete-directory tempdir t))
+        (run-hooks 'vc-test--cleanup-hook)))))
+
 (defun vc-test--rename-file (backend)
   "Check the rename-file action."
 
@@ -712,6 +771,18 @@ vc-test--arch-enabled
               ',(intern
                  (format "vc-test-%s01-register" backend-string))))))
           (vc-test--rename-file ',backend))
+
+        (ert-deftest
+	    ,(intern (format "vc-test-%s06-version-diff" backend-string)) ()
+	  ,(format "Check `vc-version-diff' for the %s backend."
+		   backend-string)
+	  (skip-unless
+	   (ert-test-passed-p
+	    (ert-test-most-recent-result
+	     (ert-get-test
+	      ',(intern
+		 (format "vc-test-%s01-register" backend-string))))))
+	  (vc-test--version-diff ',backend))
         ))))
 
 (provide 'vc-tests)
-- 
2.30.2


[-- Attachment #3: Type: text/plain, Size: 14 bytes --]


-- 
Matthias

Re: Is vc-diff-mergebase broken on master?

[Dmitry Gutov]

On 03.09.2021 01:54, Matthias Meulien wrote:
> I tried to write a test for with vc-version-diff to start with something
> that looks easier to me. (Almost copy-pasta I'm afraid of.)
> 
> I installed RCS, CVS, SVN, Git, Bzr. Unfortunately the test fails with
> the first two; Still no idea why...
> 
> Since it's the first time I use ERT and before I spend more time
> investigating, can you confirm it's the kind of test you had in mind?

Yes, very good. Thank you.

I don't have either of RCS/CVS installed, and have very little real 
experience with either as well.

We can wait a few days for somebody to chime in with a fix, or failing 
that, commit this version (maybe someone else would like to contribute a 
fix upon seeing a test failure).

We can also disable the test on those VCS, of course (adding an (or ...) 
to the skip-unless form near the end of the patch).

Re: Is vc-diff-mergebase broken on master?

[Matthias Meulien]

Dmitry Gutov <dgutov@yandex.ru> writes:

> I don't have either of RCS/CVS installed, and have very little real
> experience with either as well.
>
> We can wait a few days for somebody to chime in with a fix, or failing
> that, commit this version (maybe someone else would like to contribute
> a fix upon seeing a test failure).

Don't waste time with this. I've learned to execute tests step by step
and now see meaningfull errors. For example, RCS expects files to be
locked before checkin. I'll work on an updated patch fixing RCS/CVS
tests.
-- 
Matthias

Re: Is vc-diff-mergebase broken on master?

[Matthias Meulien]

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

Matthias Meulien <orontee@gmail.com> writes:

> (...) I'll work on an updated patch fixing RCS/CVS tests.

Here it is:


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

From b9f8b097701a78d4e11b1e85699ba8070b98f163 Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Fri, 3 Sep 2021 00:42:29 +0200
Subject: [PATCH] Test vc-version-diff

* test/lisp/vc/vc-tests.el (vc-test--version-diff): Test vc-version-diff
---
 test/lisp/vc/vc-tests.el | 78 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/test/lisp/vc/vc-tests.el b/test/lisp/vc/vc-tests.el
index dfe89c519b..f8c843ff50 100644
--- a/test/lisp/vc/vc-tests.el
+++ b/test/lisp/vc/vc-tests.el
@@ -547,6 +547,72 @@ vc-test--checkout-model
         (if tempdir (delete-directory tempdir t))
         (run-hooks 'vc-test--cleanup-hook)))))
 
+(defun vc-test--version-diff (backend)
+  "Check the diff version of a repository."
+
+  (let ((vc-handled-backends `(,backend))
+	(default-directory
+	  (file-name-as-directory
+	   (expand-file-name
+	    (make-temp-name "vc-test") temporary-file-directory)))
+	(process-environment process-environment)
+	tempdir
+	vc-test--cleanup-hook)
+    (when (eq backend 'Bzr)
+      (setq tempdir (make-temp-file "vc-test--version-diff" t)
+	    process-environment (cons (format "BZR_HOME=%s" tempdir)
+				      process-environment)))
+
+    (unwind-protect
+	(progn
+	  ;; Cleanup.
+	  (add-hook
+	   'vc-test--cleanup-hook
+	   `(lambda () (delete-directory ,default-directory 'recursive)))
+
+	  ;; Create empty repository.  Check repository checkout model.
+	  (make-directory default-directory)
+	  (vc-test--create-repo-function backend)
+
+	  (let* ((tmp-name (expand-file-name "foo" default-directory))
+                 (files (list (file-name-nondirectory tmp-name))))
+	    ;; Write and register a new file.
+	    (write-region "originaltext" nil tmp-name nil 'nomessage)
+	    (vc-register (list backend files))
+
+            (let ((buff (find-file tmp-name)))
+              (with-current-buffer buff
+                (progn
+                  ;; Optionally checkout file.
+                  (when (or (eq backend 'RCS) (eq backend 'CVS))
+                    (vc-checkout tmp-name))
+
+                  ;; Checkin file.
+                  (vc-checkin files backend)
+                  (insert "Testing vc-version-diff")
+                  (log-edit-done))))
+
+            ;; Modify file content.
+            (when (or (eq backend 'RCS) (eq backend 'CVS))
+              (vc-checkout tmp-name))
+            (write-region "updatedtext" nil tmp-name nil 'nomessage)
+
+	    ;; Check version diff.
+            (vc-version-diff files nil nil)
+            (should (bufferp (get-buffer "*vc-diff*")))
+
+            (with-current-buffer "*vc-diff*"
+              (progn
+                (let ((rawtext (buffer-substring-no-properties (point-min)
+                                                               (point-max))))
+                  (should (string-search "-originaltext" rawtext))
+                  (should (string-search "+updatedtext" rawtext)))))))
+
+      ;; Save exit.
+      (ignore-errors
+        (if tempdir (delete-directory tempdir t))
+        (run-hooks 'vc-test--cleanup-hook)))))
+
 (defun vc-test--rename-file (backend)
   "Check the rename-file action."
 
@@ -715,6 +781,18 @@ vc-test--arch-enabled
           ;; "Really want to delete ...?"
           (skip-unless (not (eq 'CVS ',backend)))
           (vc-test--rename-file ',backend))
+
+        (ert-deftest
+	    ,(intern (format "vc-test-%s06-version-diff" backend-string)) ()
+	  ,(format "Check `vc-version-diff' for the %s backend."
+		   backend-string)
+	  (skip-unless
+	   (ert-test-passed-p
+	    (ert-test-most-recent-result
+	     (ert-get-test
+	      ',(intern
+		 (format "vc-test-%s01-register" backend-string))))))
+	  (vc-test--version-diff ',backend))
         ))))
 
 (provide 'vc-tests)
-- 
2.30.2


[-- Attachment #3: Type: text/plain, Size: 13 bytes --]

-- 
Matthias

Re: Is vc-diff-mergebase broken on master?

[Dmitry Gutov]

On 04.09.2021 23:24, Matthias Meulien wrote:
> Matthias Meulien<orontee@gmail.com>  writes:
> 
>> (...) I'll work on an updated patch fixing RCS/CVS tests.
> Here it is:
> 

Thanks!

Applied and pushed with some tweaks:

- Moved the function below vc-test--rename-file to mirror the order of 
tests.
- Some annotations (declare-function, backend action coverage at the top).
- Untabified the added lines: if you can, try to edit the files in the 
repository checkout, or customize indent-tabs-mode to nil globally. We 
have it set in the repository's .dir-locals.el. The rule is we don't 
mass-untabify existing code, but we try to obey the current setting when 
changing code or adding new lines.