Or perhaps we can mark the dirstate entries invalid.

Aside from commit being inherently racy, I think the fixable parts of this have
been addressed.

See also issue552 (Errors in test-clone-pull-corruption)

Observation: in the presence of hooks that can mutate the local filesystem,
there is a general problem that the "order of operation" for commit processing
needs to be explicitly defined. There was another bug (can't find it at the
moment) wherein a file being committed was modified by a commit script.

The main point is that anything of the form "read after write after read" is bad
news, and the only way to resolve these in general is to have a well defined and
documented processing model for handling the workspace.

On 3/31/06, Alexis S. L. Carvalho <mercurial-bugs@selenic.com> wrote:
>
> Two questions about the changes to the commit function:
>
> - With these changes, the precommit hook will run while holding the wlock.
>   Is that a problem?

I don't think it matters, the user probably don't want the working
copy to be modified (eg update/adding/removing/...) in the precommit
hook.
>
> - Should the wlock be passed to the changes function?
>
It should if possible, I'll correct it if the patch is ok.

thanks,

Benoit

Two questions about the changes to the commit function:

- With these changes, the precommit hook will run while holding the wlock.
  Is that a problem?

- Should the wlock be passed to the changes function?

Other than that, it looks ok to me.

On 3/28/06, Bryan O'Sullivan <mercurial-bugs@selenic.com> wrote:
>
> Which patch?  :-)  There are 4 patches attached to the bug, and I don't know
> which one you're talking about.

race.diff was already applied, I am talking about wlock_race.diff

Benoit

Which patch?  :-)  There are 4 patches attached to the bug, and I don't know
which one you're talking about.

back to this, is something missing in the patch ?

set to in-progress, as wlock_race.diff isn't applied yet.

There's no harm in checking all of mtime, ctime and size.

If the size is the same, then mtime/ctime are probably the same
(except when stripping, but we don't care for that case).

A patch is attached for the possible race in commit/update/...

On Wed, Feb 22, 2006 at 06:45:18AM +0000, Thomas Arendsen Hein wrote:
> 
> Thomas Arendsen Hein <thomas@intevation.de> added the comment:
> 
> Looks good and works fine.
> 
> Pushed to http://hg.intevation.org/mercurial/tah with test cases added.
> Should I push to crew?

Yes.

Looks good and works fine.

Pushed to http://hg.intevation.org/mercurial/tah with test cases added.
Should I push to crew?

updated patch against tip

Proposed fix attached.

And here's a script to reproduce the "pull+pull" corruption.

The same comments from the last message apply.

bos has suggested using hooks to easily reproduce the situations that I
described.

Here's a script to reproduces the "clone+pull" corruption.  You can specify
an alternative hg command to use through the HG environment variable.

hg verify reports a different corruption, but it's still avoidable by 
rereading the manifest and changelog after locking the repo in the pull
method.

Note that this is not just an issue with commits or rollbacks.  Here's a
way to corrupt the repo with two (successful!) pulls:

Add something like this to localrepository.pull after locking the repo,
just to make the race very easy to exploit:

        import time
	time.sleep(5)

Then create one repo with a long history and another one with a shorter
(but different) history.  And then pull them both into a third
repository.  Boom.

hg init source1
cd source1
touch foo
hg add foo
for i in $(seq 10); do echo $1 >> foo; hg ci -m $i; done
cd ..
hg clone -r 0 source1 source2
cd source2
echo a >> foo
hg ci -m a
cd ..
hg init corrupted
cd corrupted
hg pull ../source1 & sleep 1; hg pull ../source2

Both pulls think there was no problem, but:
$ hg verify
checking changesets
changelog data length off by 854 bytes
Segmentation fault

Rereading the manifest and the changelog after locking the repo fixes
this.

The planned reordering of commit would help here a little bit, but there still
will be a race condition then if a different error causes a rollback.

Nice piece of detective work.  I'm bumping this to urgent, since it can corrupt
the repository.

A while ago somebody mentioned on IRC that hg pull would corrupt the
repo if it was started while a (soon to be aborted) commit was running
(more details on how to reproduce this below).

After poking it a bit, I think this happens because the manifest is read
in localrepository.__init__ (when the file has already been changed for
the commit), but when we actually get to use the manifest, the aborted
commit has already rolledback the repo.

To try to confirm this, I've tried to reread the manifest after locking
the repo in the pull method, and this does indeed fix this particular
issue.  This is of course only a quick hack, since there are probably a
few other places that would need some fixing.

How to reproduce it:

# create 2 repos, such that one of them can pull from the other
hg init a
cd a
touch foo
hg add foo
hg ci -m 'add foo'
hg clone . ../b
echo >> foo
hg ci -m 'change foo'
cd ..

# go into repo b and start a new commit
cd b
touch bar
hg add bar
hg ci
# do not close your editor

# in another shell, do an hg pull
cd b
hg pull ../a
# this will block, waiting for the lock
# go back to the editor and quit it without saving, to abort the commit.
# it will abort the transaction and rollback everything, without
# problems, but the hg pull will print something like:
pulling from ../a
waiting for lock held by 5592
searching for changes
adding changesets
adding manifests
abort: integrity check failed on 00manifest.d:1!
transaction abort!
rollback completed

# the output of hg verify afterwards
checking changesets
checking manifests
manifest data length off by 103 bytes
crosschecking files in changesets and manifests
checking files
1 files, 1 changesets, 1 total revisions
1 integrity errors encountered!