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@sfconservancy.org
https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
