[PATCH v2 0/1] patch.py: don't apply striplevel to git am command

[PATCH v2 0/1] patch.py: don't apply striplevel to git am command

[bkylerussell]

I thought I had sent this out a month ago, but I can't find it in patchwork,
so I'm resending.  This is an updated patch that addresses feedback on v1, and
should hopefully be more clear.

I've dropped the change to 'git apply' since that wasn't behaving as I originally
thought, then I added a selftest on llvm that demonstrates the issue I'm trying to
resolve.  Without the change in patch.py included here, the selftest that I'm
proposing will fail.

One might argue, "Well, if you change the default PATCHTOOL, you could also
just change your patch SRC_URIs and drop the striplevel parameter."  While
that may be true, the same failure can also be demonstrated using devtool on
llvm without modifying PATCHTOOL at all, since by default devtool also uses
GitApplyTree.  So if you don't consider "modifying a recipe's PATCHTOOL without
also modify its SRC_URI patches" to be a valid workflow, devtool is another
common workflow where this behavior could be encountered.

Hope that helps,
Kyle

Kyle Russell (1):
  patch.py: don't apply striplevel to git am command

 .../llvm/files/0001-Test-new-file.patch        | 17 +++++++++++++++++
 .../recipes-test/llvm/llvm_%.bbappend          |  2 ++
 meta/lib/oe/patch.py                           |  2 +-
 meta/lib/oeqa/selftest/cases/bbtests.py        | 18 ++++++++++++++++++
 4 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch
 create mode 100644 meta-selftest/recipes-test/llvm/llvm_%.bbappend

-- 
2.17.1

[PATCH v2 1/1] patch.py: don't apply striplevel to git am command

[bkylerussell]

Don't pass a path removal parameter to git am since it always applies
patches from the root of the git repo, not from each individual patchdir
(like quilt).

GitApplyTree uses `git rev-parse --show-toplevel` to find the root of
the repo no matter which patchdir is passed into the GitApplyTree instance.
But for recipes who set S to a subdirectory within their repo, patches
may be added to SRC_URI using a striplevel parameter to match the depth of S.

Currently when these patches are applied with git am in GitApplyTree instead
of quilt, the striplevel still takes effect even though git applies the
patch itself from the repo root, not from within S.

In most cases when a patch modifies existing files within a repo, git am
works fine and the desired outcome is still achieved because the three-way
merge figures out which file the patch was intended to modify based on the
repo history.

An exception to this occurs if the patch adds a new file to the repo.

For example, consider a patch that adds a new file src/foo to a repo.
Additionally, assume the recipe for this repo sets S = "${WORKDIR}/git/src",
and the patch is added to SRC_URI using "striplevel=2" since the default
patch tool, quilt, applies each patch at its patchdir, ${S}.

  --- /dev/null
  +++ b/src/foo
  @@ -0,0 +1 @@
  +new file test

When the GitApplyTree patch class is used (through either devtool or
PATCHTOOL = "git"), the root of the repo is passed to git as the work tree,
in addition to the path removal paramter.  In the case above, GitApplyTree
creates ${WORKDIR}/git/foo instead of ${WORKDIR}/git/src/foo.

This is demonstrated with the included selftest.  By not passing the striplevel
parameter to git am, the patch creates the new file at the intended depth in
the repo.
---
 .../llvm/files/0001-Test-new-file.patch        | 17 +++++++++++++++++
 .../recipes-test/llvm/llvm_%.bbappend          |  2 ++
 meta/lib/oe/patch.py                           |  2 +-
 meta/lib/oeqa/selftest/cases/bbtests.py        | 18 ++++++++++++++++++
 4 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch
 create mode 100644 meta-selftest/recipes-test/llvm/llvm_%.bbappend

diff --git a/meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch b/meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch
new file mode 100644
index 0000000000..8af0afeb12
--- /dev/null
+++ b/meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch
@@ -0,0 +1,17 @@
+From 057c3da44affb47c154ec1ef67bd11646a6429fa Mon Sep 17 00:00:00 2001
+From: Kyle Russell <bkylerussell@gmail.com>
+Date: Tue, 28 Apr 2020 15:18:43 -0400
+Subject: [PATCH] Test new file
+
+---
+ llvm/README.new | 1 +
+ 1 file changed, 1 insertion(+)
+ create mode 100644 llvm/README.new
+
+diff --git a/llvm/README.new b/llvm/README.new
+new file mode 100644
+index 00000000000..0527e6bd2d7
+--- /dev/null
++++ b/llvm/README.new
+@@ -0,0 +1 @@
++This is a test
diff --git a/meta-selftest/recipes-test/llvm/llvm_%.bbappend b/meta-selftest/recipes-test/llvm/llvm_%.bbappend
new file mode 100644
index 0000000000..205720982c
--- /dev/null
+++ b/meta-selftest/recipes-test/llvm/llvm_%.bbappend
@@ -0,0 +1,2 @@
+# This bbappend is used to alter the recipe using the test_recipe.inc file created by tests.
+include test_recipe.inc
diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
index 7ca2e28b1f..870d183e8b 100644
--- a/meta/lib/oe/patch.py
+++ b/meta/lib/oe/patch.py
@@ -512,7 +512,7 @@ class GitApplyTree(PatchTree):
             try:
                 shellcmd = [patchfilevar, "git", "--work-tree=%s" % reporoot]
                 self.gitCommandUserOptions(shellcmd, self.commituser, self.commitemail)
-                shellcmd += ["am", "-3", "--keep-cr", "-p%s" % patch['strippath']]
+                shellcmd += ["am", "-3", "--keep-cr"]
                 return _applypatchhelper(shellcmd, patch, force, reverse, run)
             except CmdError:
                 # Need to abort the git am, or we'll still be within it at the end
diff --git a/meta/lib/oeqa/selftest/cases/bbtests.py b/meta/lib/oeqa/selftest/cases/bbtests.py
index dc423ec439..6d863b425a 100644
--- a/meta/lib/oeqa/selftest/cases/bbtests.py
+++ b/meta/lib/oeqa/selftest/cases/bbtests.py
@@ -4,6 +4,7 @@
 
 import os
 import re
+import tempfile
 
 import oeqa.utils.ftools as ftools
 from oeqa.utils.commands import runCmd, bitbake, get_bb_var, get_bb_vars
@@ -81,6 +82,23 @@ class BitbakeTests(OESelftestTestCase):
                 found = l
         self.assertTrue(found and found.startswith("ERROR:"), msg = "Incorrectly formed patch application didn't fail. bitbake output: %s" % result.output)
 
+    def test_patchtool_git(self):
+        test_recipe = 'llvm'
+        tmpdir = tempfile.mkdtemp(prefix='bitbakeqa')
+        bb_vars = get_bb_vars(['S'], test_recipe)
+        src_dir = bb_vars['S']
+
+        self.write_recipeinc('llvm', 'FILESEXTRAPATHS_prepend := "${THISDIR}/files:"\nSRC_URI += "file://0001-Test-new-file.patch;striplevel=2"')
+        bitbake('llvm -c patch')
+        self.assertExists('%s/README.new' % src_dir)
+
+        bitbake('%s -c clean' % test_recipe)
+        self.append_recipeinc('llvm', 'PATCHTOOL = "git"')
+        bitbake('llvm -c patch')
+        self.assertExists('%s/README.new' % src_dir)
+
+        self.delete_recipeinc('llvm')
+
     def test_force_task_1(self):
         # test 1 from bug 5875
         test_recipe = 'zlib'
-- 
2.17.1

✗ patchtest: failure for patch.py: don't apply striplevel to git am command

[Patchwork]

== Series Details ==

Series: patch.py: don't apply striplevel to git am command
Revision: 1
URL   : https://patchwork.openembedded.org/series/24491/
State : failure

== Summary ==


Thank you for submitting this patch series to OpenEmbedded Core. This is
an automated response. Several tests have been executed on the proposed
series by patchtest resulting in the following failures:



* Patch            [v2,1/1] patch.py: don't apply striplevel to git am command
 Issue             Patch is missing Signed-off-by [test_signed_off_by_presence] 
  Suggested fix    Sign off the patch (either manually or with "git commit --amend -s")

* Issue             A patch file has been added, but does not have a Signed-off-by tag [test_signed_off_by_presence] 
  Suggested fix    Sign off the added patch file (meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch)

* Issue             Added patch file is missing Upstream-Status in the header [test_upstream_status_presence_format] 
  Suggested fix    Add Upstream-Status: <Valid status> to the header of meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch
  Standard format  Upstream-Status: <Valid status>
  Valid status     Pending, Accepted, Backport, Denied, Inappropriate [reason], Submitted [where]



If you believe any of these test results are incorrect, please reply to the
mailing list (openembedded-core@lists.openembedded.org) raising your concerns.
Otherwise we would appreciate you correcting the issues and submitting a new
version of the patchset if applicable. Please ensure you add/increment the
version number when sending the new version (i.e. [PATCH] -> [PATCH v2] ->
[PATCH v3] -> ...).

---
Guidelines:     https://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite:     http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe

[PATCH v2 0/1] patch.py: don't apply striplevel to git am command

[bkylerussell]

Here's an updated patch that I hope will be a little more clear.  I've dropped
the change to git apply since that wasn't behaving as I expected, then added
a selftest on llvm that demonstrates the issue I'm trying to resolve.  Without
the change in patch.py included here, the selftest that I'm proposing will fail.

One might argue, "Well, if you change the default PATCHTOOL, you could also
just change your patch SRC_URIs and drop the striplevel parameter."  While
that may be true, the same failure can also be demonstrated using devtool on
llvm without modifying PATCHTOOL at all, since by default devtool also uses
GitApplyTree.  So if you don't consider "modifying a recipe's PATCHTOOL without
also modify its SRC_URI patches" to be a valid workflow, devtool is another
common workflow where this behavior could be encountered.

Hope that helps,

Kyle

Kyle Russell (1):
  patch.py: don't apply striplevel to git am command

 .../llvm/files/0001-Test-new-file.patch        | 17 +++++++++++++++++
 .../recipes-test/llvm/llvm_%.bbappend          |  2 ++
 meta/lib/oe/patch.py                           |  2 +-
 meta/lib/oeqa/selftest/cases/bbtests.py        | 18 ++++++++++++++++++
 4 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch
 create mode 100644 meta-selftest/recipes-test/llvm/llvm_%.bbappend

-- 
2.17.1