D2939: fsmonitor: layer on another hack in bser.c for os.stat() compat (issue5811)

indygreg (Gregory Szorc) phabricator at mercurial-scm.org
Mon Mar 26 12:04:17 EDT 2018


indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  This still fails for me with:
  
      File "/home/gps/src/hg/mercurial/context.py", line 1590, in _dirstatestatus
        clean=clean, unknown=unknown)
      File "/home/gps/src/hg/mercurial/dirstate.py", line 1073, in status
        elif (time != st[stat.ST_MTIME]
    IndexError: tuple index out of range
  
  Also, hacking the low-level bser data type at the bser level feels **very** wrong. I would rather we roll our own C type that holds a reference to the `bserObject` and provides the necessary object API. Although there are already Mercurial hacks in `bser.c` for `st_*` attribute access. So I could be convinced that this hack is acceptable.
  
  I can try debugging this failure if that would be helpful. Perhaps something in CPython land is evaluating `len()` and short-circuiting the `[8]` access.

INLINE COMMENTS

> bser.c:105
>  
> +  if (i == 8 && PySequence_Size(obj->values) < 7) {
> +    // Hack alert: Python 3 removed support for os.stat().st_mtime

Is there an off by 2 with `7`? Shouldn't it be `9` (since accesses up to `[7]` - an object with 8 elements) should be allowed?

> bser.c:112
> +    // st_mtime instead.
> +    return bserobj_getattrro(o, PyBytes_FromString("st_mtime"));
> +  }

This leaks the `st_mtime` PyObject.

Also, strictly speaking, we should verify that the return value is not `NULL`.

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel


More information about the Mercurial-devel mailing list