Page MenuHomePhabricator

merge: move cwd-missing detection to helper functions
ClosedPublic

Authored by phillco on Sep 4 2017, 9:38 PM.

Details

Summary

This will exist in two places with defered writes, so we want to avoid
duplication.

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

phillco created this revision.Sep 4 2017, 9:38 PM
quark accepted this revision.Sep 8 2017, 3:09 PM
quark added a subscriber: quark.

Nice code de-duplication

pulkit accepted this revision.Sep 9 2017, 2:00 PM

Oops, I just found some old unsubmitted comments here. Sorry

mercurial/merge.py
1776–1778

Now that we this is a function and "return" can be used to prevent execution of lines after it, maybe not negating the condition helps a little:

if err.errno == errno.ENOENT:
    return None
raise
1783

I think it would be clearer to move at least the "if oldcwd" back to the caller. I think the entire function seems simple enough to be in the caller now _getcwd() has been extracted, actually.

martinvonz added inline comments.Sep 12 2017, 6:37 PM
mercurial/merge.py
1772

i think we usually put helpers close to (and before?) the functions that use them, not at the end of the file

phillco updated this revision to Diff 1775.Sep 12 2017, 10:31 PM
This revision was automatically updated to reflect the committed changes.