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
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
martinvonz added a subscriber: martinvonz.Sep 11 2017, 7:39 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
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 (33 lines)

Diff 1776

mercurial/merge.py

fractions = _forgetremoved(wctx, mctx, branchmerge) fractions = _forgetremoved(wctx, mctx, branchmerge)
actions.update(fractions) actions.update(fractions)
prunedactions = sparse.filterupdatesactions(repo, wctx, mctx, branchmerge, prunedactions = sparse.filterupdatesactions(repo, wctx, mctx, branchmerge,
actions) actions)
return prunedactions, diverge, renamedelete return prunedactions, diverge, renamedelete
def _getcwd():
try:
return pycompat.getcwd()
except OSError as err:
if err.errno == errno.ENOENT:
return None
raise
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 if cwd and not _getcwd():
# let's check if it is present after we removed them # cwd was removed in the course of removing files; print a helpful
try: # warning.
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" repo.ui.warn(_("current directory was removed\n"
"(consider changing to repo root: %s)\n") % "(consider changing to repo root: %s)\n") % repo.root)
repo.root)
# It's necessary to flush here in case we're inside a worker fork and will # It's necessary to flush here in case we're inside a worker fork and will
# quit after this function. # quit after this function.
wctx.flushall() wctx.flushall()
def batchget(repo, mctx, wctx, actions): def batchget(repo, mctx, wctx, actions):
"""apply gets to the working directory """apply gets to the working directory
parents.remove(pctx) parents.remove(pctx)
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
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…
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…
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…