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

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
[off-list ref] wrote:

quoted

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 <redacted>
---
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/  |

(+cc: bug-patch)

Hi,

Uwe Kleine-König wrote:

On Fri, Sep 03, 2010 at 07:18:43PM +0900, Magnus Damm wrote:

quoted

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?

On Fri, Sep 03, 2010 at 01:43:51PM -0500, Jonathan Nieder wrote:

(+cc: bug-patch)

Hi,

Uwe Kleine-König wrote:

quoted

On Fri, Sep 03, 2010 at 07:18:43PM +0900, Magnus Damm wrote:

quoted

quoted

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.

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:

quoted

Uwe Kleine-König wrote:

quoted

On Fri, Sep 03, 2010 at 07:18:43PM +0900, Magnus Damm wrote:

quoted

quoted

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

Hi,

On Friday 03 September 2010 20:43:51 Jonathan Nieder wrote:

Uwe Kleine-König wrote:

quoted

On Fri, Sep 03, 2010 at 07:18:43PM +0900, Magnus Damm wrote:

quoted

quoted

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):

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

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

On Fri, Sep 03, 2010 at 09:33:09PM +0200, Uwe Kleine-König wrote:

quoted

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.

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.

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

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:

quoted

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.

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

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:

quoted

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.

On Saturday 04 September 2010 01:32:52 Jonathan Nieder wrote:

Andreas Gruenbacher wrote:

quoted

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

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:

quoted

On Saturday 04 September 2010 23:45:27 Russell King - ARM Linux wrote:

quoted

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