( )⚙ D541 effectflag: detect when diff changed

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D541

effectflag: detect when diff changed
AbandonedPublic

Authored by durin42 on Aug 28 2017, 7:03 AM.

Details

Reviewers
lothiraldan
Group Reviewers
hg-reviewers
Summary

Store in effect flag when the diff changed between the predecessor and
its successors.

Comparing the diff is not easy because we do not want to incorrectly detect a
diff modification when the changeset has only been rebased.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lothiraldan created this revision.Aug 28 2017, 7:03 AM
lothiraldan updated this revision to Diff 2112.Sep 27 2017, 4:08 AM
martinvonz added a subscriber: martinvonz.Sep 30 2017, 12:54 PM
martinvonz added inline comments.
mercurial/obsutil.py
341

I think this should be "cleanhunk"

360

I see this as an argument against storing it in the obsmarker. Can we remove all shortcomings before it's no longer experimental?

But, as I said before, the feature is experimental, so we have some time to decide whether to calculate it at obsmarker creation time or on the fly (possibly with caching).

370–377

I haven't tested this, but I think the following would remove the need for _getdifflines():

leftiter = itertools.chain(leftdiff, itertools.repeat(None))
rightiter = itertools.chain(rightdiff, itertools.repeat(None))
while True:
    left, right = next(leftiter), next(rightiter)
    if left is None and right is None:
        return True
    if left != right:
        return False
lothiraldan updated this revision to Diff 2258.Oct 1 2017, 7:52 AM
lothiraldan marked 3 inline comments as done.Oct 1 2017, 7:59 AM
lothiraldan added inline comments.
mercurial/obsutil.py
360

Yes we should be able to remove all shortcomings before it's no longer experimental. I think we should have this diff of diff algorithm somewhere else in core, I think they would be other users of the algorithm, maybe phabsend?

370–377

No that doesn't work because if you rebase a changeset, context would change and the diff would be detected as different, that's why we call _prepare_hunk

martinvonz added inline comments.Oct 1 2017, 9:20 AM
mercurial/obsutil.py
370–377

Oops, I didn't mean to drop _prepare_hunk, I just don't like StopIteration and wanted to make _getdifffiles() a generator instead and simplified a bit too much. The following would probably work, but maybe StopIteration is fine and should get used it :-)

def _getdifflines(iterdiff):
    """return a cleaned up lines"""
    for lines in iterdiff:
        yield _prepare_hunk(lines)
    while True:
        yield None

leftiter = _getdifflines(leftdiff)
rightiter = _getdifflines(rightdiff)
while True:
    left, right = next(leftiter), next(rightiter)
    if (left, right) == (None, None):
        return True
    if left != right:
        return False
lothiraldan marked 2 inline comments as done.Oct 1 2017, 9:44 AM
lothiraldan updated this revision to Diff 2287.
lothiraldan marked an inline comment as done.Oct 1 2017, 9:46 AM
lothiraldan added inline comments.
mercurial/obsutil.py
370–377

I've updated the patch with using next with a default value, tell me if it's better.

lothiraldan marked an inline comment as done.Oct 4 2017, 9:11 AM
lothiraldan added a subscriber: quark.

This patch seems to have been queued (https://www.mercurial-scm.org/repo/hg/rev/187bc224554a), it's weird that Phabricator doesn't close it. Ping Phabricator expert @quark

quark added a comment.Oct 4 2017, 12:03 PM
In D541#15503, @lothiraldan wrote:

This patch seems to have been queued (https://www.mercurial-scm.org/repo/hg/rev/187bc224554a), it's weird that Phabricator doesn't close it. Ping Phabricator expert @quark

The commit message looks incomplete - it should have a "Differential Revision" line. Without that line, Phabricator cannot detect which commit is associated with which revision.

durin42 added a reviewer: lothiraldan.Oct 4 2017, 2:34 PM
durin42 commandeered this revision.
durin42 abandoned this revision.

(Marking as abandoned because this is landed.)

lothiraldan added a comment.Oct 5 2017, 2:24 AM

Thank you for the explanation, I might have deleted the "Differential Revision" line while changing the changeset message, I will be more careful the next time.

Revision Contents
Changeset List

Diff 2112

mercurial/obsutil.py

foreground = set(repo.set('%ln::', known)) foreground = set(repo.set('%ln::', known))
return set(c.node() for c in foreground) return set(c.node() for c in foreground)
# logic around storing and using effect flags # logic around storing and using effect flags
EFFECTFLAGFIELD = "ef1" EFFECTFLAGFIELD = "ef1"
DESCCHANGED = 1 << 0 # action changed the description DESCCHANGED = 1 << 0 # action changed the description
METACHANGED = 1 << 1 # action change the meta METACHANGED = 1 << 1 # action change the meta
DIFFCHANGED = 1 << 3 # action change diff introduced by the changeset
PARENTCHANGED = 1 << 2 # action change the parent PARENTCHANGED = 1 << 2 # action change the parent
USERCHANGED = 1 << 4 # the user changed USERCHANGED = 1 << 4 # the user changed
DATECHANGED = 1 << 5 # the date changed DATECHANGED = 1 << 5 # the date changed
BRANCHCHANGED = 1 << 6 # the branch changed BRANCHCHANGED = 1 << 6 # the branch changed
METABLACKLIST = [ METABLACKLIST = [
re.compile('^branch$'), re.compile('^branch$'),
re.compile('^.*-source$'), re.compile('^.*-source$'),
re.compile('^.*_source$'), re.compile('^.*_source$'),
re.compile('^source$'), re.compile('^source$'),
] ]
def ismetablacklisted(metaitem): def ismetablacklisted(metaitem):
""" Check that the key of a meta item (extrakey, extravalue) does not """ Check that the key of a meta item (extrakey, extravalue) does not
match at least one of the blacklist pattern match at least one of the blacklist pattern
""" """
metakey = metaitem[0] metakey = metaitem[0]
for pattern in METABLACKLIST: for pattern in METABLACKLIST:
if pattern.match(metakey): if pattern.match(metakey):
return False return False
return True return True
def _prepare_hunk(hunk):
"""Drop all information but the username and patch"""
cleanunk = []
martinvonzUnsubmitted
Done

I think this should be "cleanhunk"

martinvonz: I think this should be "cleanhunk"
for line in hunk.splitlines():
if line.startswith(b'# User') or not line.startswith(b'#'):
if line.startswith(b'@@'):
line = b'@@\n'
cleanunk.append(line)
return cleanunk
def _getdifflines(iterdiff):
"""return a cleaned up lines"""
try:
lines = next(iterdiff)
except StopIteration:
return None
return _prepare_hunk(lines)
def _cmpdiff(leftctx, rightctx):
"""return True if both ctx introduce the "same diff"
This is a first and basic implementation, with many shortcoming.
martinvonzUnsubmitted
Done

I see this as an argument against storing it in the obsmarker. Can we remove all shortcomings before it's no longer experimental?

But, as I said before, the feature is experimental, so we have some time to decide whether to calculate it at obsmarker creation time or on the fly (possibly with caching).

martinvonz: I see this as an argument against storing it in the obsmarker. Can we remove all shortcomings…
lothiraldanUnsubmitted
Not Done

Yes we should be able to remove all shortcomings before it's no longer experimental. I think we should have this diff of diff algorithm somewhere else in core, I think they would be other users of the algorithm, maybe phabsend?

lothiraldan: Yes we should be able to remove all shortcomings before it's no longer experimental. I think we…
"""
# Leftctx or right ctx might be filtered, so we need to use the contexts
# with an unfiltered repository to safely compute the diff
leftunfi = leftctx._repo.unfiltered()[leftctx.rev()]
leftdiff = leftunfi.diff(git=1)
rightunfi = rightctx._repo.unfiltered()[rightctx.rev()]
rightdiff = rightunfi.diff(git=1)
left, right = (0, 0)
while None not in (left, right):
left = _getdifflines(leftdiff)
right = _getdifflines(rightdiff)
if left != right:
return False
return True
martinvonzUnsubmitted
Done

I haven't tested this, but I think the following would remove the need for _getdifflines():

leftiter = itertools.chain(leftdiff, itertools.repeat(None))
rightiter = itertools.chain(rightdiff, itertools.repeat(None))
while True:
    left, right = next(leftiter), next(rightiter)
    if left is None and right is None:
        return True
    if left != right:
        return False
martinvonz: I haven't tested this, but I think the following would remove the need for _getdifflines()…
lothiraldanUnsubmitted
Not Done

No that doesn't work because if you rebase a changeset, context would change and the diff would be detected as different, that's why we call _prepare_hunk

lothiraldan: No that doesn't work because if you rebase a changeset, context would change and the diff would…
martinvonzUnsubmitted
Done

Oops, I didn't mean to drop _prepare_hunk, I just don't like StopIteration and wanted to make _getdifffiles() a generator instead and simplified a bit too much. The following would probably work, but maybe StopIteration is fine and should get used it :-)

def _getdifflines(iterdiff):
    """return a cleaned up lines"""
    for lines in iterdiff:
        yield _prepare_hunk(lines)
    while True:
        yield None

leftiter = _getdifflines(leftdiff)
rightiter = _getdifflines(rightdiff)
while True:
    left, right = next(leftiter), next(rightiter)
    if (left, right) == (None, None):
        return True
    if left != right:
        return False
martinvonz: Oops, I didn't mean to drop _prepare_hunk, I just don't like StopIteration and wanted to make…
lothiraldanUnsubmitted
Not Done

I've updated the patch with using next with a default value, tell me if it's better.

lothiraldan: I've updated the patch with using next with a default value, tell me if it's better.
def geteffectflag(relation): def geteffectflag(relation):
""" From an obs-marker relation, compute what changed between the """ From an obs-marker relation, compute what changed between the
predecessor and the successor. predecessor and the successor.
""" """
effects = 0 effects = 0
source = relation[0] source = relation[0]
ctxmeta = filter(ismetablacklisted, changeextra) ctxmeta = filter(ismetablacklisted, changeextra)
sourceextra = source.extra().items() sourceextra = source.extra().items()
srcmeta = filter(ismetablacklisted, sourceextra) srcmeta = filter(ismetablacklisted, sourceextra)
if ctxmeta != srcmeta: if ctxmeta != srcmeta:
effects |= METACHANGED effects |= METACHANGED
# Check if the diff has changed
if not _cmpdiff(source, changectx):
effects |= DIFFCHANGED
return effects return effects
def getobsoleted(repo, tr): def getobsoleted(repo, tr):
"""return the set of pre-existing revisions obsoleted by a transaction""" """return the set of pre-existing revisions obsoleted by a transaction"""
torev = repo.unfiltered().changelog.nodemap.get torev = repo.unfiltered().changelog.nodemap.get
phase = repo._phasecache.phase phase = repo._phasecache.phase
succsmarkers = repo.obsstore.successors.get succsmarkers = repo.obsstore.successors.get
public = phases.public public = phases.public

tests/test-obsmarkers-effectflag.t

$ mkcommit E0 $ mkcommit E0
$ echo 42 >> E0 $ echo 42 >> E0
$ hg commit --amend $ hg commit --amend
check result check result
$ hg debugobsolete --rev . $ hg debugobsolete --rev .
ebfe0333e0d96f68a917afd97c0a0af87f1c3b5f 75781fdbdbf58a987516b00c980bccda1e9ae588 0 (Thu Jan 01 00:00:00 1970 +0000) {'ef1': '0', 'operation': 'amend', 'user': 'test'} ebfe0333e0d96f68a917afd97c0a0af87f1c3b5f 75781fdbdbf58a987516b00c980bccda1e9ae588 0 (Thu Jan 01 00:00:00 1970 +0000) {'ef1': '8', 'operation': 'amend', 'user': 'test'}
amend with multiple effect (desc and meta) amend with multiple effect (desc and meta)
------------------------------------------- -------------------------------------------
$ mkcommit F0 $ mkcommit F0
$ hg branch my-other-branch $ hg branch my-other-branch
marked working directory as branch my-other-branch marked working directory as branch my-other-branch
$ hg commit --amend -m F1 -u "bob <bob@bob.com>" -d "42 0" $ hg commit --amend -m F1 -u "bob <bob@bob.com>" -d "42 0"