[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 <pierre-yves.david at 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.