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

[Denis 'GNUtoo' Carikli]

On Sat, 22 Feb 2020 10:24:52 +0100
doak <doak+list at posteo.net> wrote:

> Hi Denis and all,
Hi,

> just a few remark (as a non-Replicant developer) a.k.a my 2-cents:
The remarks of everybody are more than welcome.

> > 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.  
> 
> I would argue, that rebasing looses quite important information, i.e.
> you loose the state on which the changes had worked *and* had been
> tested. In other words it will get harder to reconstruct the version
> that had worked based on history of current branch.

> That has also implications if you try to bisect.
I don't think it applies that much here as there are huge jump between
Replicant versions anyway. You can't bisect linearly between Replicant
4.0 to 6.0 for instance, you still need to skip all the commits in
between. 

In addition to that some repositories also depends on other, but it's
not the case for libsamsung-ipc for instance, where the same code can
be compiled for several Replicant versions.

I was for instance able to use recent libsamsung-ipc on Replicant 4.2
(to see if it would fix the bug where some SIM cards were not
recognized).

Though from within a Replicant version, my conversion from merges to
rebases broke git bisect. Before that rebase, after finding the
repository to bisect, you just start bisecting a given version
(Replicant 6.0), but now, in you need to find which history to
bisect before starting to bisect.

The cost is one test per (additional) rebases. I've to think more about
it, because since the last FOSDEM meeting, we started considering to
always rebase on top of LineageOS because, as we were told, they merge
security fixes, and they also continue to do it even way after Android
Open Source project stopped to do it.

There are also mixed possibilities like rebasing for a new version and
then doing merges to keep the ability to bisect.

> Sure, there are tags containing that etc., but it is just out of
> current history. IMHO that's an anti-pattern (if that history could
> matter in the future).
Since we have tags and that I also tag just before a rebase, we can
still find the history, but it requires one extra step per rebase.

> > 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.  
> 
> Shouldn't a plain git 'git log upstream/master..' do the job?
> This shows all commits of current HEAD not present on
> 'upstream/master', which IMHO is exactly what you want.
> 
> > - 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.  
> 
> Personally I prefer reviews with SCM support, that is I can use all my
> beloved and well known tools for that. Obviously, for small changes
> (or if it happens that you /just know/ all the context) it's easier
> to just review the patch included in some mail. (It guess it works
> for the Linux kernel developers since they are able cache a lot of
> context ;D) Theoretically a patch workflow lowers the entry barrier,
> but I am not sure if that is really true, since it's quite
> complicated to do it really right.
[...]
> Back to topic: I see the advantage of patch based workflow (no
> overhead to set up your environment), but I would argue that a
> solution which provides that view based on underlying SCM, would be
> the best solution. In short: Is there any way to show that diffs in
> the web frontend of Git? It seems 'cgit' is not capable of that.
> Perhaps such support can be added.
cgit seems to support that:
https://git.replicant.us/contrib/GNUtoo/hardware_replicant_libsamsung-ipc/commit/?id=c137579d09b0de9a903bd764bb0cb8196190dff3

I'll try to see if there is a way to have all the 3 at the same time:
- Patches sent with git-send-email
- The commands to fetch the commit
- The link for the web version of git show

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/20200222/6e19b98c/attachment.asc>