Patchwork [1,of,3] simplemerge: fix function name that tests if ctx is not null revision

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

Comments

Yuya Nishihara - June 4, 2020, 2:15 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1591101547 -32400
#      Tue Jun 02 21:39:07 2020 +0900
# Node ID 8e111a6aabc5efd7ef414b9b1b6fa46eeaa569f4
# Parent  03ba7de6a8b9ab339a6dafab969e890228b4c2f1
simplemerge: fix function name that tests if ctx is not null revision
Pierre-Yves David - June 5, 2020, 2:21 p.m.
Naming wise is would probably be better to keep `is_null` but fix the 
return value and add a `not` at the calling site.

(However this patch is correct and can be taken as is)

On 6/4/20 4:15 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1591101547 -32400
> #      Tue Jun 02 21:39:07 2020 +0900
> # Node ID 8e111a6aabc5efd7ef414b9b1b6fa46eeaa569f4
> # Parent  03ba7de6a8b9ab339a6dafab969e890228b4c2f1
> simplemerge: fix function name that tests if ctx is not null revision
> 
> diff --git a/mercurial/simplemerge.py b/mercurial/simplemerge.py
> --- a/mercurial/simplemerge.py
> +++ b/mercurial/simplemerge.py
> @@ -456,7 +456,7 @@ def _bytes_to_set(b):
>       return set(b[x : x + 1] for x in range(len(b)))
>   
>   
> -def is_null(ctx):
> +def is_not_null(ctx):
>       if not util.safehasattr(ctx, "node"):
>           return False
>       return ctx.node() != nodemod.nullid
> @@ -520,7 +520,7 @@ def simplemerge(ui, localctx, basectx, o
>       flags = localctx.flags()
>       localflags = _bytes_to_set(flags)
>       otherflags = _bytes_to_set(otherctx.flags())
> -    if is_null(basectx) and localflags != otherflags:
> +    if is_not_null(basectx) and localflags != otherflags:
>           baseflags = _bytes_to_set(basectx.flags())
>           flags = localflags & otherflags
>           for f in localflags.symmetric_difference(otherflags):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - June 6, 2020, 3:13 a.m.
On Fri, 5 Jun 2020 16:21:15 +0200, Pierre-Yves David wrote:
> Naming wise is would probably be better to keep `is_null` but fix the 
> return value and add a `not` at the calling site.

Just inverting the condition wouldn't make much sense since the function
also tests util.safehasattr(ctx, "node"):

> > -def is_null(ctx):
> > +def is_not_null(ctx):
> >       if not util.safehasattr(ctx, "node"):
> >           return False
> >       return ctx.node() != nodemod.nullid

def is_null(ctx):
    if not util.safehasattr(ctx, "node"):
        # if ctx.node is missing, I would say it's unknown and falsy
        return True
Pierre-Yves David - June 6, 2020, 12:45 p.m.
On 6/6/20 5:13 AM, Yuya Nishihara wrote:
> On Fri, 5 Jun 2020 16:21:15 +0200, Pierre-Yves David wrote:
>> Naming wise is would probably be better to keep `is_null` but fix the
>> return value and add a `not` at the calling site.
> 
> Just inverting the condition wouldn't make much sense since the function
> also tests util.safehasattr(ctx, "node"):

I am talking about the condition in the call: what would be something like:



def is_null(ctx):
     if util.safehasattr(ctx, "node"):
         # if ctx.node is missing, I would say it's unknown and falsy
         return return ctx.node() == nodemod.nullid
     return False


And later:

     if not is_null(basectx) and localflags != otherflags:


(a ctx without `node` would be considered "not null", I suspect the 
current code is false on that regard)

> 
>>> -def is_null(ctx):
>>> +def is_not_null(ctx):
>>>        if not util.safehasattr(ctx, "node"):
>>>            return False
>>>        return ctx.node() != nodemod.nullid
> 
> def is_null(ctx):
>      if not util.safehasattr(ctx, "node"):
>          # if ctx.node is missing, I would say it's unknown and falsy
>          return True
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/mercurial/simplemerge.py b/mercurial/simplemerge.py
--- a/mercurial/simplemerge.py
+++ b/mercurial/simplemerge.py
@@ -456,7 +456,7 @@  def _bytes_to_set(b):
     return set(b[x : x + 1] for x in range(len(b)))
 
 
-def is_null(ctx):
+def is_not_null(ctx):
     if not util.safehasattr(ctx, "node"):
         return False
     return ctx.node() != nodemod.nullid
@@ -520,7 +520,7 @@  def simplemerge(ui, localctx, basectx, o
     flags = localctx.flags()
     localflags = _bytes_to_set(flags)
     otherflags = _bytes_to_set(otherctx.flags())
-    if is_null(basectx) and localflags != otherflags:
+    if is_not_null(basectx) and localflags != otherflags:
         baseflags = _bytes_to_set(basectx.flags())
         flags = localflags & otherflags
         for f in localflags.symmetric_difference(otherflags):