[PATCH 01 of 11] commitctx: document the None return for "touched" value

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH 01 of 11] commitctx: document the None return for "touched" value

Pierre-Yves David-2
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1595427033 -7200
#      Wed Jul 22 16:10:33 2020 +0200
# Node ID 2727e91ffa6e9063bd9c29671b5008cfef22dd97
# Parent  4ccd5ec565c2baaa1d598b20a7ea14d3c4fd39dc
# EXP-Topic commitctx-cleanup-2
# Available At https://foss.heptapod.net/octobus/mercurial-devel/
#              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 2727e91ffa6e
commitctx: document the None return for "touched" value

diff --git a/mercurial/commit.py b/mercurial/commit.py
--- a/mercurial/commit.py
+++ b/mercurial/commit.py
@@ -238,7 +238,7 @@ def _filecommit(
     output: (filenode, touched)
 
         filenode: the filenode that should be used by this changeset
-        touched:  one of: None, 'added' or 'modified'
+        touched:  one of: None (mean untouched), 'added' or 'modified'
     """
 
     fname = fctx.path()

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 02 of 11] commitctx: stop using weakref proxy for transaction

Pierre-Yves David-2
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1595587952 -7200
#      Fri Jul 24 12:52:32 2020 +0200
# Node ID 76a585b26acdaf884e1c40252e351b1d45cbbcf1
# Parent  2727e91ffa6e9063bd9c29671b5008cfef22dd97
# EXP-Topic commitctx-cleanup-2
# Available At https://foss.heptapod.net/octobus/mercurial-devel/
#              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 76a585b26acd
commitctx: stop using weakref proxy for transaction

This weakref proxy was introduced in 2007 by 30d4d8985dd8.

If I understand it correctly, the logic at that time was relying on the
transaction destructor, triggered at garbage collection time to rollback failed
transaction. passing the object to sub function directly mean it would live in
the function scope and be trapped in the traceback on exception, leading to the
transaction rollback too late in some case.

Modern transaction usage use explicit opening and closing of transaction and no
longer rely on some internal reference counting details. So this weakref proxy
is no longer necessary. Absolutely no test are affected when we drop it.

diff --git a/mercurial/commit.py b/mercurial/commit.py
--- a/mercurial/commit.py
+++ b/mercurial/commit.py
@@ -6,7 +6,6 @@
 from __future__ import absolute_import
 
 import errno
-import weakref
 
 from .i18n import _
 from .node import (
@@ -62,8 +61,6 @@ def commitctx(repo, ctx, error=False, or
         p2copies = ctx.p2copies()
     filesadded, filesremoved = None, None
     with repo.lock(), repo.transaction(b"commit") as tr:
-        trp = weakref.proxy(tr)
-
         if ctx.manifestnode():
             # reuse an existing manifest revision
             repo.ui.debug(b'reusing known manifest\n')
@@ -102,7 +99,7 @@ def commitctx(repo, ctx, error=False, or
                     else:
                         added.append(f)
                         m[f], is_touched = _filecommit(
-                            repo, fctx, m1, m2, linkrev, trp, writefilecopymeta,
+                            repo, fctx, m1, m2, linkrev, tr, writefilecopymeta,
                         )
                         if is_touched:
                             touched.append(f)
@@ -156,7 +153,7 @@ def commitctx(repo, ctx, error=False, or
                 # case where the merge has files outside of the narrowspec,
                 # so this is safe.
                 mn = mctx.write(
-                    trp,
+                    tr,
                     linkrev,
                     p1.manifestnode(),
                     p2.manifestnode(),
@@ -191,7 +188,7 @@ def commitctx(repo, ctx, error=False, or
             mn,
             files,
             ctx.description(),
-            trp,
+            tr,
             p1.node(),
             p2.node(),
             user,

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 03 of 11] commitctx: extract the function that commit a new manifest

Pierre-Yves David-2
In reply to this post by Pierre-Yves David-2
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1595509101 -7200
#      Thu Jul 23 14:58:21 2020 +0200
# Node ID 113fcffb030358b64b4cf7331bbde2e85758ab27
# Parent  76a585b26acdaf884e1c40252e351b1d45cbbcf1
# EXP-Topic commitctx-cleanup-2
# Available At https://foss.heptapod.net/octobus/mercurial-devel/
#              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 113fcffb0303
commitctx: extract the function that commit a new manifest

The logic is large enough and isolated enough to be extracted, this reduce the
size of the main function, making it simpler to follow.

diff --git a/mercurial/commit.py b/mercurial/commit.py
--- a/mercurial/commit.py
+++ b/mercurial/commit.py
@@ -132,41 +132,7 @@ def commitctx(repo, ctx, error=False, or
                 filesremoved = removed
 
             files = touched
-            md = None
-            if not files:
-                # if no "files" actually changed in terms of the changelog,
-                # try hard to detect unmodified manifest entry so that the
-                # exact same commit can be reproduced later on convert.
-                md = m1.diff(m, scmutil.matchfiles(repo, ctx.files()))
-            if not files and md:
-                repo.ui.debug(
-                    b'not reusing manifest (no file change in '
-                    b'changelog, but manifest differs)\n'
-                )
-            if files or md:
-                repo.ui.note(_(b"committing manifest\n"))
-                # we're using narrowmatch here since it's already applied at
-                # other stages (such as dirstate.walk), so we're already
-                # ignoring things outside of narrowspec in most cases. The
-                # one case where we might have files outside the narrowspec
-                # at this point is merges, and we already error out in the
-                # case where the merge has files outside of the narrowspec,
-                # so this is safe.
-                mn = mctx.write(
-                    tr,
-                    linkrev,
-                    p1.manifestnode(),
-                    p2.manifestnode(),
-                    added,
-                    drop,
-                    match=repo.narrowmatch(),
-                )
-            else:
-                repo.ui.debug(
-                    b'reusing manifest from p1 (listed files '
-                    b'actually unchanged)\n'
-                )
-                mn = p1.manifestnode()
+            mn = _commit_manifest(tr, linkrev, ctx, mctx, files, added, drop)
 
         if writecopiesto == b'changeset-only':
             # If writing only to changeset extras, use None to indicate that
@@ -349,3 +315,65 @@ def _filecommit(
     else:
         fnode = fparent1
     return fnode, touched
+
+
+def _commit_manifest(tr, linkrev, ctx, mctx, files, added, drop):
+    """make a new manifest entry (or reuse a new one)
+
+    given an initialised manifest context and precomputed list of
+    - files: files affected by the commit
+    - added: new entries in the manifest
+    - drop:  entries present in parents but absent of this one
+
+    Create a new manifest revision, reuse existing ones if possible.
+
+    Return the nodeid of the manifest revision.
+    """
+    repo = ctx.repo()
+
+    md = None
+
+    # all this is cached, so it is find to get them all from the ctx.
+    p1 = ctx.p1()
+    p2 = ctx.p2()
+    m1ctx = p1.manifestctx()
+
+    m1 = m1ctx.read()
+
+    manifest = mctx.read()
+
+    if not files:
+        # if no "files" actually changed in terms of the changelog,
+        # try hard to detect unmodified manifest entry so that the
+        # exact same commit can be reproduced later on convert.
+        md = m1.diff(manifest, scmutil.matchfiles(repo, ctx.files()))
+    if not files and md:
+        repo.ui.debug(
+            b'not reusing manifest (no file change in '
+            b'changelog, but manifest differs)\n'
+        )
+    if files or md:
+        repo.ui.note(_(b"committing manifest\n"))
+        # we're using narrowmatch here since it's already applied at
+        # other stages (such as dirstate.walk), so we're already
+        # ignoring things outside of narrowspec in most cases. The
+        # one case where we might have files outside the narrowspec
+        # at this point is merges, and we already error out in the
+        # case where the merge has files outside of the narrowspec,
+        # so this is safe.
+        mn = mctx.write(
+            tr,
+            linkrev,
+            p1.manifestnode(),
+            p2.manifestnode(),
+            added,
+            drop,
+            match=repo.narrowmatch(),
+        )
+    else:
+        repo.ui.debug(
+            b'reusing manifest from p1 (listed files ' b'actually unchanged)\n'
+        )
+        mn = p1.manifestnode()
+
+    return mn

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 04 of 11] commitctx: no longer use the `writecopiesto` variable in the function

Pierre-Yves David-2
In reply to this post by Pierre-Yves David-2
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1595531010 -7200
#      Thu Jul 23 21:03:30 2020 +0200
# Node ID 93c606831026b6c9644ad84a89204a2338a8aa03
# Parent  113fcffb030358b64b4cf7331bbde2e85758ab27
# EXP-Topic commitctx-cleanup-2
# Available At https://foss.heptapod.net/octobus/mercurial-devel/
#              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 93c606831026
commitctx: no longer use the `writecopiesto` variable in the function

The `writefilecopymeta` variable already carry the same information, so we can
use `writefilecopymeta` in the one conditional where `writecopiesto` was used.

diff --git a/mercurial/commit.py b/mercurial/commit.py
--- a/mercurial/commit.py
+++ b/mercurial/commit.py
@@ -47,7 +47,6 @@ def commitctx(repo, ctx, error=False, or
     if repo.filecopiesmode == b'changeset-sidedata':
         writechangesetcopy = True
         writefilecopymeta = True
-        writecopiesto = None
     else:
         writecopiesto = repo.ui.config(b'experimental', b'copies.write-to')
         writefilecopymeta = writecopiesto != b'changeset-only'
@@ -134,7 +133,7 @@ def commitctx(repo, ctx, error=False, or
             files = touched
             mn = _commit_manifest(tr, linkrev, ctx, mctx, files, added, drop)
 
-        if writecopiesto == b'changeset-only':
+        if not writefilecopymeta:
             # If writing only to changeset extras, use None to indicate that
             # no entry should be written. If writing to both, write an empty
             # entry to prevent the reader from falling back to reading

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 05 of 11] commitctx: move copy meta config reading in a dedicated function

Pierre-Yves David-2
In reply to this post by Pierre-Yves David-2
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1595531382 -7200
#      Thu Jul 23 21:09:42 2020 +0200
# Node ID 87553bac39d2fec8dd7b23b44152ec89bcb2f74d
# Parent  93c606831026b6c9644ad84a89204a2338a8aa03
# EXP-Topic commitctx-cleanup-2
# Available At https://foss.heptapod.net/octobus/mercurial-devel/
#              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 87553bac39d2
commitctx: move copy meta config reading in a dedicated function

The logic is non trivial, make it contained in a function is clearer. I also
unlock easy re-use of tha logic without having the pass the value around.

diff --git a/mercurial/commit.py b/mercurial/commit.py
--- a/mercurial/commit.py
+++ b/mercurial/commit.py
@@ -24,6 +24,25 @@ from . import (
 )
 
 
+def _write_copy_meta(repo):
+    """return a (changelog, filelog) boolean tuple
+
+    changelog: copy related information should be stored in the changeset
+    filelof:   copy related information should be written in the file revision
+    """
+    if repo.filecopiesmode == b'changeset-sidedata':
+        writechangesetcopy = True
+        writefilecopymeta = True
+    else:
+        writecopiesto = repo.ui.config(b'experimental', b'copies.write-to')
+        writefilecopymeta = writecopiesto != b'changeset-only'
+        writechangesetcopy = writecopiesto in (
+            b'changeset-only',
+            b'compatibility',
+        )
+    return writechangesetcopy, writefilecopymeta
+
+
 def commitctx(repo, ctx, error=False, origctx=None):
     """Add a new revision to the target repository.
     Revision information is passed via the context argument.
@@ -44,16 +63,8 @@ def commitctx(repo, ctx, error=False, or
     p1, p2 = ctx.p1(), ctx.p2()
     user = ctx.user()
 
-    if repo.filecopiesmode == b'changeset-sidedata':
-        writechangesetcopy = True
-        writefilecopymeta = True
-    else:
-        writecopiesto = repo.ui.config(b'experimental', b'copies.write-to')
-        writefilecopymeta = writecopiesto != b'changeset-only'
-        writechangesetcopy = writecopiesto in (
-            b'changeset-only',
-            b'compatibility',
-        )
+    writechangesetcopy, writefilecopymeta = _write_copy_meta(repo)
+
     p1copies, p2copies = None, None
     if writechangesetcopy:
         p1copies = ctx.p1copies()

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 06 of 11] commitctx: move `writechangesetcopy` business at the end a code section

Pierre-Yves David-2
In reply to this post by Pierre-Yves David-2
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1595537909 -7200
#      Thu Jul 23 22:58:29 2020 +0200
# Node ID 4eeff7ebf46eb591767ca79fbceb447b2d3ca17d
# Parent  87553bac39d2fec8dd7b23b44152ec89bcb2f74d
# EXP-Topic commitctx-cleanup-2
# Available At https://foss.heptapod.net/octobus/mercurial-devel/
#              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 4eeff7ebf46e
commitctx: move `writechangesetcopy` business at the end a code section

This code is to handle a specific subcase so we move it a the end. This allow to gather the rest of the "core" code closer to the related logic.

diff --git a/mercurial/commit.py b/mercurial/commit.py
--- a/mercurial/commit.py
+++ b/mercurial/commit.py
@@ -138,12 +138,12 @@ def commitctx(repo, ctx, error=False, or
 
             touched.extend(removed)
 
+            files = touched
+            mn = _commit_manifest(tr, linkrev, ctx, mctx, files, added, drop)
+
             if writechangesetcopy:
                 filesremoved = removed
 
-            files = touched
-            mn = _commit_manifest(tr, linkrev, ctx, mctx, files, added, drop)
-
         if not writefilecopymeta:
             # If writing only to changeset extras, use None to indicate that
             # no entry should be written. If writing to both, write an empty

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 07 of 11] commitctx: treat `filesadded` more like `filesremoved`

Pierre-Yves David-2
In reply to this post by Pierre-Yves David-2
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1595538480 -7200
#      Thu Jul 23 23:08:00 2020 +0200
# Node ID aa121e67dfaa9d793dcd375c16f91d21060f22fb
# Parent  4eeff7ebf46eb591767ca79fbceb447b2d3ca17d
# EXP-Topic commitctx-cleanup-2
# Available At https://foss.heptapod.net/octobus/mercurial-devel/
#              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r aa121e67dfaa
commitctx: treat `filesadded` more like `filesremoved`

Accumulating the filename in a list will have a negligible cost and deal with
the list of added files like the other ones will make is code cleaning simpler.
The two variable with very close name is not great, but my plan is to split most
of the code in a separated function which will make the "problem" go away by
itself.

diff --git a/mercurial/commit.py b/mercurial/commit.py
--- a/mercurial/commit.py
+++ b/mercurial/commit.py
@@ -94,7 +94,7 @@ def commitctx(repo, ctx, error=False, or
 
             # check in files
             added = []
-            filesadded = []
+            files_added = []
             removed = list(ctx.removed())
             touched = []
             linkrev = len(repo)
@@ -113,8 +113,8 @@ def commitctx(repo, ctx, error=False, or
                         )
                         if is_touched:
                             touched.append(f)
-                            if writechangesetcopy and is_touched == 'added':
-                                filesadded.append(f)
+                            if is_touched == 'added':
+                                files_added.append(f)
                         m.setflag(f, fctx.flags())
                 except OSError:
                     repo.ui.warn(_(b"trouble committing %s!\n") % uipathfn(f))
@@ -143,6 +143,7 @@ def commitctx(repo, ctx, error=False, or
 
             if writechangesetcopy:
                 filesremoved = removed
+                filesadded = files_added
 
         if not writefilecopymeta:
             # If writing only to changeset extras, use None to indicate that

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 08 of 11] commitctx: extract all the manual logic to process the files

Pierre-Yves David-2
In reply to this post by Pierre-Yves David-2
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1595521545 -7200
#      Thu Jul 23 18:25:45 2020 +0200
# Node ID 59f2136c091bc68c96fb56e7618128b77b62194e
# Parent  aa121e67dfaa9d793dcd375c16f91d21060f22fb
# EXP-Topic commitctx-cleanup-2
# Available At https://foss.heptapod.net/octobus/mercurial-devel/
#              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 59f2136c091b
commitctx: extract all the manual logic to  process the files

that branch of the if is significantly more complicated than the other two.
Moving it to its own function make it simple to keep the scope limited and to
read to the higher level function.

diff --git a/mercurial/commit.py b/mercurial/commit.py
--- a/mercurial/commit.py
+++ b/mercurial/commit.py
@@ -84,66 +84,10 @@ def commitctx(repo, ctx, error=False, or
             mn = p1.manifestnode()
             files = []
         else:
-            m1ctx = p1.manifestctx()
-            m2ctx = p2.manifestctx()
-            mctx = m1ctx.copy()
-
-            m = mctx.read()
-            m1 = m1ctx.read()
-            m2 = m2ctx.read()
-
-            # check in files
-            added = []
-            files_added = []
-            removed = list(ctx.removed())
-            touched = []
-            linkrev = len(repo)
-            repo.ui.note(_(b"committing files:\n"))
-            uipathfn = scmutil.getuipathfn(repo)
-            for f in sorted(ctx.modified() + ctx.added()):
-                repo.ui.note(uipathfn(f) + b"\n")
-                try:
-                    fctx = ctx[f]
-                    if fctx is None:
-                        removed.append(f)
-                    else:
-                        added.append(f)
-                        m[f], is_touched = _filecommit(
-                            repo, fctx, m1, m2, linkrev, tr, writefilecopymeta,
-                        )
-                        if is_touched:
-                            touched.append(f)
-                            if is_touched == 'added':
-                                files_added.append(f)
-                        m.setflag(f, fctx.flags())
-                except OSError:
-                    repo.ui.warn(_(b"trouble committing %s!\n") % uipathfn(f))
-                    raise
-                except IOError as inst:
-                    errcode = getattr(inst, 'errno', errno.ENOENT)
-                    if error or errcode and errcode != errno.ENOENT:
-                        repo.ui.warn(
-                            _(b"trouble committing %s!\n") % uipathfn(f)
-                        )
-                    raise
-
-            # update manifest
-            removed = [f for f in removed if f in m1 or f in m2]
-            drop = sorted([f for f in removed if f in m])
-            for f in drop:
-                del m[f]
-            if p2.rev() != nullrev:
-                rf = metadata.get_removal_filter(ctx, (p1, p2, m1, m2))
-                removed = [f for f in removed if not rf(f)]
-
-            touched.extend(removed)
-
-            files = touched
-            mn = _commit_manifest(tr, linkrev, ctx, mctx, files, added, drop)
-
+            mn, files, added, removed = _process_files(tr, ctx, error=error)
             if writechangesetcopy:
                 filesremoved = removed
-                filesadded = files_added
+                filesadded = added
 
         if not writefilecopymeta:
             # If writing only to changeset extras, use None to indicate that
@@ -192,6 +136,71 @@ def commitctx(repo, ctx, error=False, or
         return n
 
 
+def _process_files(tr, ctx, error=False):
+    repo = ctx.repo()
+    p1 = ctx.p1()
+    p2 = ctx.p2()
+
+    writechangesetcopy, writefilecopymeta = _write_copy_meta(repo)
+
+    m1ctx = p1.manifestctx()
+    m2ctx = p2.manifestctx()
+    mctx = m1ctx.copy()
+
+    m = mctx.read()
+    m1 = m1ctx.read()
+    m2 = m2ctx.read()
+
+    # check in files
+    added = []
+    filesadded = []
+    removed = list(ctx.removed())
+    touched = []
+    linkrev = len(repo)
+    repo.ui.note(_(b"committing files:\n"))
+    uipathfn = scmutil.getuipathfn(repo)
+    for f in sorted(ctx.modified() + ctx.added()):
+        repo.ui.note(uipathfn(f) + b"\n")
+        try:
+            fctx = ctx[f]
+            if fctx is None:
+                removed.append(f)
+            else:
+                added.append(f)
+                m[f], is_touched = _filecommit(
+                    repo, fctx, m1, m2, linkrev, tr, writefilecopymeta,
+                )
+                if is_touched:
+                    touched.append(f)
+                    if is_touched == 'added':
+                        filesadded.append(f)
+                m.setflag(f, fctx.flags())
+        except OSError:
+            repo.ui.warn(_(b"trouble committing %s!\n") % uipathfn(f))
+            raise
+        except IOError as inst:
+            errcode = getattr(inst, 'errno', errno.ENOENT)
+            if error or errcode and errcode != errno.ENOENT:
+                repo.ui.warn(_(b"trouble committing %s!\n") % uipathfn(f))
+            raise
+
+    # update manifest
+    removed = [f for f in removed if f in m1 or f in m2]
+    drop = sorted([f for f in removed if f in m])
+    for f in drop:
+        del m[f]
+    if p2.rev() != nullrev:
+        rf = metadata.get_removal_filter(ctx, (p1, p2, m1, m2))
+        removed = [f for f in removed if not rf(f)]
+
+    touched.extend(removed)
+
+    files = touched
+    mn = _commit_manifest(tr, linkrev, ctx, mctx, files, added, drop)
+
+    return mn, files, filesadded, removed
+
+
 def _filecommit(
     repo, fctx, manifest1, manifest2, linkrev, tr, includecopymeta,
 ):

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 09 of 11] commitctx: move a special case about files earlier

Pierre-Yves David-2
In reply to this post by Pierre-Yves David-2
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1595540459 -7200
#      Thu Jul 23 23:40:59 2020 +0200
# Node ID 8bd4fa80f2550e94ab3592ae6543649bf8af5449
# Parent  59f2136c091bc68c96fb56e7618128b77b62194e
# EXP-Topic commitctx-cleanup-2
# Available At https://foss.heptapod.net/octobus/mercurial-devel/
#              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 8bd4fa80f255
commitctx: move a special case about files earlier

Same logic as a changeset a bit earlier, the `writefilecopymeta` section is more
a post processing details so we move the conditional about `files` value closer
to the rest of the code computing `files` value.

diff --git a/mercurial/commit.py b/mercurial/commit.py
--- a/mercurial/commit.py
+++ b/mercurial/commit.py
@@ -89,6 +89,9 @@ def commitctx(repo, ctx, error=False, or
                 filesremoved = removed
                 filesadded = added
 
+        if origctx and origctx.manifestnode() == mn:
+            files = origctx.files()
+
         if not writefilecopymeta:
             # If writing only to changeset extras, use None to indicate that
             # no entry should be written. If writing to both, write an empty
@@ -99,9 +102,6 @@ def commitctx(repo, ctx, error=False, or
             filesadded = filesadded or None
             filesremoved = filesremoved or None
 
-        if origctx and origctx.manifestnode() == mn:
-            files = origctx.files()
-
         # update changelog
         repo.ui.note(_(b"committing changelog\n"))
         repo.changelog.delayupdate(tr)

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 10 of 11] commitctx: gather more preparation code within the lock context

Pierre-Yves David-2
In reply to this post by Pierre-Yves David-2
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1595541503 -7200
#      Thu Jul 23 23:58:23 2020 +0200
# Node ID 8c926a9ea89ea18ba068fd59314b73d7f6f7db27
# Parent  8bd4fa80f2550e94ab3592ae6543649bf8af5449
# EXP-Topic commitctx-cleanup-2
# Available At https://foss.heptapod.net/octobus/mercurial-devel/
#              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 8c926a9ea89e
commitctx: gather more preparation code within the lock context

This is a small change that exist mostly for clarification. I am about to move a
large amount of code in its own function. having all that code next to each
other will make the next changeset clearer.

diff --git a/mercurial/commit.py b/mercurial/commit.py
--- a/mercurial/commit.py
+++ b/mercurial/commit.py
@@ -63,14 +63,14 @@ def commitctx(repo, ctx, error=False, or
     p1, p2 = ctx.p1(), ctx.p2()
     user = ctx.user()
 
-    writechangesetcopy, writefilecopymeta = _write_copy_meta(repo)
+    with repo.lock(), repo.transaction(b"commit") as tr:
+        writechangesetcopy, writefilecopymeta = _write_copy_meta(repo)
 
-    p1copies, p2copies = None, None
-    if writechangesetcopy:
-        p1copies = ctx.p1copies()
-        p2copies = ctx.p2copies()
-    filesadded, filesremoved = None, None
-    with repo.lock(), repo.transaction(b"commit") as tr:
+        p1copies, p2copies = None, None
+        if writechangesetcopy:
+            p1copies = ctx.p1copies()
+            p2copies = ctx.p2copies()
+        filesadded, filesremoved = None, None
         if ctx.manifestnode():
             # reuse an existing manifest revision
             repo.ui.debug(b'reusing known manifest\n')

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 11 of 11] commitctx: extract all the file preparation logic in a new function

Pierre-Yves David-2
In reply to this post by Pierre-Yves David-2
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1595541151 -7200
#      Thu Jul 23 23:52:31 2020 +0200
# Node ID 68263067cc8120b2f6434ed282147966b67e7214
# Parent  8c926a9ea89ea18ba068fd59314b73d7f6f7db27
# EXP-Topic commitctx-cleanup-2
# Available At https://foss.heptapod.net/octobus/mercurial-devel/
#              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 68263067cc81
commitctx: extract all the file preparation logic in a new function

Before we actually start to create a new commit we have a large block of logic
that do the necessary file and manifest commit and that determine which files
are been affected by the commit (and how).

This is a complex process on its own. It return a "simple" output that can be
fed to the next step. The output itself is not that simple as we return a lot of
individual items (files, added, removed, ...). My next step (and actual goal for
this cleanup) will be to simplify the return by returning a richer object that
will be more suited for the variation of data we want to store.

After this changeset the `commitctx` is a collection of smaller function with
limited scope. The largest one is still `_filecommit` without about 100 lines of
code.

diff --git a/mercurial/commit.py b/mercurial/commit.py
--- a/mercurial/commit.py
+++ b/mercurial/commit.py
@@ -64,43 +64,8 @@ def commitctx(repo, ctx, error=False, or
     user = ctx.user()
 
     with repo.lock(), repo.transaction(b"commit") as tr:
-        writechangesetcopy, writefilecopymeta = _write_copy_meta(repo)
-
-        p1copies, p2copies = None, None
-        if writechangesetcopy:
-            p1copies = ctx.p1copies()
-            p2copies = ctx.p2copies()
-        filesadded, filesremoved = None, None
-        if ctx.manifestnode():
-            # reuse an existing manifest revision
-            repo.ui.debug(b'reusing known manifest\n')
-            mn = ctx.manifestnode()
-            files = ctx.files()
-            if writechangesetcopy:
-                filesadded = ctx.filesadded()
-                filesremoved = ctx.filesremoved()
-        elif not ctx.files():
-            repo.ui.debug(b'reusing manifest from p1 (no file change)\n')
-            mn = p1.manifestnode()
-            files = []
-        else:
-            mn, files, added, removed = _process_files(tr, ctx, error=error)
-            if writechangesetcopy:
-                filesremoved = removed
-                filesadded = added
-
-        if origctx and origctx.manifestnode() == mn:
-            files = origctx.files()
-
-        if not writefilecopymeta:
-            # If writing only to changeset extras, use None to indicate that
-            # no entry should be written. If writing to both, write an empty
-            # entry to prevent the reader from falling back to reading
-            # filelogs.
-            p1copies = p1copies or None
-            p2copies = p2copies or None
-            filesadded = filesadded or None
-            filesremoved = filesremoved or None
+        r = _prepare_files(tr, ctx, error=error, origctx=origctx)
+        mn, files, p1copies, p2copies, filesadded, filesremoved = r
 
         # update changelog
         repo.ui.note(_(b"committing changelog\n"))
@@ -136,6 +101,51 @@ def commitctx(repo, ctx, error=False, or
         return n
 
 
+def _prepare_files(tr, ctx, error=False, origctx=None):
+    repo = ctx.repo()
+    p1 = ctx.p1()
+
+    writechangesetcopy, writefilecopymeta = _write_copy_meta(repo)
+
+    p1copies, p2copies = None, None
+    if writechangesetcopy:
+        p1copies = ctx.p1copies()
+        p2copies = ctx.p2copies()
+    filesadded, filesremoved = None, None
+    if ctx.manifestnode():
+        # reuse an existing manifest revision
+        repo.ui.debug(b'reusing known manifest\n')
+        mn = ctx.manifestnode()
+        files = ctx.files()
+        if writechangesetcopy:
+            filesadded = ctx.filesadded()
+            filesremoved = ctx.filesremoved()
+    elif not ctx.files():
+        repo.ui.debug(b'reusing manifest from p1 (no file change)\n')
+        mn = p1.manifestnode()
+        files = []
+    else:
+        mn, files, added, removed = _process_files(tr, ctx, error=error)
+        if writechangesetcopy:
+            filesremoved = removed
+            filesadded = added
+
+    if origctx and origctx.manifestnode() == mn:
+        files = origctx.files()
+
+    if not writefilecopymeta:
+        # If writing only to changeset extras, use None to indicate that
+        # no entry should be written. If writing to both, write an empty
+        # entry to prevent the reader from falling back to reading
+        # filelogs.
+        p1copies = p1copies or None
+        p2copies = p2copies or None
+        filesadded = filesadded or None
+        filesremoved = filesremoved or None
+
+    return mn, files, p1copies, p2copies, filesadded, filesremoved
+
+
 def _process_files(tr, ctx, error=False):
     repo = ctx.repo()
     p1 = ctx.p1()

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 01 of 11] commitctx: document the None return for "touched" value

Yuya Nishihara
In reply to this post by Pierre-Yves David-2
On Fri, 24 Jul 2020 16:38:26 +0200, Pierre-Yves David wrote:

> # HG changeset patch
> # User Pierre-Yves David <[hidden email]>
> # Date 1595427033 -7200
> #      Wed Jul 22 16:10:33 2020 +0200
> # Node ID 2727e91ffa6e9063bd9c29671b5008cfef22dd97
> # Parent  4ccd5ec565c2baaa1d598b20a7ea14d3c4fd39dc
> # EXP-Topic commitctx-cleanup-2
> # Available At https://foss.heptapod.net/octobus/mercurial-devel/
> #              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 2727e91ffa6e
> commitctx: document the None return for "touched" value

Queued, thanks.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 03 of 11] commitctx: extract the function that commit a new manifest

Yuya Nishihara
In reply to this post by Pierre-Yves David-2
On Fri, 24 Jul 2020 16:38:28 +0200, Pierre-Yves David wrote:

> # HG changeset patch
> # User Pierre-Yves David <[hidden email]>
> # Date 1595509101 -7200
> #      Thu Jul 23 14:58:21 2020 +0200
> # Node ID 113fcffb030358b64b4cf7331bbde2e85758ab27
> # Parent  76a585b26acdaf884e1c40252e351b1d45cbbcf1
> # EXP-Topic commitctx-cleanup-2
> # Available At https://foss.heptapod.net/octobus/mercurial-devel/
> #              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 113fcffb0303
> commitctx: extract the function that commit a new manifest

> +def _commit_manifest(tr, linkrev, ctx, mctx, files, added, drop):
> +    """make a new manifest entry (or reuse a new one)
> +
> +    given an initialised manifest context and precomputed list of
> +    - files: files affected by the commit
> +    - added: new entries in the manifest
> +    - drop:  entries present in parents but absent of this one
> +
> +    Create a new manifest revision, reuse existing ones if possible.
> +
> +    Return the nodeid of the manifest revision.
> +    """
> +    repo = ctx.repo()
> +
> +    md = None
> +
> +    # all this is cached, so it is find to get them all from the ctx.
> +    p1 = ctx.p1()
> +    p2 = ctx.p2()
> +    m1ctx = p1.manifestctx()
> +
> +    m1 = m1ctx.read()
> +
> +    manifest = mctx.read()

Reading manifest which could be previously mutated looks a bit weird unless
you know mctx.read() returns a cached object.

> +    if not files:
> +        # if no "files" actually changed in terms of the changelog,
> +        # try hard to detect unmodified manifest entry so that the
> +        # exact same commit can be reproduced later on convert.
> +        md = m1.diff(manifest, scmutil.matchfiles(repo, ctx.files()))
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 02 of 11] commitctx: stop using weakref proxy for transaction

Gregory Szorc
In reply to this post by Pierre-Yves David-2
On Fri, Jul 24, 2020 at 8:49 AM Pierre-Yves David <[hidden email]> wrote:
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1595587952 -7200
#      Fri Jul 24 12:52:32 2020 +0200
# Node ID 76a585b26acdaf884e1c40252e351b1d45cbbcf1
# Parent  2727e91ffa6e9063bd9c29671b5008cfef22dd97
# EXP-Topic commitctx-cleanup-2
# Available At https://foss.heptapod.net/octobus/mercurial-devel/
#              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 76a585b26acd
commitctx: stop using weakref proxy for transaction

I have concerns about this patch.

I believe the reason we continue to use a weak ref here is due to cycles leading to leaked objects due to failure to garbage collect.

I believe there are still places where we leak transaction objects because of cycles. Try doing a `hg convert` between 2 Mercurial repos involving thousands of changesets. I suspect this change may make the memory leak worse since the garbage collector isn't able to break cycles as easily.
 

This weakref proxy was introduced in 2007 by 30d4d8985dd8.

If I understand it correctly, the logic at that time was relying on the
transaction destructor, triggered at garbage collection time to rollback failed
transaction. passing the object to sub function directly mean it would live in
the function scope and be trapped in the traceback on exception, leading to the
transaction rollback too late in some case.

Modern transaction usage use explicit opening and closing of transaction and no
longer rely on some internal reference counting details. So this weakref proxy
is no longer necessary. Absolutely no test are affected when we drop it.

diff --git a/mercurial/commit.py b/mercurial/commit.py
--- a/mercurial/commit.py
+++ b/mercurial/commit.py
@@ -6,7 +6,6 @@
 from __future__ import absolute_import

 import errno
-import weakref

 from .i18n import _
 from .node import (
@@ -62,8 +61,6 @@ def commitctx(repo, ctx, error=False, or
         p2copies = ctx.p2copies()
     filesadded, filesremoved = None, None
     with repo.lock(), repo.transaction(b"commit") as tr:
-        trp = weakref.proxy(tr)
-
         if ctx.manifestnode():
             # reuse an existing manifest revision
             repo.ui.debug(b'reusing known manifest\n')
@@ -102,7 +99,7 @@ def commitctx(repo, ctx, error=False, or
                     else:
                         added.append(f)
                         m[f], is_touched = _filecommit(
-                            repo, fctx, m1, m2, linkrev, trp, writefilecopymeta,
+                            repo, fctx, m1, m2, linkrev, tr, writefilecopymeta,
                         )
                         if is_touched:
                             touched.append(f)
@@ -156,7 +153,7 @@ def commitctx(repo, ctx, error=False, or
                 # case where the merge has files outside of the narrowspec,
                 # so this is safe.
                 mn = mctx.write(
-                    trp,
+                    tr,
                     linkrev,
                     p1.manifestnode(),
                     p2.manifestnode(),
@@ -191,7 +188,7 @@ def commitctx(repo, ctx, error=False, or
             mn,
             files,
             ctx.description(),
-            trp,
+            tr,
             p1.node(),
             p2.node(),
             user,

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 02 of 11] commitctx: stop using weakref proxy for transaction

Manuel Jacob
On 2020-07-27 00:18, Gregory Szorc wrote:

> On Fri, Jul 24, 2020 at 8:49 AM Pierre-Yves David <
> [hidden email]> wrote:
>
>> # HG changeset patch
>> # User Pierre-Yves David <[hidden email]>
>> # Date 1595587952 -7200
>> #      Fri Jul 24 12:52:32 2020 +0200
>> # Node ID 76a585b26acdaf884e1c40252e351b1d45cbbcf1
>> # Parent  2727e91ffa6e9063bd9c29671b5008cfef22dd97
>> # EXP-Topic commitctx-cleanup-2
>> # Available At https://foss.heptapod.net/octobus/mercurial-devel/
>> #              hg pull
>> https://foss.heptapod.net/octobus/mercurial-devel/
>> -r 76a585b26acd
>> commitctx: stop using weakref proxy for transaction
>>
>
> I have concerns about this patch.
>
> I believe the reason we continue to use a weak ref here is due to
> cycles
> leading to leaked objects due to failure to garbage collect.
>
> I believe there are still places where we leak transaction objects
> because
> of cycles. Try doing a `hg convert` between 2 Mercurial repos involving
> thousands of changesets. I suspect this change may make the memory leak
> worse since the garbage collector isn't able to break cycles as easily.

What exactly do you mean by "memory leak"? That objects are never freed
(before the command terminates) or freed too late, increasing temporary
memory usage more than necessary?

>
>>
>> This weakref proxy was introduced in 2007 by 30d4d8985dd8.
>>
>> If I understand it correctly, the logic at that time was relying on
>> the
>> transaction destructor, triggered at garbage collection time to
>> rollback
>> failed
>> transaction. passing the object to sub function directly mean it would
>> live in
>> the function scope and be trapped in the traceback on exception,
>> leading
>> to the
>> transaction rollback too late in some case.
>>
>> Modern transaction usage use explicit opening and closing of
>> transaction
>> and no
>> longer rely on some internal reference counting details. So this
>> weakref
>> proxy
>> is no longer necessary. Absolutely no test are affected when we drop
>> it.
>>
>> diff --git a/mercurial/commit.py b/mercurial/commit.py
>> --- a/mercurial/commit.py
>> +++ b/mercurial/commit.py
>> @@ -6,7 +6,6 @@
>>  from __future__ import absolute_import
>>
>>  import errno
>> -import weakref
>>
>>  from .i18n import _
>>  from .node import (
>> @@ -62,8 +61,6 @@ def commitctx(repo, ctx, error=False, or
>>          p2copies = ctx.p2copies()
>>      filesadded, filesremoved = None, None
>>      with repo.lock(), repo.transaction(b"commit") as tr:
>> -        trp = weakref.proxy(tr)
>> -
>>          if ctx.manifestnode():
>>              # reuse an existing manifest revision
>>              repo.ui.debug(b'reusing known manifest\n')
>> @@ -102,7 +99,7 @@ def commitctx(repo, ctx, error=False, or
>>                      else:
>>                          added.append(f)
>>                          m[f], is_touched = _filecommit(
>> -                            repo, fctx, m1, m2, linkrev, trp,
>> writefilecopymeta,
>> +                            repo, fctx, m1, m2, linkrev, tr,
>> writefilecopymeta,
>>                          )
>>                          if is_touched:
>>                              touched.append(f)
>> @@ -156,7 +153,7 @@ def commitctx(repo, ctx, error=False, or
>>                  # case where the merge has files outside of the
>> narrowspec,
>>                  # so this is safe.
>>                  mn = mctx.write(
>> -                    trp,
>> +                    tr,
>>                      linkrev,
>>                      p1.manifestnode(),
>>                      p2.manifestnode(),
>> @@ -191,7 +188,7 @@ def commitctx(repo, ctx, error=False, or
>>              mn,
>>              files,
>>              ctx.description(),
>> -            trp,
>> +            tr,
>>              p1.node(),
>>              p2.node(),
>>              user,
>>
>> _______________________________________________
>> Mercurial-devel mailing list
>> [hidden email]
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
>
> _______________________________________________
> Mercurial-devel mailing list
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 02 of 11] commitctx: stop using weakref proxy for transaction

Yuya Nishihara
In reply to this post by Gregory Szorc
On Sun, 26 Jul 2020 16:18:43 -0600, Gregory Szorc wrote:

> On Fri, Jul 24, 2020 at 8:49 AM Pierre-Yves David <
> [hidden email]> wrote:
>
> > # HG changeset patch
> > # User Pierre-Yves David <[hidden email]>
> > # Date 1595587952 -7200
> > #      Fri Jul 24 12:52:32 2020 +0200
> > # Node ID 76a585b26acdaf884e1c40252e351b1d45cbbcf1
> > # Parent  2727e91ffa6e9063bd9c29671b5008cfef22dd97
> > # EXP-Topic commitctx-cleanup-2
> > # Available At https://foss.heptapod.net/octobus/mercurial-devel/
> > #              hg pull https://foss.heptapod.net/octobus/mercurial-devel/
> > -r 76a585b26acd
> > commitctx: stop using weakref proxy for transaction
> >
>
> I have concerns about this patch.
>
> I believe the reason we continue to use a weak ref here is due to cycles
> leading to leaked objects due to failure to garbage collect.

AFAIK, trp is passed around as function arguments which wouldn't be captured
by lambda, so I don't see any cycles in this specific weakref usage.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 02 of 11] commitctx: stop using weakref proxy for transaction

Augie Fackler-2
In reply to this post by Manuel Jacob
On Mon, Jul 27, 2020 at 12:47:04AM +0200, Manuel Jacob wrote:

> On 2020-07-27 00:18, Gregory Szorc wrote:
> > On Fri, Jul 24, 2020 at 8:49 AM Pierre-Yves David <
> > [hidden email]> wrote:
> >
> > > # HG changeset patch
> > > # User Pierre-Yves David <[hidden email]>
> > > # Date 1595587952 -7200
> > > #      Fri Jul 24 12:52:32 2020 +0200
> > > # Node ID 76a585b26acdaf884e1c40252e351b1d45cbbcf1
> > > # Parent  2727e91ffa6e9063bd9c29671b5008cfef22dd97
> > > # EXP-Topic commitctx-cleanup-2
> > > # Available At https://foss.heptapod.net/octobus/mercurial-devel/
> > > #              hg pull
> > > https://foss.heptapod.net/octobus/mercurial-devel/
> > > -r 76a585b26acd
> > > commitctx: stop using weakref proxy for transaction
> > >
> >
> > I have concerns about this patch.
> >
> > I believe the reason we continue to use a weak ref here is due to cycles
> > leading to leaked objects due to failure to garbage collect.
> >
> > I believe there are still places where we leak transaction objects
> > because
> > of cycles. Try doing a `hg convert` between 2 Mercurial repos involving
> > thousands of changesets. I suspect this change may make the memory leak
> > worse since the garbage collector isn't able to break cycles as easily.
>
> What exactly do you mean by "memory leak"? That objects are never freed
> (before the command terminates) or freed too late, increasing temporary
> memory usage more than necessary?
>

My recollection from the last time we had this discussion was that we
had actual /leaks/ where in the cycles were rendered uncollectable
until process exit (because some of the types in play have __del__
methods).
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 02 of 11] commitctx: stop using weakref proxy for transaction

Pierre-Yves David-2
In reply to this post by Gregory Szorc


On 7/27/20 12:18 AM, Gregory Szorc wrote:

> On Fri, Jul 24, 2020 at 8:49 AM Pierre-Yves David
> <[hidden email] <mailto:[hidden email]>>
> wrote:
>
>     # HG changeset patch
>     # User Pierre-Yves David <[hidden email]
>     <mailto:[hidden email]>>
>     # Date 1595587952 -7200
>     #      Fri Jul 24 12:52:32 2020 +0200
>     # Node ID 76a585b26acdaf884e1c40252e351b1d45cbbcf1
>     # Parent  2727e91ffa6e9063bd9c29671b5008cfef22dd97
>     # EXP-Topic commitctx-cleanup-2
>     # Available At https://foss.heptapod.net/octobus/mercurial-devel/
>     #              hg pull
>     https://foss.heptapod.net/octobus/mercurial-devel/ -r 76a585b26acd
>     commitctx: stop using weakref proxy for transaction
>
>
> I have concerns about this patch.
>
> I believe the reason we continue to use a weak ref here is due to cycles
> leading to leaked objects due to failure to garbage collect.

We need weakref usage at higher level to avoid cycle. However we are
quite low level here and I could not found any justification for the
weakref other than the one listed in the commit I pointed.

Do you have some concrete piece of code under this one that concerns you
especially?

> I believe there are still places where we leak transaction objects
> because of cycles. Try doing a `hg convert` between 2 Mercurial repos
> involving thousands of changesets. I suspect this change may make the
> memory leak worse since the garbage collector isn't able to break cycles
> as easily.

I agree we still have memory leak in some place, however I don't think
they would originate from this code.

--
Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 03 of 11] commitctx: extract the function that commit a new manifest

Pierre-Yves David-2
In reply to this post by Yuya Nishihara


On 7/26/20 6:09 AM, Yuya Nishihara wrote:

> On Fri, 24 Jul 2020 16:38:28 +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <[hidden email]>
>> # Date 1595509101 -7200
>> #      Thu Jul 23 14:58:21 2020 +0200
>> # Node ID 113fcffb030358b64b4cf7331bbde2e85758ab27
>> # Parent  76a585b26acdaf884e1c40252e351b1d45cbbcf1
>> # EXP-Topic commitctx-cleanup-2
>> # Available At https://foss.heptapod.net/octobus/mercurial-devel/
>> #              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 113fcffb0303
>> commitctx: extract the function that commit a new manifest
>
>> +def _commit_manifest(tr, linkrev, ctx, mctx, files, added, drop):
>> +    """make a new manifest entry (or reuse a new one)
>> +
>> +    given an initialised manifest context and precomputed list of
>> +    - files: files affected by the commit
>> +    - added: new entries in the manifest
>> +    - drop:  entries present in parents but absent of this one
>> +
>> +    Create a new manifest revision, reuse existing ones if possible.
>> +
>> +    Return the nodeid of the manifest revision.
>> +    """
>> +    repo = ctx.repo()
>> +
>> +    md = None
>> +
>> +    # all this is cached, so it is find to get them all from the ctx.
>> +    p1 = ctx.p1()
>> +    p2 = ctx.p2()
>> +    m1ctx = p1.manifestctx()
>> +
>> +    m1 = m1ctx.read()
>> +
>> +    manifest = mctx.read()
>
> Reading manifest which could be previously mutated looks a bit weird unless
> you know mctx.read() returns a cached object.

I agree, here I am trying to balance between clarity from reducing the
number of function argument (by taking advantage of other argument to
retrieve the value) and clarity from passing important stuff explicitly.

Do you think we should:

1) leave the code as is.
2) leave the code as is with clarification comment.
3) passe the manifest explicitly
4) something else.

>
>> +    if not files:
>> +        # if no "files" actually changed in terms of the changelog,
>> +        # try hard to detect unmodified manifest entry so that the
>> +        # exact same commit can be reproduced later on convert.
>> +        md = m1.diff(manifest, scmutil.matchfiles(repo, ctx.files()))

--
Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 03 of 11] commitctx: extract the function that commit a new manifest

Yuya Nishihara
On Tue, 28 Jul 2020 21:24:28 +0200, Pierre-Yves David wrote:

> On 7/26/20 6:09 AM, Yuya Nishihara wrote:
> > On Fri, 24 Jul 2020 16:38:28 +0200, Pierre-Yves David wrote:
> >> # HG changeset patch
> >> # User Pierre-Yves David <[hidden email]>
> >> # Date 1595509101 -7200
> >> #      Thu Jul 23 14:58:21 2020 +0200
> >> # Node ID 113fcffb030358b64b4cf7331bbde2e85758ab27
> >> # Parent  76a585b26acdaf884e1c40252e351b1d45cbbcf1
> >> # EXP-Topic commitctx-cleanup-2
> >> # Available At https://foss.heptapod.net/octobus/mercurial-devel/
> >> #              hg pull https://foss.heptapod.net/octobus/mercurial-devel/ -r 113fcffb0303
> >> commitctx: extract the function that commit a new manifest
> >
> >> +def _commit_manifest(tr, linkrev, ctx, mctx, files, added, drop):
> >> +    """make a new manifest entry (or reuse a new one)
> >> +
> >> +    given an initialised manifest context and precomputed list of
> >> +    - files: files affected by the commit
> >> +    - added: new entries in the manifest
> >> +    - drop:  entries present in parents but absent of this one
> >> +
> >> +    Create a new manifest revision, reuse existing ones if possible.
> >> +
> >> +    Return the nodeid of the manifest revision.
> >> +    """
> >> +    repo = ctx.repo()
> >> +
> >> +    md = None
> >> +
> >> +    # all this is cached, so it is find to get them all from the ctx.
> >> +    p1 = ctx.p1()
> >> +    p2 = ctx.p2()
> >> +    m1ctx = p1.manifestctx()
> >> +
> >> +    m1 = m1ctx.read()
> >> +
> >> +    manifest = mctx.read()
> >
> > Reading manifest which could be previously mutated looks a bit weird unless
> > you know mctx.read() returns a cached object.
>
> I agree, here I am trying to balance between clarity from reducing the
> number of function argument (by taking advantage of other argument to
> retrieve the value) and clarity from passing important stuff explicitly.
>
> Do you think we should:
>
> 1) leave the code as is.
> 2) leave the code as is with clarification comment.
> 3) passe the manifest explicitly
> 4) something else.

Maybe _commit_manifest() can be inlined into _process_files()?
_commit_manifest() isn't short, but that's mainly because of comments and
debug messages. The logic looks pretty simple.

I think (3) is also fine.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
12