| 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
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 >
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))
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.
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.
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 >
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)