This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8432

dirstate: force _checkexec to return a bool
ClosedPublic

Authored by mplamann on Apr 15 2020, 4:58 PM.

Details

Reviewers
marmoute
Alphare
Group Reviewers
hg-reviewers
Commits
rHG373dd22ae60e: dirstate: force _checkexec to return a bool
Summary

posix.checkexec can return True, False, or None. The rust status
implementation expects a boolean, so make sure _checkexec returns a boolean.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

mplamann created this revision.Apr 15 2020, 4:58 PM
marmoute added a subscriber: marmoute.Apr 15 2020, 5:38 PM

That looks valid, but can we get a fix the assignment too ?

mplamann added a comment.Apr 16 2020, 4:56 PM

What do you mean by fix the assignment?

marmoute added a comment.Apr 16 2020, 5:21 PM
In D8432#126079, @mplamann wrote:

What do you mean by fix the assignment?

thre are some code somewhere putting a None in this attribute. given you issue you face. It seems reasonable to make that code set a False instead.

In practice this means doing this instead

diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -180,7 +180,7 @@ class dirstate(object):
 
     @propertycache
     def _checkexec(self):
-        return util.checkexec(self._root)
+        return bool(util.checkexec(self._root))
 
     @propertycache
     def _checkcase(self):

(we could also consider going up to the "plateform" file, but this seems a good enough spot.

mplamann retitled this revision from dirstate: convert _checkexec to a bool when calling rustmod.status to dirstate: force _checkexec to return a bool.Apr 17 2020, 2:01 PM
mplamann edited the summary of this revision. (Show Details)
mplamann updated this revision to Diff 21154.
marmoute accepted this revision.Apr 17 2020, 2:09 PM
Alphare accepted this revision.Apr 20 2020, 3:48 AM
Alphare added a subscriber: Alphare.

Thanks for the fix, I probably forgot about this because I remember writing a similar fix.

mplamann added a commit: rHG373dd22ae60e: dirstate: force _checkexec to return a bool.May 7 2020, 2:20 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG373dd22ae60e: dirstate: force _checkexec to return a bool (authored by mplamann). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/dirstate.py (2 lines)

Diff 21154

mercurial/dirstate.py

return self._ui.configbool(b'ui', b'slash') and pycompat.ossep != b'/' return self._ui.configbool(b'ui', b'slash') and pycompat.ossep != b'/'
@propertycache @propertycache
def _checklink(self): def _checklink(self):
return util.checklink(self._root) return util.checklink(self._root)
@propertycache @propertycache
def _checkexec(self): def _checkexec(self):
return util.checkexec(self._root) return bool(util.checkexec(self._root))
@propertycache @propertycache
def _checkcase(self): def _checkcase(self):
return not util.fscasesensitive(self._join(b'.hg')) return not util.fscasesensitive(self._join(b'.hg'))
def _join(self, f): def _join(self, f):
# much faster than os.path.join() # much faster than os.path.join()
# it's safe because f is always a relative path # it's safe because f is always a relative path