Message-ID: <ZAvqm9EqGq/kJpkT@debian.me>
Date:   Sat, 11 Mar 2023 09:42:35 +0700
From:   Bagas Sanjaya <bagasdotme@...il.com>
To:     Vegard Nossum <vegard.nossum@...cle.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 Tue, Mar 07, 2023 at 12:43:42PM +0100, Vegard Nossum wrote:
> 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.

OK.

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

Nice.

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

What I mean is "hey, we have changes that make context lines
conflicted". By "patches not in the branch", I interpret that as "we
have possible non-prereqs that cause this (messy) conflict".

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

In any case, the HEAD you mentioned is at target branch (linux-x.y),
right?

> > > +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?

I thought that the sequence was:

```
$ git checkout -b my-backport linux-x.y
$ git cherry-pick <upstream commit>
# we get mv/modified content conflict
$ git mv <original path> <intended path> && git commit
$ git cherry-pick --abort
$ git cherry-pick <upstream commit>
# resolve content conflicts
$ git add <conflicted path>... && git commit
$ git rebase -i linux-x.y
```

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

So, in that case, what would the correct resoultion be regarding to
registers?

> > 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 recently worked on Documentation/bpf/bpf_devel_QA.rst, where I mention
this linking syntax.

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

OK. For now I'm OK with either separating targets or including them.

Thanks for reply!

-- 
An old man doll... just what I always wanted! - Clara

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists