This is an archive of the discontinued Mercurial Phabricator instance.

repo: skip invalidation of changelog if it has 'delayed' changes (API)
ClosedPublic

Authored by martinvonz on Jul 19 2017, 6:09 PM.

Download Raw Diff

Details

Reviewers

quark
indygreg

Group Reviewers

hg-reviewers

Commits

rHG01a1c4e66816: repo: skip invalidation of changelog if it has 'delayed' changes (API)

Summary

The changelog object can store recently added revisions in memory
until the transaction is committed. We don't want to lose those
changes even if repo.invalidate(clearfilecache=True), so let's skip
the changelog when it has such 'delayed' changes.

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

martinvonz created this revision.Jul 19 2017, 6:09 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptJul 19 2017, 6:09 PM

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

Could we call changelog._writepending() automatically to solve this problem?

In D152#2246, @quark wrote:

Could we call changelog._writepending() automatically to solve this problem?

Good question. I spent quite a lot of time trying to see if that would work. It was easy to get all tests to pass, but I couldn't get it to work with our extensions (and it didn't seem to be the extensions' faults). One problem was that cl.delayupdate() would happen again after repo.invalidate() and delayupdate() doesn't have a way of taking revisions already written to .a into account (it doesn't check if self._divert is set). Also, I think that writing to .a would mean that hooks would see changes they were not meant to see.

I think this is fine as a workaround now. It's better than losing data.

tests/test-context.py
185	Seems like `f.write(b'4')` is better.

Aside from the test issue pointed out by @quark, I think this seems reasonable.

@martinvonz is correct that writing the .i.a file has hooks implications. We definitely need to keep the changes in memory.

I'm kinda surprised we haven't hit this bug in core. Unless you can find somewhere we can hit it, I'm leaning towards not taking this on stable.

FWIW, these are the kinds of caching bugs that I think a refactor of the repo class into immutable and mutable components can help prevent. The whole property cache mechanism for various things on localrepo is a bug's nest.

martinvonz marked an inline comment as done.Aug 8 2017, 12:29 AM

martinvonz updated this revision to Diff 619.

Can this be queued for default now?

tests/test-context.py
185	Oops, definitely. I also updated the line above for consistency. I don't know if other places should be updated, so I didn't.

Closed by commit rHG01a1c4e66816: repo: skip invalidation of changelog if it has 'delayed' changes (API) (authored by martinvonz). · Explain WhyAug 8 2017, 2:24 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/localrepo.py (7 lines)
M			tests/test-context.py (11 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	321		Jul 19 2017, 6:09 PM	★	★
Diff 2	619		Aug 8 2017, 12:29 AM	★	★
Diff 3	633	rHG01a1c4e668169b6c3f3a962373772c0c02460d44	Jul 19 2017, 4:34 PM	★	★

Diff 633

mercurial/localrepo.py

	(e.g. incomplete fncache causes unintentional failure, but			(e.g. incomplete fncache causes unintentional failure, but
	redundant one doesn't).			redundant one doesn't).
	'''			'''
	unfiltered = self.unfiltered() # all file caches are stored unfiltered			unfiltered = self.unfiltered() # all file caches are stored unfiltered
	for k in list(self._filecache.keys()):			for k in list(self._filecache.keys()):
	# dirstate is invalidated separately in invalidatedirstate()			# dirstate is invalidated separately in invalidatedirstate()
	if k == 'dirstate':			if k == 'dirstate':
	continue			continue
				if (k == 'changelog' and
				self.currenttransaction() and
				self.changelog._delayed):
				# The changelog object may store unwritten revisions. We don't
				# want to lose them.
				# TODO: Solve the problem instead of working around it.
				continue

	if clearfilecache:			if clearfilecache:
	del self._filecache[k]			del self._filecache[k]
	try:			try:
	delattr(unfiltered, k)			delattr(unfiltered, k)
	except AttributeError:			except AttributeError:
	pass			pass
	self.invalidatecaches()			self.invalidatecaches()

tests/test-context.py

	repo.svfs.utime('00manifest.i', (st.st_mtime + 1, st.st_mtime + 1))			repo.svfs.utime('00manifest.i', (st.st_mtime + 1, st.st_mtime + 1))

	# read the file just committed			# read the file just committed
	try:			try:
	if repo[n][i].data() != i:			if repo[n][i].data() != i:
	print('data mismatch')			print('data mismatch')
	except Exception as ex:			except Exception as ex:
	print('cannot read data: %r' % ex)			print('cannot read data: %r' % ex)

				with repo.wlock(), repo.lock(), repo.transaction('test'):
				with open(b'4', 'wb') as f:
				f.write(b'4')
				quarkUnsubmitted Done Seems like `f.write(b'4')` is better. quark: Seems like `f.write(b'4')` is better.
				martinvonzAuthorUnsubmitted Not Done Oops, definitely. I also updated the line above for consistency. I don't know if other places should be updated, so I didn't. martinvonz: Oops, definitely. I also updated the line above for consistency. I don't know if other places…
				repo.dirstate.normal('4')
				repo.commit('4')
				revsbefore = len(repo.changelog)
				repo.invalidate(clearfilecache=True)
				revsafter = len(repo.changelog)
				if revsbefore != revsafter:
				print('changeset lost by repo.invalidate()')