From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Dan Nicolaescu Newsgroups: gmane.emacs.devel Subject: Re: vc-*-root finctions Date: Thu, 21 Feb 2008 10:35:38 -0800 Message-ID: <200802211835.m1LIZcZ5005055@sallyv1.ics.uci.edu> References: <87skzn3mq9.fsf@ambire.localdomain> <87mypvxzd2.fsf@ambire.localdomain> <200802201850.m1KIofIk000198@sallyv1.ics.uci.edu> <87tzk2xr2q.fsf@ambire.localdomain> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1203619125 2199 80.91.229.12 (21 Feb 2008 18:38:45 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 21 Feb 2008 18:38:45 +0000 (UTC) Cc: Stefan Monnier , emacs-devel@gnu.org To: Thien-Thi Nguyen Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Feb 21 19:39:09 2008 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 1JSGJd-0006i7-0i for ged-emacs-devel@m.gmane.org; Thu, 21 Feb 2008 19:38:41 +0100 Original-Received: from localhost ([127.0.0.1] helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1JSGJ7-0005SO-U9 for ged-emacs-devel@m.gmane.org; Thu, 21 Feb 2008 13:38:09 -0500 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1JSGJ2-0005RZ-VH for emacs-devel@gnu.org; Thu, 21 Feb 2008 13:38:05 -0500 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1JSGIy-0005Ph-Nj for emacs-devel@gnu.org; Thu, 21 Feb 2008 13:38:04 -0500 Original-Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1JSGIx-0005Pe-UJ for emacs-devel@gnu.org; Thu, 21 Feb 2008 13:37:59 -0500 Original-Received: from sallyv1.ics.uci.edu ([128.195.1.109]) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.60) (envelope-from ) id 1JSGIx-0003Ls-Bu for emacs-devel@gnu.org; Thu, 21 Feb 2008 13:37:59 -0500 X-ICS-MailScanner-Watermark: 1204223741.4224@wnWZU4JfwxsHn8PLG32CIQ Original-Received: from mothra.ics.uci.edu (mothra.ics.uci.edu [128.195.6.93]) by sallyv1.ics.uci.edu (8.13.7+Sun/8.13.7) with ESMTP id m1LIZcZ5005055; Thu, 21 Feb 2008 10:35:38 -0800 (PST) In-Reply-To: <87tzk2xr2q.fsf@ambire.localdomain> (Thien-Thi Nguyen's message of "Thu, 21 Feb 2008 16:33:01 +0100") Original-Lines: 114 X-ICS-MailScanner: Found to be clean X-ICS-MailScanner-SpamCheck: not spam, SpamAssassin (score=-1.363, required 5, autolearn=disabled, ALL_TRUSTED -1.44, TW_SV 0.08) X-ICS-MailScanner-From: dann@mothra.ics.uci.edu X-detected-kernel: by monty-python.gnu.org: Solaris 10 (beta) 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:89846 Archived-At: Thien-Thi Nguyen writes: > () Dan Nicolaescu > () Wed, 20 Feb 2008 10:50:41 -0800 > > Allowing the user to look at the status from the root of the VC tree > seems like a good idea. But is this the right UI for it? It does not > seem very intuitive to me... > > It's just a shortcut for M-x vc-status RET RET. In the following > updated patch, all subdirs from root downward are buttonized (not just root). It seems that this patch contains multiple independent things. It would be better to split them, so they can be judged separately. > > ! ;; for the files in DIR. This function is called twice, > ^^^^^^^^^^^^^^^^ > Can you please explain why? > > > ! ;; time with KICKP t, the second time, with KICKP nil. > > What's the meaning of KICKP ? > > There is now a lengthy comment in vc-status-refresh that addresses > these questions. As a documentation stickler you should know that having a backend implementer look for documentation in both the beginning of vc.el and in the comment in vc-status-refresh is not a good idea. More, the comment in vc-status-refresh talks about "We used to do this:" is just pointless. Frankly, having a function that is called twice once with an argument t and another time with nil, and does completely different things depending on the argument just screams WEIRD. Better have 2 different functions. The fact that there's a need for about 70 lines of comments for one backend function, points to some design problems. > > ! ;; If the backend workings are asynchronous, > > Why bother making the backend synchronous? > > To support asynchronous behavior well, we wish to keep the user informed, > i.e, updating some visible status at the asynchronous boundaries (twice). Huh? Please explain this. In general, it hard to see what you are trying to accomplish here, and why you changes are better than what is there now. > If the backend is very fast (completes below some threshold, say 0.5 sec), > this double update appears as a flickering, and is not only uninformative, > it may be downright bewildering. Thus, a friendly backend may choose to > operate synchronously if it is confident that it can do its job under some > other reasonable threshold for user patience (say 3-5 seconds). This is the > backend's business; vc.el should not presume to know. You haven't explained why it is better to introduce the synchronous behavior when I said that it can be easily used with the current API. > You can see this consideration in play in vc-git-dir-status, which eschews > asynchronous operation completely, so confident it is. Which would be wrong. git might be fast, but it takes a long time if it has to read the inodes from disk or NFS on a big tree (which happens every morning or after a big compilation job). > in vc-svn-dir-status, i have placed a TODO comment where someone who knows > subversion better can add code to dynamically determine how to DTRT there. > > The current API can be used by a synchronous backend too. It just needs > to call the UPDATE-FUNCTION when done processing. > > Yes, but removing the need to specify UPDATE-FUNCTION is better. Why is that a big problem. The usage model is very simple, and it could be easily extended to deal with doing incremental update, which we haven't yet completely excluded as unnecessary. Avoiding UPDATE-FUNCTION can be done by passing instead a buffer in which dir-status could do its thing (and I think Tom Tromey had a patch for that). But it's hard to see that making any difference, there are many other things to worry about now... > - (defun vc-status-headers (backend dir) > - (concat > - (format "VC backend : %s\n" backend) > - "Repository : The repository goes here\n" > - (format "Working dir: %s\n" dir))) > - > > Why remove this? > > For several reasons (w/ current patch): > - For some backends, "working dir" and "repository" are one and the same. > - Which backend is reflected in the mode line (re-using var vc-mode). > - The vc-BACKEND-dir-status return value now allows the backend to > include arbitrary backend- and/or dir-specific metainfo. > See vc-svn-dir-status for an example. Yeah, this was intended to call a backend specific function at some point, so better keep it a separate function that mix this code in the middle of something else. +(defsubst vc-overview-p () + "Return non-nil if current buffer is in VC Dired or VC Status mode." + (memq major-mode '(vc-dired-mode vc-status-mode))) There are a few other places that could use this. But this should go at the time vc-dired is completely replaced. Why not check this in separately?