( )⚙ D541 effectflag: detect when diff changed

This is an archive of the discontinued Mercurial Phabricator instance.

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 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

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 commandeered this revision.
durin42 abandoned this revision.

(Marking as abandoned because this is landed.)

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.