Manuel,
I investigated this a bit further.
Your patch points out some problems in the code.
I missed some points in my initial reading of the patch:
- At the level of the get_changesets function, the problematic case is
when combining start/end filters with other filters such as branch
filtering. But I'm not aware of any way to unintentionally expose any
actual problem.
- The mix between different "kinds" of indices is entirely inside this
function.
- The problem is not tied to hidden changesets. The problem is seen when
filtering by branch.
- Mercurial revision numbers is not so much the problem here. The
problem is in whether we are using indices in self.revisions (which has
all visible revisions) or revrange (which might be filtered by branch,
and thus only have some of the visible revisions).
I think the patch is correct, but the code remains quite unelegant. The
todo is kept (and moved around ... even though the old position seemed
better). The added list comprehension for converting revrange to raw_id
hashes seems a bit like a separate initial bugfix step, but it also
seems to be potentially expensive ... and it can be avoided.
Alternatively, how about
https://kallithea-scm.org/repos/kallithea-incoming/changeset/34e66c7feb5dcde3adc32d5bacf94306d2ab99d8
? Perhaps a bit invasive for the stable branch, but with the slow pace
of Kallithea, it might be for the right place to put it. What's your
thoughts on that? Do you agree with my analysis, here and in the commit
message? And does it solve your problem?
But most important: What is the actual problem you experienced with
this? What problem are we trying to solve?
/Mads
On 2/26/22 05:06, Manuel Jacob wrote:
# HG changeset patch
# User Manuel Jacob <[email protected]>
# Date 1645848272 -3600
# Sat Feb 26 05:04:32 2022 +0100
# Branch stable
# Node ID 84c94153376698aa028c03dbad1fc0dcf6f081ed
# Parent 397f73d1cdd4b39c9c17bb8d45592e866fcab88c
hg: use correct start / stop indices when slicing revisions
Previously, the indices where determined on self.revisions, which contains all
visible revisions in the repository. However, in some cases, these indices were
used on a list which had the items at different positions.
With this change, the indices are always determined on the list which is sliced
with these indices.
I didn’t fully understand the comment that I moved. If it doesn’t make sense to
move it, I’d happy to send a new patch series removing or changing the comment
before or after this change.
diff --git a/kallithea/lib/vcs/backends/hg/repository.py
b/kallithea/lib/vcs/backends/hg/repository.py
--- a/kallithea/lib/vcs/backends/hg/repository.py
+++ b/kallithea/lib/vcs/backends/hg/repository.py
@@ -525,20 +525,9 @@
:param branch_name:
:param reversed: return changesets in reversed order
"""
- start_raw_id = self._get_revision(start)
- start_pos = None if start is None else
self.revisions.index(start_raw_id)
- end_raw_id = self._get_revision(end)
- end_pos = None if end is None else self.revisions.index(end_raw_id)
-
- if start_pos is not None and end_pos is not None and start_pos >
end_pos:
- raise RepositoryError("Start revision '%s' cannot be "
- "after end revision '%s'" % (start, end))
-
if branch_name and branch_name not in self.allbranches:
msg = "Branch %r not found in %s" % (branch_name, self.name)
raise BranchDoesNotExistError(msg)
- if end_pos is not None:
- end_pos += 1
# filter branches
filter_ = []
if branch_name:
@@ -554,13 +543,22 @@
revspec = b'all()'
if max_revisions:
revspec = b'limit(%s, %d)' % (revspec, max_revisions)
- revisions = mercurial.scmutil.revrange(self._repo, [revspec])
+ # this is very much a hack to turn this into a list; a better
solution
+ # would be to get rid of this function entirely and use revsets
+ revisions = [self._get_revision(r) for r in
mercurial.scmutil.revrange(self._repo, [revspec])]
else:
revisions = self.revisions
- # this is very much a hack to turn this into a list; a better solution
- # would be to get rid of this function entirely and use revsets
- revs = list(revisions)[start_pos:end_pos]
+ start_raw_id = self._get_revision(start)
+ start_pos = None if start is None else revisions.index(start_raw_id)
+ end_raw_id = self._get_revision(end)
+ end_pos = None if end is None else revisions.index(end_raw_id)
+ if start_pos is not None and end_pos is not None and start_pos >
end_pos:
+ raise RepositoryError("Start revision '%s' cannot be "
+ "after end revision '%s'" % (start, end))
+ if end_pos is not None:
+ end_pos += 1
+ revs = revisions[start_pos:end_pos]
if reverse:
revs.reverse()
_______________________________________________
kallithea-general mailing list
[email protected]
https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
_______________________________________________
kallithea-general mailing list
[email protected]
https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
