This is an archive of the discontinued Mercurial Phabricator instance.

context: check file exists before getting data from _wrappedctx
ClosedPublic

Authored by pulkit on Apr 24 2019, 1:09 PM.

Details

Summary

overlayworkingctx class is used to do in-memory merging. The data() function of
that class has logic to look for data() in the wrappedctx if the file data in
cache is empty and if the file is dirty. This assumes that if a file is dirty
and cache has empty data for it, it will exists in the _wrappedctx.

However this assumption can be False in case when we are merging a file which is
empty in destination. In these cases, the backup file 'foo.orig' created by our
internal merge algorithms will be empty, however it won't be present in
_wrappedctx. This case will lead us to error like the one this patch is fixing.

Let's only fallback to getting data from wrappedctx if cache has 'None' as data.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Apr 24 2019, 1:09 PM
martinvonz added inline comments.
mercurial/context.py
1827

I would have expected the only change to be to add is not None to this line. Isn't that enough?

pulkit added inline comments.Apr 24 2019, 1:59 PM
mercurial/context.py
1827

I was unable to find some code which can set this to None. If you look at write() below in line 1993, it makes sure data is not None.

I am not sure I understand what this if does. Do you have ideas?

martinvonz added inline comments.Apr 24 2019, 2:10 PM
mercurial/context.py
1827

_markdirty() without a specified data argument seems to do that. The documentation also seems to say that it can be None (line 1804 in this version)

pulkit added inline comments.Apr 24 2019, 2:14 PM
mercurial/context.py
1827

Does str? means that it can be a str or a None?

martinvonz added inline comments.Apr 24 2019, 2:16 PM
mercurial/context.py
1827

That would be my interpretation

pulkit updated this revision to Diff 14910.Apr 24 2019, 2:22 PM
pulkit updated this revision to Diff 14911.
pulkit updated this revision to Diff 14913.Apr 24 2019, 2:30 PM
pulkit edited the summary of this revision. (Show Details)Apr 24 2019, 2:33 PM
This revision was automatically updated to reflect the committed changes.