Transferring changeset approval status to rebased successors

[Manuel Jacob]

On 31/03/2023 22.53, Mads Kiilerich wrote:
> 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.)

Although it might not happen in practice, it’s possible to approve or 
otherwise change the status of predecessors.

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

The obsmarkers contain information about what changed between the 
changesets. As written in the original mail, the logic would only 
trigger if only the parent changed in between them (according to the 
information stored in the obsmarker). The algorithm that’s used for 
computing whether the diff changed is essentially what you described.

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

It’s right that we don’t use and don’t plan to use pull requests. Before 
my idea gets to the stage where I would send patches, I will consider 
how it works together with pull requests. If no better semantics can be 
found, pull requests would continue to work exactly as they do now.

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