This is an archive of the discontinued Mercurial Phabricator instance.

dirstate: add missing return on platforms without exec or symlink
ClosedPublic

Authored by Alphare on Oct 20 2021, 1:07 PM.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Oct 20 2021, 1:07 PM
spectral accepted this revision.Oct 20 2021, 2:27 PM
spectral added a subscriber: spectral.

I'm not sure the etiquette here; I'm accepting the revision since it obviously fixes a bug, but still would like the comments to be considered/responded to?

mercurial/dirstate.py
303

I think it'd be easier to read and easier to see the issue if this used fewer returns:

if self._checklink:
  def f(x):
    ...
elif self._checkexec:
  def f(x):
    ...
else:
  def f(x):
    ...

return f

Ideally we'd be able to get the first branch as well, perhaps like:

def flagfunc(self, buildfallback):
  # fallback is unused in this case and expensive to calculate
  if not (self._checklink and self._checkexec):
    fallback = buildfallback()

  if self._checklink and self._checkexec:
    def f(x):
      ...
  elif self._checklink:
  ...

  return f

I would also accept moving these functions to be at module scope with "obvious" names like flagfunc_link_and_exec, flagfunc_link_only, flagfunc_exec_only, flagfunc_fallback_only (or whatever), and shrinking flagfunc considerably by just making it return the appropriate function, which would make it small enough to make the forgotten return quite a bit more obvious.

I'm normally a fan of early returns (I don't think there's a strong reason for functions to have "only one return"), but I think in this case it would improve readability and maintainability.

I'm not sure the etiquette here; I'm accepting the revision since it obviously fixes a bug, but still would like the comments to be considered/responded to?

I think it's fine to accept a change to say "I'm ok with it going through like so because it's important" and expect at the same time to get an answer for the other comments. I've done it multiple times with the author then updating the patch or sending a follow-up.

mercurial/dirstate.py
303

I agree that factoring the returns is a good idea here. I'm not so sure that moving the functions to module scope is worth the (albeit very small churn) since they are so small and don't offer much use outside of this context.

spectral added inline comments.Oct 20 2021, 9:11 PM
mercurial/dirstate.py
303

Yeah, keeping them inlined but with useful names and having the decision tree be short would also be fine, not sure why I didn't think of that before "move them to module scope" :)

Something like:

def flagfunc(self, buildfallback):
  if not (self._checklink and self._checkexec):
    fallback = buildfallback()

  def check_both(x):
    ...
 
  def check_link(x):
  ...

  if self._checklink and self._checkexec:
    return check_both
  elif self._checklink:
    return check_link
  ...
Alphare added inline comments.Oct 21 2021, 3:43 AM
mercurial/dirstate.py
303

I've followed up on this here: D11715

baymax updated this revision to Diff 30976.Oct 21 2021, 1:51 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.