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

[doak]

Hi Denis and all,

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

What is the benefit of a rebase in that case? Can't you just merge that
as well? In fact it should be a ff-merge as long we didn't changed anything.

I like rebase very well, but only in cases where I intentionally want to
*change history* like
     * rephrasing commit messages.
     * restructuring/reordering commits.
     * fixing commits.

Some tend to want achieve a linear history and rebase feature branches
regularly. I don't see the point there either, but warn about not changing
history after you tested that code (without rerunning *all* tests *and* have
a very high coverage).

> [...] but it requires one extra step per rebase.

I would argue, that every additional step is one too much, since it makes
it harder to use the power of Git to gather the needed information.

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

This seems to be a quite important aspect. What about the mentioned command?
Does it not work?

>> In short: Is there any way to show that diffs in the web frontend of Git?
>> [...]
> cgit seems to support that:
> https://git.replicant.us/contrib/GNUtoo/hardware_replicant_libsamsung-ipc/commit/?id=c137579d09b0de9a903bd764bb0cb8196190dff3

Not exactly what I was looking for. I meant a diff for arbitrary revisions. This matches
a plain 'git show <rev>'.


Regards,
doak




On 22.02.20 21:24, Denis 'GNUtoo' Carikli wrote:
> 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.
>