Re: Be more careful about updating refs

["Shawn O. Pearce" <spearce@spearce.org>]

Junio C Hamano <gitster@pobox.com> wrote:
> 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.
 
> 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.