Re: [PATCH 0/3] fix unparsed object access in upload-pack

From: Jeff King <peff@peff.net>
Date: Mar 17, 2013, 05:40 UTC
Message-ID: <20130317054039.GA16070@sigill.intra.peff.net>
In-Reply-To: <7v7gl6txl3.fsf@alter.siamese.dyndns.org>

On Sat, Mar 16, 2013 at 10:16:40PM -0700, Junio C Hamano wrote:

Show 6 quoted lines

> > ... (I had several bug reports
> > within a few hours of deploying v1.8.1.5 on github.com)
> 
> Nice to have a pro at the widely used site ;-)  I often wish it had
> a mechanism to deploy the tip of 'master' or 'maint', or even 'next'
> for 0.2% of its users' repositories to give us an early feedback.

We are quite slow and conservative at deploying new git, preferring instead to let the rest of the world act as our testbed. :)

As seen with this bug, though, we really do get a lot more coverage of weird cases due to our size. In the cases I looked at, the trigger seemed to be clients doing the equivalent of of "git clone --depth=X --no-single-branch". Almost nobody would do that intentionally, but prior to v1.7.10, we did not have --single-branch; older clients using --depth on a repository with multiple branches started failing clones almost immediately.

We do have the capability to roll out to one or a few of our servers (the granularity is not 0.2%, but it is still small). I'm going to try to keep us more in sync with upstream git, but I don't know if I will get to the point of ever deploying "master" or "next", even for a small portion of the population. We are accumulating more hacks[1] on top of git, so it is not just "run master for an hour on this server"; I have to actually merge our fork.

I had been handling our hacks as patch series to be rebased on top of upstream git releases, but that was getting increasingly unwieldy (especially as people besides me work on it). Going forward, I'm going to treat upstream git as a vendor branch and merge in occasionally to get fixes.

One thing I might try is to keep a local "next-upstream" branch, that is continually merging what is on upstream master with our local production version. Then I could graduate it to production just like any other topic branch when it comes time (instead of doing a gigantic painful merge when we decide to upgrade upstream git).

That would mean I could test-deploy the "next-upstream" branch to a few servers on any given day without doing a lot of work. So it might make sense to do it at key times, like when we are in -rc here, to help shake out any existing bugs before you make a release.

Show 10 quoted lines

> >   [3/3]: upload-pack: load non-tip "want" objects from disk
> >
> >     While investigating the bug, I found some weirdness around the
> >     stateless-rpc check_non_tip code. As far as I can tell, that code
> >     never actually gets triggered. It's not too surprising that we
> >     wouldn't have noticed, because it is about falling back due to a
> >     race condition. But please sanity check my explanation and patch.
> 
> Thanks. That fall-back is Shawn's doing and I suspect that nobody is
> exercising the codepath (he isn't).

I almost wonder if we should cut it out entirely. It is definitely a possible race condition, but I wonder if anybody actually hits it in practice (and if they do, the consequence is that the fetch fails and needs to be retried). As far as I can tell, the code path has never actually been followed, and I do not recall ever seeing a bug report or complaint about it (though perhaps it happened once, which spurred the initial development?).

-Peff