This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8702

commitctx: return "touched" status from _filecommit
ClosedPublic

Authored by marmoute on Jul 8 2020, 4:36 AM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG509f5b6c0b7e: commitctx: return "touched" status from _filecommit
rHG4bc65fb8d6d1: commitctx: return "touched" status from _filecommit
Summary

Instead of mutating a list passed in argument, we simply return the information
from the _filecommit function. This make for a cleaner API and allow for
richer information to be returned. That richer information will be used in the
next commit to avoid duplicated computation.

This is part of a larger refactoring/cleanup of the commitctx code to clarify
and augment the logic gathering metadata useful for copy tracing. The current
code is a tad too long and entangled to make such update easy. We start with
easy and small cleanup.

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.Jul 8 2020, 4:36 AM
pulkit accepted this revision.Jul 21 2020, 10:28 AM
This revision is now accepted and ready to land.Jul 21 2020, 10:28 AM
pulkit added inline comments.Jul 21 2020, 10:39 AM
mercurial/localrepo.py
2794

Can you followup with documenting what None value for touched means?

2802

A nice followup will be to have some constants for these couple of strings.

marmoute added inline comments.Jul 21 2020, 11:35 AM
mercurial/localrepo.py
2794

None means the file is not affected by the commit.

2802

I considered that, but since the value are not going anywhere (simple single producer, simple single consumer) I considered overkill. Do you think we should get some constant anyway ?

marmoute added a commit: rHG4bc65fb8d6d1: commitctx: return "touched" status from _filecommit.Jul 21 2020, 12:16 PM
Closed by commit rHG4bc65fb8d6d1: commitctx: return "touched" status from _filecommit (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 22011

mercurial/localrepo.py

return None return None
return l return l
def currentwlock(self): def currentwlock(self):
"""Returns the wlock if it's held, or None if it's not.""" """Returns the wlock if it's held, or None if it's not."""
return self._currentlock(self._wlockref) return self._currentlock(self._wlockref)
def _filecommit( def _filecommit(
self, self, fctx, manifest1, manifest2, linkrev, tr, includecopymeta,
fctx,
manifest1,
manifest2,
linkrev,
tr,
changelist,
includecopymeta,
): ):
""" """
commit an individual file as part of a larger transaction commit an individual file as part of a larger transaction
input: input:
fctx: a file context with the content we are trying to commit fctx: a file context with the content we are trying to commit
manifest1: manifest of changeset first parent manifest1: manifest of changeset first parent
manifest2: manifest of changeset second parent manifest2: manifest of changeset second parent
linkrev: revision number of the changeset being created linkrev: revision number of the changeset being created
tr: current transation tr: current transation
changelist: list of file being changed (modified inplace)
individual: boolean, set to False to skip storing the copy data individual: boolean, set to False to skip storing the copy data
(only used by the Google specific feature of using (only used by the Google specific feature of using
changeset extra as copy source of truth). changeset extra as copy source of truth).
output: output: (filenode, touched)
The resulting filenode filenode: the filenode that should be used by this changeset
touched: one of: None, 'added' or 'modified'
pulkitUnsubmitted
Not Done

Can you followup with documenting what None value for touched means?

pulkit: Can you followup with documenting what `None` value for touched means?
marmouteAuthorUnsubmitted
Done

None means the file is not affected by the commit.

marmoute: None means the file is not affected by the commit.
""" """
fname = fctx.path() fname = fctx.path()
fparent1 = manifest1.get(fname, nullid) fparent1 = manifest1.get(fname, nullid)
fparent2 = manifest2.get(fname, nullid) fparent2 = manifest2.get(fname, nullid)
touched = None
if fparent1 == fparent2 == nullid:
touched = 'added'
pulkitUnsubmitted
Not Done

A nice followup will be to have some constants for these couple of strings.

pulkit: A nice followup will be to have some constants for these couple of strings.
marmouteAuthorUnsubmitted
Done

I considered that, but since the value are not going anywhere (simple single producer, simple single consumer) I considered overkill. Do you think we should get some constant anyway ?

marmoute: I considered that, but since the value are not going anywhere (simple single producer, simple…
if isinstance(fctx, context.filectx): if isinstance(fctx, context.filectx):
node = fctx.filenode() node = fctx.filenode()
if node in [fparent1, fparent2]: if node in [fparent1, fparent2]:
self.ui.debug(b'reusing %s filelog entry\n' % fname) self.ui.debug(b'reusing %s filelog entry\n' % fname)
if ( if (
fparent1 != nullid fparent1 != nullid
and manifest1.flags(fname) != fctx.flags() and manifest1.flags(fname) != fctx.flags()
) or ( ) or (
fparent2 != nullid fparent2 != nullid
and manifest2.flags(fname) != fctx.flags() and manifest2.flags(fname) != fctx.flags()
): ):
changelist.append(fname) touched = 'modified'
return node return node, touched
flog = self.file(fname) flog = self.file(fname)
meta = {} meta = {}
cfname = fctx.copysource() cfname = fctx.copysource()
fnode = None
if cfname and cfname != fname: if cfname and cfname != fname:
# Mark the new revision of this file as a copy of another # Mark the new revision of this file as a copy of another
# file. This copy data will effectively act as a parent # file. This copy data will effectively act as a parent
# of this new revision. If this is a merge, the first # of this new revision. If this is a merge, the first
# parent will be the nullid (meaning "look up the copy data") # parent will be the nullid (meaning "look up the copy data")
# and the second one will be the other parent. For example: # and the second one will be the other parent. For example:
# #
# 0 --- 1 --- 3 rev1 changes file foo # 0 --- 1 --- 3 rev1 changes file foo
fname in ms fname in ms
and ms[fname] == mergestatemod.MERGE_RECORD_MERGED_OTHER and ms[fname] == mergestatemod.MERGE_RECORD_MERGED_OTHER
): ):
fparent1, fparent2 = fparent2, nullid fparent1, fparent2 = fparent2, nullid
# is the file changed? # is the file changed?
text = fctx.data() text = fctx.data()
if fparent2 != nullid or meta or flog.cmp(fparent1, text): if fparent2 != nullid or meta or flog.cmp(fparent1, text):
changelist.append(fname) if touched is None: # do not overwrite added
return flog.add(text, meta, tr, linkrev, fparent1, fparent2) touched = 'modified'
fnode = flog.add(text, meta, tr, linkrev, fparent1, fparent2)
# are just the flags changed during merge? # are just the flags changed during merge?
elif fname in manifest1 and manifest1.flags(fname) != fctx.flags(): elif fname in manifest1 and manifest1.flags(fname) != fctx.flags():
changelist.append(fname) touched = 'modified'
fnode = fparent1
return fparent1 else:
fnode = fparent1
return fnode, touched
def checkcommitpatterns(self, wctx, match, status, fail): def checkcommitpatterns(self, wctx, match, status, fail):
"""check for commit arguments that aren't committable""" """check for commit arguments that aren't committable"""
if match.isexact() or match.prefix(): if match.isexact() or match.prefix():
matched = set(status.modified + status.added + status.removed) matched = set(status.modified + status.added + status.removed)
for f in match.files(): for f in match.files():
f = self.dirstate.normalize(f) f = self.dirstate.normalize(f)
for f in sorted(ctx.modified() + ctx.added()): for f in sorted(ctx.modified() + ctx.added()):
self.ui.note(uipathfn(f) + b"\n") self.ui.note(uipathfn(f) + b"\n")
try: try:
fctx = ctx[f] fctx = ctx[f]
if fctx is None: if fctx is None:
removed.append(f) removed.append(f)
else: else:
added.append(f) added.append(f)
m[f] = self._filecommit( m[f], is_touched = self._filecommit(
fctx, fctx, m1, m2, linkrev, trp, writefilecopymeta,
m1,
m2,
linkrev,
trp,
changed,
writefilecopymeta,
) )
if is_touched:
changed.append(f)
m.setflag(f, fctx.flags()) m.setflag(f, fctx.flags())
except OSError: except OSError:
self.ui.warn( self.ui.warn(
_(b"trouble committing %s!\n") % uipathfn(f) _(b"trouble committing %s!\n") % uipathfn(f)
) )
raise raise
except IOError as inst: except IOError as inst:
errcode = getattr(inst, 'errno', errno.ENOENT) errcode = getattr(inst, 'errno', errno.ENOENT)

tests/test-annotate.t

and its ancestor by overriding "repo._filecommit". and its ancestor by overriding "repo._filecommit".
$ cat > ../legacyrepo.py <<EOF $ cat > ../legacyrepo.py <<EOF
> from __future__ import absolute_import > from __future__ import absolute_import
> from mercurial import error, node > from mercurial import error, node
> def reposetup(ui, repo): > def reposetup(ui, repo):
> class legacyrepo(repo.__class__): > class legacyrepo(repo.__class__):
> def _filecommit(self, fctx, manifest1, manifest2, > def _filecommit(self, fctx, manifest1, manifest2,
> linkrev, tr, changelist, includecopymeta): > linkrev, tr, includecopymeta):
> fname = fctx.path() > fname = fctx.path()
> text = fctx.data() > text = fctx.data()
> flog = self.file(fname) > flog = self.file(fname)
> fparent1 = manifest1.get(fname, node.nullid) > fparent1 = manifest1.get(fname, node.nullid)
> fparent2 = manifest2.get(fname, node.nullid) > fparent2 = manifest2.get(fname, node.nullid)
> meta = {} > meta = {}
> copy = fctx.copysource() > copy = fctx.copysource()
> if copy and copy != fname: > if copy and copy != fname:
> raise error.Abort('copying is not supported') > raise error.Abort('copying is not supported')
> if fparent2 != node.nullid: > if fparent2 != node.nullid:
> changelist.append(fname)
> return flog.add(text, meta, tr, linkrev, > return flog.add(text, meta, tr, linkrev,
> fparent1, fparent2) > fparent1, fparent2), 'modified'
> raise error.Abort('only merging is supported') > raise error.Abort('only merging is supported')
> repo.__class__ = legacyrepo > repo.__class__ = legacyrepo
> EOF > EOF
$ cat > baz <<EOF $ cat > baz <<EOF
> 1 > 1
> 2 > 2
> 3 > 3

tests/test-fastannotate-hg.t

and (2) the extension to allow filelog merging between the revision and (2) the extension to allow filelog merging between the revision
and its ancestor by overriding "repo._filecommit". and its ancestor by overriding "repo._filecommit".
$ cat > ../legacyrepo.py <<EOF $ cat > ../legacyrepo.py <<EOF
> from mercurial import error, node > from mercurial import error, node
> def reposetup(ui, repo): > def reposetup(ui, repo):
> class legacyrepo(repo.__class__): > class legacyrepo(repo.__class__):
> def _filecommit(self, fctx, manifest1, manifest2, > def _filecommit(self, fctx, manifest1, manifest2,
> linkrev, tr, changelist, includecopymeta): > linkrev, tr, includecopymeta):
> fname = fctx.path() > fname = fctx.path()
> text = fctx.data() > text = fctx.data()
> flog = self.file(fname) > flog = self.file(fname)
> fparent1 = manifest1.get(fname, node.nullid) > fparent1 = manifest1.get(fname, node.nullid)
> fparent2 = manifest2.get(fname, node.nullid) > fparent2 = manifest2.get(fname, node.nullid)
> meta = {} > meta = {}
> copy = fctx.renamed() > copy = fctx.renamed()
> if copy and copy[0] != fname: > if copy and copy[0] != fname:
> raise error.Abort('copying is not supported') > raise error.Abort('copying is not supported')
> if fparent2 != node.nullid: > if fparent2 != node.nullid:
> changelist.append(fname)
> return flog.add(text, meta, tr, linkrev, > return flog.add(text, meta, tr, linkrev,
> fparent1, fparent2) > fparent1, fparent2), 'modified'
> raise error.Abort('only merging is supported') > raise error.Abort('only merging is supported')
> repo.__class__ = legacyrepo > repo.__class__ = legacyrepo
> EOF > EOF
$ cat > baz <<EOF $ cat > baz <<EOF
> 1 > 1
> 2 > 2
> 3 > 3