Re: Be more careful about updating refs

Junio C Hamano [off-list ref] wrote:

Junio C Hamano [off-list ref] writes:

quoted

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.