Message-ID: <e70bf38e-af6a-dc63-3249-adbf168a1233@oracle.com>
Date:   Tue, 7 Mar 2023 12:43:42 +0100
From:   Vegard Nossum <vegard.nossum@...cle.com>
To:     Bagas Sanjaya <bagasdotme@...il.com>,
        Jonathan Corbet <corbet@....net>
Cc:     linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org, backports@...r.kernel.org,
        Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        "Jason A . Donenfeld" <Jason@...c4.com>
Subject: Re: [PATCH] docs: add backporting and conflict resolution document


On 3/5/23 05:03, Bagas Sanjaya wrote:
> On Fri, Mar 03, 2023 at 05:25:53PM +0100, Vegard Nossum wrote:
>> +It is strongly recommended to instead find an appropriate base version
>> +where the patch applies cleanly and *then* cherry-pick it over to your
>> +destination tree, as this will make git output conflict markers and let
>> +you resolve conflicts with the help of git and any other conflict
>> +resolution tools you might prefer to use.
>> +
>> +It's generally better to use the exact same base as the one the patch
>> +was generated from, but it doesn't really matter that much as long as it
>> +applies cleanly and isn't too far from the original base. The only
>> +problem with applying the patch to the "wrong" base is that it may pull
>> +in more unrelated changes in the context of the diff when cherry-picking
>> +it to the older branch.
>> +
>> +If you are using
>> +`b4 <https://people.kernel.org/monsieuricon/introducing-b4-and-patch-attestation>`__
>> +and you are applying the patch directly from an email, you can use
>> +``b4 am`` with the options ``-g``/``--guess-base`` and
>> +``-3``/``--prep-3way`` to do some of this automatically (see `this
>> +presentation <https://youtu.be/mF10hgVIx9o?t=2996>`__ for more
>> +information). However, the rest of this article will assume that you are
>> +doing a plain ``git cherry-pick``.
> 
> Above are from applier's perspective (maintainers and/or developers
> doing the backport). For patch submitter, don't forget to pass
> --base=<base-commit> to git-format-patch(1) so that the applier can know
> the base commit of the patch to be applied. For patches intended for
> mainline submission, the applier could create a temporary branch based on
> specified base commit (as described above), apply the patch, and rebase
> to latest appropriate subsystem tree (and resolve conflicts if any).
> Others could instead directly apply the patch on top of subsystem tree.

This whole document is meant for the developer doing the backport.

git-format-patch --base= is already covered here:

https://docs.kernel.org/process/submitting-patches.html#providing-base-tree-information

I don't think we need to repeat it in this document.

>> +
>> +Once you have the patch in git, you can go ahead and cherry-pick it into
>> +your source tree. Don't forget to cherry-pick with ``-x`` if you want a
>> +written record of where the patch came from!
> 
> "In most cases, you will likely want to cherry-pick with ``-x`` option
> to record upstream commit in the resulting backport commit description,
> which looks like::
> 
>      (cherry picked from commit <upstream commit>)
> 
> However, for backporting to stable, you need to edit the description
> above to either::
> 
>      commit <upstream commit> upstream
> 
> or
>      [ Upstream commit <upstream commit> ]
> 
> "

Good point -- the original blog post where this came from was meant to
be more general than just stable backports, but this document in
particular is partly also meant to aid stable contributors we might as
well include it.

>> +For backports, what likely happened was that your older branch is
>> +missing a patch compared to the branch you are backporting from --
>> +however, it is also possible that your older branch has some commit that
>> +doesn't exist in the newer branch. In any case, the result is a conflict
>> +that needs to be resolved.
> 
> Another conflict culprit that there are non-prerequisite commits that
> change the context line.

I think that's already covered by "missing a patch", or at least that
was my intention. I guess we can change it to something like:

+For backports, what likely happened was that the branch you are
+backporting from contains patches not in the branch you are backporting
+to. However, it is also possible that your older branch has some commit
+that doesn't exist in the newer branch. In any case, the result is a
+conflict that needs to be resolved.

I'll fiddle a bit more with the exact phrasing.

>> +git log
>> +^^^^^^^
>> +
>> +A good first step is to look at ``git log`` for the file that has the
>> +conflict -- this is usually sufficient when there aren't a lot of
>> +patches to the file, but may get confusing if the file is big and
>> +frequently patched. You should run ``git log`` on the range of commits
>> +between your currently checked-out branch (``HEAD``) and the parent of
>> +the patch you are picking (``COMMIT``), i.e.::
>> +
>> +    git log HEAD..COMMIT^ -- PATH
> 
> HEAD and <commit> swapped, giving empty log. The correct way is:
> 
> ```
> git log <commit>^..HEAD -- <path>
> ```

Hrrm, I've double checked this and I think the original text is correct.

HEAD..<commit>^ gives you commits reachable from <commit>^ (parent of
the commit we are backporting), excluding all commits that are reachable
from HEAD (the branch we are backporting to).

<commit>^..HEAD, on the other hand, would give you commits reachable
from HEAD excluding all commits that are reachable from the parent of
the commit we are backporting.

With a diagram like this:

o--o--x--y--<commit>
     \
      \--u--v--HEAD

HEAD..<commit>^ would give you x and y while
<commit>^..HEAD would give you u and v.

I agree conflicts could come from either branch, but when backporting it
is more likely that they come from the old branch (so x/y above).

<commit>^...HEAD (with 3 dots) would give commits from either/both, but
then it's harder to tell whether you need to apply it or revert it, so
it's probably better to check the ranges separately.

> Note that for placeholder arguments, I'd like to write the
> placeholder name inside chevrons, like above (git manpage style).

Agreed, will update.

>> +It might be a good idea to ``git show`` these commits and see if they
> ... to show these commits with ``git show <commit>`` ...

Fixed.

>> +Prerequisite vs. incidental patches
>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +Having found the patch that caused the conflict, you need to determine
>> +whether it is a prerequisite for the patch you are backporting or
>> +whether it is just incidental and can be skipped. An incidental patch
>> +would be one that touches the same code as the patch you are
>> +backporting, but does not change the semantics of the code in any
>> +material way. For example, a whitespace cleanup patch is completely
>> +incidental -- likewise, a patch that simply renames a function or a
>> +variable would be incidental as well. On the other hand, if the function
>> +being changed does not even exist in your current branch then this would
>> +not be incidental at all and you need to carefully consider whether the
>> +patch adding the function should be cherry-picked first.
>> +
>> +If you find that there is a necessary prerequisite patch, then you need
>> +to stop and cherry-pick that instead. If you've already resolved some
>> +conflicts in a different file and don't want to do it again, you can
>> +create a temporary copy of that file.
>> +
>> +To abort the current cherry-pick, go ahead and run
>> +``git cherry-pick --abort``, then restart the cherry-picking process
>> +with the commit ID of the prerequisite patch instead.
> 
> IMO, finding prerequisite commits can be done without attempting to
> cherry-pick the desired commit first.

Yeah, of course. I'm in two minds about mentioning this, though. On the
one hand, yes, it would make sense to look at prerequisites before even
starting.

On the other hand, a very common scenario (and the one this document is
using as an example) is that you've attempted a cherry-pick and git gave
you a conflict.

In the end, I think it's much more common that you're in the middle of a
cherry-pick and you need to know what to do to get back to a
non-conflicting state without actually resolving the conflict.

>> +Sometimes the right thing to do will be to also backport the patch that
>> +did the rename, but that's definitely not the most common case. Instead,
>> +what you can do is to temporarily rename the file in the branch you're
>> +backporting to (using ``git mv`` and committing the result), restart the
>> +attempt to cherry-pick the patch, rename the file back (``git mv`` and
>> +committing again), and finally squash the result using ``git rebase -i``
>> +(`tutorial <https://medium.com/@slamflipstrom/a-beginners-guide-to-squashing-commits-with-git-rebase-8185cf6e62ec>`__)
>> +so it appears as a single commit when you are done.
> 
> I'm kinda confused with above. Did you mean that after renaming file, I
> have to abort cherry-picking (``git cherry-pick --abort``) first and
> then redo cherry-picking?

Yes, the idea is that instead of trying to resolve it as a conflict, you
rename the file first, do a (clean) cherry-pick, and then rename it back.

What caused the confusion, specifically?

>> +Build testing
>> +~~~~~~~~~~~~~
>> +
>> +We won't cover runtime testing here, but it can be a good idea to build
> Runtime testing is described in the next section.
>> +just the files touched by the patch as a quick sanity check. For the
>> +Linux kernel you can build single files like this, assuming you have the
>> +``.config`` and build environment set up correctly::
>> +
>> +    make path/to/file.o
>> +
>> +Note that this won't discover linker errors, so you should still do a
>> +full build after verifying that the single file compiles. By compiling
>> +the single file first you can avoid having to wait for a full build *in
>> +case* there are compiler errors in any of the files you've changed.
>> +
> 
> plain ``make``?

Yes, but I don't think we need to spell that out as it's the common case
(in other words, it is presupposed that you know this).

>> +One concrete example of this was where a patch to the system call entry
>> +code saved/restored a register and a later patch made use of the saved
>> +register somewhere in the middle -- since there was no conflict, one
>> +could backport the second patch and believe that everything was fine,
>> +but in fact the code was now scribbling over an unsaved register.
> 
> Did you mean the later patch is the backported syscall patch?

Yes. I'll fiddle a bit with this paragraph to make it clearer.

>> +
>> +Although the vast majority of errors will be caught during compilation
>> +or by superficially exercising the code, the only way to *really* verify
>> +a backport is to review the final patch with the same level of scrutiny
>> +as you would (or should) give to any other patch. Having unit tests and
> "... patches intended for mainline."
> 
>> +The above shows roughly the idealized process of backporting a patch.
>> +For a more concrete example, see this video tutorial where two patches
>> +are backported from mainline to stable:
>> +`Backporting Linux Kernel patches <https://youtu.be/sBR7R1V2FeA>`__
> 
> For the external link targets, I'd like to separate them from
> corresponding link texts (see
> https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#external-links
> for details).

We can probably do that, but it doesn't seem to be used much in existing
kernel documentation, I find no existing instances of it:

$ git grep '\.\._.*: http' Documentation/
$

I know that lots of people really prefer to minimize the amount of
markup in these files (as they consume them in source form), so I'd
really like an ack from others before doing this.

Thanks for the review/comments! I'll give other people a chance to look
it over before sending out a v2.


Vegard

Powered by blists - more mailing lists