Re: [PATCH 03 of 11] commitctx: extract the function that commit a new manifest
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. -- Pierre-Yves David ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 03 of 11] commitctx: extract the function that commit a new manifest
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(). ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 03 of 11] commitctx: extract the function that commit a new manifest
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 # 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. -- Pierre-Yves David ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 03 of 11] commitctx: extract the function that commit a new manifest
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 > >> # 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 Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 03 of 11] commitctx: extract the function that commit a new manifest
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 # 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 Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 03 of 11] commitctx: extract the function that commit a new manifest
On Fri, 24 Jul 2020 16:38:28 +0200, Pierre-Yves David wrote: > # HG changeset patch > # User Pierre-Yves David > # 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 Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
[PATCH 03 of 11] commitctx: extract the function that commit a new manifest
# HG changeset patch # User Pierre-Yves David # 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 Mercurial-devel@mercurial-scm.org https://www.me
