Re: vc-*-root finctions

[Dan Nicolaescu]

Thien-Thi Nguyen <address@hidden> writes:

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

Because if forces a backend implementor to read two things instead of one.

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

Well, you are effectively introducing two functions, but squeezing them
into one with a very weird interface. Explain how is that allows for
less 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?

How about way too complex?

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

Can you please explain what you mean rather than hand wave?  Given that
you are replacing existing working code, it would be nice if you'd
explain what problem you are trying to solve, and why the approach you
took is the right way to solve the problem (other than applying the NIH 
principle).

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

You are changing code that is simple to something that is strange and
complex. Enquiring minds would like to know the reasoning behind those
changes and to make sure that the changes you are making are the right
way to solve whatever problem you are trying to solve.

  >    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

That's five seconds on a local disk, right?  Put that on an busy NFS
server, and do it for a bigger tree than emacs and those five seconds go
up really fast.

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

Occam's razor also applies to functions with terribly complicated
interfaces.  So no, the above is not a good motivation.


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

And why is it better to not have a separate function?

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

As you wish. But please don't commit anything that changes the way
vc-status works before posting it here.