[v2] support/scripts/apply-patches.sh: do not apply patches with renames

Message ID 20170624195936.31528-1-thomas.petazzoni@free-electrons.com
State Accepted
Headers show

Commit Message

Thomas Petazzoni June 24, 2017, 7:59 p.m.
Patches with renames apply properly with patch >= 2.7, but not with
older patch versions. Since "git format-patch" by default generates
patches with renames, Buildroot developers often don't realize that
their patches will not apply properly on build machines that have
patch < 2.7. In order to prevent such a situation from happening
again, this commit adds some logic in apply-patches.sh to refuse
applying patches that contain renames.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
Changes since v1:
 - Use "&&" for the "grep -q && grep -q" condition, as noticed by Yann
   E. Morin.
---
 support/scripts/apply-patches.sh | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Arnout Vandecappelle June 24, 2017, 10:08 p.m. | #1
On 24-06-17 21:59, Thomas Petazzoni wrote:
> Patches with renames apply properly with patch >= 2.7, but not with
> older patch versions. Since "git format-patch" by default generates
> patches with renames, Buildroot developers often don't realize that
> their patches will not apply properly on build machines that have
> patch < 2.7. In order to prevent such a situation from happening
> again, this commit adds some logic in apply-patches.sh to refuse
> applying patches that contain renames.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 Perhaps add to the commit message why you don't just grep for '^rename':

Since the patch commit message may contain the words "rename from" or
"rename to" as well, the grep expression is made as accurate as possible,
checking both.

 Ideally the test should start from the first ^diff, but that makes things a lot
more complicated...

 An alternative would be to do

patch --dry-run -i ${path}/${patch} | grep "(renamed from"

but I can't guarantee that that works with all patch versions that understand
rename. This the current patch is OK.

 Regards,
 Arnout


> ---
> Changes since v1:
>  - Use "&&" for the "grep -q && grep -q" condition, as noticed by Yann
>    E. Morin.
> ---
>  support/scripts/apply-patches.sh | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
> index 7ccb39d..0c24175 100755
> --- a/support/scripts/apply-patches.sh
> +++ b/support/scripts/apply-patches.sh
> @@ -113,6 +113,11 @@ function apply_patch {
>          echo "  to be applied  : ${path}/${patch}"
>          exit 1
>      fi
> +    if grep -q "^rename from" ${path}/${patch} && \
> +       grep -q "^rename to" ${path}/${patch} ; then
> +	echo "Error: patch contains some renames, not supported by old patch versions"
> +	exit 1
> +    fi
>      echo "${path}/${patch}" >> ${builddir}/.applied_patches_list
>      ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" -t -N $silent
>      if [ $? != 0 ] ; then
>
Yann E. MORIN June 25, 2017, 8:42 a.m. | #2
Thomas, All,

On 2017-06-24 21:59 +0200, Thomas Petazzoni spake thusly:
> Patches with renames apply properly with patch >= 2.7, but not with
> older patch versions. Since "git format-patch" by default generates
> patches with renames, Buildroot developers often don't realize that
> their patches will not apply properly on build machines that have
> patch < 2.7. In order to prevent such a situation from happening
> again, this commit adds some logic in apply-patches.sh to refuse
> applying patches that contain renames.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

A small nit below...

> ---
> Changes since v1:
>  - Use "&&" for the "grep -q && grep -q" condition, as noticed by Yann
>    E. Morin.
> ---
>  support/scripts/apply-patches.sh | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
> index 7ccb39d..0c24175 100755
> --- a/support/scripts/apply-patches.sh
> +++ b/support/scripts/apply-patches.sh
> @@ -113,6 +113,11 @@ function apply_patch {
>          echo "  to be applied  : ${path}/${patch}"
>          exit 1
>      fi
> +    if grep -q "^rename from" ${path}/${patch} && \
> +       grep -q "^rename to" ${path}/${patch} ; then
> +	echo "Error: patch contains some renames, not supported by old patch versions"
> +	exit 1

This file a space-leading, but here you have a tab...

Otherwise:

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> +    fi
>      echo "${path}/${patch}" >> ${builddir}/.applied_patches_list
>      ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" -t -N $silent
>      if [ $? != 0 ] ; then
> -- 
> 2.9.4
>
Arnout Vandecappelle Sept. 19, 2017, 9:01 p.m. | #3
On 24-06-17 21:59, Thomas Petazzoni wrote:
> Patches with renames apply properly with patch >= 2.7, but not with
> older patch versions. Since "git format-patch" by default generates
> patches with renames, Buildroot developers often don't realize that
> their patches will not apply properly on build machines that have
> patch < 2.7. In order to prevent such a situation from happening
> again, this commit adds some logic in apply-patches.sh to refuse
> applying patches that contain renames.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

 Applied to master after incorporating Yann's and my suggestions, thanks.

 Regards,
 Arnout
Yann E. MORIN Sept. 19, 2017, 9:07 p.m. | #4
Peter, Bernd, All,

On 2017-09-19 23:01 +0200, Arnout Vandecappelle spake thusly:
> On 24-06-17 21:59, Thomas Petazzoni wrote:
> > Patches with renames apply properly with patch >= 2.7, but not with
> > older patch versions. Since "git format-patch" by default generates
> > patches with renames, Buildroot developers often don't realize that
> > their patches will not apply properly on build machines that have
> > patch < 2.7. In order to prevent such a situation from happening
> > again, this commit adds some logic in apply-patches.sh to refuse
> > applying patches that contain renames.
> > 
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
>  Applied to master after incorporating Yann's and my suggestions, thanks.

Which leaves the tree with a single patch that contains renames, which
was added by 74a56295 (softether: don't download patch from Github) by
Peter. ;-)

Regards,
Yann E. MORIN.
Peter Korsgaard Sept. 20, 2017, 5:14 p.m. | #5
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 >> Applied to master after incorporating Yann's and my suggestions, thanks.

 > Which leaves the tree with a single patch that contains renames, which
 > was added by 74a56295 (softether: don't download patch from Github) by
 > Peter. ;-)

Ups, fixed now.

Patch

diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
index 7ccb39d..0c24175 100755
--- a/support/scripts/apply-patches.sh
+++ b/support/scripts/apply-patches.sh
@@ -113,6 +113,11 @@  function apply_patch {
         echo "  to be applied  : ${path}/${patch}"
         exit 1
     fi
+    if grep -q "^rename from" ${path}/${patch} && \
+       grep -q "^rename to" ${path}/${patch} ; then
+	echo "Error: patch contains some renames, not supported by old patch versions"
+	exit 1
+    fi
     echo "${path}/${patch}" >> ${builddir}/.applied_patches_list
     ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" -t -N $silent
     if [ $? != 0 ] ; then