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

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

[Federico Cuello]

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

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

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

[Junio C Hamano]

Federico Cuello <fedux@lugmen.org.ar> writes:

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

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

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

[Jonathan Nieder]

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.

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

[Fede]

Jonathan Nieder wrote:
> 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:

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.

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

[Jonathan Nieder]

Fede wrote:
> Jonathan Nieder wrote:

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

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

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

[Junio C Hamano]

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?

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

[Federico Cuello]

El 22/10/10 15:41, Junio C Hamano escribió:
> 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:

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

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

[Federico Cuello]

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

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

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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