This is an archive of the discontinued Mercurial Phabricator instance.

Differential D778

merge: backup conflicting directories when getting files
ClosedPublic

Authored by mbthomas on Sep 22 2017, 5:28 AM.

Details

Reviewers
ryanmce
Group Reviewers
hg-reviewers
Commits
rHGa991e1d6bc82: merge: backup conflicting directories when getting files
rHGbb8928212e3a: merge: backup conflicting directories when getting files
Summary

During batchget, if a target file conflicts with a directory, or if the
directory a target file is in conflicts with a file, backup and remove the
conflicting file or directory before performing the get.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mbthomas created this revision.Sep 22 2017, 5:28 AM
ryanmce requested changes to this revision.Sep 25 2017, 3:56 PM
ryanmce added a subscriber: ryanmce.

It seems like this one should have an externally-testable result. Could you adda test that shows the new behavior working better than it did before? If there a bug on bugzilla open that this solves?

This revision now requires changes to proceed.Sep 25 2017, 3:56 PM
mbthomas added a comment.Sep 26 2017, 11:54 AM

This doesn't actually become a problem until D781. For now backup can only be true if f conflicts with a real file. If it has a path conflict then we fail earlier. In D781 I will add the possibility to detect path conflicts and mark them for backup, at which point we need this function to work for path conflicts as well as normal file conflicts.

mbthomas updated this revision to Diff 2214.Oct 1 2017, 5:32 AM
• kiilerix added a subscriber: • kiilerix.Oct 1 2017, 7:05 AM
• kiilerix added inline comments.
mercurial/merge.py
1174

This seems quite a bit slower. But I guess it never will happen in tight loops? If we have to backup a lot of files, then we have lost anyway?

mbthomas added inline comments.Oct 2 2017, 5:11 PM
mercurial/merge.py
1174

It will happen in batchget, which is for each file that is being created in the update. The new loop is O(path-length), which should be small. We should also only be touching things the OS needed to look at anyway to answer the original question, and which we looked at when we audited the path earlier on.

mbthomas updated this revision to Diff 2355.Oct 2 2017, 5:15 PM
ryanmce accepted this revision.Oct 5 2017, 12:22 PM

lgtm

mercurial/merge.py
1174

If we have to backup a lot of files, then we have lost anyway?

I think that's essentially the case. The other option here is to always rename conflcting files/directories, but then you end up with unbounded growth in the size of backup files (either in working copy or in some other dir), so that's not really awesome either.

Closed by commit rHGbb8928212e3a: merge: backup conflicting directories when getting files (authored by mbthomas). · Explain WhyOct 9 2017, 9:25 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/merge.py (16 lines)

Diff 1996

mercurial/merge.py

i = 0 i = 0
with repo.wvfs.backgroundclosing(ui, expectedcount=len(actions)): with repo.wvfs.backgroundclosing(ui, expectedcount=len(actions)):
for f, (flags, backup), msg in actions: for f, (flags, backup), msg in actions:
repo.ui.debug(" %s: %s -> g\n" % (f, msg)) repo.ui.debug(" %s: %s -> g\n" % (f, msg))
if verbose: if verbose:
repo.ui.note(_("getting %s\n") % f) repo.ui.note(_("getting %s\n") % f)
if backup: if backup:
# If a file or directory exists with the same name, back that
# up. Otherwise, look to see if there is a file that conflicts
# with a directory this file is in, and if so, back that up.
absf = repo.wjoin(f) absf = repo.wjoin(f)
if not repo.wvfs.lexists(f):
for p in util.finddirs(f):
if repo.wvfs.isfileorlink(p):
absf = repo.wjoin(p)
break
kiilerixUnsubmitted
Not Done

This seems quite a bit slower. But I guess it never will happen in tight loops? If we have to backup a lot of files, then we have lost anyway?

kiilerix: This seems quite a bit slower. But I guess it never will happen in tight loops? If we have to…
mbthomasAuthorUnsubmitted
Not Done

It will happen in batchget, which is for each file that is being created in the update. The new loop is O(path-length), which should be small. We should also only be touching things the OS needed to look at anyway to answer the original question, and which we looked at when we audited the path earlier on.

mbthomas: It will happen in `batchget`, which is for each file that is being created in the update. The…
ryanmceUnsubmitted
Not Done

If we have to backup a lot of files, then we have lost anyway?

I think that's essentially the case. The other option here is to always rename conflcting files/directories, but then you end up with unbounded growth in the size of backup files (either in working copy or in some other dir), so that's not really awesome either.

ryanmce: > If we have to backup a lot of files, then we have lost anyway? I think that's essentially…
orig = scmutil.origpath(ui, repo, absf) orig = scmutil.origpath(ui, repo, absf)
try: if repo.wvfs.lexists(absf):
if repo.wvfs.isfileorlink(f):
util.rename(absf, orig) util.rename(absf, orig)
except OSError as e:
if e.errno != errno.ENOENT:
raise
wctx[f].clearunknown() wctx[f].clearunknown()
wctx[f].write(fctx(f).data(), flags, backgroundclose=True) wctx[f].write(fctx(f).data(), flags, backgroundclose=True)
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