This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6308

context: check file exists before getting data from _wrappedctx
ClosedPublic

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

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG14589f1989e9: context: check file exists before getting data from _wrappedctx
rHG2f3eae30719a: context: check file exists before getting data from _wrappedctx
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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Apr 24 2019, 1:09 PM
martinvonz added a subscriber: martinvonz.Apr 24 2019, 1:35 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
Closed by commit rHG2f3eae30719a: context: check file exists before getting data from _wrappedctx (authored by pulkit). · Explain WhyApr 24 2019, 3:04 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/context.py (2 lines)
Mtests/test-rebase-inmemory.t (8 lines)

Diff 14916

mercurial/context.py

# Drop old manifest cache as it is now out of date. # Drop old manifest cache as it is now out of date.
# This is necessary when, e.g., rebasing several nodes with one # This is necessary when, e.g., rebasing several nodes with one
# ``overlayworkingctx`` (e.g. with --collapse). # ``overlayworkingctx`` (e.g. with --collapse).
util.clearcachedproperty(self, '_manifest') util.clearcachedproperty(self, '_manifest')
def data(self, path): def data(self, path):
if self.isdirty(path): if self.isdirty(path):
if self._cache[path]['exists']: if self._cache[path]['exists']:
if self._cache[path]['data']: if self._cache[path]['data'] is not None:
martinvonzUnsubmitted
Not Done

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

martinvonz: I would have expected the only change to be to add ` is not None` to this line. Isn't that…
pulkitAuthorUnsubmitted
Not Done

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?

pulkit: I was unable to find some code which can set this to `None`. If you look at write() below in…
martinvonzUnsubmitted
Not Done

_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)

martinvonz: `_markdirty()` without a specified `data` argument seems to do that. The documentation also…
pulkitAuthorUnsubmitted
Not Done

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

pulkit: Does `str?` means that it can be a str or a None?
martinvonzUnsubmitted
Not Done

That would be my interpretation

martinvonz: That would be my interpretation
return self._cache[path]['data'] return self._cache[path]['data']
else: else:
# Must fallback here, too, because we only set flags. # Must fallback here, too, because we only set flags.
return self._wrappedctx[path].data() return self._wrappedctx[path].data()
else: else:
raise error.ProgrammingError("No such file or directory: %s" % raise error.ProgrammingError("No such file or directory: %s" %
path) path)
else: else:

tests/test-rebase-inmemory.t

-a -a
+b +b
$ hg ci -m "add b to foo" $ hg ci -m "add b to foo"
created new head created new head
$ hg rebase -r . -d 1 --config ui.merge=internal:merge3 $ hg rebase -r . -d 1 --config ui.merge=internal:merge3
rebasing 2:fb62b706688e "add b to foo" (tip) rebasing 2:fb62b706688e "add b to foo" (tip)
merging foo merging foo
abort: foo.orig@e780cf6f9041: not found in manifest! hit merge conflicts; re-running rebase without in-memory merge
[255] rebasing 2:fb62b706688e "add b to foo" (tip)
merging foo
warning: conflicts while merging foo! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
[1]