threads / patch / 25509

patchFix git-apply with -p greater than 1

Subject: [PATCH] Fix git-apply with -p greater than 1

## tl;dr

9 messages between Oct 21, 2010 and Oct 25, 2010. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Federico Cuello· Oct 21, 2010, 22:12 UTC · lore

Fix the case when the patch is a rename or mode-change only and -p is used with a value greater than one. The git_header_name function did not remove more than one path component.

Signed-off-by: Federico Cuello <fedux@lugmen.org.ar>
---
 builtin/apply.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)
Show changes to builtin/apply.c +5 −9
diff --git a/builtin/apply.c b/builtin/apply.c
index 23c18c5..14996f8 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1132,15 +1132,11 @@ static char *git_header_name(char *line, int llen)
 		case '\n':
 			return NULL;
 		case '\t': case ' ':
-			second = name+len;
-			for (;;) {
-				char c = *second++;
-				if (c == '\n')
-					return NULL;
-				if (c == '/')
-					break;
-			}
-			if (second[len] == '\n' && !memcmp(name, second, len)) {
+			second = stop_at_slash(name + len, (name - line) + len);
+			if (!second)
+				return NULL;
+			second++;
+			if (second[len] == '\n' && !strncmp(name, second, len)) {
 				return xmemdupz(name, len);
 			}
 		}

-- 
1.7.3.1
Junio C Hamano· Oct 22, 2010, 05:01 UTC · re: Federico Cuello · lore

Re: [PATCH] Fix git-apply with -p greater than 1

Federico Cuello <fedux@lugmen.org.ar> writes:
Show 6 quoted lines
> Fix the case when the patch is a rename or mode-change only
> and -p is used with a value greater than one.
> The git_header_name function did not remove more than one path
> component.
>
> Signed-off-by: Federico Cuello <fedux@lugmen.org.ar>
Thanks.

It is customary to protect your fix with additional test script so that you do not have to be constantly monitoring the mailing list traffic to make sure somebody else will not break your changes.

Perhaps something like this? Note that I didn't run the test myself, though (for that matter, I haven't applied your patch to see if it compiles, yet).

 t/t4120-apply-popt.sh |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)
Show changes to t/t4120-apply-popt.sh +26 −0
diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh
index 2b2d00b..579c9e6 100755
--- a/t/t4120-apply-popt.sh
+++ b/t/t4120-apply-popt.sh
@@ -56,4 +56,30 @@ test_expect_success 'apply with too large -p and fancy filename' '
 	grep "removing 3 leading" err
 '
 
+test_expect_success 'apply (-p2) diff, mode change only' '
+	cat >patch.chmod <<-\EOF &&
+	diff --git a/sub/file1 b/sub/file1
+	old mode 100644
+	new mode 100755
+	EOF
+	chmod 644 file1 &&
+	git apply -p2 patch.chmod &&
+	test -x file1
+'
+
+test_expect_success 'apply (-p2) diff, rename' '
+	cat >patch.rename <<-\EOF &&
+	diff --git a/sub/file1 b/sub/file2
+	similarity index 100%
+	rename from sub/file1
+	rename to sub/file2
+	EOF
+	echo A >expected &&
+
+	cp file1.saved file1 &&
+	rm -f file2 &&
+	git apply -p2 patch.rename &&
+	test_cmp expected file2
+'
+
 test_done
Jonathan Nieder· Oct 22, 2010, 05:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix git-apply with -p greater than 1

Junio C Hamano wrote:
Show 9 quoted lines
> +test_expect_success 'apply (-p2) diff, mode change only' '
> +	cat >patch.chmod <<-\EOF &&
> +	diff --git a/sub/file1 b/sub/file1
> +	old mode 100644
> +	new mode 100755
> +	EOF
> +	chmod 644 file1 &&
> +	git apply -p2 patch.chmod &&
> +	test -x file1

I had thought -p was only supposed to apply to traditional patches. Maybe a documentation update would avoid confusion?

	-p<n>
	   Remove <n> leading slashes from traditional diff paths.
	   The default is 1.
Fede· Oct 22, 2010, 13:42 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Fix git-apply with -p greater than 1

Jonathan Nieder wrote:
Show 16 quoted lines
> Junio C Hamano wrote:
>> +test_expect_success 'apply (-p2) diff, mode change only' '
>> +	cat >patch.chmod <<-\EOF &&
>> +	diff --git a/sub/file1 b/sub/file1
>> +	old mode 100644
>> +	new mode 100755
>> +	EOF
>> +	chmod 644 file1 &&
>> +	git apply -p2 patch.chmod &&
>> +	test -x file1
> I had thought -p was only supposed to apply to traditional patches.
> Maybe a documentation update would avoid confusion?
>
> 	-p<n>
> 	   Remove <n> leading slashes from traditional diff paths.
> 	   The default is 1.

Currently, if the patch is mode-change only then the filename is taken from the line "diff --git ...". There is a function git_header_name() that extracts that name. This function could be a lot simpler and I'm working on that, but meanwhile I'd like to provide a quick fix.

Right now, if at least one of the filenames is double quoted then -p greater than 1 works correctly. The only failing case is when both names are unquoted and my patch fixes that.

This work with -p2:
Show changes to sub/file1 +0 −0
diff --git "a/sub/file1" b/sub/file1
old mode 100644
new mode 100755

diff --git a/sub/file1 "b/sub/file1"
old mode 100644
new mode 100755

diff --git "a/sub/file1" "b/sub/file1"
old mode 100644
new mode 100755

This one doesn't:

diff --git a/sub/file1 b/sub/file1
old mode 100644
new mode 100755

My patch fixes the later case.

There is a similar issue with renames and I'm working on that.
Meanwhile, I'd like to know what do you think of that.

PS: Hamano's test  'apply (-p2) diff, mode change only' fails without my
patch and passes with it. I'll provide a new patch including the test.
Jonathan Nieder· Oct 22, 2010, 15:38 UTC · re: Fede · lore

Re: [PATCH] Fix git-apply with -p greater than 1

Fede wrote:
> Jonathan Nieder wrote:
Show 9 quoted lines
>> I had thought -p was only supposed to apply to traditional patches.
>> Maybe a documentation update would avoid confusion?
>>
>> 	-p<n>
>> 	   Remove <n> leading slashes from traditional diff paths.
>> 	   The default is 1.
>
> Currently, if the patch is mode-change only then the filename is taken
> from the line "diff --git ...".
Ah, found it:
commit 79ee194e52a140412da475e102145bad80d5d00a
Author: Shawn O. Pearce <spearce@spearce.org>
Date:   Wed Apr 4 11:19:14 2007 -0400
    Honor -p<n> when applying git diffs
    
    If the user is trying to apply a Git generated diff file and they
    have specified a -p<n> option, where <n> is not 1, the user probably
    has a good reason for doing this.  Such as they are me, trying to
    apply a patch generated in git.git for the git-gui subdirectory to
    the git-gui.git repository, where there is no git-gui subdirectory
    present.
    
    Users shouldn't supply -p2 unless they mean it.  But if they are
    supplying it, they probably have thought about how to make this
    patch apply to their working directory, and want to risk whatever
    results may come from that.
    
    Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
    Signed-off-by: Junio C Hamano <junkio@cox.net>
-- 8< --
Subject: Documentation: update description of "git apply -p"

The "git apply -p" (strip path components) option has gone through some changes since it was last documented:

 ec7fc0b (builtin-apply.c: pay attention to -p<n> when..., 2009-11-25)
 79ee194 (Honor -p<n> when applying git diffs, 2007-04-04)
 3e8a5db (git-apply: guess correct -p<n> value for non-git..., 2007-02-21)
 56185f4 (git-apply: require -p<n> when working in a subdirectory, 2007-02-19)
Document some of the new rules:
 - the -p option applies to both traditional and --git format diffs
 - in traditional diffs, since there is no customary standard -p value,
   'git apply' will try to guess
 - in all cases, the default is 1
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thoughts?  Improvements?
Show changes to Documentation/git-apply.txt +5 −2
diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 4a74b23..8bb7422 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -103,8 +103,11 @@ respectively, and the pathname will be enclosed in double quotes if
 any of those replacements occurred.
 
 -p<n>::
-	Remove <n> leading slashes from traditional diff paths. The
-	default is 1.
+	Remove <n> leading path components from paths found in the
+	diff. The default is 1 for --git format diffs.  For
+	traditional diffs, if this option is not supplied, 'git apply'
+	will try to detect an appropriate -p value, defaulting to 1
+	if there is not enough information to guess.
 
 -C<n>::
 	Ensure at least <n> lines of surrounding context match before
Junio C Hamano· Oct 22, 2010, 18:41 UTC · re: Fede · lore

Re: [PATCH] Fix git-apply with -p greater than 1

Fede <fedux@lugmen.org.ar> writes:
> There is a similar issue with renames and I'm working on that.

I think I queued a fix-up in 'pu' on top of your patch last night. Does it work for you?

Federico Cuello· Oct 22, 2010, 18:51 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix git-apply with -p greater than 1

El 22/10/10 15:41, Junio C Hamano escribió:
Show 5 quoted lines
> Fede <fedux@lugmen.org.ar> writes:
>
>> There is a similar issue with renames and I'm working on that.
> I think I queued a fix-up in 'pu' on top of your patch last night.  Does
> it work for you?
Yes, is almost what I got. I also checked p_value not to be 0.

Anyway, the whole thing is broken when -p 0 , and it requires a little bit more work. Specially in apply.c:stop_at_slash().

I have this:
Show changes to builtin/apply.c +4 −4
diff --git a/builtin/apply.c b/builtin/apply.c
index 14996f8..3197e38 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -919,28 +919,28 @@ static int gitdiff_newfile(const char *line,
struct patch *patch)
 static int gitdiff_copysrc(const char *line, struct patch *patch)
 {
        patch->is_copy = 1;
-       patch->old_name = find_name(line, NULL, 0, 0);
+       patch->old_name = find_name(line, NULL, p_value ? p_value - 1 :
0, 0);
        return 0;
 }
 
 static int gitdiff_copydst(const char *line, struct patch *patch)
 {
        patch->is_copy = 1;
-       patch->new_name = find_name(line, NULL, 0, 0);
+       patch->new_name = find_name(line, NULL, p_value ? p_value - 1 :
0, 0);
        return 0;
 }
 
 static int gitdiff_renamesrc(const char *line, struct patch *patch)
 {
        patch->is_rename = 1;
-       patch->old_name = find_name(line, NULL, 0, 0);
+       patch->old_name = find_name(line, NULL, p_value ? p_value - 1 :
0, 0);
        return 0;
 }
 
 static int gitdiff_renamedst(const char *line, struct patch *patch)
 {
        patch->is_rename = 1;
-       patch->new_name = find_name(line, NULL, 0, 0);
+       patch->new_name = find_name(line, NULL, p_value ? p_value - 1 :
0, 0);
        return 0;
 }
Federico Cuello· Oct 25, 2010, 14:11 UTC · re: Junio C Hamano · lore

Fix for rename/copy patches. Filenames don't include 'a/' or 'b/' prefix, then use p_value minus one.

Also add 'copy' test script.
Signed-off-by: Federico Cuello <fedux@lugmen.org.ar>
---
 builtin/apply.c       |    8 ++++----
 t/t4120-apply-popt.sh |   13 +++++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)
Show changes to 2 files +17 −4

builtin/apply.c, t/t4120-apply-popt.sh

diff --git a/builtin/apply.c b/builtin/apply.c
index 14996f8..3197e38 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -919,28 +919,28 @@ static int gitdiff_newfile(const char *line, struct patch *patch)
 static int gitdiff_copysrc(const char *line, struct patch *patch)
 {
 	patch->is_copy = 1;
-	patch->old_name = find_name(line, NULL, 0, 0);
+	patch->old_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);
 	return 0;
 }
 
 static int gitdiff_copydst(const char *line, struct patch *patch)
 {
 	patch->is_copy = 1;
-	patch->new_name = find_name(line, NULL, 0, 0);
+	patch->new_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);
 	return 0;
 }
 
 static int gitdiff_renamesrc(const char *line, struct patch *patch)
 {
 	patch->is_rename = 1;
-	patch->old_name = find_name(line, NULL, 0, 0);
+	patch->old_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);
 	return 0;
 }
 
 static int gitdiff_renamedst(const char *line, struct patch *patch)
 {
 	patch->is_rename = 1;
-	patch->new_name = find_name(line, NULL, 0, 0);
+	patch->new_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);
 	return 0;
 }
 
diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh
index 579c9e6..4ed06f0 100755
--- a/t/t4120-apply-popt.sh
+++ b/t/t4120-apply-popt.sh
@@ -82,4 +82,17 @@ test_expect_success 'apply (-p2) diff, rename' '
 	test_cmp expected file2
 '
 
+test_expect_success 'apply (-p2) diff, copy' '
+	cat >patch.copy <<-\EOF &&
+	diff --git a/sub/file1 b/sub/file2
+	similarity index 100%
+	copy from sub/file1
+	copy to sub/file2
+	EOF
+	cp file1.saved file1 &&
+	rm -f file2 &&
+	git apply -p2 patch.copy &&
+	test_cmp file1 file2
+'
+
 test_done

-- 
1.7.3.2
Junio C Hamano· Oct 22, 2010, 18:40 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Fix git-apply with -p greater than 1

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 13 quoted lines
> Junio C Hamano wrote:
>
>> +test_expect_success 'apply (-p2) diff, mode change only' '
>> +	cat >patch.chmod <<-\EOF &&
>> +	diff --git a/sub/file1 b/sub/file1
>> +	old mode 100644
>> +	new mode 100755
>> +	EOF
>> +	chmod 644 file1 &&
>> +	git apply -p2 patch.chmod &&
>> +	test -x file1
>
> I had thought -p was only supposed to apply to traditional patches.

You are correct that historically it was invented for use with non-git patches, but I didn't think of a good reason why we should forbid it offhand.

Show 5 quoted lines
> Maybe a documentation update would avoid confusion?
>
> 	-p<n>
> 	   Remove <n> leading slashes from traditional diff paths.
> 	   The default is 1.

If we have a good reason to forbid this option on git generated patches, perhaps yes, and with either error/warning codepath, but first lets find out a good reason why we should forbid it. There may indeed be some.

← back to recent threads