# Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c 16 messages from 2010-09-03 to 2010-09-04. Participants: Uwe Kleine-König, Jonathan Nieder, Russell King - ARM Linux, Matthieu Moy, Andreas Schwab, Andreas Gruenbacher, Jean-Christophe PLAGNIOL-VILLARD. Thread: https://gitlist.dev/t/24960 ## Uwe Kleine-König, 2010-09-03 18:23 Subject: Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c Message-ID: <20100903182323.GA17152@pengutronix.de> URL: https://gitlist.dev/e/20100903182323.GA17152%40pengutronix.de In-Reply-To: ``` 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 > 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 > > --- > > 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/ | ``` ## Jonathan Nieder, 2010-09-03 18:43 Subject: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <20100903184351.GC2341@burratino> URL: https://gitlist.dev/e/20100903184351.GC2341%40burratino In-Reply-To: <20100903182323.GA17152@pengutronix.de> ``` (+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? ``` ## Russell King - ARM Linux, 2010-09-03 19:29 Subject: Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <20100903192907.GA2978@n2100.arm.linux.org.uk> URL: https://gitlist.dev/e/20100903192907.GA2978%40n2100.arm.linux.org.uk In-Reply-To: <20100903184351.GC2341@burratino> ``` 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. ``` ## Uwe Kleine-König, 2010-09-03 19:33 Subject: Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <20100903193309.GC29821@pengutronix.de> URL: https://gitlist.dev/e/20100903193309.GC29821%40pengutronix.de In-Reply-To: <20100903192907.GA2978@n2100.arm.linux.org.uk> ``` 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/ | ``` ## Matthieu Moy, 2010-09-03 19:34 Subject: Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: URL: https://gitlist.dev/e/vpqr5habp05.fsf%40bauges.imag.fr In-Reply-To: <20100903192907.GA2978@n2100.arm.linux.org.uk> ``` Russell King - ARM Linux 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/ ``` ## Andreas Schwab, 2010-09-03 19:45 Subject: Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: URL: https://gitlist.dev/e/m2r5ha7gri.fsf%40igel.home In-Reply-To: <20100903193309.GC29821@pengutronix.de> ``` Uwe Kleine-König 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." ``` ## Andreas Gruenbacher, 2010-09-03 22:58 Subject: Re: [bug-patch] [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <201009040058.18028.agruen@suse.de> URL: https://gitlist.dev/e/201009040058.18028.agruen%40suse.de In-Reply-To: <20100903184351.GC2341@burratino> ``` 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 ``` ## Jonathan Nieder, 2010-09-03 23:32 Subject: Re: [bug-patch] [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <20100903233252.GD30310@burratino> URL: https://gitlist.dev/e/20100903233252.GD30310%40burratino In-Reply-To: <201009040058.18028.agruen@suse.de> ``` 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 ``` ## Russell King - ARM Linux, 2010-09-04 00:03 Subject: Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <20100904000348.GA7498@n2100.arm.linux.org.uk> URL: https://gitlist.dev/e/20100904000348.GA7498%40n2100.arm.linux.org.uk In-Reply-To: <20100903193309.GC29821@pengutronix.de> ``` 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. ``` ## Jean-Christophe PLAGNIOL-VILLARD, 2010-09-04 03:21 Subject: Re: [bug-patch] [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <20100904032117.GL17702@game.jcrosoft.org> URL: https://gitlist.dev/e/20100904032117.GL17702%40game.jcrosoft.org In-Reply-To: <201009040058.18028.agruen@suse.de> ``` > > 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. ``` ## Andreas Gruenbacher, 2010-09-04 21:33 Subject: Re: [bug-patch] Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <201009042333.51419.agruen@suse.de> URL: https://gitlist.dev/e/201009042333.51419.agruen%40suse.de In-Reply-To: <20100904000348.GA7498@n2100.arm.linux.org.uk> ``` 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 ``` ## Russell King - ARM Linux, 2010-09-04 21:45 Subject: Re: [bug-patch] Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <20100904214527.GA20444@n2100.arm.linux.org.uk> URL: https://gitlist.dev/e/20100904214527.GA20444%40n2100.arm.linux.org.uk In-Reply-To: <201009042333.51419.agruen@suse.de> ``` 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. ``` ## Andreas Gruenbacher, 2010-09-04 21:46 Subject: Re: [bug-patch] Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <201009042346.11787.agruen@suse.de> URL: https://gitlist.dev/e/201009042346.11787.agruen%40suse.de In-Reply-To: <20100904214527.GA20444@n2100.arm.linux.org.uk> ``` 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 ``` ## Andreas Gruenbacher, 2010-09-04 21:57 Subject: Re: [bug-patch] [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <201009042357.34237.agruen@suse.de> URL: https://gitlist.dev/e/201009042357.34237.agruen%40suse.de In-Reply-To: <20100903233252.GD30310@burratino> ``` 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 ``` ## Russell King - ARM Linux, 2010-09-04 22:01 Subject: Re: [bug-patch] Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <20100904220152.GB20444@n2100.arm.linux.org.uk> URL: https://gitlist.dev/e/20100904220152.GB20444%40n2100.arm.linux.org.uk In-Reply-To: <201009042346.11787.agruen@suse.de> ``` 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. ``` ## Andreas Gruenbacher, 2010-09-04 22:26 Subject: Re: [bug-patch] Re: [BUG?] rename patch accepted with --dry-run, rejected without (Re: [PATCH V3] arm & sh: factorised duplicated clkdev.c) Message-ID: <201009050026.08235.agruen@suse.de> URL: https://gitlist.dev/e/201009050026.08235.agruen%40suse.de In-Reply-To: <20100904220152.GB20444@n2100.arm.linux.org.uk> ``` 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 ```