Transferring changeset approval status to rebased successors

[Mads Kiilerich]

Hi

Some brief comments to the big questions:

c.cs_repo.statuses() is already used for finding status for changeset 
hashes in bulk. It will perhaps also be able to handle that you pass all 
ancestor hashes for all pending PR changes. But you will of course have 
to process the result and pick the most recent approval to be the one 
that applies.

If you don't want to compute that when rendering web pages, it can also 
be computed in a push hook. (You can probably ignore the possibility of 
obsoleted changesets changing review status. Only the latest changeset 
will get new reviews from the web UI, and that will overrule any old 
result anyway.)

I guess you ideally also should verify that the changeset didn't change 
significantly since the previous approval. Perhaps by looking at the 
textual diff (without line numbers) and see if it is the same.

This seems to only be about one reviewer on each changeset. Great if 
that works for you. Doing the same for PRs with multiple reviewers with 
independent review status will be more tricky.

/Mads


On 25/03/2023 22:10, Manuel Jacob wrote:
> Hi,
>
> In one project I’m working on, we do code review of single changesets 
> in a feature branch (usually the changesets are quite small and on 
> average more than 10 are submitted for review at the same time). We 
> also use Mercurial’s changeset evolution quite heavily. Feature 
> branches are rebased regularly and single changesets are amended 
> between two reviews (causing the descendants of these changesets to be 
> rebased by the evolve extension).
>
> Currently, we track the review status of each of these changesets 
> manually. After the branch is rebased, each of the rebased changesets 
> is shown as unreviewed in Kallihea. It would be a significant 
> improvement if Kallithea showed for each changeset whether an 
> “unchanged” predecessor was already approved.
>
> Thanks to the obsoleteness markers provided by Mercurial, this is easy 
> to determine. The algorithm would walk through the predecessors if 
> there is only one and only the parent changed in between them, until 
> it hits a changeset whose status is not “unreviewed”.
>
> One question is how to show this information to the user. What would 
> work for me is to show "approved predecessor" in all places where 
> "approved" can be shown. Instead of a green circle, it could show the 
> outline of a green circle. (The same could be applied to “under 
> review” and “not approved”).
>
> Another question is when to run the logic. Running it each time the 
> review status is shown somewhere would work good enough for us. 
> Caching this is not easy. It would need to be invalidated each time a 
> predecessor is added or its review status is changed. Recomputing it 
> each time shouldn’t be a problem in practice because the obsoleteness 
> markers are stored in-memory, the number of considered predecessors is 
> limited (until the algorithm hits a “changed” or already reviewed 
> predecessor) and in most places where the review status is shown, the 
> changeset description is also shown, which has to be read from disk, 
> so walking the predecessors should not contribute much to the total time.
>
> What do you think?
> _______________________________________________
> kallithea-general mailing list
> kallithea-general at sfconservancy.org
> https://lists.sfconservancy.org/mailman/listinfo/kallithea-general