From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: "Eric S. Raymond" Newsgroups: gmane.emacs.devel Subject: Re: New VC mode -- review request Date: Wed, 3 Oct 2007 13:00:48 -0400 Organization: Eric Conspiracy Secret Labs Message-ID: <20071003170048.GB7014@thyrsus.com> References: <20071003103501.GA4997@thyrsus.com> <200710031431.l93EVfFZ021233@oogie-boogie.ics.uci.edu> Reply-To: esr@thyrsus.com NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: sea.gmane.org 1191430868 15398 80.91.229.12 (3 Oct 2007 17:01:08 GMT) X-Complaints-To: usenet@sea.gmane.org NNTP-Posting-Date: Wed, 3 Oct 2007 17:01:08 +0000 (UTC) Cc: emacs-devel@gnu.org To: Dan Nicolaescu Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Oct 03 19:01:04 2007 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1Id7aV-0000Xo-5i for ged-emacs-devel@m.gmane.org; Wed, 03 Oct 2007 19:00:43 +0200 Original-Received: from localhost ([127.0.0.1] helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Id7aQ-0000ev-Nx for ged-emacs-devel@m.gmane.org; Wed, 03 Oct 2007 13:00:38 -0400 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Id7aN-0000dK-Ld for emacs-devel@gnu.org; Wed, 03 Oct 2007 13:00:35 -0400 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Id7aN-0000cn-59 for emacs-devel@gnu.org; Wed, 03 Oct 2007 13:00:35 -0400 Original-Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Id7aM-0000ca-Tm for emacs-devel@gnu.org; Wed, 03 Oct 2007 13:00:35 -0400 Original-Received: from static-71-162-243-5.phlapa.fios.verizon.net ([71.162.243.5] helo=snark) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Id7aM-0004kd-Cu for emacs-devel@gnu.org; Wed, 03 Oct 2007 13:00:34 -0400 Original-Received: by snark (Postfix, from userid 23) id 300443807B; Wed, 3 Oct 2007 13:00:48 -0400 (EDT) Content-Disposition: inline In-Reply-To: <200710031431.l93EVfFZ021233@oogie-boogie.ics.uci.edu> X-Eric-Conspiracy: There is no conspiracy User-Agent: Mutt/1.5.15+20070412 (2007-04-11) X-Detected-Kernel: Linux 2.6 (newer, 2) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:80194 Archived-At: Dan Nicolaescu : > 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. -- Eric S. Raymond