{"thread":{"id":"25509","subject":"[PATCH] Fix git-apply with -p greater than 1","startedAt":"2010-10-21T22:12:02Z","lastAt":"2010-10-25T14:11:37Z","messageCount":9,"participants":["Federico Cuello","Junio C Hamano","Jonathan Nieder","Fede"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"154037","messageId":"1287699122-26702-1-git-send-email-fedux@lugmen.org.ar","threadId":"25509","inReplyTo":null,"subject":"[PATCH] Fix git-apply with -p greater than 1","fromName":"Federico Cuello","fromEmail":"fedux@lugmen.org.ar","sentAt":"2010-10-21T22:12:02Z","receivedAt":"2010-10-21T22:12:02Z","isPatch":true,"sender":{"key":"fedux@lugmen.org.ar","avatar":"https://gravatar.com/avatar/c8a677962541cdbea703958c87586cf2ff6f2fbc6e6f21ad189dd75d6f029d50?d=mp&s=160"},"body":"Fix the case when the patch is a rename or mode-change only\nand -p is used with a value greater than one.\nThe git_header_name function did not remove more than one path\ncomponent.\n\nSigned-off-by: Federico Cuello \n---\n builtin/apply.c | 14 +++++---------\n 1 files changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 23c18c5..14996f8 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1132,15 +1132,11 @@ static char *git_header_name(char *line, int llen)\n \t\tcase '\\n':\n \t\t\treturn NULL;\n \t\tcase '\\t': case ' ':\n-\t\t\tsecond = name+len;\n-\t\t\tfor (;;) {\n-\t\t\t\tchar c = *second++;\n-\t\t\t\tif (c == '\\n')\n-\t\t\t\t\treturn NULL;\n-\t\t\t\tif (c == '/')\n-\t\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (second[len] == '\\n' && !memcmp(name, second, len)) {\n+\t\t\tsecond = stop_at_slash(name + len, (name - line) + len);\n+\t\t\tif (!second)\n+\t\t\t\treturn NULL;\n+\t\t\tsecond++;\n+\t\t\tif (second[len] == '\\n' && !strncmp(name, second, len)) {\n \t\t\t\treturn xmemdupz(name, len);\n \t\t\t}\n \t\t}\n-- \n1.7.3.1\n"},{"id":"154097","messageId":"7viq0urfbz.fsf@alter.siamese.dyndns.org","threadId":"25509","inReplyTo":"1287699122-26702-1-git-send-email-fedux@lugmen.org.ar","subject":"Re: [PATCH] Fix git-apply with -p greater than 1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-22T05:01:20Z","receivedAt":"2010-10-22T05:01:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Federico Cuello writes:\n\n> Fix the case when the patch is a rename or mode-change only\n> and -p is used with a value greater than one.\n> The git_header_name function did not remove more than one path\n> component.\n>\n> Signed-off-by: Federico Cuello \n\nThanks.\n\nIt is customary to protect your fix with additional test script so that\nyou do not have to be constantly monitoring the mailing list traffic to\nmake sure somebody else will not break your changes.\n\nPerhaps something like this? Note that I didn't run the test myself,\nthough (for that matter, I haven't applied your patch to see if it\ncompiles, yet).\n\n\n\n t/t4120-apply-popt.sh | 26 ++++++++++++++++++++++++++\n 1 files changed, 26 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh\nindex 2b2d00b..579c9e6 100755\n--- a/t/t4120-apply-popt.sh\n+++ b/t/t4120-apply-popt.sh\n@@ -56,4 +56,30 @@ test_expect_success 'apply with too large -p and fancy filename' '\n \tgrep \"removing 3 leading\" err\n '\n \n+test_expect_success 'apply (-p2) diff, mode change only' '\n+\tcat >patch.chmod <<-\\EOF &&\n+\tdiff --git a/sub/file1 b/sub/file1\n+\told mode 100644\n+\tnew mode 100755\n+\tEOF\n+\tchmod 644 file1 &&\n+\tgit apply -p2 patch.chmod &&\n+\ttest -x file1\n+'\n+\n+test_expect_success 'apply (-p2) diff, rename' '\n+\tcat >patch.rename <<-\\EOF &&\n+\tdiff --git a/sub/file1 b/sub/file2\n+\tsimilarity index 100%\n+\trename from sub/file1\n+\trename to sub/file2\n+\tEOF\n+\techo A >expected &&\n+\n+\tcp file1.saved file1 &&\n+\trm -f file2 &&\n+\tgit apply -p2 patch.rename &&\n+\ttest_cmp expected file2\n+'\n+\n test_done\n"},{"id":"154103","messageId":"20101022053140.GB786@burratino","threadId":"25509","inReplyTo":"7viq0urfbz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix git-apply with -p greater than 1","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-22T05:31:40Z","receivedAt":"2010-10-22T05:31:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> +test_expect_success 'apply (-p2) diff, mode change only' '\n> +\tcat >patch.chmod <<-\\EOF &&\n> +\tdiff --git a/sub/file1 b/sub/file1\n> +\told mode 100644\n> +\tnew mode 100755\n> +\tEOF\n> +\tchmod 644 file1 &&\n> +\tgit apply -p2 patch.chmod &&\n> +\ttest -x file1\n\nI had thought -p was only supposed to apply to traditional patches.\nMaybe a documentation update would avoid confusion?\n\n\t-p\n\t Remove leading slashes from traditional diff paths.\n\t The default is 1.\n"},{"id":"154144","messageId":"4CC194DF.9040803@lugmen.org.ar","threadId":"25509","inReplyTo":"20101022053140.GB786@burratino","subject":"Re: [PATCH] Fix git-apply with -p greater than 1","fromName":"Fede","fromEmail":"fedux@lugmen.org.ar","sentAt":"2010-10-22T13:42:55Z","receivedAt":"2010-10-22T13:42:55Z","isPatch":true,"sender":{"key":"fedux@lugmen.org.ar","avatar":"https://gravatar.com/avatar/c8a677962541cdbea703958c87586cf2ff6f2fbc6e6f21ad189dd75d6f029d50?d=mp&s=160"},"body":"Jonathan Nieder wrote:\n> Junio C Hamano wrote:\n>> +test_expect_success 'apply (-p2) diff, mode change only' '\n>> +\tcat >patch.chmod <<-\\EOF &&\n>> +\tdiff --git a/sub/file1 b/sub/file1\n>> +\told mode 100644\n>> +\tnew mode 100755\n>> +\tEOF\n>> +\tchmod 644 file1 &&\n>> +\tgit apply -p2 patch.chmod &&\n>> +\ttest -x file1\n> I had thought -p was only supposed to apply to traditional patches.\n> Maybe a documentation update would avoid confusion?\n>\n> \t-p\n> \t Remove leading slashes from traditional diff paths.\n> \t The default is 1.\n\nCurrently, if the patch is mode-change only then the filename is taken\nfrom the line \"diff --git ...\". There is a function git_header_name()\nthat extracts that name. This function could be a lot simpler and I'm\nworking on that, but meanwhile I'd like to provide a quick fix.\n\nRight now, if at least one of the filenames is double quoted then -p\ngreater than 1 works correctly. The only failing case is when both names\nare unquoted and my patch fixes that.\n\nThis work with -p2:\n\ndiff --git \"a/sub/file1\" b/sub/file1\nold mode 100644\nnew mode 100755\n\ndiff --git a/sub/file1 \"b/sub/file1\"\nold mode 100644\nnew mode 100755\n\ndiff --git \"a/sub/file1\" \"b/sub/file1\"\nold mode 100644\nnew mode 100755\n\nThis one doesn't:\n\ndiff --git a/sub/file1 b/sub/file1\nold mode 100644\nnew mode 100755\n\nMy patch fixes the later case.\n\nThere is a similar issue with renames and I'm working on that.\nMeanwhile, I'd like to know what do you think of that.\n\nPS: Hamano's test 'apply (-p2) diff, mode change only' fails without my\npatch and passes with it. I'll provide a new patch including the test.\n"},{"id":"154159","messageId":"20101022153803.GD9224@burratino","threadId":"25509","inReplyTo":"4CC194DF.9040803@lugmen.org.ar","subject":"Re: [PATCH] Fix git-apply with -p greater than 1","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-22T15:38:03Z","receivedAt":"2010-10-22T15:38:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Fede wrote:\n> Jonathan Nieder wrote:\n\n>> I had thought -p was only supposed to apply to traditional patches.\n>> Maybe a documentation update would avoid confusion?\n>>\n>> \t-p\n>> \t Remove leading slashes from traditional diff paths.\n>> \t The default is 1.\n>\n> Currently, if the patch is mode-change only then the filename is taken\n> from the line \"diff --git ...\".\n\nAh, found it:\n\ncommit 79ee194e52a140412da475e102145bad80d5d00a\nAuthor: Shawn O. Pearce \nDate: Wed Apr 4 11:19:14 2007 -0400\n\n Honor -p when applying git diffs\n \n If the user is trying to apply a Git generated diff file and they\n have specified a -p option, where is not 1, the user probably\n has a good reason for doing this. Such as they are me, trying to\n apply a patch generated in git.git for the git-gui subdirectory to\n the git-gui.git repository, where there is no git-gui subdirectory\n present.\n \n Users shouldn't supply -p2 unless they mean it. But if they are\n supplying it, they probably have thought about how to make this\n patch apply to their working directory, and want to risk whatever\n results may come from that.\n \n Signed-off-by: Shawn O. Pearce \n Signed-off-by: Junio C Hamano \n\n-- 8< --\nSubject: Documentation: update description of \"git apply -p\"\n\nThe \"git apply -p\" (strip path components) option has gone through some\nchanges since it was last documented:\n\n ec7fc0b (builtin-apply.c: pay attention to -p when..., 2009-11-25)\n 79ee194 (Honor -p when applying git diffs, 2007-04-04)\n 3e8a5db (git-apply: guess correct -p value for non-git..., 2007-02-21)\n 56185f4 (git-apply: require -p when working in a subdirectory, 2007-02-19)\n\nDocument some of the new rules:\n\n - the -p option applies to both traditional and --git format diffs\n - in traditional diffs, since there is no customary standard -p value,\n 'git apply' will try to guess\n - in all cases, the default is 1\n\nSigned-off-by: Jonathan Nieder \n---\nThoughts? Improvements?\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex 4a74b23..8bb7422 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -103,8 +103,11 @@ respectively, and the pathname will be enclosed in double quotes if\n any of those replacements occurred.\n \n -p::\n-\tRemove leading slashes from traditional diff paths. The\n-\tdefault is 1.\n+\tRemove leading path components from paths found in the\n+\tdiff. The default is 1 for --git format diffs. For\n+\ttraditional diffs, if this option is not supplied, 'git apply'\n+\twill try to detect an appropriate -p value, defaulting to 1\n+\tif there is not enough information to guess.\n \n -C::\n \tEnsure at least lines of surrounding context match before\n"},{"id":"154185","messageId":"7vd3r2m5p2.fsf@alter.siamese.dyndns.org","threadId":"25509","inReplyTo":"20101022053140.GB786@burratino","subject":"Re: [PATCH] Fix git-apply with -p greater than 1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-22T18:40:41Z","receivedAt":"2010-10-22T18:40:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>\n>> +test_expect_success 'apply (-p2) diff, mode change only' '\n>> +\tcat >patch.chmod <<-\\EOF &&\n>> +\tdiff --git a/sub/file1 b/sub/file1\n>> +\told mode 100644\n>> +\tnew mode 100755\n>> +\tEOF\n>> +\tchmod 644 file1 &&\n>> +\tgit apply -p2 patch.chmod &&\n>> +\ttest -x file1\n>\n> I had thought -p was only supposed to apply to traditional patches.\n\nYou are correct that historically it was invented for use with non-git\npatches, but I didn't think of a good reason why we should forbid it\noffhand.\n\n> Maybe a documentation update would avoid confusion?\n>\n> \t-p\n> \t Remove leading slashes from traditional diff paths.\n> \t The default is 1.\n\nIf we have a good reason to forbid this option on git generated patches,\nperhaps yes, and with either error/warning codepath, but first lets find\nout a good reason why we should forbid it. There may indeed be some.\n"},{"id":"154184","messageId":"7v8w1qm5n2.fsf@alter.siamese.dyndns.org","threadId":"25509","inReplyTo":"4CC194DF.9040803@lugmen.org.ar","subject":"Re: [PATCH] Fix git-apply with -p greater than 1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-22T18:41:53Z","receivedAt":"2010-10-22T18:41:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fede writes:\n\n> There is a similar issue with renames and I'm working on that.\n\nI think I queued a fix-up in 'pu' on top of your patch last night. Does\nit work for you?\n"},{"id":"154187","messageId":"4CC1DD20.1080500@lugmen.org.ar","threadId":"25509","inReplyTo":"7v8w1qm5n2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix git-apply with -p greater than 1","fromName":"Federico Cuello","fromEmail":"fedux@lugmen.org.ar","sentAt":"2010-10-22T18:51:12Z","receivedAt":"2010-10-22T18:51:12Z","isPatch":true,"sender":{"key":"fedux@lugmen.org.ar","avatar":"https://gravatar.com/avatar/c8a677962541cdbea703958c87586cf2ff6f2fbc6e6f21ad189dd75d6f029d50?d=mp&s=160"},"body":"El 22/10/10 15:41, Junio C Hamano escribió:\n> Fede writes:\n>\n>> There is a similar issue with renames and I'm working on that.\n> I think I queued a fix-up in 'pu' on top of your patch last night. Does\n> it work for you?\n\nYes, is almost what I got. I also checked p_value not to be 0.\n\nAnyway, the whole thing is broken when -p 0 , and it requires a little\nbit more work. Specially in apply.c:stop_at_slash().\n\nI have this:\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 14996f8..3197e38 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -919,28 +919,28 @@ static int gitdiff_newfile(const char *line,\nstruct patch *patch)\n static int gitdiff_copysrc(const char *line, struct patch *patch)\n {\n patch->is_copy = 1;\n- patch->old_name = find_name(line, NULL, 0, 0);\n+ patch->old_name = find_name(line, NULL, p_value ? p_value - 1 :\n0, 0);\n return 0;\n }\n \n static int gitdiff_copydst(const char *line, struct patch *patch)\n {\n patch->is_copy = 1;\n- patch->new_name = find_name(line, NULL, 0, 0);\n+ patch->new_name = find_name(line, NULL, p_value ? p_value - 1 :\n0, 0);\n return 0;\n }\n \n static int gitdiff_renamesrc(const char *line, struct patch *patch)\n {\n patch->is_rename = 1;\n- patch->old_name = find_name(line, NULL, 0, 0);\n+ patch->old_name = find_name(line, NULL, p_value ? p_value - 1 :\n0, 0);\n return 0;\n }\n \n static int gitdiff_renamedst(const char *line, struct patch *patch)\n {\n patch->is_rename = 1;\n- patch->new_name = find_name(line, NULL, 0, 0);\n+ patch->new_name = find_name(line, NULL, p_value ? p_value - 1 :\n0, 0);\n return 0;\n }\n"},{"id":"154408","messageId":"1288015897-21565-1-git-send-email-fedux@lugmen.org.ar","threadId":"25509","inReplyTo":"7v8w1qm5n2.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Fix git-apply with -p greater than 1","fromName":"Federico Cuello","fromEmail":"fedux@lugmen.org.ar","sentAt":"2010-10-25T14:11:37Z","receivedAt":"2010-10-25T14:11:37Z","isPatch":true,"sender":{"key":"fedux@lugmen.org.ar","avatar":"https://gravatar.com/avatar/c8a677962541cdbea703958c87586cf2ff6f2fbc6e6f21ad189dd75d6f029d50?d=mp&s=160"},"body":"Fix for rename/copy patches. Filenames don't include\n'a/' or 'b/' prefix, then use p_value minus one.\n\nAlso add 'copy' test script.\n\nSigned-off-by: Federico Cuello \n---\n builtin/apply.c | 8 ++++----\n t/t4120-apply-popt.sh | 13 +++++++++++++\n 2 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 14996f8..3197e38 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -919,28 +919,28 @@ static int gitdiff_newfile(const char *line, struct patch *patch)\n static int gitdiff_copysrc(const char *line, struct patch *patch)\n {\n \tpatch->is_copy = 1;\n-\tpatch->old_name = find_name(line, NULL, 0, 0);\n+\tpatch->old_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);\n \treturn 0;\n }\n \n static int gitdiff_copydst(const char *line, struct patch *patch)\n {\n \tpatch->is_copy = 1;\n-\tpatch->new_name = find_name(line, NULL, 0, 0);\n+\tpatch->new_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);\n \treturn 0;\n }\n \n static int gitdiff_renamesrc(const char *line, struct patch *patch)\n {\n \tpatch->is_rename = 1;\n-\tpatch->old_name = find_name(line, NULL, 0, 0);\n+\tpatch->old_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);\n \treturn 0;\n }\n \n static int gitdiff_renamedst(const char *line, struct patch *patch)\n {\n \tpatch->is_rename = 1;\n-\tpatch->new_name = find_name(line, NULL, 0, 0);\n+\tpatch->new_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);\n \treturn 0;\n }\n \ndiff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh\nindex 579c9e6..4ed06f0 100755\n--- a/t/t4120-apply-popt.sh\n+++ b/t/t4120-apply-popt.sh\n@@ -82,4 +82,17 @@ test_expect_success 'apply (-p2) diff, rename' '\n \ttest_cmp expected file2\n '\n \n+test_expect_success 'apply (-p2) diff, copy' '\n+\tcat >patch.copy <<-\\EOF &&\n+\tdiff --git a/sub/file1 b/sub/file2\n+\tsimilarity index 100%\n+\tcopy from sub/file1\n+\tcopy to sub/file2\n+\tEOF\n+\tcp file1.saved file1 &&\n+\trm -f file2 &&\n+\tgit apply -p2 patch.copy &&\n+\ttest_cmp file1 file2\n+'\n+\n test_done\n-- \n1.7.3.2\n"}]}