This is an archive of the discontinued Mercurial Phabricator instance.

Differential D626

merge: move cwd-missing detection to helper functions
ClosedPublic

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

Details

Reviewers
quark
pulkit
Group Reviewers
hg-reviewers
Commits
rHG24bf823377fc: merge: move cwd-missing detection to helper functions
Summary

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

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

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
martinvonz added a subscriber: martinvonz.Sep 11 2017, 7:39 PM

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

mercurial/merge.py
1749–1751

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
1756

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
1745

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
Closed by commit rHG24bf823377fc: merge: move cwd-missing detection to helper functions (authored by phillco). · Explain WhySep 12 2017, 11:55 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/merge.py (38 lines)

Diff 1606

mercurial/merge.py

return prunedactions, diverge, renamedelete return prunedactions, diverge, renamedelete
def batchremove(repo, wctx, actions): def batchremove(repo, wctx, actions):
"""apply removes to the working directory """apply removes to the working directory
yields tuples for progress updates yields tuples for progress updates
""" """
verbose = repo.ui.verbose verbose = repo.ui.verbose
try: cwd = _getcwd()
cwd = pycompat.getcwd()
except OSError as err:
if err.errno != errno.ENOENT:
raise
cwd = None
i = 0 i = 0
for f, args, msg in actions: for f, args, msg in actions:
repo.ui.debug(" %s: %s -> r\n" % (f, msg)) repo.ui.debug(" %s: %s -> r\n" % (f, msg))
if verbose: if verbose:
repo.ui.note(_("removing %s\n") % f) repo.ui.note(_("removing %s\n") % f)
wctx[f].audit() wctx[f].audit()
try: try:
wctx[f].remove(ignoremissing=True) wctx[f].remove(ignoremissing=True)
except OSError as inst: except OSError as inst:
repo.ui.warn(_("update failed to remove %s: %s!\n") % repo.ui.warn(_("update failed to remove %s: %s!\n") %
(f, inst.strerror)) (f, inst.strerror))
if i == 100: if i == 100:
yield i, f yield i, f
i = 0 i = 0
i += 1 i += 1
if i > 0: if i > 0:
yield i, f yield i, f
if cwd:
# cwd was present before we started to remove files _warnifmissingcwd(repo, cwd)
# let's check if it is present after we removed them
try:
pycompat.getcwd()
except OSError as err:
if err.errno != errno.ENOENT:
raise
# Print a warning if cwd was deleted
repo.ui.warn(_("current directory was removed\n"
"(consider changing to repo root: %s)\n") %
repo.root)
def batchget(repo, mctx, wctx, actions): def batchget(repo, mctx, wctx, actions):
"""apply gets to the working directory """apply gets to the working directory
mctx is the context to get from mctx is the context to get from
yields tuples for progress updates yields tuples for progress updates
""" """
pother = parents[0].node() pother = parents[0].node()
with repo.dirstate.parentchange(): with repo.dirstate.parentchange():
repo.setparents(repo['.'].node(), pother) repo.setparents(repo['.'].node(), pother)
repo.dirstate.write(repo.currenttransaction()) repo.dirstate.write(repo.currenttransaction())
# fix up dirstate for copies and renames # fix up dirstate for copies and renames
copies.duplicatecopies(repo, ctx.rev(), pctx.rev()) copies.duplicatecopies(repo, ctx.rev(), pctx.rev())
return stats return stats
def _getcwd():
martinvonzUnsubmitted
Not Done

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

martinvonz: i think we usually put helpers close to (and before?) the functions that use them, not at the…
try:
return pycompat.getcwd()
except OSError as err:
if err.errno != errno.ENOENT:
raise
return None
martinvonzUnsubmitted
Not Done

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
martinvonz: Now that we this is a function and "return" can be used to prevent execution of lines after it…
def _warnifmissingcwd(repo, oldcwd):
# cwd was present before we started to remove files
# let's check if it is present after we removed them
if oldcwd and not _getcwd():
martinvonzUnsubmitted
Not Done

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: I think it would be clearer to move at least the "if oldcwd" back to the caller. I think the…
# Print a warning if cwd was deleted
repo.ui.warn(_("current directory was removed\n"
"(consider changing to repo root: %s)\n") %
repo.root)