Re: vc-*-root finctions

[Thien-Thi Nguyen]

() Dan Nicolaescu <address@hidden>
() Thu, 21 Feb 2008 10:35:38 -0800

   It seems that this patch contains multiple independent things.  It would
   be better to split them, so they can be judged separately.

Good point.  Will do so in the next few days.

   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.

Why is not a good idea?  One explains the interface, the other the rationale.

   More, the comment in vc-status-refresh talks about "We used to
   do this:" is just pointless.

Lots of comments in Emacs are likewise pointless, i suppose.  In
any case, i'll move it to top-level and make it less time-oriented.

   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.

I disagree.  More functions is more maintenance.

   The fact that there's a need for about 70 lines of comments for one
   backend function, points to some design problems.

How so?

   In general, it hard to see what you are trying to accomplish here, and
   why you changes are better than what is there now.

The changes try to reduce failure modes, which, being "potential"
rather than actual issues, may indeed be difficult to appreciate.

   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.

Well, i haven't introduced synchronous behavior in vc.el, so why
would i want to explain that?

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

I don't know about that.  I observe that "git status" on emacs.git
takes a little longer (maybe five seconds) the first time it is
run and then less than a second for subsequent times.  (Perhaps
you're not talking about "git status" that vc-git-dir-status uses?)

     > Yes, but removing the need to specify UPDATE-FUNCTION is better.

   Why is that a big problem.

More functions, more maintenance: It's not a big problem when used
correctly; it's a big problem when misunderstood and subsequently
used incorrectly.  Occam's razor and all that.

   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.

Regardless, the best place is during vc-status-refresh.

   +(defsubst vc-overview-p () ...)

   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?

Good idea.  I will commit it in the next day or two.

In the meantime, interested git users can look at:
http://www.gnuvola.org/wip/  (vc-status-hacking)

I will henceforth be munging there instead of posting patches.

thi