Re: cherry-picking something to -stable which might require other changes

[Daniel P . Berrangé]

On Tue, Sep 12, 2023 at 09:01:43PM +0300, Michael Tokarev wrote:
> 12.09.2023 18:23, Daniel P. Berrangé wrote:
> ..
> > I tend to try to cherry-pick the dependancies in case (1) too
> > unless they are functionally invasive. Any time you manually
> > adjust a patch, you increase the likelihood that later cherry
> > picks will also require manual work. So I always favour a clean
> > cherry-pick until the point the functional risk becomes
> > unacceptable in the context of testing the change I'm pulling
> > back.
> 
> Yeah, that's exactly my thought: if something in the subsystem
> has changed, esp. when the new thing is now widely used, it is
> best to try to pick it up (unless it is a big change by itself
> or is a part of big change).
> 
> I already mentioned a trivial fix c255946e3df4 in this thread,
> which can be applied cleanly if two other no-change patches are
> in, 753ae97abc7 and dadee9e3ce6.  It is much more likely to hit
> conflicts in this area in future updates if such updates will
> happen if such renames like these two aren't picked up.
> 
> So, right in this same patch series, there's one more very similar
> change:
> 
> commit 9ff31406312500053ecb5f92df01dd9ce52e635d
> Author: Conor Dooley <conor.dooley@microchip.com>
> Date:   Thu Jul 27 15:24:17 2023 +0100
> 
>     hw/riscv: virt: Fix riscv,pmu DT node path
> 
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -719,7 +719,7 @@ static void create_fdt_pmu(RISCVVirtState *s)
>      MachineState *ms = MACHINE(s);
>      RISCVCPU hart = s->soc[0].harts[0];
> 
> -    pmu_name = g_strdup_printf("/soc/pmu");
> +    pmu_name = g_strdup_printf("/pmu");
>      qemu_fdt_add_subnode(ms->fdt, pmu_name);
>      qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
>      riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name);
> 
> But all the nearby lines are touched by previous patch:
> 
> commit 568e0614d0979e0431a8d9dc0503a63b8b0f2d81
> Author: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Date:   Tue Jan 24 18:22:33 2023 -0300
> 
>     hw/riscv/virt.c: rename MachineState 'mc' pointers to 'ms'
> ...
>     Rename all 'mc' MachineState pointers to 'ms'. This is a very tedious
>     and mechanical patch that was produced by doing the following:
> 
>     - find/replace all 'MachineState *mc' to 'MachineState *ms';
>     - find/replace all 'mc->fdt' to 'ms->fdt';
>     - find/replace all 'mc->smp.cpus' to 'ms->smp.cpus';
>     - replace any remaining occurrences of 'mc' that the compiler complained
>     about.
> 
> This patch by Daniel is a no-code-change, it really is just a rename of
> variables.  I can rename variable back from ms to mc in the fix patch,
> or I can apply this rename first and apply the fix patch cleanly, and
> all subsequent changes will have much more chance to apply cleanly too.
> 
> What a wonderful world.. ;)
> 
> Thankfully, such cases are rare.  But we do have a few famous cases like this
> still, some of which I also mentioned in the first message in this thread.

Also this is the key reason why many reviewers will complain if patches
are too large, or contain a mixture of functional and non-functional
changes, or do two jobs at once. Bigger commits with varying & unrelated
changes makes cherry picking much more painful

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|