This is an archive of the discontinued Mercurial Phabricator instance.

memfilectx: make changectx argument mandatory in constructor (API)
ClosedPublic

Authored by martinvonz on Dec 11 2017, 12:47 PM.

Details

Summary

committablefilectx has three subclasses: workingfilectx, memfilectx,
and overlayfilectx. committablefilectx takes an optional (change) ctx
instance to its constructor. If it's provided, it's set on the
instance as self._changectx. If not, that property is supposed to be
defined by the class. However, only workingfilectx does that. The
other two will have the property undefined if it's not passed in the
constructor. That seems bad to me. This patch makes the changectx
argument to the memfilectx constructor mandatory because that fixes
the failure I ran into. It seems like we should also fix the
overlayfilectx case.

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.Dec 11 2017, 12:47 PM
martinvonz updated this revision to Diff 4358.Dec 11 2017, 12:56 PM
durin42 accepted this revision.Dec 11 2017, 1:14 PM
This revision is now accepted and ready to land.Dec 11 2017, 1:14 PM
durin42 requested changes to this revision.Dec 11 2017, 1:35 PM

This seems to break test-rebase-inmemory.t - could you take a look?

This revision now requires changes to proceed.Dec 11 2017, 1:35 PM
martinvonz updated this revision to Diff 4365.Dec 11 2017, 3:33 PM

This seems to break test-rebase-inmemory.t - could you take a look?

Sorry, I was lazy and didn't run the tests after the last rebase. Phil's new test-rebase-inmemory.t caught a bug (thanks!). I also found another bug in synthrepo that I have now fixed.

yuja added a subscriber: yuja.Dec 12 2017, 8:36 AM

Looks good, but can you flag this as (API) change?

martinvonz updated this revision to Diff 4381.Dec 12 2017, 2:34 PM
martinvonz retitled this revision from memfilectx: make changectx argument mandatory in constructor to memfilectx: make changectx argument mandatory in constructor (API).Dec 12 2017, 2:39 PM
In D1658#28514, @yuja wrote:

Looks good, but can you flag this as (API) change?

Good point. Done.

yuja accepted this revision.Dec 13 2017, 7:36 AM
This revision was automatically updated to reflect the committed changes.