Re: Be more careful about updating refs

From: Shawn O. Pearce <spearce@spearce.org>
Date: Jan 18, 2008, 02:13 UTC
Message-ID: <20080118021300.GR24004@spearce.org>
In-Reply-To: <7v63xrki29.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > The following patch seems to fix the issue for me, but this is
> > primarily meant for discussion, as I do not quite understand why
> > the same issue does not manifest itself when NO_MMAP is not
> > used.
> > ...

...

> What I do not quite understand is how this can be a new issue.

It isn't.  It is indeed a pretty old issue, as far as git issues go.
Its probably about as old as fast-import being accepted into your
tree is.

Show 13 quoted lines

> The codepath to allow updating an existing branch shown above
> (i.e. "if it is not force and old is not NULL") uses the usual
> lookup_commit_reference_gently() interface to access b->sha1,
> and does not use gfi-aware gfi_unpack_entry() or anything
> magical, which means it would have passed the same codepath down
> to trigger the same issue.  IOW, even before this tightening of
> write_ref_sha1(), we already should have had the issue of not
> being to able to grab the object b->sha1 refers to out of the
> newly built packfile.
> 
> Is it just nobody seriously exercised the codepath yet, or is
> there a difference between these two calls that is more subtle
> than that?

I think the problem is nobody has tested fast-import updating an existing ref while using NO_MMAP. Or if they did, they didn't report the problem as they didn't figure they needed fast-import that badly.

Updating an existing ref is not a common operation, but the test suite does test for it. So it must be the NO_MMAP configuration is simply not being tested well enough.

-- 
Shawn.