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