Re: New VC diff for review

[Dan Nicolaescu]

Here are some comments on the patch:

 
+;; $Id: vc.el,v 1.130 2007/10/06 14:54:22 esr Exp esr $
+
Delete this

@@ -1013,13 +1016,13 @@
   ;; FIXME: file-relative-name can return a bogus result because
   ;; it doesn't look at the actual file-system to see if symlinks
   ;; come into play.
-  (let* ((files
+  (let* ((files 
^^^^^^^^^^^^^^^^^^
Just whitespace differences, better avoid it.

          (mapcar (lambda (f) (file-relative-name (expand-file-name f)))
                  (if (listp file-or-list) file-or-list (list file-or-list))))
-         (full-command
-         (concat command " " (vc-delistify flags) " " (vc-delistify files))))
-    (if vc-command-messages
-       (message "Running %s..." full-command))
+        (full-command
+         (concat command " " 
+                 (vc-delistify (mapcar (lambda (s) (if (> (length s) 20) 
(concat (substring s 0 2) "<stuff>")  s)) flags)) 
                                        
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                        Is this debugging code? If not
                                        please add a comment to say
                                        what it is doing because it is
                                        not obvious

@@ -1065,11 +1071,12 @@
              (shrink-window-if-larger-than-buffer)
              (error "Running %s...FAILED (%s)" full-command
                     (if (integerp status) (format "status %d" status) 
status))))
+         ;; We're done
          (if vc-command-messages
-             (message "Running %s...OK" full-command)))
+             (message "Running %s...OK = %d" full-command status)))
        (vc-exec-after
-        `(run-hook-with-args 'vc-post-command-functions
-                              ',command ',file-or-list ',flags))
+        `(run-hook-with-args 'vc-post-command-functions 
+                             ',command ',file-or-list ',flags))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Just whitespace differences, maybe better avoid it.

-  (if (not vc-log-operation)
+  (if (not vc-log-operation) 
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Just whitespace differences, maybe better avoid it.

+;;;###autoload
+(defun vc-history-diff (rev1 rev2)
+  "Report diffs between revisions of files in the repository history."
+  (interactive "sOlder revision: \nsNewer revision: ")
+  (let* ((files (vc-deduce-fileset t))
+          (first (car files))
+          (backend 
+           (cond ((file-directory-p first)
+                  (vc-responsible-backend first))
+                 (t
+                  (vc-backend first)))))
+    (if (string= rev1 "") (setq rev1 nil))
+    (if (string= rev2 "") (setq rev2 nil))
+    (if (and (not rev1) rev2)
+       (error "Not a valid revision range."))
+    (vc-diff-internal backend t files rev1 rev2 (interactive-p))))
 
One of these vc-diff* functions needs to do (vc-call revision-completion-table 
file)
like vc-version-diff does in order to get completions for
versions.

 (defun vc-version-other-window (rev)
@@ -2277,7 +2244,6 @@
        ((setq subdir (dired-get-subdir))
        ;; if the backend supports it, get the state
        ;; of all files in this directory at once
-       (let ((backend (vc-responsible-backend subdir)))
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        Isn't this line still needed, `backend' is still used below.
          ;; check `backend' can really handle `subdir'.
          (if (and (vc-call-backend backend 'responsible-p subdir)
                   (vc-find-backend-function backend 'dir-state))

     (vc-exec-after
      `(let ((inhibit-read-only t))
-       (vc-call-backend ',(vc-backend file) 'log-view-mode)
+       (log-view-mode)
        ^^^^^^^^^^^^^^^^^^^^^
This is incorrect, the original is right.

-        (vc-checkout file nil "")
+       (vc-checkout file nil "")
^^^^^^^^^^^^^^^^^^^^^^^
Only whitespace, better avoid.


-(defun vc-update-changelog-rcs2log (files)
+(defun vc-default-update-changelog (backend files)
   "Default implementation of update-changelog.
 Uses `rcs2log' which only works for RCS and CVS."
   ;; FIXME: We (c|sh)ould add support for cvs2cl
@@ -2994,7 +2926,9 @@
                                     (mapcar
                                      (lambda (f)
                                        (file-relative-name
-                                        (expand-file-name f odefault)))
+                                        (if (file-name-absolute-p f)
+                                            f
+                                          (concat odefault f))))
                                      files)))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Is this change intentional or a merge problem? It's hard to tell...

                        "done"
                     (pop-to-buffer (get-buffer-create "*vc*"))
 
 (defun vc-default-rename-file (backend old new)
   (condition-case nil
@@ -3049,8 +2989,8 @@
   "Return a string with all log entries stored in BACKEND for FILE."
   (if (vc-find-backend-function backend 'print-log)
       (with-current-buffer "*vc*"
-       (vc-call print-log (list file))
-       (vc-call wash-log file)
+       (vc-call print-log file)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This line is incorrect, the original is right. 

 
 (defun vc-default-unregister (backend file)
@@ -3119,26 +3059,6 @@
               (and (not vc-make-backup-files) (delete-file backup-name))))))
       (message "Checking out %s...done" file))))
 
-(defun vc-default-wash-log (backend file)
-  "Remove all non-comment information from log output.
-This default implementation works for RCS logs; backends should override
-it if their logs are not in RCS format."
-  (let ((separator (concat "^-+\nrevision [0-9.]+\ndate: .*\n"
-                          "\\(branches: .*;\n\\)?"
-                          "\\(\\*\\*\\* empty log message \\*\\*\\*\n\\)?")))
-    (goto-char (point-max)) (forward-line -1)
-    (while (looking-at "=*\n")
-      (delete-char (- (match-end 0) (match-beginning 0)))
-      (forward-line -1))
-    (goto-char (point-min))
-    (if (looking-at "[\b\t\n\v\f\r ]+")
-       (delete-char (- (match-end 0) (match-beginning 0))))
-    (goto-char (point-min))
-    (re-search-forward separator nil t)
-    (delete-region (point-min) (point))
-    (while (re-search-forward separator nil t)
-      (delete-region (match-beginning 0) (match-end 0)))))
^^^^^^^^^^^^^^^^^^^^^^^^^
What is wrong with this function? Why not keep it?


@@ -3204,13 +3124,13 @@
     ;; Run through this file and find the oldest and newest dates annotated.
     (save-excursion
       (goto-char (point-min))
-      (while (not (eobp))
-        (when (setq date (vc-call-backend vc-annotate-backend 'annotate-time))
-          (if (> date newest)
-              (setq newest date))
-          (if (< date oldest)
-              (setq oldest date)))
-        (forward-line 1)))
+      (while (setq date (prog1 (vc-call-backend vc-annotate-backend
+                                                'annotate-time)
+                          (forward-line 1)))
+       (if (> date newest)
+           (setq newest date))
+       (if (< date oldest)
+           (setq oldest date))))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I this intentional? It seems that the CVS version might have changed
after you branched your version...