threads / patch / 14387

patchapply: fix copy/rename breakage

Subject: [PATCH] apply: fix copy/rename breakage

## tl;dr

6 messages between Jul 10, 2008 and Jul 10, 2008. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Junio C Hamano· Jul 10, 2008, 03:10 UTC · lore

Recently, 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 files, by recording the postimage of patch application of previous round and using it as the preimage for later rounds.

However, this "incremental" mode of patch application contradicts with the way git rename/copy patches are fundamentally designed. When a git patch talks about a file A getting modified, and a new file B created out of B, 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 (this is explicitly done so for reviewability of individual patches).

With this patch, we disable the postimage record 'fn_table' when applying a patch to produce new files out of existing file by copying to fix this issue.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Applies to 'master'.  I am CC'ing Linus not because he is in any way
   responsible for this breakage, but because this breakage can affect
   heavy users of "git apply".
 builtin-apply.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)
Show changes to builtin-apply.c +7 −3
diff --git a/builtin-apply.c b/builtin-apply.c
index b3fc290..d13313f 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2296,7 +2296,8 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
 
 	strbuf_init(&buf, 0);
 
-	if ((tpatch = in_fn_table(patch->old_name)) != NULL) {
+	if (!(patch->is_copy || patch->is_rename) &&
+	    ((tpatch = in_fn_table(patch->old_name)) != NULL)) {
 		if (tpatch == (struct patch *) -1) {
 			return error("patch %s has been renamed/deleted",
 				patch->old_name);
@@ -2375,7 +2376,7 @@ static int verify_index_match(struct cache_entry *ce, struct stat *st)
 static int check_preimage(struct patch *patch, struct cache_entry **ce, struct stat *st)
 {
 	const char *old_name = patch->old_name;
-	struct patch *tpatch;
+	struct patch *tpatch = NULL;
 	int stat_ret = 0;
 	unsigned st_mode = 0;
 
@@ -2389,7 +2390,9 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 		return 0;
 
 	assert(patch->is_new <= 0);
-	if ((tpatch = in_fn_table(old_name)) != NULL) {
+
+	if (!(patch->is_copy || patch->is_rename) &&
+	    (tpatch = in_fn_table(old_name)) != NULL) {
 		if (tpatch == (struct patch *) -1) {
 			return error("%s: has been deleted/renamed", old_name);
 		}
@@ -2399,6 +2402,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 		if (stat_ret && errno != ENOENT)
 			return error("%s: %s", old_name, strerror(errno));
 	}
+
 	if (check_index && !tpatch) {
 		int pos = cache_name_pos(old_name, strlen(old_name));
 		if (pos < 0) {

-- 
1.5.6.2.291.g7eef3
Junio C Hamano· Jul 10, 2008, 03:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] apply: fix copy/rename breakage

Junio C Hamano <gitster@pobox.com> writes:
> Recently, 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 files, by recording the
Eh, s/files/times/;
Show 14 quoted lines
> postimage of patch application of previous round and using it as the
> preimage for later rounds.
>
> However, this "incremental" mode of patch application contradicts with the
> way git rename/copy patches are fundamentally designed....
> a patch to produce new files out of existing file by copying to fix this
> issue.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * Applies to 'master'.  I am CC'ing Linus not because he is in any way
>    responsible for this breakage, but because this breakage can affect
>    heavy users of "git apply".
Stephan Beyer· Jul 10, 2008, 04:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] apply: fix copy/rename breakage

Hi,
On Wed, Jul 09, 2008 at 08:10:58PM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> 
> 	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 ...
Big thanks! Now my patch applies cleanly again and many others, too. So:
Tested-by: Stephan Beyer <s-beyer@gmx.net>
;)
Regards,
  Stephan
-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
Don Zickus· Jul 10, 2008, 14:01 UTC · re: Junio C Hamano · lore

Re: [PATCH] apply: fix copy/rename breakage

On Wed, Jul 09, 2008 at 08:10:58PM -0700, Junio C Hamano wrote:
Show 29 quoted lines
> Recently, 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 files, by recording the
> postimage of patch application of previous round and using it as the
> preimage for later rounds.
> 
> However, this "incremental" mode of patch application contradicts with the
> way git rename/copy patches are fundamentally designed.  When a git patch
> talks about a file A getting modified, and a new file B created out of B,
> 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 (this is explicitly done so for reviewability of
> individual patches).
> 
> With this patch, we disable the postimage record 'fn_table' when applying
> a patch to produce new files out of existing file by copying to fix this
> issue.
Odd.  I guess the way I read this workflow is
apply change X to A, copy A' to B, apply change Y to B => B' now has changes X+Y

But instead you are saying B' only has change Y because A is copied to B not A'.

Regardless, it doesn't affect my workflow.
ACK.

Cheers, Don

Johannes Sixt· Jul 10, 2008, 15:22 UTC · re: Don Zickus · lore

Re: [PATCH] apply: fix copy/rename breakage

Don Zickus schrieb:
Show 33 quoted lines
> On Wed, Jul 09, 2008 at 08:10:58PM -0700, Junio C Hamano wrote:
>> However, this "incremental" mode of patch application contradicts with the
>> way git rename/copy patches are fundamentally designed.  When a git patch
>> talks about a file A getting modified, and a new file B created out of B,
>> like this:
>>
>> 	diff --git a/A b/A
>> 	--- a/A
>> 	+++ b/A
>> 	... change text X here ...
>> 	diff --git a/A b/B
>> 	copy from A
>> 	copy to B
>> 	--- a/A
>> 	+++ b/B
>> 	... change text Y here ...
>>
>> the second change to produce B does not depend on what is done to A with
>> the first change (this is explicitly done so for reviewability of
>> individual patches).
>>
>> With this patch, we disable the postimage record 'fn_table' when applying
>> a patch to produce new files out of existing file by copying to fix this
>> issue.
> 
> Odd.  I guess the way I read this workflow is
> 
> apply change X to A, copy A' to B, apply change Y to B => B' now has changes X+Y
> 
> But instead you are saying B' only has change Y because A is copied to B
> not A'.
> 
> Regardless, it doesn't affect my workflow.
Oh, it does. It's a normal git diff where a copy was detected!

Don't let you distract by the word "incremental" and by the names A and B. In the example above, the change X comes first because 'A' is sorted before 'B'. If the roles of A and B were swapped, then you have this patch:

 	diff --git a/A b/A
 	copy from B
 	copy to A
 	--- a/A
 	+++ b/A
 	... change text Y here ...
 	diff --git a/A b/B
 	--- a/A
 	+++ b/B
 	... change text X here ...
See?
-- Hannes
Don Zickus· Jul 10, 2008, 15:43 UTC · re: Johannes Sixt · lore

Re: [PATCH] apply: fix copy/rename breakage

On Thu, Jul 10, 2008 at 05:22:01PM +0200, Johannes Sixt wrote:
Show 31 quoted lines
> >> With this patch, we disable the postimage record 'fn_table' when applying
> >> a patch to produce new files out of existing file by copying to fix this
> >> issue.
> > 
> > Odd.  I guess the way I read this workflow is
> > 
> > apply change X to A, copy A' to B, apply change Y to B => B' now has changes X+Y
> > 
> > But instead you are saying B' only has change Y because A is copied to B
> > not A'.
> > 
> > Regardless, it doesn't affect my workflow.
> 
> Oh, it does. It's a normal git diff where a copy was detected!
> 
> Don't let you distract by the word "incremental" and by the names A and B.
> In the example above, the change X comes first because 'A' is sorted
> before 'B'. If the roles of A and B were swapped, then you have this patch:
> 
>  	diff --git a/A b/A
>  	copy from B
>  	copy to A
>  	--- a/A
>  	+++ b/A
>  	... change text Y here ...
>  	diff --git a/A b/B
>  	--- a/A
>  	+++ b/B
>  	... change text X here ...
> 
> See?
Yes, thank you!

Cheers, Don

← back to recent threads