bug#13547: svn annotate - incorrect previous/next revision

bug#13547: svn annotate - incorrect previous/next revision

[Lars Ljung]

Hi,

The functions vc-svn-previous-revision and vc-svn-next-revision just add
or subtract 1 from the revision number. The is usually not correct.

This patch uses "svn log" to get the correct previous/next revision of
the file.

Kind regards,
Lars Ljung

=== modified file 'lisp/vc/vc-svn.el'
*** lisp/vc/vc-svn.el	2013-01-02 16:13:04 +0000
--- lisp/vc/vc-svn.el	2013-01-25 08:26:53 +0000
*************** RESULT is a list of conses (FILE . STATE
*** 259,279 ****
  ;; works just fine.

  (defun vc-svn-previous-revision (file rev)
!   (let ((newrev (1- (string-to-number rev))))
!     (when (< 0 newrev)
!       (number-to-string newrev))))

  (defun vc-svn-next-revision (file rev)
!   (let ((newrev (1+ (string-to-number rev))))
!     ;; The "working revision" is an uneasy conceptual fit under
Subversion;
!     ;; we use it as the upper bound until a better idea comes along.
If the
!     ;; workfile version W coincides with the tree's latest revision R,
then
!     ;; this check prevents a "no such revision: R+1" error.  Otherwise, it
!     ;; inhibits showing of W+1 through R, which could be considered
anywhere
!     ;; from gracious to impolite.
!     (unless (< (string-to-number (vc-file-getprop file
'vc-working-revision))
!                newrev)
!       (number-to-string newrev))))


  ;;;
--- 259,280 ----
  ;; works just fine.

  (defun vc-svn-previous-revision (file rev)
!   (with-temp-buffer
!     (vc-svn-command t 0 file "log" "-q" "--limit" "2"
! 		    (format "-r%s:1" rev))
!     (let ((revision-list '()))
!       (while (re-search-backward "^r\\([0-9]+\\)" nil t)
! 	(setq revision-list (cons (match-string 1) revision-list)))
!       (cadr revision-list))))

  (defun vc-svn-next-revision (file rev)
!   (with-temp-buffer
!     (vc-svn-command t 0 file "log" "-q" "--limit" "2"
! 		    (format "-r%s:HEAD" rev))
!     (let ((revision-list '()))
!       (while (re-search-backward "^r\\([0-9]+\\)" nil t)
! 	(setq revision-list (cons (match-string 1) revision-list)))
!       (cadr revision-list))))


  ;;;

bug#13547: svn annotate - incorrect previous/next revision

[Glenn Morris]

Lars Ljung wrote:

> The functions vc-svn-previous-revision and vc-svn-next-revision just add
> or subtract 1 from the revision number. The is usually not correct.
>
> This patch uses "svn log" to get the correct previous/next revision of
> the file.

Thanks. I guess the drawback here is, that will contact the svn
repository, which may be on a remote server, so could be slow.

It seems this is used by vc-annotate, vc-rollback, and vc-diff.

bug#13547: svn annotate - incorrect previous/next revision

[Lars Ljung]

2013-01-27 03:24, Glenn Morris skrev:
> Thanks. I guess the drawback here is, that will contact the svn
> repository, which may be on a remote server, so could be slow.
> 
> It seems this is used by vc-annotate, vc-rollback, and vc-diff. 

Yes, it will be slower. But as far as I know there is no other way to
get the previous/next revision of a file.

I suppose the output from an unlimited "svn log" could be saved to speed
up future operations. vc-annotate-warp-revision might actually call
these functions multiple times.

vc-rollback is not supported by the svn backend.

I don'f fully understand how vc-diff works, but these functions are not
called for "C-x v =". This is good since that operation should not
require server access at all.

vc-annotate-show-diff-revision-at-line calls vc-svn-previous-revision.
In that case the server will be contacted twice for no good reason, a
simple "svn diff -r rev" would suffice (isn't this true for all backends?).

vc-annotate-show-changeset-diff-revision-at-line calls
vc-svn-previous-revision with filename set to nil. It that case it makes
sense to return rev-1. The modified patch below takes care of that.

=== modified file 'lisp/vc/vc-svn.el'
*** lisp/vc/vc-svn.el	2013-01-02 16:13:04 +0000
--- lisp/vc/vc-svn.el	2013-01-27 11:04:07 +0000
***************
*** 259,279 ****
  ;; works just fine.

  (defun vc-svn-previous-revision (file rev)
!   (let ((newrev (1- (string-to-number rev))))
!     (when (< 0 newrev)
!       (number-to-string newrev))))

  (defun vc-svn-next-revision (file rev)
!   (let ((newrev (1+ (string-to-number rev))))
!     ;; The "working revision" is an uneasy conceptual fit under
Subversion;
!     ;; we use it as the upper bound until a better idea comes along.
If the
!     ;; workfile version W coincides with the tree's latest revision R,
then
!     ;; this check prevents a "no such revision: R+1" error.  Otherwise, it
!     ;; inhibits showing of W+1 through R, which could be considered
anywhere
!     ;; from gracious to impolite.
!     (unless (< (string-to-number (vc-file-getprop file
'vc-working-revision))
!                newrev)
!       (number-to-string newrev))))


  ;;;
--- 259,286 ----
  ;; works just fine.

  (defun vc-svn-previous-revision (file rev)
!   (if file
!       (with-temp-buffer
! 	(let (process-file-side-effects)
! 	  (vc-svn-command t 0 file "log" "-q" "--limit" "2"
! 			  (format "-r%s:1" rev)))
! 	(let ((revision-list '()))
! 	  (while (re-search-backward "^r\\([0-9]+\\)" nil t)
! 	    (push (match-string 1) revision-list))
! 	  (cadr revision-list)))
!     (let ((newrev (1- (string-to-number rev))))
!       (when (< 0 newrev)
! 	(number-to-string newrev)))))

  (defun vc-svn-next-revision (file rev)
!   (with-temp-buffer
!     (let (process-file-side-effects)
!       (vc-svn-command t 0 file "log" "-q" "--limit" "2"
! 		      (format "-r%s:HEAD" rev)))
!     (let ((revision-list '()))
!       (while (re-search-backward "^r\\([0-9]+\\)" nil t)
! 	(push (match-string 1) revision-list))
!       (cadr revision-list))))


  ;;;

bug#13547: svn annotate - incorrect previous/next revision

[Lars Ingebrigtsen]

Lars Ljung <lars@matholka.se> writes:

>> Thanks. I guess the drawback here is, that will contact the svn
>> repository, which may be on a remote server, so could be slow.
>> 
>> It seems this is used by vc-annotate, vc-rollback, and vc-diff. 
>
> Yes, it will be slower. But as far as I know there is no other way to
> get the previous/next revision of a file.

I haven't used svn in many years, so I don't have much of an opinion
here.  I've respun the patch for Emacs 28; included below.

Anybody got an opinion here?

diff --git a/lisp/vc/vc-svn.el b/lisp/vc/vc-svn.el
index c30920dd15..0cc7bda1ba 100644
--- a/lisp/vc/vc-svn.el
+++ b/lisp/vc/vc-svn.el
@@ -248,23 +248,29 @@ vc-svn-working-revision
 ;; vc-svn-mode-line-string doesn't exist because the default implementation
 ;; works just fine.
 
-(defun vc-svn-previous-revision (_file rev)
-  (let ((newrev (1- (string-to-number rev))))
-    (when (< 0 newrev)
-      (number-to-string newrev))))
+(defun vc-svn-previous-revision (file rev)
+  (if file
+      (with-temp-buffer
+ 	(let (process-file-side-effects)
+ 	  (vc-svn-command t 0 file "log" "-q" "--limit" "2"
+ 			  (format "-r%s:1" rev)))
+ 	(let ((revision-list '()))
+ 	  (while (re-search-backward "^r\\([0-9]+\\)" nil t)
+ 	    (push (match-string 1) revision-list))
+ 	  (cadr revision-list)))
+    (let ((newrev (1- (string-to-number rev))))
+      (when (< 0 newrev)
+ 	(number-to-string newrev)))))
 
 (defun vc-svn-next-revision (file rev)
-  (let ((newrev (1+ (string-to-number rev))))
-    ;; The "working revision" is an uneasy conceptual fit under Subversion;
-    ;; we use it as the upper bound until a better idea comes along.  If the
-    ;; workfile version W coincides with the tree's latest revision R, then
-    ;; this check prevents a "no such revision: R+1" error.  Otherwise, it
-    ;; inhibits showing of W+1 through R, which could be considered anywhere
-    ;; from gracious to impolite.
-    (unless (< (string-to-number (vc-file-getprop file 'vc-working-revision))
-               newrev)
-      (number-to-string newrev))))
-
+  (with-temp-buffer
+    (let (process-file-side-effects)
+      (vc-svn-command t 0 file "log" "-q" "--limit" "2"
+ 		      (format "-r%s:HEAD" rev)))
+    (let ((revision-list '()))
+      (while (re-search-backward "^r\\([0-9]+\\)" nil t)
+ 	(push (match-string 1) revision-list))
+      (cadr revision-list))))
 
 ;;;
 ;;; State-changing functions


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

bug#13547: svn annotate - incorrect previous/next revision

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>>> Thanks. I guess the drawback here is, that will contact the svn
>>> repository, which may be on a remote server, so could be slow.
>>> 
>>> It seems this is used by vc-annotate, vc-rollback, and vc-diff. 
>>
>> Yes, it will be slower. But as far as I know there is no other way to
>> get the previous/next revision of a file.
>
> I haven't used svn in many years, so I don't have much of an opinion
> here.  I've respun the patch for Emacs 28; included below.
>
> Anybody got an opinion here?

I forgot to put Dmitry in the CCs; perhaps he'll have an opinion.

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

bug#13547: svn annotate - incorrect previous/next revision

[Dmitry Gutov]

Hi!

On 30.07.2021 14:54, Lars Ingebrigtsen wrote:
> Lars Ingebrigtsen<larsi@gnus.org>  writes:
> 
>>>> Thanks. I guess the drawback here is, that will contact the svn
>>>> repository, which may be on a remote server, so could be slow.
>>>>
>>>> It seems this is used by vc-annotate, vc-rollback, and vc-diff.
>>> Yes, it will be slower. But as far as I know there is no other way to
>>> get the previous/next revision of a file.
>> I haven't used svn in many years, so I don't have much of an opinion
>> here.  I've respun the patch for Emacs 28; included below.
>>
>> Anybody got an opinion here?
> I forgot to put Dmitry in the CCs; perhaps he'll have an opinion.

Since this is about SVN, it's hard for me to have an opinion as well: 
all practical recollections are from ~10 years ago. How slow are SVN 
servers to respond to requests like this these days?

Could someone also describe the effect that our current "not correct" 
revision numbers have? Do they trigger errors? Do them cause 
'vc-annotate' to show wrong file contents?

If it's either of these, then we probably should go ahead with the patch.

bug#13547: svn annotate - incorrect previous/next revision

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Since this is about SVN, it's hard for me to have an opinion as well:
> all practical recollections are from ~10 years ago. How slow are SVN
> servers to respond to requests like this these days?
>
> Could someone also describe the effect that our current "not correct"
> revision numbers have? Do they trigger errors? Do them cause
> 'vc-annotate' to show wrong file contents?
>
> If it's either of these, then we probably should go ahead with the patch.

This was half a year ago, and nobody's piped up, so I'm closing this bug
report.  (If somebody that uses svn wants this feature, they can
resurrect the patch, probably.)

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