[PATCH stable] hg: use correct start / stop indices when slicing revisions

[Mads Kiilerich]

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 <me at manueljacob.de>
> # 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
> kallithea-general at sfconservancy.org
> https://lists.sfconservancy.org/mailman/listinfo/kallithea-general