[PATCH] apply: fix copy/rename breakage

[PATCH] apply: fix copy/rename breakage

[Junio C Hamano]

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

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

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

[Junio C Hamano]

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

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

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

[Stephan Beyer]

Hi,

On Wed, Jul 09, 2008 at 08:10:58PM -0700, Junio C Hamano wrote:
> 
> 	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

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

[Don Zickus]

On Wed, Jul 09, 2008 at 08:10:58PM -0700, Junio C Hamano wrote:
> 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

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

[Johannes Sixt]

Don Zickus schrieb:
> 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

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

[Don Zickus]

On Thu, Jul 10, 2008 at 05:22:01PM +0200, Johannes Sixt wrote:
> >> 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