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

[Mads Kiilerich]

On 01/25/2011 03:18 PM, Greg Ward wrote:
> # HG changeset patch
> # User Greg Ward<greg-hg at gerg.ca>
> # Date 1295964871 18000
> # Branch stable
> # Node ID f501e673e1188511f3bd22959dd8ed693d458011
> # Parent  d0e0d3d43e1439d63564ab4dddfe0daa69ae2d86
> transplant: avoid a dirstate race when transplanting multiple changesets
> (issue2264, issue2516)
>
> The race happens when adjacent transplanted changesets change the same
> file but keep its size the same.  When those changesets are applied to
> the working dir before re-committing them, the mtime is almost
> certainly the same, so dirstate fails to notice the change.  The
> result depends on circumstances: if the transplanted changesets affect
> only one file, then we get a big noisy crash: "RuntimeError: nothing
> committed after transplant".  But if other files are included, we get
> subtle data loss, i.e. the file whose size does not change is silently
> dropped from the second transplant.
>
> I couldn't think of a way to fix this in dirstate, which means other
> extensions or scripts that do multiple commits in rapid succession
> could suffer the same problem.  But it's not too hard to fix in
> transplant: just mark each file involved in a patch as "normal
> lookup", forcing repo.status() to work harder when committing the next
> transplant in the series.
>
> diff --git a/hgext/transplant.py b/hgext/transplant.py
> --- 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.)

> @@ -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?

/Mads