Re: New VC mode -- review request

[Dan Nicolaescu <dann@ics.uci.edu>]

"Eric S. Raymond" <esr@thyrsus.com> writes:

  > I am the original author of the VC mode shipped with Emacs, back in 1992-1993.
  > Its design was well matched to the file-oriented version-control systems 
  > (VCSes) of its day, and it has proven a useful tool.  But it has long been in
  > need of a rewrite, for two reasons:
  >
  > (1) The code has accreted cruft over the years, and
  > 
  > (2) the design is poorly matched to modern changeset-oriented VCSes,
  > beginning with Subversion and including third-generation systems such
  > as Mercurial and Bazaar.

Thank you for working doing this, such features are sorely needed now.
 
  > Now I think it has come time for the front end to be merged.  I have
  > been using it in my everyday development work with Subversion for months.
  > Stefan asked me to submit it to this list for final review, and I am
  > doing so.  Please find it attached.

The vc.el file in CVS trunk has changed since your last commit, some
new features have been added, bugs have been fixed. Could you please
merge your changes with the top of the trunk?

  > Here are the user-visible changes you should expect relative to what's
  > in the manual:
  > 
  > * The following commands now operate on filesets rather than files.  
  > 
  > 	  vc-next-action = C-x v v
  > 	  vc-diff = C-x v =
  > 	  vc-print-log = C-x v l 
  > 	  vc-revert = C-x v u

This does not seem to work for me from vc-dired. I have 2 changed
files in a Mercurial. After marking both of them in vc-dired and
pressing 
v u
I get this error "Please kill or save all modified buffers before
reverting".
None of the files is in an emacs buffer.

  > 	  vc-rollback = C-x v c

Do you have an implementation for this function for any backend? It
would be interesting to see it.

  > Changes other than these are probably bugs and you should report them.  

You introduced "focus version". This looks like a new term, could we
use something that is already in use in at least one version control
system?

  > You need not restrict criticism to outright bugs; the user interface hasn't
  > been reviewed or refreshed in a very long time before this, and it is
  > possible we could be doing things more gracefully.

You have moved functions around in the file. This makes doing a diff
between the top of the trunk and your version harder than it needs to
be. Could you please restore the function order (at least for review
purposes) and repost?

In vc-deduce-fileset:
This: 
   (let ((regexp (dired-marker-regexp))
          (marked '()))
             (save-excursion
                  (goto-char (point-min))
                       (while (re-search-forward regexp nil t)
                              (if (setq filename (dired-get-filename  nil t))
          (setq marked (append marked (list filename))))))

Can be replaced with this:
           (dired-map-over-marks (dired-get-filename) nil))

Also:
           ((and vc-parent-buffer (buffer-file-name vc-parent-buffer))
            (progn
               (if vc-dired-mode 
                  ^^^^^^^^^^^^^^
                  Can this ever be true, there's a previous 
                  vc-dired-mode clause in the `cond'...

This code in vc-dired-hook:

         ;; ordinary file
         ((and (vc-backend filename)
                (not (and vc-dired-terse-mode
                           (vc-up-to-date-p filename))))
          (vc-dired-reformat-line (vc-call dired-state-info filename))
          (forward-line 1))

Is the vc-backend call necessary? It ends up calling vc-registered and
that can be expensive for the backends that run a program to determine
the registered state. The -dir-state method computes the state for
most (all?)  files...
I don't understand this code very well, but if it can be simplified,
it might result in a significant speedup for vc-dired.

It seems that you want to keep vc-dired. IMHO vc-dired is not very
good because 
- it is slow because of running too many vc commands to find out the
status for each file.
- it does not show non-registered files, so one can't select them and
register them
- most of the dired commands are not useful in the VC context, they
are just clutter...
- and interface like PCL-CVS, psvn.el or git.el seems that is targeted only
for version control seems better.
But this is not necessarily related to your changes and should not
preclude you from checking in.

Do you have any plans for updating the branch infrastructure?
VC now uses the term "snapshot" for branches, most people have no idea
what that means...

Also for people that have not used RCS "locked" is not a familiar
term, would it be possible to minimize it's use?

I hope this helps. 

Thanks!
                --dan