Re: New VC diff for review

[Eric S. Raymond]

Dan Nicolaescu <address@hidden>:
> Here are some comments on the patch:
> 
>  
> +;; $Id: vc.el,v 1.130 2007/10/06 14:54:22 esr Exp esr $
> +
> Delete this

Of course, it's just included for completeness.

> Just whitespace differences, better avoid it.

I'm getting rid of those and won't mention them further.  I'll post my next diff
using -ub, too.

> 
>         (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

Not strictly speaking debug code; more in the nature of progress messages.

          ;; What we're doing here is preparing a version of the command 
          ;; for display in a debug-progess message.  If it's fewer than
          ;; 20 characters display the entire command (without trailing 
          ;; newline).  Otherwise display the first 20 followed by an ellipsis.

> @@ -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.

Actually one of these isn't.   But it's trivial.

> +;;;###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.

There's a conceptual problem here.  Remember that new VC operates on filesets,
not files.  If the fileset has more than one member, which file should be used 
to key into the revision completion table?
 
>  (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.

Yes, it appears to have been accidentally deleted in my version --
though I'm puzzled why I didn't notice this sooner, as it should 
have caused paren-balance problems.  Restored.


>         ;; 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.

Not quite.  The problem with filesets crops up here as well.  Fortunately
you only want the symbol for the relevant back end here, which is 
computed earlier in the calling function.  The correct equivalent 
of your line is         

      (vc-call-backend ',backend 'log-view-mode)
 
> -(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*"))

I was hoping you could tell me that :-)  But looking into it, I think it's
a result of a simple name change I made that isn't in Emacs head, combined
with a slight optimization done after the fork.  I'll reconcile it.
  
>    "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. 

That's half-correct.  I missed converting this function, and someone 
(probably Stefan) did it later in CVS.  But he got the wash-log call
wrong; in the new backend API it doesn't take a file argument, because
print-log operates on filesets like most of the other back-end 
calls and wash-log has to be smart enough to do its work 
depending strictly on what it sees in the result buffer.

The reason neither of us noticed our errors is that
vc-default-comment-history is a special purpose function that's not
called as part of the normal edit-commit cycle.  I'm not actually
sure why it exists at all.
 
> -(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?

It moved to the RCS back end.  There is no longer a default log-washer
in new VC; the "all the world is RCS unless otherwise specified" assumption 
was silly.

Probably the real error here was that I didn't happen to remove this
function from my vc.el before I accidentally dropped it in head.  I'll
fix that.

> 
> @@ -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... 

Yes, we already know this.  You're seeing a tweak applied to Emacs CVS
after the fork.  I will reconcile.
-- 
                <a href="http://www.catb.org/~esr/";>Eric S. Raymond</a>