Unified diff of changeset sometimes include wrong changes?

[anonymous]

For example the changesets before the merge log:mercurial-plugin@17:

• changeset:16/mercurial-plugin: The Unified Diff there looks correct.
• changeset:15/mercurial-plugin: The Unified Diff there looks correct.
• changeset:14/mercurial-plugin: The Unified Diff includes a lot more changes than it should!

Another example is before the merge log:mercurial-plugin@46:

• changeset:45/mercurial-plugin: The Unified Diff again includes a lot more changes than it should.

[Jun Omae]

It seems that MercurialNode.get_previous() with @14 wrongly returns 13. It should return 11. Unified diff for 14/mercurial-plugin should be changes between 11 and 14.

[Peter Suter]

Right. Here's a unittest showing the issue:

tracext/hg/tests/backend.py

@@ -473 +473 @@
         self.assertEqual('delete', delete[3])
         self.assertEqual('lunch/lasagna/pasta.txt', delete[0].path)
 
+    def test_get_previous(self):
+        # regression test for #13092
+        self._hg_init(data=False)
+        self._add_repository()
+        repos = self._repomgr.get_repository('hgrepos')
+        self._create_commit('lunch', 'lunch/menu.txt')
+        self._create_commit('pasta', 'lunch/lasagna/pasta.txt')
+        self._hg_update('0')
+        self._create_commit('fish', 'lunch/fish/chips.txt')
+        self._create_commit('coffee', 'lunch/fish/drink.txt', content='coffee')
+        self._hg_update('1')
+        self._create_commit('coke', 'lunch/lasagna/drink.txt', content='coke')
+        repos.repo.invalidate()
+
+        node = repos.get_node('/', 4)
+        prev_rev = node.get_previous()[1]
+        self.assertEqual(repos.display_rev(1), repos.display_rev(prev_rev))
+
 
 def test_suite():
     suite = unittest.TestSuite()

I assume get_previous()​ using get_history() is ok. So probably get_history()​ isn't right somehow. I'm guessing %s:0 is a ​revset x:y but what is needed is a x::y, but for some reason it that x>y doesn't work, so reverse(y::x) must be used instead:

tracext/hg/backend.py

@@ -1167 +1167 @@
         pats = []
         if self.str_path:
             pats.append('path:' + self.str_path)
-        opts = {'rev': ['%s:0' % self.changectx.hex()]}
+        opts = {'rev': ['reverse(0::%s)' % self.changectx.hex()]}
         if self.isfile and self.repos.version_info < (2, 1, 1):
             opts['follow'] = True
         if arity(cmdutil.walkchangerevs) == 4:

This makes the unittest above pass at least.

[Jun Omae]

attachment:t13092.diff​

I consider the get_history() should walk parents of the commit and your changes are right, but log view for mercurial repository would show only ancestors of the given revision after the changes. Currently, the log view shows all branches.

[Peter Suter]

Good point! I would not like changing the log to show only ancestors.

(Do Git repositories only show ancestors in the log? Is that intentional?)

[Jun Omae]

Replying to Peter Suter:

Good point! I would not like changing the log to show only ancestors.

(Do Git repositories only show ancestors in the log? Is that intentional?)

Yes. That behavior on git repository is intentional. On git repository, determining previous revision is cost too much since no integer revisions.

I think it would be good that the log view shows ancestors of the given revision or branch by default and add an option to show all branches without the given revision.

[Peter Suter]

Have you considered if the bug is in using repos.get_node(new_path, new).get_previous()​ for the old_rev of a changeset, instead of e.g. repos.previous_rev(new, new_path)? That seems to give the "correct" changeset.

(For Git: Maybe Git 2.18+ ​commit-graph feature would be useful? It sounds somewhat similar.)

[Jun Omae]

Replying to Peter Suter:

Have you considered if the bug is in using repos.get_node(new_path, new).get_previous()​ for the old_rev of a changeset, instead of e.g. repos.previous_rev(new, new_path)? That seems to give the "correct" changeset.

The Node.get_previous() for Subversion is absolutely clear and correct. However, it is ambiguous for non-linear repository (e.g. Git, Mercurial). I consider it should always return an ancestor of the node.

(For Git: Maybe Git 2.18+ ​commit-graph feature would be useful? It sounds somewhat similar.)

Not useful. We cannot read commit-graph cache because it is binary. We still have to construct the graph and sorting of revisions using git rev-list.

$ file .git/objects/info/commit-graph
.git/objects/info/commit-graph: data
$ git commit-graph read
header: 43475048 1 1 4 0
num_commits: 19368
chunks: oid_fanout oid_lookup commit_metadata large_edges