Download Raw Diff

Details

Reviewers

ryanmce
durham
yuja
ikostia

Group Reviewers

hg-reviewers

Commits

rHG6fad8059a970: scmutil: handle conflicting files and dirs in origbackuppath
rHGe68deea20e9a: scmutil: handle conflicting files and dirs in origbackuppath

Summary

When ui.origbackuppath is set, .orig files are stored outside of the working
copy. However conflicts can occur when files or directories end up having the
same name. These conflicts cause Mercurial to abort, even if they've been
created as a result of different backups.

Make sure we always replace files or directories in the origbackuppath if
they conflict with another file or directory.

Test Plan

Add new unit test for conflicting paths.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

mbthomas created this revision.Sep 11 2017, 2:03 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptSep 11 2017, 2:03 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

mbthomas added a parent revision: D679: scmutil: don't append .orig to backups in origbackuppath.Sep 11 2017, 2:04 PM

mbthomas mentioned this in D677: scmutil: handle conflicting files and dirs in origbackuppath.

looks good

durham accepted this revision.Sep 12 2017, 12:01 PM

durham added a subscriber: durham.

durham added inline comments.

mercurial/scmutil.py
582	I tried to think of where this could go wrong or be abused. I think it's fine though. Like if someone set ui.origbackuppath to empty string I think the wjoin above would result in paths inside the working copy, and rmtree would then delete parts of the working copy. So maybe the `if origbackuppath is None:` in line 559 should be `if not origbackuppath:`

yuja requested changes to this revision.Sep 13 2017, 10:54 AM

yuja added a subscriber: yuja.

yuja added inline comments.

mercurial/scmutil.py
574	Perhaps fullorigpath may be a Windows path, but util.finddirs() can only handle internal path.
578	It shouldn't delete files out of the origbackuppath directory even if the configured path points to (or under) a file. Maybe we could use a new vfs dedicated to backup directory to audit operations.

This revision now requires changes to proceed.Sep 13 2017, 10:54 AM

mbthomas updated this revision to Diff 1892.Sep 19 2017, 5:46 AM

mbthomas planned changes to this revision.Sep 19 2017, 5:47 AM

mbthomas updated this revision to Diff 1893.Sep 19 2017, 6:17 AM

mbthomas updated this revision to Diff 1894.

mbthomas marked 2 inline comments as done.Sep 19 2017, 6:21 AM

mbthomas added inline comments.

mercurial/scmutil.py
574	I've changed it to use `util.normpath`, which I think converts back to `/` as separator on windows (`windows.normpath` calls `windows.pconvert`), and is probably a good idea anyway. Is this sufficient?

yuja added inline comments.Sep 19 2017, 11:12 AM

mercurial/scmutil.py
574	That's probably okay now. I'm not sure if it conflicts with the long paths plan. IIRC, forward slash isn't allowed in `\\?\` path.

Calling in windows path expert @ikostia

Please wait until the util.finddirs() is tweaked and remove normpath call.

mercurial/scmutil.py
574	I think we should adjust `finddirs` to split on `os.path.sep`, not hardcoded `/`, then we can get rid of `util.normpath` a couple of lines above. I am long overdue with re-sending my series, I will add this change there as well.

This revision now requires changes to proceed.Sep 19 2017, 11:36 AM

Here's a patch: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-September/104837.html

mbthomas added inline comments.Sep 20 2017, 9:12 AM

mercurial/scmutil.py
574	At this point the path is a vfs-path within the repo, so it should be using `/` as a separator. In the case of UNC paths, `os.path.relpath` seems to do the right thing, so `os.path.relpath(r'\\?\c:\repo\dir\file', r'\\?\c:\repo')` gives `dir\file`, which `util.normpath` converts to `dir/file`. It's a bit of a shame that `scmutil.origpath` takes an absolute path rather than a vfs path within the repo, as it means we have to jump through these hoops. It doesn't look like we can easily change it, though.

mbthomas added a child revision: D773: tests: add a test demonstrating basic path conflict failures.Sep 22 2017, 5:27 AM

If it's just file and directory conflict, maybe we can encode the path so it becomes impossible to have such conflict. See _encodedir in store.py for an example. It requires a suffix so partially reverts D679 though.

mbthomas updated this revision to Diff 2208.Oct 1 2017, 5:32 AM

Just as an FYI: I revoke my previous comments, as stuff started working for non-prefixed paths and I no longer intend to land anything related to this.

I am concerned that this seems to rely even more on not having conflicts inside origbackuppath from files/dirs with same name but in different directories. If we have a backup file handling, it must be 100% reliable. That seems to be what this series is trying to fix - then it seems unfortunate it just moves the problem elsewhere.

mercurial/scmutil.py
561	Now, this seems like a separate trivial change, fixing the problem that origbackuppath couldn't be overridden.

With ui.origbackuppath set to some value, backup should be completely reliable. The file at $origbackuppath/path/to/file will be the most recent version of that file that was backed up. It will be overwritten/deleted if a backup of any file named $origbackuppath/path/to/file, $origbackuppath/path/to or $origbackuppath/path is subsequently made. This seems reasonable to me.

If ui.origbackuppath not set, then behaviour is still unreliable in the presence of files also named something.orig, but that is not changed behaviour. We discussed this at the sprint and decided that we didn't want to change the default behaviour of creating .orig files.

mercurial/scmutil.py
561	Happy to split this out, but this stack is already pretty tall.

mbthomas updated this revision to Diff 2350.Oct 2 2017, 5:14 PM

It looks like all concerns have been addressed. This looks good to me.

mercurial/scmutil.py
567–570	In the future, we may want to move this to the repo class like other `vfs` instances. That will mean we don't have to re-instantiate this each time. However, these instantiations look cheap so I'm not worried and it's outside the scope of this change.
574	I agree that it seems out of scope to fix this in this patch series. Might be worth opening a task in bugzilla to track improving this, but I think this series is already fairly long and too important to hold up on a small thing like this. Since it works, I think we should move towards shipping it.
578	@mbthomas and I had talked about adding a new vfs; thanks for pushing us in that direction. Getting a dedicated vfs feels like a good win here.

Closed by commit rHGe68deea20e9a: scmutil: handle conflicting files and dirs in origbackuppath (authored by mbthomas). · Explain WhyOct 9 2017, 9:25 PM

This revision was automatically updated to reflect the committed changes.

			Path	Packages
M			mercurial/scmutil.py (33 lines)
A	M		tests/test-origbackup-conflict.t (118 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	1721		Sep 11 2017, 2:03 PM	★	★
Diff 2	1892		Sep 19 2017, 5:46 AM	★	★
Diff 3	1893		Sep 19 2017, 6:17 AM	★	★
Diff 4	1894		Sep 19 2017, 6:19 AM	★	★
Diff 5	2208		Oct 1 2017, 5:32 AM	★	★
Diff 6	2350		Oct 2 2017, 5:14 PM	★	★
Diff 7	2543	rHGe68deea20e9a5052eff94be3fa672965d9a24ead	Oct 2 2017, 5:05 PM	★	★

Status	Author	Revision
Closed	mbthomas	D955 merge: improve comments in mergestate._makerecords
Closed	mbthomas	D861 commands: tidy up merge state decoding in resolve
Closed	mbthomas	D787 tests: add test for path conflicts during merge
Closed	mbthomas	D786 tests: add test for path conflicts during update
Closed	mbthomas	D785 context: also consider path conflicts when clearing unknown files
Closed	mbthomas	D784 merge: check for path conflicts when merging (issue5628)
Closed	mbthomas	D783 util: add safename function for generating safe names to rename to
Closed	mbthomas	D782 merge: improve error messages for path conflicts during update
Closed	mbthomas	D781 merge: check for path conflicts when updating (issue5628)
Closed	mbthomas	D780 merge: rename conflicts to fileconflicts in _checkunknownfiles
Closed	mbthomas	D779 merge: add _checkunknowndirs function for detecting path conflicts
Closed	mbthomas	D778 merge: backup conflicting directories when getting files
Closed	mbthomas	D777 merge: add merge action 'pr' to rename files during update
Closed	mbthomas	D776 merge: add merge action 'p' to record path conflicts during update
Closed	mbthomas	D775 commands: update the resolve command to handle path conflicts
Closed	mbthomas	D774 merge: add pathconflict merge state
Closed	mbthomas	D773 tests: add a test demonstrating basic path conflict failures
Closed	mbthomas	D680 scmutil: handle conflicting files and dirs in origbackuppath
Closed	mbthomas	D679 scmutil: don't append .orig to backups in origbackuppath

Diff 1894

mercurial/scmutil.py

	obsolete,			obsolete,
	obsutil,			obsutil,
	pathutil,			pathutil,
	phases,			phases,
	pycompat,			pycompat,
	revsetlang,			revsetlang,
	similar,			similar,
	util,			util,
				vfs,
	)			)

	if pycompat.osname == 'nt':			if pycompat.osname == 'nt':
	from . import scmwindows as scmplatform			from . import scmwindows as scmplatform
	else:			else:
	from . import scmposix as scmplatform			from . import scmposix as scmplatform

	termsize = scmplatform.termsize			termsize = scmplatform.termsize

	def origpath(ui, repo, filepath):			def origpath(ui, repo, filepath):
	'''customize where .orig files are created			'''customize where .orig files are created

	Fetch user defined path from config file: [ui] origbackuppath = <path>			Fetch user defined path from config file: [ui] origbackuppath = <path>
	Fall back to default (filepath with .orig suffix) if not specified			Fall back to default (filepath with .orig suffix) if not specified
	'''			'''
	origbackuppath = ui.config('ui', 'origbackuppath')			origbackuppath = ui.config('ui', 'origbackuppath')
	if origbackuppath is None:			if not origbackuppath:
	return filepath + ".orig"			return filepath + ".orig"
				kiilerixUnsubmitted Not Done Now, this seems like a separate trivial change, fixing the problem that origbackuppath couldn't be overridden. kiilerix: Now, this seems like a separate trivial change, fixing the problem that origbackuppath couldn't…
				mbthomasAuthorUnsubmitted Not Done Happy to split this out, but this stack is already pretty tall. mbthomas: Happy to split this out, but this stack is already pretty tall.

	filepathfromroot = os.path.relpath(filepath, start=repo.root)			# Convert filepath from an absolute path into a path inside the repo.
	fullorigpath = repo.wjoin(origbackuppath, filepathfromroot)			filepathfromroot = util.normpath(os.path.relpath(filepath,
				start=repo.root))

				origvfs = vfs.vfs(repo.wjoin(origbackuppath))
				origbackupdir = origvfs.dirname(filepathfromroot)
				if not origvfs.isdir(origbackupdir) or origvfs.islink(origbackupdir):
				ui.note(_('creating directory: %s\n') % origvfs.join(origbackupdir))
				ryanmceUnsubmitted Not Done In the future, we may want to move this to the repo class like other `vfs` instances. That will mean we don't have to re-instantiate this each time. However, these instantiations look cheap so I'm not worried and it's outside the scope of this change. ryanmce: In the future, we may want to move this to the repo class like other `vfs` instances. That will…

				# Remove any files that conflict with the backup file's path
				for f in reversed(list(util.finddirs(filepathfromroot))):
				if origvfs.isfileorlink(f):
				yujaUnsubmitted Not Done Perhaps fullorigpath may be a Windows path, but util.finddirs() can only handle internal path. yuja: Perhaps fullorigpath may be a Windows path, but util.finddirs() can only handle internal path.
				mbthomasAuthorUnsubmitted Not Done I've changed it to use `util.normpath`, which I think converts back to `/` as separator on windows (`windows.normpath` calls `windows.pconvert`), and is probably a good idea anyway. Is this sufficient? mbthomas: I've changed it to use `util.normpath`, which I think converts back to `/` as separator on…
				yujaUnsubmitted Not Done That's probably okay now. I'm not sure if it conflicts with the long paths plan. IIRC, forward slash isn't allowed in `\\?\` path. yuja: That's probably okay now. I'm not sure if it conflicts with the long paths plan. IIRC, forward…
				ikostiaUnsubmitted Not Done I think we should adjust `finddirs` to split on `os.path.sep`, not hardcoded `/`, then we can get rid of `util.normpath` a couple of lines above. I am long overdue with re-sending my series, I will add this change there as well. ikostia: I think we should adjust `finddirs` to split on `os.path.sep`, not hardcoded `/`, then we can…
				mbthomasAuthorUnsubmitted Not Done At this point the path is a vfs-path within the repo, so it should be using `/` as a separator. In the case of UNC paths, `os.path.relpath` seems to do the right thing, so `os.path.relpath(r'\\?\c:\repo\dir\file', r'\\?\c:\repo')` gives `dir\file`, which `util.normpath` converts to `dir/file`. It's a bit of a shame that `scmutil.origpath` takes an absolute path rather than a vfs path within the repo, as it means we have to jump through these hoops. It doesn't look like we can easily change it, though. mbthomas: At this point the path is a vfs-path within the repo, so it should be using `/` as a separator.
				ryanmceUnsubmitted Not Done I agree that it seems out of scope to fix this in this patch series. Might be worth opening a task in bugzilla to track improving this, but I think this series is already fairly long and too important to hold up on a small thing like this. Since it works, I think we should move towards shipping it. ryanmce: I agree that it seems out of scope to fix this in this patch series. Might be worth opening a…
				ui.note(_('removing conflicting file: %s\n')
				% origvfs.join(f))
				origvfs.unlink(f)
				break
				yujaUnsubmitted Done It shouldn't delete files out of the origbackuppath directory even if the configured path points to (or under) a file. Maybe we could use a new vfs dedicated to backup directory to audit operations. yuja: It shouldn't delete files out of the origbackuppath directory even if the configured path…
				ryanmceUnsubmitted Not Done @mbthomas and I had talked about adding a new vfs; thanks for pushing us in that direction. Getting a dedicated vfs feels like a good win here. ryanmce: @mbthomas and I had talked about adding a new vfs; thanks for pushing us in that direction.

				origvfs.makedirs(origbackupdir)

	origbackupdir = repo.vfs.dirname(fullorigpath)			if origvfs.isdir(filepathfromroot):
				durhamUnsubmitted Done I tried to think of where this could go wrong or be abused. I think it's fine though. Like if someone set ui.origbackuppath to empty string I think the wjoin above would result in paths inside the working copy, and rmtree would then delete parts of the working copy. So maybe the `if origbackuppath is None:` in line 559 should be `if not origbackuppath:` durham: I tried to think of where this could go wrong or be abused. I //think// it's fine though. Like…
	if not repo.vfs.exists(origbackupdir):			ui.note(_('removing conflicting directory: %s\n')
	ui.note(_('creating directory: %s\n') % origbackupdir)			% origvfs.join(filepathfromroot))
	util.makedirs(origbackupdir)			origvfs.rmtree(filepathfromroot, forcibly=True)

	return fullorigpath			return origvfs.join(filepathfromroot)

	class _containsnode(object):			class _containsnode(object):
	"""proxy __contains__(node) to container.__contains__ which accepts revs"""			"""proxy __contains__(node) to container.__contains__ which accepts revs"""

	def __init__(self, repo, revcontainer):			def __init__(self, repo, revcontainer):
	self._torev = repo.changelog.rev			self._torev = repo.changelog.rev
	self._revcontains = revcontainer.__contains__			self._revcontains = revcontainer.__contains__

tests/test-origbackup-conflict.t

This file was added.

				Set up repo

				$ cat << EOF >> $HGRCPATH
				> [ui]
				> origbackuppath=.hg/origbackups
				> [merge]
				> checkunknown=warn
				> EOF
				$ hg init repo
				$ cd repo
				$ echo base > base
				$ hg add base
				$ hg commit -m "base"

				Make a dir named b that contains a file

				$ mkdir -p b
				$ echo c1 > b/c
				$ hg add b/c
				$ hg commit -m "c1"
				$ hg bookmark c1

				Peform an update that causes b/c to be backed up

				$ hg up -q 0
				$ mkdir -p b
				$ echo c2 > b/c
				$ hg up --verbose c1
				resolving manifests
				b/c: replacing untracked file
				getting b/c
				creating directory: $TESTTMP/repo/.hg/origbackups/b
				1 files updated, 0 files merged, 0 files removed, 0 files unresolved
				(activating bookmark c1)
				$ test -f .hg/origbackups/b/c

				Make a file named b

				$ hg up -q 0
				$ echo b1 > b
				$ hg add b
				$ hg commit -m b1
				created new head
				$ hg bookmark b1

				Perform an update that causes b to be backed up - it should replace the backup b dir

				$ hg up -q 0
				$ echo b2 > b
				$ hg up --verbose b1
				resolving manifests
				b: replacing untracked file
				getting b
				removing conflicting directory: $TESTTMP/repo/.hg/origbackups/b
				1 files updated, 0 files merged, 0 files removed, 0 files unresolved
				(activating bookmark b1)
				$ test -f .hg/origbackups/b

				Perform an update the causes b/c to be backed up again - it should replace the backup b file

				$ hg up -q 0
				$ mkdir b
				$ echo c3 > b/c
				$ hg up --verbose c1
				resolving manifests
				b/c: replacing untracked file
				getting b/c
				creating directory: $TESTTMP/repo/.hg/origbackups/b
				removing conflicting file: $TESTTMP/repo/.hg/origbackups/b
				1 files updated, 0 files merged, 0 files removed, 0 files unresolved
				(activating bookmark c1)
				$ test -d .hg/origbackups/b

				Cause a symlink to be backed up that points to a valid location from the backup dir

				$ hg up -q 0
				$ mkdir ../sym-link-target
				$ ln -s ../../../sym-link-target b
				$ hg up b1
				b: replacing untracked file
				1 files updated, 0 files merged, 0 files removed, 0 files unresolved
				(activating bookmark b1)
				$ readlink .hg/origbackups/b
				../../../sym-link-target

				Perform an update that causes b/c to be backed up again - it should not go into the target dir

				$ hg up -q 0
				$ mkdir b
				$ echo c4 > b/c
				$ hg up --verbose c1
				resolving manifests
				b/c: replacing untracked file
				getting b/c
				creating directory: $TESTTMP/repo/.hg/origbackups/b
				removing conflicting file: $TESTTMP/repo/.hg/origbackups/b
				1 files updated, 0 files merged, 0 files removed, 0 files unresolved
				(activating bookmark c1)
				$ cat .hg/origbackups/b/c
				c4
				$ ls ../sym-link-target

				Incorrectly configure origbackuppath to be under a file

				$ echo data > .hg/badorigbackups
				$ hg up -q 0
				$ mkdir b
				$ echo c5 > b/c
				$ hg up --verbose c1 --config ui.origbackuppath=.hg/badorigbackups
				resolving manifests
				b/c: replacing untracked file
				getting b/c
				creating directory: $TESTTMP/repo/.hg/badorigbackups/b
				abort: Not a directory: '$TESTTMP/repo/.hg/badorigbackups/b'
				[255]
				$ cat .hg/badorigbackups
				data

This is an archive of the discontinued Mercurial Phabricator instance.

scmutil: handle conflicting files and dirs in origbackuppath
ClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1894

mercurial/scmutil.py

tests/test-origbackup-conflict.t

This is an archive of the discontinued Mercurial Phabricator instance.

scmutil: handle conflicting files and dirs in origbackuppathClosedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1894

mercurial/scmutil.py

tests/test-origbackup-conflict.t

scmutil: handle conflicting files and dirs in origbackuppath
ClosedPublic

Revision Contents
Changeset List