Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c

Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c

[Uwe Kleine-König]

Hello,

[added git ML to Cc:]

On Fri, Sep 03, 2010 at 07:18:43PM +0900, Magnus Damm wrote:
> On Thu, Sep 2, 2010 at 10:39 PM, Jean-Christophe PLAGNIOL-VILLARD
> <plagnioj@jcrosoft.com> wrote:
> > factorise some generic infrastructure to assist looking up struct clks
> > for the ARM & SH architecture.
> >
> > as the code is identical at 99%
> >
> > put the arch specific code for allocation as example in asm/clkdev.h
> >
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> > v3:
> >        wrong comment removed
> >        headers fixed
> 
> [snip]
> 
> I can't apply this patch using GNU patch. I've tested 2.5.9 and 2.6.1
> from Gentoo.
> 
> Using --dry-run is fine, but omitting dry-run gives me:
> 
> ...
> patching file arch/arm/common/clkdev.c
> patching file arch/sh/include/asm/clkdev.h
> Hunk #1 FAILED at 1.
> Hunk #2 FAILED at 11.
> 2 out of 2 hunks FAILED -- saving rejects to file
> arch/sh/include/asm/clkdev.h.rej
> 
> I guess this is caused by the last "renaming" hunk, see below.
> 
> I thought these things were supposed to work out of the box...
Yes, they work out of the box, but only for people using git :-)

Maybe git-apply can be used instead of patch?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Jonathan Nieder]

(+cc: bug-patch)

Hi,

Uwe Kleine-König wrote:
> On Fri, Sep 03, 2010 at 07:18:43PM +0900, Magnus Damm wrote:

>> Using --dry-run is fine, but omitting dry-run gives me:
>> 
>> ...
>> patching file arch/arm/common/clkdev.c
>> patching file arch/sh/include/asm/clkdev.h
>> Hunk #1 FAILED at 1.
>> Hunk #2 FAILED at 11.
>> 2 out of 2 hunks FAILED -- saving rejects to file
>> arch/sh/include/asm/clkdev.h.rej
>> 
>> I guess this is caused by the last "renaming" hunk, see below.

Yep, I can reproduce this.  Patch applies with "git apply",
"patch --dry-run -p1" accepts it, "patch -p1" fails.

 $ patch --version | head -1
 GNU patch 2.6.1.85-423d
 $ cd ~/src/linux-2.6
 $ git checkout 2bfc96a12
 $ git clean -fd
 $ wget http://download.gmane.org/gmane.linux.ports.sh.devel/8747/8748
 $ patch -p1 --quiet --dry-run <8748 
 $ echo $?
 0
 $ patch -p1 --quiet <8748 
 2 out of 2 hunks FAILED -- saving rejects to file include/linux/clkdev.h.rej
 $ echo $?
 1

Andreas: ideas?

Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Russell King - ARM Linux]

On Fri, Sep 03, 2010 at 01:43:51PM -0500, Jonathan Nieder wrote:
> (+cc: bug-patch)
> 
> Hi,
> 
> Uwe Kleine-König wrote:
> > On Fri, Sep 03, 2010 at 07:18:43PM +0900, Magnus Damm wrote:
> 
> >> Using --dry-run is fine, but omitting dry-run gives me:
> >> 
> >> ...
> >> patching file arch/arm/common/clkdev.c
> >> patching file arch/sh/include/asm/clkdev.h
> >> Hunk #1 FAILED at 1.
> >> Hunk #2 FAILED at 11.
> >> 2 out of 2 hunks FAILED -- saving rejects to file
> >> arch/sh/include/asm/clkdev.h.rej
> >> 
> >> I guess this is caused by the last "renaming" hunk, see below.
> 
> Yep, I can reproduce this.  Patch applies with "git apply",
> "patch --dry-run -p1" accepts it, "patch -p1" fails.

git patches include additional metadata for renaming files, which gnu patch
will not understand.

If you want GNU patch compatible diffs, don't use -C or -M when generating
patches out of git.

Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Uwe Kleine-König]

Hey Russell,

On Fri, Sep 03, 2010 at 08:29:07PM +0100, Russell King - ARM Linux wrote:
> On Fri, Sep 03, 2010 at 01:43:51PM -0500, Jonathan Nieder wrote:
> > Uwe Kleine-König wrote:
> > > On Fri, Sep 03, 2010 at 07:18:43PM +0900, Magnus Damm wrote:
> > 
> > >> Using --dry-run is fine, but omitting dry-run gives me:
> > >> 
> > >> ...
> > >> patching file arch/arm/common/clkdev.c
> > >> patching file arch/sh/include/asm/clkdev.h
> > >> Hunk #1 FAILED at 1.
> > >> Hunk #2 FAILED at 11.
> > >> 2 out of 2 hunks FAILED -- saving rejects to file
> > >> arch/sh/include/asm/clkdev.h.rej
> > >> 
> > >> I guess this is caused by the last "renaming" hunk, see below.
> > 
> > Yep, I can reproduce this.  Patch applies with "git apply",
> > "patch --dry-run -p1" accepts it, "patch -p1" fails.
> 
> git patches include additional metadata for renaming files, which gnu patch
> will not understand.
> 
> If you want GNU patch compatible diffs, don't use -C or -M when generating
> patches out of git.
Still GNU patch should then already fail in --dry-run mode.
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Matthieu Moy]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> git patches include additional metadata for renaming files, which gnu patch
> will not understand.
>
> If you want GNU patch compatible diffs, don't use -C or -M when generating
> patches out of git.

Fyi: actually, GNU patch will support git-style patches in the next
version according to

http://savannah.gnu.org/forum/forum.php?forum_id=6320

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Andreas Schwab]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> Still GNU patch should then already fail in --dry-run mode.

Since --dry-run doesn't actually perform any changes it can easily be
fooled when a file is patched twice.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [bug-patch] [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Andreas Gruenbacher]

Hi,

On Friday 03 September 2010 20:43:51 Jonathan Nieder wrote:
> Uwe Kleine-König wrote:
> > On Fri, Sep 03, 2010 at 07:18:43PM +0900, Magnus Damm wrote:
> 
> >> Using --dry-run is fine, but omitting dry-run gives me:
> >> 
> >> ...
> >> patching file arch/arm/common/clkdev.c
> >> patching file arch/sh/include/asm/clkdev.h
> >> Hunk #1 FAILED at 1.
> >> Hunk #2 FAILED at 11.
> >> 2 out of 2 hunks FAILED -- saving rejects to file
> >> arch/sh/include/asm/clkdev.h.rej
> >> 
> >> I guess this is caused by the last "renaming" hunk, see below.
> 
> Yep, I can reproduce this.  Patch applies with "git apply",
> "patch --dry-run -p1" accepts it, "patch -p1" fails.
> 
>  $ patch --version | head -1
>  GNU patch 2.6.1.85-423d
>  $ cd ~/src/linux-2.6
>  $ git checkout 2bfc96a12
>  $ git clean -fd
>  $ wget http://download.gmane.org/gmane.linux.ports.sh.devel/8747/8748
> [...]

something pretty bizarre is going on here.  The wget output modifies the same 
file twice, but both patches to this file have the same source sha1 (5645f35):

> diff --git a/arch/sh/include/asm/clkdev.h b/arch/sh/include/asm/clkdev.h
> dissimilarity index 69%
> index 5645f35..6ba9186 100644
> --- a/arch/sh/include/asm/clkdev.h
> +++ b/arch/sh/include/asm/clkdev.h

> diff --git a/arch/sh/include/asm/clkdev.h b/include/linux/clkdev.h
> similarity index 85%
> rename from arch/sh/include/asm/clkdev.h
> rename to include/linux/clkdev.h
> index 5645f35..457bcb0 100644

So "git apply" and "patch --dry-run" seem to work only by accident.

How was this patch generated: with git itself?

The fact that "patch --dry-run" may not work for patches that modify the same 
file twice is a known defect.  I don't know how to solve this in a reasonably 
elegant way.  Luckily the problem only triggers when people are doing 
something "strange" such as concatenating patches.

Andreas

Re: [bug-patch] [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Jonathan Nieder]

Andreas Gruenbacher wrote:

> something pretty bizarre is going on here.  The wget output modifies the same 
> file twice, but both patches to this file have the same source sha1 (5645f35):

>From the git v1.6.0-rc0~92 changelog entry:

    apply: fix copy/rename breakage
    
    7ebd52a (Merge branch 'dz/apply-again', 2008-07-01) taught "git-apply" to
    grok a (non-git) patch that is a concatenation of separate patches that
    touch the same file number of times, by recording the postimage of patch
    application of previous round and using it as the preimage for later
    rounds.
    
    This "incremental" mode of patch application fundamentally contradicts
    with the way git rename/copy patches are designed.  When a git patch talks
    about a file A getting modified, and a new file B created out of A, like
    this:
    
        diff --git a/A b/A
        --- a/A
        +++ b/A
        ... change text here ...
        diff --git a/A b/B
        copy from A
        copy to B
        --- a/A
        +++ b/B
        ... change text here ...
    
    the second change to produce B does not depend on what is done to A with
    the first change in any way.  This is explicitly done so for reviewability
    of individual patches.
    
    With this commit, we do not look at 'fn_table' that records the postimage
    of previous round when applying a patch to produce a new file out of an
    existing file.

> How was this patch generated: with git itself?

Yes, the patch basically agrees with what I get by applying it and running

 git format-patch -M -B HEAD^..HEAD

Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Russell King - ARM Linux]

On Fri, Sep 03, 2010 at 09:33:09PM +0200, Uwe Kleine-König wrote:
> > git patches include additional metadata for renaming files, which gnu patch
> > will not understand.
> > 
> > If you want GNU patch compatible diffs, don't use -C or -M when generating
> > patches out of git.
> Still GNU patch should then already fail in --dry-run mode.

And now look at the patch - it touches arch/sh/include/asm/clkdev.h twice.
Once to remove it and once as a rename.

GNU patch not in --dry-run mode will first remove arch/sh/include/asm/clkdev.h,
and then not have a file to deal with when it tries to patch the rename
part.  Whereas with --dry-run, the file stays around.

As I say, it's because GNU patch doesn't (currently) understand GIT
patches.  I wouldn't call that a bug in GNU patch.

Re: [bug-patch] [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Jean-Christophe PLAGNIOL-VILLARD]

> 
> So "git apply" and "patch --dry-run" seem to work only by accident.
> 
> How was this patch generated: with git itself?
I did as usual
git format-patch -M -B -C HEAD~
> 
> The fact that "patch --dry-run" may not work for patches that modify the same 
> file twice is a known defect.  I don't know how to solve this in a reasonably 
> elegant way.  Luckily the problem only triggers when people are doing 
> something "strange" such as concatenating patches.

Best Regards,
J.

Re: [bug-patch] Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Andreas Gruenbacher]

On Saturday 04 September 2010 02:03:48 Russell King - ARM Linux wrote:
> As I say, it's because GNU patch doesn't (currently) understand GIT
> patches.  I wouldn't call that a bug in GNU patch.

GNU patch in the version used does understand GIT patch headers and does 
support things like renames.

The --dry-run option often will not work when the same file is modified more 
than once in the same patch, though.  This is because GNU patch doesn't 
remember the intermediary states of files.

In this case, the patch itself is broken.

Andreas

Re: [bug-patch] Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Russell King - ARM Linux]

On Sat, Sep 04, 2010 at 11:33:51PM +0200, Andreas Gruenbacher wrote:
> On Saturday 04 September 2010 02:03:48 Russell King - ARM Linux wrote:
> > As I say, it's because GNU patch doesn't (currently) understand GIT
> > patches.  I wouldn't call that a bug in GNU patch.
> 
> GNU patch in the version used does understand GIT patch headers and does 
> support things like renames.
> 
> The --dry-run option often will not work when the same file is modified more 
> than once in the same patch, though.  This is because GNU patch doesn't 
> remember the intermediary states of files.
> 
> In this case, the patch itself is broken.

GIT has many options to control how it produces patches, and -C or -M
allow it to reduce the size of patches making them more reviewable.  It
also makes them incompatible with GNU patch, whether or not GNU patch
understands the GIT headers.

As I've already said - if you want GNU compatible patches, don't generate
GIT patches using -C or -M.

Simples.

Re: [bug-patch] Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Andreas Gruenbacher]

On Saturday 04 September 2010 23:45:27 Russell King - ARM Linux wrote:
> It also makes them incompatible with GNU patch, whether or not GNU patch
> understands the GIT headers.

Aha?  Then why do you think GNU patch tries to understand the GIt patch 
headers?  So that it can be incompatible with GIT?

Andreas

Re: [bug-patch] [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Andreas Gruenbacher]

On Saturday 04 September 2010 01:32:52 Jonathan Nieder wrote:
> Andreas Gruenbacher wrote:
> 
> > something pretty bizarre is going on here.  The wget output modifies the same 
> > file twice, but both patches to this file have the same source sha1 (5645f35):
> 
> From the git v1.6.0-rc0~92 changelog entry:
> 
>     apply: fix copy/rename breakage
>     
>     7ebd52a (Merge branch 'dz/apply-again', 2008-07-01) taught "git-apply" to
>     grok a (non-git) patch that is a concatenation of separate patches that
>     touch the same file number of times, by recording the postimage of patch
>     application of previous round and using it as the preimage for later
>     rounds.
>     
>     This "incremental" mode of patch application fundamentally contradicts
>     with the way git rename/copy patches are designed.  When a git patch talks
>     about a file A getting modified, and a new file B created out of A, like
>     this:
>     
>         diff --git a/A b/A
>         --- a/A
>         +++ b/A
>         ... change text here ...
>         diff --git a/A b/B
>         copy from A
>         copy to B
>         --- a/A
>         +++ b/B
>         ... change text here ...
>     
>     the second change to produce B does not depend on what is done to A with
>     the first change in any way.  This is explicitly done so for reviewability
>     of individual patches.
>     
>     With this commit, we do not look at 'fn_table' that records the postimage
>     of previous round when applying a patch to produce a new file out of an
>     existing file.

Ouch ... this gets really messy when a user concatenates git style patches
and they are not applied to exactly the same source tree.

Thanks for digging out this commit message!

Andreas

Re: [bug-patch] Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Russell King - ARM Linux]

On Sat, Sep 04, 2010 at 11:46:11PM +0200, Andreas Gruenbacher wrote:
> On Saturday 04 September 2010 23:45:27 Russell King - ARM Linux wrote:
> > It also makes them incompatible with GNU patch, whether or not GNU patch
> > understands the GIT headers.
> 
> Aha?  Then why do you think GNU patch tries to understand the GIt patch 
> headers?  So that it can be incompatible with GIT?

Read what you said last time around.  "In this case, the patch itself is
broken."

So, because GNU patch doesn't understand the patch file, the patch file
must be broken?  No, the patch file is fine with GIT which can apply it
correctly, but incompatible with GNU patch because of the way GNU patch
works (as you yourself said, GNU patch doesn't keep the intermediate
states.)

I repeat - if you want maximum compatibility, want GNU patch to be able
to apply the patch with or without --dry-run, then don't use -C or -M
when generating patches with git.

Simples.

Re: [bug-patch] Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c)

[Andreas Gruenbacher]

On Sunday 05 September 2010 00:01:52 Russell King - ARM Linux wrote:
> On Sat, Sep 04, 2010 at 11:46:11PM +0200, Andreas Gruenbacher wrote:
> > On Saturday 04 September 2010 23:45:27 Russell King - ARM Linux wrote:
> > > It also makes them incompatible with GNU patch, whether or not GNU patch
> > > understands the GIT headers.
> > 
> > Aha?  Then why do you think GNU patch tries to understand the GIt patch 
> > headers?  So that it can be incompatible with GIT?
> 
> Read what you said last time around.  "In this case, the patch itself is
> broken."

I was corrected on that by Jonathan Nieder's mail which describes a detail of 
the GIT patch format that I didn't know about.  So it's a bug in the way GNU 
patch handles the git patch format and not an error in the patch.  Not nice 
and probably not easy to fix, but not fundamentally unfixable.

Andreas