This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11781

dirstate: add a comment about a racy piece of code during updates
ClosedPublic

Authored by marmoute on Nov 24 2021, 6:11 AM.

Download Raw Diff

Details

Reviewers

Alphare

Group Reviewers

hg-reviewers

Commits

rHGc655483ea6e2: dirstate: add a comment about a racy piece of code during updates

Summary

This is a bit that is not really correct but works "fine" in practice. Let us
write the details down so that people stop wondering how that logic might be
correct… It is not.

Diff Detail

Repository

rHG Mercurial

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

marmoute created this revision.Nov 24 2021, 6:11 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptNov 24 2021, 6:11 AM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

marmoute added a child revision: D11782: dirstate-item: allow mtime to be None in "parentdata".Nov 24 2021, 6:11 AM

Alphare accepted this revision.Nov 24 2021, 8:04 AM

This revision is now accepted and ready to land.Nov 24 2021, 8:04 AM

marmoute edited the summary of this revision. (Show Details)Nov 30 2021, 6:53 PM

marmoute updated this revision to Diff 31223.

marmoute added a commit: rHGc655483ea6e2: dirstate: add a comment about a racy piece of code during updates.Dec 1 2021, 10:20 AM

Closed by commit rHGc655483ea6e2: dirstate: add a comment about a racy piece of code during updates (authored by marmoute). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/merge.py (28 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	31103		Nov 24 2021, 6:11 AM	★	★
Diff 2	31223		Nov 30 2021, 6:53 PM	★	★
Diff 3	31243	rHGc655483ea6e21562af96d3397575093cf255ae59	Nov 16 2021, 8:58 PM	★	★

Status	Author	Revision
Closed	marmoute	D11800 largefile: use the proper "mtime boundary" logic during fixup
Closed	marmoute	D11799 status: move the boundary comparison logic within the timestamp module
Closed	marmoute	D11798 tests: remove potential mtime ambiguity in a dirstate test
Closed	marmoute	D11797 dirstate: cleanup remaining of "now" during write
Closed	marmoute	D11796 dirstate: remove need_delay logic
Closed	marmoute	D11795 dirstate: remove `lastnormaltime` mechanism
Closed	marmoute	D11794 status: use filesystem time boundary to invalidate racy mtime
Closed	marmoute	D11793 test: use a different timestamp for the updated file
Closed	marmoute	D11792 win32text: drop associated dirstate cache information on revert
Closed	marmoute	D11791 dirstate: stop gathering parentfiledata in update_file
Closed	marmoute	D11790 dirstate: stop gathering parentfiledata in update_file_p1
Closed	marmoute	D11789 dirstate: make it mandatory to provide parentfiledata in `set_clean`
Closed	marmoute	D11788 dirstate: do no use `set_clean` in revert
Closed	marmoute	D11787 status: adapt the "keyword" extensions to gather stats at lookup time
Closed	marmoute	D11786 status: adapt largefile to gather stats at lookup time
Closed	marmoute	D11785 status: gather fixup info at comparison time
Closed	marmoute	D11784 update: filter the ambiguous mtime in update directly
Closed	marmoute	D11783 dirstate: move "get fs now" in the timestamp utility module
Closed	marmoute	D11782 dirstate-item: allow mtime to be None in "parentdata"
Closed	marmoute	D11781 dirstate: add a comment about a racy piece of code during updates
Closed	marmoute	D11780 tests: add missing `head` for when things go wrong
Closed	marmoute	D11779 tests: ensure a status will have non ambiguous mtime in some race test
Closed	marmoute	D11778 tests: make sure no ambiguities remains after the commit
Closed	marmoute	D11777 dirstate: clarify a `hg update` invocation in a test
Closed	marmoute	D11833 test: mark rhg output as flaky

Diff 31243

mercurial/merge.py

	atomictemp = ui.configbool(b"experimental", b"update.atomic-file")			atomictemp = ui.configbool(b"experimental", b"update.atomic-file")
	size = wfctx.write(			size = wfctx.write(
	fctx(f).data(),			fctx(f).data(),
	flags,			flags,
	backgroundclose=True,			backgroundclose=True,
	atomictemp=atomictemp,			atomictemp=atomictemp,
	)			)
	if wantfiledata:			if wantfiledata:
				# XXX note that there is a race window between the time we
				# write the clean data into the file and we stats it. So another
				# writing process meddling with the file content right after we
				# wrote it could cause bad stat data to be gathered.
				#
				# They are 2 data we gather here
				# - the mode:
				# That we actually just wrote, we should not need to read
				# it from disk, (except not all mode might have survived
				# the disk round-trip, which is another issue: we should
				# not depends on this)
				# - the mtime,
				# On system that support nanosecond precision, the mtime
				# could be accurate enough to tell the two writes appart.
				# However gathering it in a racy way make the mtime we
				# gather "unreliable".
				#
				# (note: we get the size from the data we write, which is sane)
				#
				# So in theory the data returned here are fully racy, but in
				# practice "it works mostly fine".
				#
				# Do not be surprised if you end up reading this while looking
				# for the causes of some buggy status. Feel free to improve
				# this in the future, but we cannot simply stop gathering
				# information. Otherwise `hg status` call made after a large `hg
				# update` runs would have to redo a similar amount of work to
				# restore and compare all files content.
	s = wfctx.lstat()			s = wfctx.lstat()
	mode = s.st_mode			mode = s.st_mode
	mtime = timestamp.mtime_of(s)			mtime = timestamp.mtime_of(s)
	# for dirstate.update_file's parentfiledata argument:			# for dirstate.update_file's parentfiledata argument:
	filedata[f] = (mode, size, mtime)			filedata[f] = (mode, size, mtime)
	if i == 100:			if i == 100:
	yield False, (i, f)			yield False, (i, f)
	i = 0			i = 0