Patchwork D3704: py3: prevent comparison of integers with None

login
register
mail settings
Submitter phabricator
Date June 9, 2018, 5:37 p.m.
Message ID <differential-rev-PHID-DREV-lpjdvm5ktelnzbgenstk-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32040/
State Superseded
Headers show

Comments

phabricator - June 9, 2018, 5:37 p.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  On py2, we can compare integer type with NoneType but on py3 we can't.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3704

AFFECTED FILES
  hgext/convert/cvsps.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - June 10, 2018, 2:15 a.m.
>      for e in log:
>          if e.commitid:
> -            mindate[e.commitid] = min(e.date, mindate.get(e.commitid))
> +            if mindate.get(e.commitid) is None:
> +                mindate[e.commitid] = None
> +            else:
> +                mindate[e.commitid] = min(e.date, mindate.get(e.commitid))
>  
>      # Merge changesets
>      log.sort(key=lambda x: (mindate.get(x.commitid), x.commitid, x.comment,

`mindate` is also used here as a comparison key. Filling it to `None` would
make things worse.

I think the original code, which just fills every mindate element with `None`,
was wrong. Instead, it should probably keep the minimum value only if exists,

```
            if e.commitid in mindate:
                mindate[e.commitid] = min(e.date, mindate[e.commitid])
            else:
                mindate[e.commitid] = e.date
```

and later falls back to the null value.

```
    log.sort(key=lambda x: (mindate.get(x.commitid, (lowest_or_highest_value?, 0)
```
phabricator - June 10, 2018, 2:18 a.m.
yuja added a comment.


  >   for e in log:
  >       if e.commitid:
  > 
  > - mindate[e.commitid] = min(e.date, mindate.get(e.commitid)) +            if mindate.get(e.commitid) is None: +                mindate[e.commitid] = None +            else: +                mindate[e.commitid] = min(e.date, mindate.get(e.commitid))
  >   1. Merge changesets log.sort(key=lambda x: (mindate.get(x.commitid), x.commitid, x.comment,
  
  `mindate` is also used here as a comparison key. Filling it to `None` would
  make things worse.
  
  I think the original code, which just fills every mindate element with `None`,
  was wrong. Instead, it should probably keep the minimum value only if exists,
  
    if e.commitid in mindate:
        mindate[e.commitid] = min(e.date, mindate[e.commitid])
    else:
        mindate[e.commitid] = e.date
  
  and later falls back to the null value.
  
    log.sort(key=lambda x: (mindate.get(x.commitid, (lowest_or_highest_value?, 0)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3704

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - June 14, 2018, 11:48 a.m.
pulkit abandoned this revision.
pulkit added a comment.


  Not required anymore.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3704

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/hgext/convert/cvsps.py b/hgext/convert/cvsps.py
--- a/hgext/convert/cvsps.py
+++ b/hgext/convert/cvsps.py
@@ -567,7 +567,10 @@ 
     mindate = {}
     for e in log:
         if e.commitid:
-            mindate[e.commitid] = min(e.date, mindate.get(e.commitid))
+            if mindate.get(e.commitid) is None:
+                mindate[e.commitid] = None
+            else:
+                mindate[e.commitid] = min(e.date, mindate.get(e.commitid))
 
     # Merge changesets
     log.sort(key=lambda x: (mindate.get(x.commitid), x.commitid, x.comment,