[Replicant] Patches review procedure Was: [libsamsung-ipc][PATCH 5/6] ipc.c: cosmetic cleanups

[Denis 'GNUtoo' Carikli]

On Sat, 15 Feb 2020 23:38:34 +0000
Fil Lupin <fillupin at protonmail.com> wrote:

> Hi,
Hi,

> don't know if needed but Ok for me.
Unfortunately I already pushed the last part of the patches that
weren't reviewed, and here we're the upstream so we can't easily
rewrite the git history.

The current procedure that is in use for Replicant 6.0 is to send
patches, and if there is no review for one week, the people that are
able to push patches can push them. If someone gets 1 positive review
before that, the people that can push are allowed to push.

Though it's not optimal:
- Here I sent a huge number of patches, that take time to review. Maybe
  I should give more time in that case or just plainly ask if people
  need more time to review them.
- Sometimes I've no review at all.

But it's still better than some other procedures:
- Several bugs were caught and fixed thanks to that procedure, and that
  can even work with 0 reviews. 
  For instance I sent for review a patch that adds a script that wraps
  Replicant 6.0 build, and I then changed my mind on some details of
  the implementation. 
  Several patches were also improved thanks to the reviews.
- I tried waiting for reviews before pushing, and that leads to loosing
  patches and/or becomes way to messy as some patches are never
  reviewed and forgotten about. And since such patches are useful, it's
  better to push them at some point than forgetting about them.

Rebasing Replicant 6:
---------------------
We have no review procedure for rebasing repositories like the
Replicant 6 repositories. In the past, merges commits were used to merge
Replicant changes with latest LineageOS or CyanogenMod changes.

The issue is that with merge commits it then becomes very hard and time
consuming to understand what really constitutes Replicant changes, so I
think that rebases are better.

So I've been rebasing some of the Replicant 6 repositories. The wiki
has a summary of that here:
https://redmine.replicant.us/projects/replicant/wiki/Replican6Changes

I think it would be a good idea to require reviews for that too. For
instance I probably broke booting of the Galaxy Nexus and Galaxy Tab
2 because of a rebase.

But the issue is that I'm unsure of what would be the most
convenient way of doing it:
- I could send the patches to be applied again on top of a known
  revision. For instance lineageos/cm-13. I'm unsure how practical it
  is to review as some of the information is not there (like it's
  harder to do a diff with that).
- I could point to a branch on a git repository but I'm unsure if it's
  very convenient to review as it would increase the burden on the
  people reviewing the patches, as they would need to go fetch that and
  look at it instead of already having the patch to review right in the
  mail.
- I could do both: send patches, and also have them on a branch
  somewhere. The time spent to do both is near 0 for me.

Also do people wanting to review that kind of work have some idea on
which information to include, like a diff between the old and the new
rebase, or ways that would make it easier to review?

For other repositories:
-----------------------
- Replicant 9 specific repositories have currently no review in place
  as it's still under heavy development and can be rebased at any time.
  Though some of the work is still reviewed as people working on it
  often look at each other commits afterward, and some of the work is
  going in libsamsung-ipc, Linux, and will also go in libsamsung-ril,
  so that is being reviewed.
- Obviously, user repositories obviously require no reviews.

Denis.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.osuosl.org/pipermail/replicant/attachments/20200216/97625e94/attachment.asc>