[PATCH 2 of 4 V3] merge: add summarizeconflicts()

[Ryan McElroy]

Just a few nitpicks inline.


On 3/17/17 2:17 AM, Phil Cohen wrote:
> # HG changeset patch
> # User Phil Cohen <phillco at fb.com>
> # Date 1489716532 25200
> #      Thu Mar 16 19:08:52 2017 -0700
> # Node ID 864def5028681cdffeaf5b440ff4753e1389f453
> # Parent  ad81d182e88ffd3da20364f18204facf6e7750a3
> merge: add summarizeconflicts()
>
> calls `filemerge.summarize()` but only requires a workingctx and a path

Again, explain what this will be used for so the change stands on its 
own feet.

>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -544,6 +544,25 @@
>           Returns whether the merge is complete, and the exit code."""
>           return self._resolve(True, dfile, wctx)
>   
> +    def summarizeconflicts(self, path, workingctx):
> +        if self[path] in 'rd':
> +            return None

I have no idea what 'rd' is or what you're checking for here. Can you 
add a comment to clarify what's going on here please?

Longer term, we should make all of these random mergestate letters into 
constants so this code is more readable and maintainable, but that's 
well outside the scope of this patch series :-)


> +        stateentry = self._state[path]
> +        state, hash, lfile, afile, anode, ofile, onode, flags = stateentry

I think the names here could be a little better, but that will likely 
take you past 80 chars if you keep it in this form. So I'll leave it up 
to your discretion if you want to change any of them. However, since 
you're not using all of the vars, you could do this slightly 
differently. Also be careful to consistently use either 'node' or 'hash' 
and don't go back and forth if possible. A suggestion:

resolvestate = stateentry[0] # unused? omit if so.
localnode = stateentry[1] # unused? omit if so.
localfile = stateentry[2] # unused? omit if so.
ancestorfile = stateentry[3]
otherfile = stateentry[4]
otherfile = stateentry[5]
othernode = stateentry[6]
localflags = stateentry[7] # unused? omit if so.

These changes would make the code a lot more self-documenting.

Also a couple of question: is localfile always the same as path? Are 
localflags the same things you're calculating in summarize for the local 
file? Did I guess right here?

> +        octx = self._repo[self._other]
> +        extras = self.extras(path)
> +        anccommitnode = extras.get('ancestorlinknode')
> +        if anccommitnode:
> +            ancestorctx = self._repo[anccommitnode]
> +        else:
> +            ancestorctx = None
> +        workingctx = self._filectxorabsent(hash, workingctx, path)
> +        otherctx = self._filectxorabsent(onode, octx, ofile)
> +        basectx = self._repo.filectx(afile, fileid=anode,
> +                                     changeid=ancestorctx)

What does this do if the ancestorctx is None? Does a future test case 
cover it?
> +
> +        return filemerge.summarize(self._repo, workingctx, otherctx, basectx)
> +
>       def resolve(self, dfile, wctx):
>           """run merge process (assuming premerge was run) for dfile
>   
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Jw8rundaE7TbmqBYd1txIQ&m=rA3crMJXqkwHAuyVyTem2744aA1bcLAyyldjmGVq_lQ&s=r7lYBxtd2Q9Q3qPFmWCDuMZdVd4RntNW6mM53xM5ToE&e=