This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11785

status: gather fixup info at comparison time
ClosedPublic

Authored by marmoute on Nov 24 2021, 6:11 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG41f40f35278a: status: gather fixup info at comparison time
Summary

This is still racy, but on a much small windows. In addition, the API now make
it possible for it to not be racy. This also unlock other cleanups that we are
about to do regarding mtime ambiguity at gathering time.

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

marmoute created this revision.Nov 24 2021, 6:11 AM
Alphare requested changes to this revision.Nov 24 2021, 8:22 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
mercurial/context.py
1820–1834

/s/not/note/

1824

You accidentally a word

1868

Maybe make the keyword explicit to be clearer? normal(f, parentfiledata=pfd)

This revision now requires changes to proceed.Nov 24 2021, 8:22 AM
marmoute added inline comments.Nov 30 2021, 12:25 PM
mercurial/context.py
1868

Actually we cannot pass keyword argument to lambda function.
(The argument becomes mandatory later anyway.)

marmoute edited the summary of this revision. (Show Details)Nov 30 2021, 6:53 PM
marmoute updated this revision to Diff 31226.
Alphare added a comment.Dec 1 2021, 5:10 AM

Sure, but one of the comments is still missing (at least) a word

marmoute updated this revision to Diff 31235.Dec 1 2021, 8:21 AM
Alphare accepted this revision.Dec 1 2021, 10:15 AM
This revision is now accepted and ready to land.Dec 1 2021, 10:15 AM
marmoute added a commit: rHG41f40f35278a: status: gather fixup info at comparison time.Dec 1 2021, 10:21 AM
Closed by commit rHG41f40f35278a: status: gather fixup info at comparison time (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/context.py (27 lines)

Diff 31248

mercurial/context.py

subrepo, subrepo,
subrepoutil, subrepoutil,
util, util,
) )
from .utils import ( from .utils import (
dateutil, dateutil,
stringutil, stringutil,
) )
from .dirstateutils import (
timestamp,
)
propertycache = util.propertycache propertycache = util.propertycache
class basectx(object): class basectx(object):
"""A basectx object represents the common logic for its children: """A basectx object represents the common logic for its children:
changectx: read-only context that is already present in the repo, changectx: read-only context that is already present in the repo,
workingctx: a context that represents the working directory and can workingctx: a context that represents the working directory and can
# directory in the interim, but fixing that is pretty hard. # directory in the interim, but fixing that is pretty hard.
if ( if (
f not in pctx f not in pctx
or self.flags(f) != pctx.flags(f) or self.flags(f) != pctx.flags(f)
or pctx[f].cmp(self[f]) or pctx[f].cmp(self[f])
): ):
modified.append(f) modified.append(f)
else: else:
fixup.append(f) # XXX note that we have a race windows here since we gather
# the stats after we compared so the file might have
# changed.
#
# However this have always been the case and the
AlphareUnsubmitted
Not Done

You accidentally a word

Alphare: You accidentally a word
# refactoring moving the code here is improving the
# situation by narrowing the race and moving the two steps
# (comparison + stat) in the same location.
#
# Making this code "correct" is now possible.
s = self[f].lstat()
mode = s.st_mode
size = s.st_size
mtime = timestamp.mtime_of(s)
fixup.append((f, (mode, size, mtime)))
AlphareUnsubmitted
Not Done

/s/not/note/

Alphare: /s/not/note/
except (IOError, OSError): except (IOError, OSError):
# A file become inaccessible in between? Mark it as deleted, # A file become inaccessible in between? Mark it as deleted,
# matching dirstate behavior (issue5584). # matching dirstate behavior (issue5584).
# The dirstate has more complex behavior around whether a # The dirstate has more complex behavior around whether a
# missing file matches a directory, etc, but we don't need to # missing file matches a directory, etc, but we don't need to
# bother with that: if f has made it to this point, we're sure # bother with that: if f has made it to this point, we're sure
# it's in the dirstate. # it's in the dirstate.
deleted.append(f) deleted.append(f)
# so we don't wait on the lock # so we don't wait on the lock
# wlock can invalidate the dirstate, so cache normal _after_ # wlock can invalidate the dirstate, so cache normal _after_
# taking the lock # taking the lock
with self._repo.wlock(False): with self._repo.wlock(False):
dirstate = self._repo.dirstate dirstate = self._repo.dirstate
if dirstate.identity() == oldid: if dirstate.identity() == oldid:
if fixup: if fixup:
if dirstate.pendingparentchange(): if dirstate.pendingparentchange():
normal = lambda f: dirstate.update_file( normal = lambda f, pfd: dirstate.update_file(
f, p1_tracked=True, wc_tracked=True f, p1_tracked=True, wc_tracked=True
) )
else: else:
normal = dirstate.set_clean normal = dirstate.set_clean
for f in fixup: for f, pdf in fixup:
normal(f) normal(f, pdf)
AlphareUnsubmitted
Not Done

Maybe make the keyword explicit to be clearer? normal(f, parentfiledata=pfd)

Alphare: Maybe make the keyword explicit to be clearer? `normal(f, parentfiledata=pfd)`
marmouteAuthorUnsubmitted
Done

Actually we cannot pass keyword argument to lambda function.
(The argument becomes mandatory later anyway.)

marmoute: Actually we cannot pass keyword argument to lambda function. (The argument becomes mandatory…
# write changes out explicitly, because nesting # write changes out explicitly, because nesting
# wlock at runtime may prevent 'wlock.release()' # wlock at runtime may prevent 'wlock.release()'
# after this block from doing so for subsequent # after this block from doing so for subsequent
# changing files # changing files
tr = self._repo.currenttransaction() tr = self._repo.currenttransaction()
self._repo.dirstate.write(tr) self._repo.dirstate.write(tr)
if poststatus: if poststatus:
# check for any possibly clean files # check for any possibly clean files
fixup = [] fixup = []
if cmp: if cmp:
modified2, deleted2, fixup = self._checklookup(cmp) modified2, deleted2, fixup = self._checklookup(cmp)
s.modified.extend(modified2) s.modified.extend(modified2)
s.deleted.extend(deleted2) s.deleted.extend(deleted2)
if fixup and clean: if fixup and clean:
s.clean.extend(fixup) s.clean.extend((f for f, _ in fixup))
self._poststatusfixup(s, fixup) self._poststatusfixup(s, fixup)
if match.always(): if match.always():
# cache for performance # cache for performance
if s.unknown or s.ignored or s.clean: if s.unknown or s.ignored or s.clean:
# "_status" is cached with list*=False in the normal route # "_status" is cached with list*=False in the normal route
self._status = scmutil.status( self._status = scmutil.status(