[PATCH 1 of 3 v4] store: implement fncache path mangling code in C
mpm at selenic.com
Fri Sep 7 18:15:45 CDT 2012
On Fri, 2012-09-07 at 13:10 -0700, Bryan O'Sullivan wrote:
> On Fri, Sep 7, 2012 at 12:45 PM, Adrian Buehlmann <adrian at cadifra.com>wrote:
> > What I still don't understand is, why is it worth the risk (and
> > the effort) here?
> Because I don't want Mercurial to act slow for arbitrary reasons. If I
> implemented only the fast encoding in C, we'd be left in a situation where
> sometimes the performance would be good, sometimes bad, and this would
> depend on the conventions around naming in a particular repo. This is both
> hard to understand and difficult to justify to users.
This all reads as a case study in why you should do incremental
Here you've got one big patch that tries to tackle a big problem when
there are obvious smaller increments that get you there:
- patch that does non-hashed subset for 90% of the gain
- follow-up that does hashed part too for remaining 10%
Arguably more than 50% of the complexity is in the second piece, judging
from the new test cases that have rolled by, so we're also looking at a
good example of diminishing returns.
There are no downsides to putting the first piece in by itself. It's
easier to review and test and gets you most of the gain with less
engineering risk. And if something does go wrong, it will be easier to
pinpoint in the future.
Further, if the second patch doesn't even show up for weeks or months,
no one will complain. Staging potentially risky introduction of complex
code across multiple releases is a good idea, especially if said code is
There are correspondingly no _upsides_ to tying the two pieces together,
except the (painfully counter-factual) efficiency of reduced submission
overhead. Right now you and Adrian are arguing over whether the second
piece is warranted and that's stalling the acceptance of the much more
important first piece.
(Which is why I suggested you restrict your scope way back on Aug 22.)
Mathematics is the supreme nostalgia of our time.
More information about the Mercurial-devel