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

login
register
mail settings
Submitter Pierre-Yves David
Date July 24, 2020, 2:38 p.m.
Message ID <113fcffb030358b64b4c.1595601508@nodosa.octobus.net>
Download mbox | patch
Permalink /patch/46868/
State Accepted
Headers show

Comments

Pierre-Yves David - July 24, 2020, 2:38 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# 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.
Yuya Nishihara - July 26, 2020, 4:09 a.m.
On Fri, 24 Jul 2020 16:38:28 +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # 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()))
Pierre-Yves David - July 28, 2020, 7:24 p.m.
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 <pierre-yves.david@octobus.net>
>> # 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()))
Yuya Nishihara - July 29, 2020, 10:48 a.m.
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 <pierre-yves.david@octobus.net>
> >> # 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.
Pierre-Yves David - July 29, 2020, 11:48 a.m.
On 7/29/20 12:48 PM, Yuya Nishihara wrote:
> 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 <pierre-yves.david@octobus.net>
>>>> # 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.

The main point of this series is to isolated independant piece of logic 
to avoid logic creep. That would be a step in the wrong direction.

> I think (3) is also fine.

Thanks, I'll send a V2 soon.
Yuya Nishihara - July 29, 2020, 12:02 p.m.
On Wed, 29 Jul 2020 13:48:17 +0200, Pierre-Yves David wrote:
> >>> 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.
> 
> The main point of this series is to isolated independant piece of logic 
> to avoid logic creep. That would be a step in the wrong direction.

Yep, but the most complex manifest logic exists in _process_files().
_commit_manifest() basically does nothing other than mctx.write().
Pierre-Yves David - July 29, 2020, 12:36 p.m.
On 7/29/20 2:02 PM, Yuya Nishihara wrote:
> On Wed, 29 Jul 2020 13:48:17 +0200, Pierre-Yves David wrote:
>>>>> 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.
>>
>> The main point of this series is to isolated independant piece of logic
>> to avoid logic creep. That would be a step in the wrong direction.
> 
> Yep, but the most complex manifest logic exists in _process_files().
> _commit_manifest() basically does nothing other than mctx.write().

_commit_manifest does all the logic of figuring out how to obtain de 
manifest-node from a prepared manifest. So it is reasonable to isolate 
that part from the preparation itself.

Patch

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