Re: vc-*-root finctions

[Dan Nicolaescu]

Thien-Thi Nguyen <address@hidden> writes:

  > () Dan Nicolaescu <address@hidden>
  > () 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 <ROOT> 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?