git apply --directory broken for new files

DISCUSSION H. Peter Anvin 3 participants 5 messages Jun 15, 2016

Share & tools

H. Peter Anvin

author

2016-06-15 22:45 UTC #0 lore raw

Trying with git 1.6.0.1:

: tazenda 124 ; git apply --directory=gpxe/ < /tmp/diff
fatal: git-apply: bad git-diff - inconsistent new filename on line 105

Line 105 in the diff is:

101 diff --git a/src/arch/i386/firmware/pcbios/fakee820.c
b/src/arch/i386/firmware/pcbios/fakee820.c

   102  new file mode 100644
   103  index 0000000..e171edf
   104  --- /dev/null
   105  +++ b/src/arch/i386/firmware/pcbios/fakee820.c

What I'm trying to do here is to apply a diff created via "git diff" to a subdirectory of another git repository.

-hpa

Jeff King

2016-06-15 22:45 UTC #1 lore raw

Quoted 4 lines · H. Peter Anvin

Trying with git 1.6.0.1:
: tazenda 124 ; git apply --directory=gpxe/ < /tmp/diff
fatal: git-apply: bad git-diff - inconsistent new filename on line 105

Hmm. It looks like this is broken for deletion, too. I believe the problem is just that when sanity checking "diff --git" patches, we weren't applying the --directory root consistently.

I think this fix is sane, but I would appreciate ACKs from Dscho and Junio, who wrote the --directory code and the patch sanity checking code, respectively.

-- >8 --
apply --directory: handle creation and deletion patches

We carefully verify that the input to git-apply is sane, including cross-checking that the filenames we see in "+++" headers match what was provided on the command line of "diff --git". When --directory is used, however, we ended up comparing the unadorned name to one with the prepended root, causing us to complain about a mismatch.

We simply need to prepend the root directory, if any, when pulling the name out of the git header.

Signed-off-by Jeff King <[email protected]>

---
 builtin-apply.c       |    7 +++++++
 t/t4128-apply-root.sh |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)

builtin-apply.c +7 -0

index 2ab4aba..f9070d5 100644
@@ -787,6 +787,13 @@ static char *git_header_name(char *line, int llen)
 					break;
 			}
 			if (second[len] == '\n' && !memcmp(name, second, len)) {
+				if (root) {
+					char *ret = xmalloc(root_len + len + 1);
+					strcpy(ret, root);
+					memcpy(ret + root_len, name, len);
+					ret[root_len + len] = '\0';
+					return ret;
+				}
 				return xmemdupz(name, len);
 			}
 		}

t/t4128-apply-root.sh +35 -0

index 2dd0c75..bd1bdf1 100755
@@ -40,4 +40,39 @@ test_expect_success 'apply --directory -p (2) ' '
 
 '
 
+cat > patch << EOF
+diff --git a/newfile b/newfile
+new file mode 100644
+index 0000000..d95f3ad
+--- /dev/null
++++ b/newfile
+@@ -0,0 +1 @@
++content
+EOF
+
+test_expect_success 'apply --directory (new file)' '
+	git reset --hard initial &&
+	git apply --directory=some/sub/dir/ --index patch &&
+	test content = $(git show :some/sub/dir/newfile) &&
+	test content = $(cat some/sub/dir/newfile)
+'
+
+cat > patch << EOF
+diff --git a/delfile b/delfile
+deleted file mode 100644
+index d95f3ad..0000000
+--- a/delfile
++++ /dev/null
+@@ -1 +0,0 @@
+-content
+EOF
+
+test_expect_success 'apply --directory (delete file)' '
+	git reset --hard initial &&
+	echo content >some/sub/dir/delfile &&
+	git add some/sub/dir/delfile &&
+	git apply --directory=some/sub/dir/ --index patch &&
+	! git ls-files | grep delfile
+'
+
 test_done

Signature · 1 line

1.6.0.2.517.g8be702.dirty

Junio C Hamano

2016-06-15 22:45 UTC #2 lore raw

Quoted 10 lines · Jeff King

apply --directory: handle creation and deletion patches
We carefully verify that the input to git-apply is sane, including cross-checking that the filenames we see in "+++" headers match what was provided on the command line of "diff --git". When --directory is used, however, we ended up comparing the unadorned name to one with the prepended root, causing us to complain about a mismatch.
We simply need to prepend the root directory, if any, when pulling the name out of the git header.

Thanks.

c4730f3 (Teach "git apply" to prepend a prefix with "--root=<root>", 2008-07-01) did a half-baked job to teach find_name() which is used to parse traditional diff and also is used to set patch->old_name and patch->new_name by gitdiff_verify_name() when parsing "copy from", "copy to", "rename from", and "rename to". The caller of git_header_name() uses the return value to set patch->def_name that is used when "deleted file" and "new file" are parsed, which should have been taught this trick by the same commit.

However,...

Quoted 8 lines

builtin-apply.c +0 -0

index 2ab4aba..f9070d5 100644
@@ -787,6 +787,13 @@ static char *git_header_name(char *line, int llen)
 					break;
 			}
 			if (second[len] == '\n' && !memcmp(name, second, len)) {

builtin-apply.c static char *git_header_name(char *line, int llen) · line 799

Quoted 10 lines

builtin-apply.c +7 -0

+				if (root) {
+					char *ret = xmalloc(root_len + len + 1);
+					strcpy(ret, root);
+					memcpy(ret + root_len, name, len);
+					ret[root_len + len] = '\0';
+					return ret;
+				}
 				return xmemdupz(name, len);
 			}
 		}

I suspect this is only half of the story, because the code to parse:

diff --git "a/f\244o/bar.c" "b/f\244o/bar.c"

in the same function before the part you patched needs similar treatment. There are two return of strbuf_detach(&first, NULL) in the if(){} block, and the return value needs to be prefixed with the value of --directory when given.

It would be easier to do this --directory prefixing in the sole caller of git_header_name(), though.

Jeff King

2016-06-15 22:45 UTC #3 lore raw

Quoted 8 lines · Junio C Hamano

I suspect this is only half of the story, because the code to parse:
diff --git "a/f\244o/bar.c" "b/f\244o/bar.c"
in the same function before the part you patched needs similar treatment. There are two return of strbuf_detach(&first, NULL) in the if(){} block, and the return value needs to be prefixed with the value of --directory when given.

Ah, indeed. I remember looking for other returns, but somehow I managed to miss these completely obvious ones. Here is an updated patch, with a third test that fails on the old patch but works here.

Quoted 2 lines

It would be easier to do this --directory prefixing in the sole caller of git_header_name(), though.

I went this route, and it is much more readable if slightly less efficient (one extra malloc/free).

-- >8 --
git apply --directory broken for new files

We simply need to prepend the root directory, if any, when pulling the name out of the git header.

Signed-off-by Jeff King <[email protected]>

---
 builtin-apply.c       |    7 ++++++
 t/t4128-apply-root.sh |   52 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 0 deletions(-)

builtin-apply.c +7 -0

index bf80610..be7e1bd 100644
@@ -810,6 +810,13 @@ static int parse_git_header(char *line, int len, unsigned int size, struct patch
 	 * the default name from the header.
 	 */
 	patch->def_name = git_header_name(line, len);
+	if (patch->def_name && root) {
+		char *s = xmalloc(root_len + strlen(patch->def_name) + 1);
+		strcpy(s, root);
+		strcpy(s + root_len, patch->def_name);
+		free(patch->def_name);
+		patch->def_name = s;
+	}
 
 	line += len;
 	size -= len;

t/t4128-apply-root.sh +52 -0

56 lines not on this page · raw message

Signature · 1 line

1.6.0.2.519.g6cb82.dirty

Junio C Hamano

Acked-by

2016-06-15 22:45 UTC #4 lore raw

Quoted 5 lines · Jeff King

Quoted 2 lines
It would be easier to do this --directory prefixing in the sole caller of git_header_name(), though.
I went this route, and it is much more readable if slightly less efficient (one extra malloc/free).

I do not think efficiency is an issue in this codepath --- especially because it is only triggered when --directory is in use.

Acked-by Junio C Hamano <[email protected]>

Quoted 101 lines

-- >8 --
git apply --directory broken for new files

We simply need to prepend the root directory, if any, when pulling the name out of the git header.

Signed-off-by Jeff King <[email protected]>

---
 builtin-apply.c       |    7 ++++++
 t/t4128-apply-root.sh |   52 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 0 deletions(-)

builtin-apply.c +7 -0

index bf80610..be7e1bd 100644
@@ -810,6 +810,13 @@ static int parse_git_header(char *line, int len, unsigned int size, struct patch
 	 * the default name from the header.
 	 */
 	patch->def_name = git_header_name(line, len);
+	if (patch->def_name && root) {
+		char *s = xmalloc(root_len + strlen(patch->def_name) + 1);
+		strcpy(s, root);
+		strcpy(s + root_len, patch->def_name);
+		free(patch->def_name);
+		patch->def_name = s;
+	}
 
 	line += len;
 	size -= len;

t/t4128-apply-root.sh +52 -0

index 2dd0c75..bc7a8a8 100755
@@ -40,4 +40,56 @@ test_expect_success 'apply --directory -p (2) ' '
 
 '
 
+cat > patch << EOF
+diff --git a/newfile b/newfile
+new file mode 100644
+index 0000000..d95f3ad
+--- /dev/null
++++ b/newfile
+@@ -0,0 +1 @@
++content
+EOF
+
+test_expect_success 'apply --directory (new file)' '
+	git reset --hard initial &&
+	git apply --directory=some/sub/dir/ --index patch &&
+	test content = $(git show :some/sub/dir/newfile) &&
+	test content = $(cat some/sub/dir/newfile)
+'
+
+cat > patch << EOF
+diff --git a/delfile b/delfile
+deleted file mode 100644
+index d95f3ad..0000000
+--- a/delfile
++++ /dev/null
+@@ -1 +0,0 @@
+-content
+EOF
+
+test_expect_success 'apply --directory (delete file)' '
+	git reset --hard initial &&
+	echo content >some/sub/dir/delfile &&
+	git add some/sub/dir/delfile &&
+	git apply --directory=some/sub/dir/ --index patch &&
+	! git ls-files | grep delfile
+'
+
+cat > patch << 'EOF'
+diff --git "a/qu\157tefile" "b/qu\157tefile"
+new file mode 100644
+index 0000000..d95f3ad
+--- /dev/null
++++ "b/qu\157tefile"
+@@ -0,0 +1 @@
++content
+EOF
+
+test_expect_success 'apply --directory (quoted filename)' '
+	git reset --hard initial &&
+	git apply --directory=some/sub/dir/ --index patch &&
+	test content = $(git show :some/sub/dir/quotefile) &&
+	test content = $(cat some/sub/dir/quotefile)
+'
+
 test_done

Signature · 1 line

1.6.0.2.519.g6cb82.dirty