This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11785

status: gather fixup info at comparison time
ClosedPublic

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

Download Raw Diff

Details

Reviewers

Alphare

Group Reviewers

hg-reviewers

Commits

rHG41f40f35278a: status: gather fixup info at comparison time

Summary

This is still racy, but on a much small windows. In addition, the API now make
it possible for it to not be racy. This also unlock other cleanups that we are
about to do regarding mtime ambiguity at gathering time.

Diff Detail

Repository

rHG Mercurial

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

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: D11786: status: adapt largefile to gather stats at lookup time.Nov 24 2021, 6:11 AM

Alphare requested changes to this revision.Nov 24 2021, 8:22 AM

Alphare added a subscriber: Alphare.

Alphare added inline comments.

mercurial/context.py
1820–1834	/s/not/note/
1824	You accidentally a word
1868	Maybe make the keyword explicit to be clearer? `normal(f, parentfiledata=pfd)`

This revision now requires changes to proceed.Nov 24 2021, 8:22 AM

marmoute added inline comments.Nov 30 2021, 12:25 PM

mercurial/context.py
1868	Actually we cannot pass keyword argument to lambda function. (The argument becomes mandatory later anyway.)

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

marmoute updated this revision to Diff 31226.

Sure, but one of the comments is still missing (at least) a word

marmoute updated this revision to Diff 31235.Dec 1 2021, 8:21 AM

Alphare accepted this revision.Dec 1 2021, 10:15 AM

This revision is now accepted and ready to land.Dec 1 2021, 10:15 AM

marmoute added a commit: rHG41f40f35278a: status: gather fixup info at comparison time.Dec 1 2021, 10:21 AM

Closed by commit rHG41f40f35278a: status: gather fixup info at comparison time (authored by marmoute). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/context.py (27 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	31107		Nov 24 2021, 6:11 AM	★	★
Diff 2	31226		Nov 30 2021, 6:53 PM	★	★
Diff 3	31235		Dec 1 2021, 8:21 AM	★	★
Diff 4	31248	rHG41f40f35278a29159afb536f7ac2baa91ce85d67	Nov 17 2021, 5:37 PM	★	★

Commit	Parents	Author	Summary	Date
9c3a61717272	c154a9c963cc	Pierre-Yves David		Nov 17 2021, 5:37 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 31235

mercurial/context.py

	subrepo,			subrepo,
	subrepoutil,			subrepoutil,
	util,			util,
	)			)
	from .utils import (			from .utils import (
	dateutil,			dateutil,
	stringutil,			stringutil,
	)			)
				from .dirstateutils import (
				timestamp,
				)

	propertycache = util.propertycache			propertycache = util.propertycache


	class basectx(object):			class basectx(object):
	"""A basectx object represents the common logic for its children:			"""A basectx object represents the common logic for its children:
	changectx: read-only context that is already present in the repo,			changectx: read-only context that is already present in the repo,
	workingctx: a context that represents the working directory and can			workingctx: a context that represents the working directory and can
	# directory in the interim, but fixing that is pretty hard.			# directory in the interim, but fixing that is pretty hard.
	if (			if (
	f not in pctx			f not in pctx
	or self.flags(f) != pctx.flags(f)			or self.flags(f) != pctx.flags(f)
	or pctx[f].cmp(self[f])			or pctx[f].cmp(self[f])
	):			):
	modified.append(f)			modified.append(f)
	else:			else:
	fixup.append(f)			# XXX note that we have a race windows here since we gather
				# the stats after we compared so the file might have
				# changed.
				#
				# However this have always been the case and the
				AlphareUnsubmitted Not Done You accidentally a word Alphare: You accidentally a word
				# refactoring moving the code here is improving the
				# situation by narrowing the race and moving the two steps
				# (comparison + stat) in the same location.
				#
				# Making this code "correct" is now possible.
				s = self[f].lstat()
				mode = s.st_mode
				size = s.st_size
				mtime = timestamp.mtime_of(s)
				fixup.append((f, (mode, size, mtime)))
				AlphareUnsubmitted Not Done /s/not/note/ Alphare: /s/not/note/
	except (IOError, OSError):			except (IOError, OSError):
	# A file become inaccessible in between? Mark it as deleted,			# A file become inaccessible in between? Mark it as deleted,
	# matching dirstate behavior (issue5584).			# matching dirstate behavior (issue5584).
	# The dirstate has more complex behavior around whether a			# The dirstate has more complex behavior around whether a
	# missing file matches a directory, etc, but we don't need to			# missing file matches a directory, etc, but we don't need to
	# bother with that: if f has made it to this point, we're sure			# bother with that: if f has made it to this point, we're sure
	# it's in the dirstate.			# it's in the dirstate.
	deleted.append(f)			deleted.append(f)
	# so we don't wait on the lock			# so we don't wait on the lock
	# wlock can invalidate the dirstate, so cache normal _after_			# wlock can invalidate the dirstate, so cache normal _after_
	# taking the lock			# taking the lock
	with self._repo.wlock(False):			with self._repo.wlock(False):
	dirstate = self._repo.dirstate			dirstate = self._repo.dirstate
	if dirstate.identity() == oldid:			if dirstate.identity() == oldid:
	if fixup:			if fixup:
	if dirstate.pendingparentchange():			if dirstate.pendingparentchange():
	normal = lambda f: dirstate.update_file(			normal = lambda f, pfd: dirstate.update_file(
	f, p1_tracked=True, wc_tracked=True			f, p1_tracked=True, wc_tracked=True
	)			)
	else:			else:
	normal = dirstate.set_clean			normal = dirstate.set_clean
	for f in fixup:			for f, pdf in fixup:
	normal(f)			normal(f, pdf)
				AlphareUnsubmitted Not Done Maybe make the keyword explicit to be clearer? `normal(f, parentfiledata=pfd)` Alphare: Maybe make the keyword explicit to be clearer? `normal(f, parentfiledata=pfd)`
				marmouteAuthorUnsubmitted Done Actually we cannot pass keyword argument to lambda function. (The argument becomes mandatory later anyway.) marmoute: Actually we cannot pass keyword argument to lambda function. (The argument becomes mandatory…
	# write changes out explicitly, because nesting			# write changes out explicitly, because nesting
	# wlock at runtime may prevent 'wlock.release()'			# wlock at runtime may prevent 'wlock.release()'
	# after this block from doing so for subsequent			# after this block from doing so for subsequent
	# changing files			# changing files
	tr = self._repo.currenttransaction()			tr = self._repo.currenttransaction()
	self._repo.dirstate.write(tr)			self._repo.dirstate.write(tr)

	if poststatus:			if poststatus:
	# check for any possibly clean files			# check for any possibly clean files
	fixup = []			fixup = []
	if cmp:			if cmp:
	modified2, deleted2, fixup = self._checklookup(cmp)			modified2, deleted2, fixup = self._checklookup(cmp)
	s.modified.extend(modified2)			s.modified.extend(modified2)
	s.deleted.extend(deleted2)			s.deleted.extend(deleted2)

	if fixup and clean:			if fixup and clean:
	s.clean.extend(fixup)			s.clean.extend((f for f, _ in fixup))

	self._poststatusfixup(s, fixup)			self._poststatusfixup(s, fixup)

	if match.always():			if match.always():
	# cache for performance			# cache for performance
	if s.unknown or s.ignored or s.clean:			if s.unknown or s.ignored or s.clean:
	# "_status" is cached with list*=False in the normal route			# "_status" is cached with list*=False in the normal route
	self._status = scmutil.status(			self._status = scmutil.status(