Re: New VC mode -- review request

[Eric S. Raymond]

Dan Nicolaescu <address@hidden>:
> 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?

I'll do that.  I should get to it today.
 
>   > 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.

Hm.  I wonder if this is a problem with the hg back end -- I haven't 
tested that.  Can you duplicate this behavior with any other back end?
I'll stare at the code around that error message for a bit when I do
the merge with top of trunk.
 
>   >     vc-rollback = C-x v c
> 
> Do you have an implementation for this function for any backend? It
> would be interesting to see it.

There is one for SCCS in old VC if you want to look at it.  I think I
at one point implemented it for one other backend, but the code and
concept were so dodgy that I decided it was best shot through the
head.  

Sorry, I don't remember which other back end I implemented it for.  I
do know that most VCSes cannot support this operation at all, it's
really an SCCSism that got baked into old VC's design by historical
accident.

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

I invented a new one because existing VCSs actually get along without
an unambiguous term for this, instead using various circumlocutions or
referring to it implicitly.

Where applicable, I have tended to prefer terminology from Subversion,
as it has the largest userbase of any VCS with changeset capability;
if Subversion had a good term for it, I'd use it.

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

Obviously you think this would help you understand the changes better,
but I don't believe that.  Supposing I reordered it, I believe the
diff would still be sufficiently large and noisy to be useless.  Even
if it were not, the thought of (in effect) having to maintain two
versions of the code with different function ordering frightens me to
the bottom of my shoes.  So...I'm afraid the answer is "no".  I'd
like to be cooperative, but I believe this effort would be wasted.

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

Yes, that seems likely.  I will test this, thanks.
 
> 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'...

Good point.  That code is a fossil remnant from a previous organization
of vc-deduce-fileset.  Removed.
 
> 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.

You're right, it might.  I'm not going to change this immediately, it will take
a little thought and testing, but it's going on my to-do list.

(Note to other reviewers: please do not consider performance issues
*in new features* a blocker against merging new VC.  What we're trying
to verify here is that new VC doesn't cause functional regressions
with respect to the single-file operations of old VC.  Performance
and UI tuning of the fileset-oriented features can come afterwards.)

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

You may be right.  But current new VC is deliberately conservative in
design -- I have tried to change the UI as little as is consistent
with supporting changesets.  

This rewrite is all about making sure the changeset-aware
infrastructure is in place. I believe it's important to stick to
doing one new thing at once; after merge will the time to experimwnt
with a revamped UI on a solid functional foundation.

> But this is not necessarily related to your changes and should not
> preclude you from checking in.

Agreed.  

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

Agreed.  But worrying about this is for after the merge.

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

I'll do a pass over the documentation strings this afternoon with 
this in mind.  But, again, this is really an issue for after
the merge.

> I hope this helps. 

Yes, it does.
-- 
                <a href="http://www.catb.org/~esr/";>Eric S. Raymond</a>