This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9965

Work around for issue6456, check content of symlinks because st_size may indicate the size of encrypted data.
AbandonedPublic

Authored by cschuhen on Feb 7 2021, 6:15 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

cschuhen created this revision.Feb 7 2021, 6:15 PM
Alphare added a subscriber: Alphare.Feb 8 2021, 8:59 AM
Alphare added inline comments.
mercurial/dirstate.py
1283

Looking at the issue, it looks like Yuya is advising to fall back to the slow path only when the size of the link differs. Is that not sufficient? If it is, please add this check.
I don't expect real-life repositories to contain a ton of symlinks, but let's not make the dirstate slower than it currently is.
I'll keep that use case in mind for the current rewrite, thanks for the report.

cschuhen abandoned this revision.Mar 15 2021, 7:13 PM

Replaced by D10146.

Revision Contents
Changeset List

PathPackages
Mmercurial/context.py (4 lines)
Mmercurial/dirstate.py (8 lines)

Diff 25496

mercurial/context.py

if self._repo._encodefilterpats: if self._repo._encodefilterpats:
# can't rely on size() because wdir content may be decoded # can't rely on size() because wdir content may be decoded
return self._filelog.cmp(self._filenode, fctx.data()) return self._filelog.cmp(self._filenode, fctx.data())
if self.size() - 4 == fctx.size(): if self.size() - 4 == fctx.size():
# size() can match: # size() can match:
# if file data starts with '\1\n', empty metadata block is # if file data starts with '\1\n', empty metadata block is
# prepended, which adds 4 bytes to filelog.size(). # prepended, which adds 4 bytes to filelog.size().
return self._filelog.cmp(self._filenode, fctx.data()) return self._filelog.cmp(self._filenode, fctx.data())
if self.size() == fctx.size(): if self.size() == fctx.size() or self.flags() == b'l':
# size() matches: need to compare content # size() matches: need to compare content
# issue6456: Always compare symlinks because size can represent
# encrypted string for EXT-4 encryption(fscrypt).
return self._filelog.cmp(self._filenode, fctx.data()) return self._filelog.cmp(self._filenode, fctx.data())
# size() differs # size() differs
return True return True
def _adjustlinkrev(self, srcrev, inclusive=False, stoprev=None): def _adjustlinkrev(self, srcrev, inclusive=False, stoprev=None):
"""return the first ancestor of <srcrev> introducing <fnode> """return the first ancestor of <srcrev> introducing <fnode>

mercurial/dirstate.py

state = t[0] state = t[0]
mode = t[1] mode = t[1]
size = t[2] size = t[2]
time = t[3] time = t[3]
if not st and state in b"nma": if not st and state in b"nma":
dadd(fn) dadd(fn)
elif state == b'n': elif state == b'n':
islink = (stat.S_IFLNK == (st.st_mode & stat.S_IFLNK))
if ( if (
size >= 0 size >= 0
and ( and (
(size != st.st_size and size != st.st_size & _rangemask) (size != st.st_size and size != st.st_size & _rangemask)
or ((mode ^ st.st_mode) & 0o100 and checkexec) or ((mode ^ st.st_mode) & 0o100 and checkexec)
) )
or size == -2 # other parent or size == -2 # other parent
or fn in copymap or fn in copymap
): ):
if (islink):
AlphareUnsubmitted
Not Done

Looking at the issue, it looks like Yuya is advising to fall back to the slow path only when the size of the link differs. Is that not sufficient? If it is, please add this check.
I don't expect real-life repositories to contain a ton of symlinks, but let's not make the dirstate slower than it currently is.
I'll keep that use case in mind for the current rewrite, thanks for the report.

Alphare: Looking at the issue, it looks like Yuya is advising to fall back to the slow path only when…
# issue6456: Size returned may be longer due to
# encryption on EXT-4 fscrypt, undecided.
ladd(fn)
else:
madd(fn) madd(fn)
elif ( elif (
time != st[stat.ST_MTIME] time != st[stat.ST_MTIME]
and time != st[stat.ST_MTIME] & _rangemask and time != st[stat.ST_MTIME] & _rangemask
): ):
ladd(fn) ladd(fn)
elif st[stat.ST_MTIME] == lastnormaltime: elif st[stat.ST_MTIME] == lastnormaltime:
# fn may have just been marked as normal and it may have # fn may have just been marked as normal and it may have
# changed in the same second without changing its size. # changed in the same second without changing its size.