This is an archive of the discontinued Mercurial Phabricator instance.

mercurial/context.py
2557	Ugh, haha
2558	Why is this sufficient? Can't the contents be the same even if the paths are different? I think you can only fastpath if the paths are the same, otherwise you have to fall back to data comparison. This is already queued, but I think we need to drop it if I'm right here.

phillco added inline comments.Oct 16 2017, 1:34 PM

mercurial/context.py
2558	Ryan and I talked offline -- but surprisingly, the default `filectx.cmp` function only compares contents: 1:~/1$ repo['.']['A'].cmp(repo['.']['B']) Out[1]: False 2:~/1$ repo['.']['A'].cmp(repo['.']['A']) Out[2]: False 3:~/1$ repo['.']['A'].cmp(repo['.']['C']) Out[3]: True (here, `A` and `B` have the same content). And `filecmp` seems to behave the same way: 7:~/1$ filecmp.cmp('A', 'B') Out[7]: True 8:~/1$ filecmp.cmp('A', 'A') Out[8]: True 9:~/1$ filecmp.cmp('A', 'C') Out[9]: False That doesn't mean that it's wrong, but I think it's consistent.

mercurial/context.py

2558

Ryan and I talked offline -- but surprisingly, the default filectx.cmp function only compares contents:

1:~/1$ repo['.']['A'].cmp(repo['.']['B'])
Out[1]: False

2:~/1$ repo['.']['A'].cmp(repo['.']['A'])
Out[2]: False

3:~/1$ repo['.']['A'].cmp(repo['.']['C'])
Out[3]: True

(here, A and B have the same content).

And filecmp seems to behave the same way:

7:~/1$ filecmp.cmp('A', 'B')
Out[7]: True

8:~/1$ filecmp.cmp('A', 'A')
Out[8]: True

9:~/1$ filecmp.cmp('A', 'C')
Out[9]: False

That doesn't mean that it's wrong, but I think it's consistent.

phillco added inline comments.Oct 16 2017, 2:03 PM

mercurial/context.py
2558	Ah, but some experimenting revealed that `filecmp` does follow symlinks, but our `filectx` comparators do not. Otherwise, both `filecmp` and `filectx.cmp` ignore any flag differences. Thus, you can demonstrate a discrepancy: A contains "foo" real_A contains "A" sym_A link to A repo['.']['real_A'].cmp(repo['.']['sym_A']) # claims the same filecmp.cmp('real_A', 'sym_A') # claims a difference, because "foo" != "A" Note this simpler case doesn't trigger the discrepancy, because the linked file is otherwise identical: A contains "A" sym_A link to A repo['.']['A'].cmp(repo['.']['sym_A']) # claims the same filecmp.cmp('A', 'sym_A') # claims the same The easiest fix is just to skip the fast-comparison path if either side is a symlink, and that's what I'll do unless others have other ideas. (@ryanmce thought we should think about what this API _should_ do).

For now, send a follow-up to not do that fast-path if it's a symlink, and we can reason more carefully about this API during the freeze with an eye towards landing something better in 4.5...

Done: D1122.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/context.py (13 lines)

Status	Author	Revision
Closed	phillco	D1106 context: add workingfilectx.markcopied
Closed	phillco	D1105 merge: don't use workers in in-memory mode
Closed	phillco	D1060 filemerge: add a missing flushall()
Closed	phillco	D1059 filemerge: store backups in the overlayworkingctx if using imm
Closed	phillco	D1058 context: add overlayfilectx.cmp()
Closed	phillco	D1057 filemerge: use arbitraryfilectx for backups
Closed	phillco	D1056 context: add a fast-comparision path between arbitraryfilectx and workingfilectx

Diff 2695

mercurial/context.py

	# context.py - changeset and file context objects for mercurial			# context.py - changeset and file context objects for mercurial
	#			#
	# Copyright 2006, 2007 Matt Mackall <mpm@selenic.com>			# Copyright 2006, 2007 Matt Mackall <mpm@selenic.com>
	#			#
	# This software may be used and distributed according to the terms of the			# This software may be used and distributed according to the terms of the
	# GNU General Public License version 2 or any later version.			# GNU General Public License version 2 or any later version.

	from __future__ import absolute_import			from __future__ import absolute_import

	import errno			import errno
				import filecmp
	import os			import os
	import re			import re
	import stat			import stat

	from .i18n import _			from .i18n import _
	from .node import (			from .node import (
	addednodeid,			addednodeid,
	bin,			bin,
	removed.append(f)			removed.append(f)

	return scmutil.status(modified, added, removed, [], [], [], [])			return scmutil.status(modified, added, removed, [], [], [], [])

	class arbitraryfilectx(object):			class arbitraryfilectx(object):
	"""Allows you to use filectx-like functions on a file in an arbitrary			"""Allows you to use filectx-like functions on a file in an arbitrary
	location on disk, possibly not in the working directory.			location on disk, possibly not in the working directory.
	"""			"""
	def __init__(self, path):			def __init__(self, path, repo=None):
				# Repo is optional because contrib/simplemerge uses this class.
				self._repo = repo
	self._path = path			self._path = path

	def cmp(self, otherfilectx):			def cmp(self, fctx):
	return self.data() != otherfilectx.data()			if isinstance(fctx, workingfilectx) and self._repo:
				# Add a fast-path for merge if both sides are disk-backed.
				# Note that filecmp uses the opposite return values as cmp.
				ryanmceUnsubmitted Not Done Ugh, haha ryanmce: Ugh, haha
				return not filecmp.cmp(self.path(), self._repo.wjoin(fctx.path()))
				ryanmceUnsubmitted Not Done Why is this sufficient? Can't the contents be the same even if the paths are different? I think you can only fastpath if the paths are the same, otherwise you have to fall back to data comparison. This is already queued, but I think we need to drop it if I'm right here. ryanmce: Why is this sufficient? Can't the contents be the same even if the paths are different? I…
				phillcoAuthorUnsubmitted Not Done Ryan and I talked offline -- but surprisingly, the default `filectx.cmp` function only compares contents: 1:~/1$ repo['.']['A'].cmp(repo['.']['B']) Out[1]: False 2:~/1$ repo['.']['A'].cmp(repo['.']['A']) Out[2]: False 3:~/1$ repo['.']['A'].cmp(repo['.']['C']) Out[3]: True (here, `A` and `B` have the same content). And `filecmp` seems to behave the same way: 7:~/1$ filecmp.cmp('A', 'B') Out[7]: True 8:~/1$ filecmp.cmp('A', 'A') Out[8]: True 9:~/1$ filecmp.cmp('A', 'C') Out[9]: False That doesn't mean that it's wrong, but I think it's consistent. phillco: Ryan and I talked offline -- but surprisingly, the default `filectx.cmp` function only compares…
				phillcoAuthorUnsubmitted Not Done Ah, but some experimenting revealed that `filecmp` does follow symlinks, but our `filectx` comparators do not. Otherwise, both `filecmp` and `filectx.cmp` ignore any flag differences. Thus, you can demonstrate a discrepancy: A contains "foo" real_A contains "A" sym_A link to A repo['.']['real_A'].cmp(repo['.']['sym_A']) # claims the same filecmp.cmp('real_A', 'sym_A') # claims a difference, because "foo" != "A" Note this simpler case doesn't trigger the discrepancy, because the linked file is otherwise identical: A contains "A" sym_A link to A repo['.']['A'].cmp(repo['.']['sym_A']) # claims the same filecmp.cmp('A', 'sym_A') # claims the same The easiest fix is just to skip the fast-comparison path if either side is a symlink, and that's what I'll do unless others have other ideas. (@ryanmce thought we should think about what this API _should_ do). phillco: Ah, but some experimenting revealed that `filecmp` does follow symlinks, but our `filectx`…
				return self.data() != fctx.data()

	def path(self):			def path(self):
	return self._path			return self._path

	def flags(self):			def flags(self):
	return ''			return ''

	def data(self):			def data(self):

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	2695		Oct 13 2017, 3:45 PM	★	★
Diff 2	2719	rHG6036e6e205cae997a5745a63316d69597416aca9	Oct 13 2017, 3:40 PM	★	★

This is an archive of the discontinued Mercurial Phabricator instance.

context: add a fast-comparision path between arbitraryfilectx and workingfilectxClosedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 2695

mercurial/context.py

context: add a fast-comparision path between arbitraryfilectx and workingfilectx
ClosedPublic

Revision Contents
Changeset List