[PATCH STABLE RFC] transplant: avoid a dirstate race when transplanting multiple changesets

[Greg Ward]

On Tue, Jan 25, 2011 at 10:29 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
>> --- a/hgext/transplant.py
>> +++ b/hgext/transplant.py
>> @@ -177,6 +177,16 @@
>>              lock.release()
>>              wlock.release()
>>
>> +        # Extra after-the-fact check for one of the symptoms of
>> +        # issue2264: if this is raised, it's too late and we've already
>> +        # committed bad changesets.  Not sure if it's worth the overhead
>> +        # of a status() call.
>> +        (modified, added, removed) = repo.status()[0:3]
>> +        if modified or added or removed:
>> +            raise RuntimeError('uncommitted changes left in working dir '
>> +                               'after transplanting %d changesets\n'
>> +                               % len(revs))
>> +
>
> Wouldn't util.Abort be better? (I guess the existing use of RuntimeError
> should be changed too.)

It's indicating an internal error, not a user error.  util.Abort()
hides the stack trace and gives a single-line error message, so it's
appropriate for user errors.  But if this happens, I *want* a great
big scary stack trace.  (Same reason for the other RuntimeError I
added to transplant.py a few months back, also related to this bug.)

However, I'm not convinced that this check adds enough value to be
worth the overhead of a repo.status() call.  It would take almost
nothing to convince me to drop it.

>> @@ -252,6 +262,12 @@
>>             m = match.exact(repo.root, '', files)
>>
>>         n = repo.commit(message, user, date, extra=extra, match=m)
>> +
>> +        # force repo.status() to look harder at each file in this patch
>> +        # when committing the next patch (avoids a dirstate race)
>> +        for fn in files:
>> +            repo.dirstate.normallookup(fn)
>> +
>
> Shouldn't this be done before commit so commit is guaranteed to pick up all
> these patched files and we get a 100% reliable fix?

In a sense, it is.  It's done after commit i in order for commit i+1
to see all the patched files.  See, there's only a problem when we are
transplanting multiple changesets (..., i, i+1, ...), and the i+1
makes a change to F that does not change F's size relative to i.  In
that case, the transplant of i is fine, but i+1 is transplanted wrong.
 Fixing the dirstate after transplanting i makes things fine for the
coming i+1.

Greg