On Tue, Nov 20, 2018 at 07:17:46AM -0500, Derrick Stolee wrote:
> > And I think that's a pattern with the delta-island code. What we really
> > care about most is that if we throw a real fork-network repository at
> > it, it produces faster clones with fewer un-reusable deltas. So I think
> > a much more interesting approach here would be perf tests. But:
> >
> > - we'd want to count those as coverage, and that likely makes your
> > coverage tests prohibitively expensive
> >
> > - it requires a specialized repo to demonstrate, which most people
> > aren't going to have handy
>
> Do you have regularly-running tests that check this in your infrastructure?
> As long as someone would notice if this code starts failing, that would be
> enough.
We do integration tests, and this feature gets exercised quite a bit in
production at GitHub. But none of that caught the breakage I fixed this
morning for the simple fact that we don't keep up with upstream master
in real-time. We're running the delta-island code I wrote years ago, and
the bug was introduced by the upstreaming. Probably in a month or three
I'll merge v2.20 into our fork, and we would have noticed then.
I've had dreams of following upstream more closely, but there are two
blockers:
- we have enough custom bits that the merges are time-consuming (and
introduce the possibility of funny interactions or regressions). I'd
like to reduce our overall diff from upstream, but it's slow going
and time is finite. (And ironically, it's made harder in some ways
by the lag, because many of our topics are backports of things I
send upstream).
- deploying the ".0" release from master can be scary. For instance, I
was really happy to have a fix for 9ac3f0e5b3 (pack-objects: fix
performance issues on packing large deltas, 2018-07-22) before
putting the bug into production.
> > Yeah, this triggers if your regex has more than one capture group (and
> > likewise, we almost certainly don't run the "you have too many groups"
> > warning).
>
> Did you know that regexes are notoriously under-tested [1]? When looking at
> this code, I didn't even know regexes were involved (but I didn't look
> enough at the context).
>
> [1] https://dl.acm.org/citation.cfm?id=3236072
Heh. Well, fortunately we are compiling regexes from the user's config,
so it's not Git's fault if the regexes are bad. ;)
(Of course on our production servers I'm on the hook for both sides!)
-Peff
