Patchwork [3,of,3] simplemerge: rewrite flag merging loop as expression

login
register
mail settings
Submitter Yuya Nishihara
Date June 4, 2020, 2:15 p.m.
Message ID <970abb8ea2a2d6a086eb.1591280143@mimosa>
Download mbox | patch
Permalink /patch/46464/
State Accepted
Headers show

Comments

Yuya Nishihara - June 4, 2020, 2:15 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1591101897 -32400
#      Tue Jun 02 21:44:57 2020 +0900
# Node ID 970abb8ea2a2d6a086ebfdd9a1130b2fb777640e
# Parent  dba0ac9b68381e9610f25d9991bd623f85cbdefd
simplemerge: rewrite flag merging loop as expression

I feel binary operations are more readable.
Pierre-Yves David - June 5, 2020, 2:24 p.m.
On 6/4/20 4:15 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1591101897 -32400
> #      Tue Jun 02 21:44:57 2020 +0900
> # Node ID 970abb8ea2a2d6a086ebfdd9a1130b2fb777640e
> # Parent  dba0ac9b68381e9610f25d9991bd623f85cbdefd
> simplemerge: rewrite flag merging loop as expression
> 
> I feel binary operations are more readable.

The fact also merge "flag removal" become implicit and might be less 
clear to reader. I like the use of binary operation, but can we add a 
comment about the removed flag ?

> 
> diff --git a/mercurial/simplemerge.py b/mercurial/simplemerge.py
> --- a/mercurial/simplemerge.py
> +++ b/mercurial/simplemerge.py
> @@ -517,11 +517,9 @@ def simplemerge(ui, localctx, basectx, o
>       otherflags = set(pycompat.iterbytestr(otherctx.flags()))
>       if is_not_null(basectx) and localflags != otherflags:
>           baseflags = set(pycompat.iterbytestr(basectx.flags()))
> -        flags = localflags & otherflags
> -        for f in localflags.symmetric_difference(otherflags):
> -            if f not in baseflags:
> -                flags.add(f)
> -        flags = b''.join(sorted(flags))
> +        commonflags = localflags & otherflags
> +        addedflags = (localflags ^ otherflags) - baseflags
> +        flags = b''.join(sorted(commonflags | addedflags))
>   
>       if not opts.get(b'print'):
>           localctx.write(mergedtext, flags)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - June 6, 2020, 3:04 a.m.
On Fri, 5 Jun 2020 16:24:17 +0200, Pierre-Yves David wrote:
> On 6/4/20 4:15 PM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1591101897 -32400
> > #      Tue Jun 02 21:44:57 2020 +0900
> > # Node ID 970abb8ea2a2d6a086ebfdd9a1130b2fb777640e
> > # Parent  dba0ac9b68381e9610f25d9991bd623f85cbdefd
> > simplemerge: rewrite flag merging loop as expression
> > 
> > I feel binary operations are more readable.
> 
> The fact also merge "flag removal" become implicit and might be less 
> clear to reader. I like the use of binary operation, but can we add a 
> comment about the removed flag ?

What do you mean with the "flag removal"?
The logic is unchanged, which is to take the common flags and
then add changes from the base.

> > -        flags = localflags & otherflags
> > -        for f in localflags.symmetric_difference(otherflags):
> > -            if f not in baseflags:
> > -                flags.add(f)
> > -        flags = b''.join(sorted(flags))
> > +        commonflags = localflags & otherflags
> > +        addedflags = (localflags ^ otherflags) - baseflags
> > +        flags = b''.join(sorted(commonflags | addedflags))
Pierre-Yves David - June 6, 2020, 12:42 p.m.
On 6/6/20 5:04 AM, Yuya Nishihara wrote:
> On Fri, 5 Jun 2020 16:24:17 +0200, Pierre-Yves David wrote:
>> On 6/4/20 4:15 PM, Yuya Nishihara wrote:
>>> # HG changeset patch
>>> # User Yuya Nishihara <yuya@tcha.org>
>>> # Date 1591101897 -32400
>>> #      Tue Jun 02 21:44:57 2020 +0900
>>> # Node ID 970abb8ea2a2d6a086ebfdd9a1130b2fb777640e
>>> # Parent  dba0ac9b68381e9610f25d9991bd623f85cbdefd
>>> simplemerge: rewrite flag merging loop as expression
>>>
>>> I feel binary operations are more readable.
>>
>> The fact also merge "flag removal" become implicit and might be less
>> clear to reader. I like the use of binary operation, but can we add a
>> comment about the removed flag ?
> 
> What do you mean with the "flag removal"?
> The logic is unchanged, which is to take the common flags and
> then add changes from the base.

If a flag is missing from one of the two merged side, but was presented 
in the base, it is "removed".

The logic is unchanged, and the new code behave correctly in this case 
(as was the older one)

  However it seems useful to highlight that this code is also dealing 
with removal (with a comment mentioning it) so that future reader do not 
miss that fact.
Yuya Nishihara - June 7, 2020, 11:54 a.m.
On Sat, 6 Jun 2020 14:42:00 +0200, Pierre-Yves David wrote:
> On 6/6/20 5:04 AM, Yuya Nishihara wrote:
> > On Fri, 5 Jun 2020 16:24:17 +0200, Pierre-Yves David wrote:
> >> On 6/4/20 4:15 PM, Yuya Nishihara wrote:
> >>> # HG changeset patch
> >>> # User Yuya Nishihara <yuya@tcha.org>
> >>> # Date 1591101897 -32400
> >>> #      Tue Jun 02 21:44:57 2020 +0900
> >>> # Node ID 970abb8ea2a2d6a086ebfdd9a1130b2fb777640e
> >>> # Parent  dba0ac9b68381e9610f25d9991bd623f85cbdefd
> >>> simplemerge: rewrite flag merging loop as expression
> >>>
> >>> I feel binary operations are more readable.
> >>
> >> The fact also merge "flag removal" become implicit and might be less
> >> clear to reader. I like the use of binary operation, but can we add a
> >> comment about the removed flag ?
> > 
> > What do you mean with the "flag removal"?
> > The logic is unchanged, which is to take the common flags and
> > then add changes from the base.
> 
> If a flag is missing from one of the two merged side, but was presented 
> in the base, it is "removed".
> 
> The logic is unchanged, and the new code behave correctly in this case 
> (as was the older one)
> 
>   However it seems useful to highlight that this code is also dealing 
> with removal (with a comment mentioning it) so that future reader do not 
> miss that fact.

Okay, can you send a follow up? It's out of scope of this series. I just
wanted to update things that I felt uncomfortable while reviewing the
original patches.
Pierre-Yves David - June 9, 2020, 11:19 a.m.
Sure, I'll wait for these series to get in first.

On 6/7/20 1:54 PM, Yuya Nishihara wrote:
> On Sat, 6 Jun 2020 14:42:00 +0200, Pierre-Yves David wrote:
>> On 6/6/20 5:04 AM, Yuya Nishihara wrote:
>>> On Fri, 5 Jun 2020 16:24:17 +0200, Pierre-Yves David wrote:
>>>> On 6/4/20 4:15 PM, Yuya Nishihara wrote:
>>>>> # HG changeset patch
>>>>> # User Yuya Nishihara <yuya@tcha.org>
>>>>> # Date 1591101897 -32400
>>>>> #      Tue Jun 02 21:44:57 2020 +0900
>>>>> # Node ID 970abb8ea2a2d6a086ebfdd9a1130b2fb777640e
>>>>> # Parent  dba0ac9b68381e9610f25d9991bd623f85cbdefd
>>>>> simplemerge: rewrite flag merging loop as expression
>>>>>
>>>>> I feel binary operations are more readable.
>>>>
>>>> The fact also merge "flag removal" become implicit and might be less
>>>> clear to reader. I like the use of binary operation, but can we add a
>>>> comment about the removed flag ?
>>>
>>> What do you mean with the "flag removal"?
>>> The logic is unchanged, which is to take the common flags and
>>> then add changes from the base.
>>
>> If a flag is missing from one of the two merged side, but was presented
>> in the base, it is "removed".
>>
>> The logic is unchanged, and the new code behave correctly in this case
>> (as was the older one)
>>
>>    However it seems useful to highlight that this code is also dealing
>> with removal (with a comment mentioning it) so that future reader do not
>> miss that fact.
> 
> Okay, can you send a follow up? It's out of scope of this series. I just
> wanted to update things that I felt uncomfortable while reviewing the
> original patches.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Augie Fackler - June 9, 2020, 9:25 p.m.
On Thu, Jun 04, 2020 at 11:15:43PM +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1591101897 -32400
> #      Tue Jun 02 21:44:57 2020 +0900
> # Node ID 970abb8ea2a2d6a086ebfdd9a1130b2fb777640e
> # Parent  dba0ac9b68381e9610f25d9991bd623f85cbdefd
> simplemerge: rewrite flag merging loop as expression

queued, thanks

Patch

diff --git a/mercurial/simplemerge.py b/mercurial/simplemerge.py
--- a/mercurial/simplemerge.py
+++ b/mercurial/simplemerge.py
@@ -517,11 +517,9 @@  def simplemerge(ui, localctx, basectx, o
     otherflags = set(pycompat.iterbytestr(otherctx.flags()))
     if is_not_null(basectx) and localflags != otherflags:
         baseflags = set(pycompat.iterbytestr(basectx.flags()))
-        flags = localflags & otherflags
-        for f in localflags.symmetric_difference(otherflags):
-            if f not in baseflags:
-                flags.add(f)
-        flags = b''.join(sorted(flags))
+        commonflags = localflags & otherflags
+        addedflags = (localflags ^ otherflags) - baseflags
+        flags = b''.join(sorted(commonflags | addedflags))
 
     if not opts.get(b'print'):
         localctx.write(mergedtext, flags)