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

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

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

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

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

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

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

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

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

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

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

> 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

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

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