From 131cd9b16dbfc4f5c3b5ba0f44e9285cea4ad5ac Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 29 Mar 2019 11:58:50 +0100 Subject: patch_parse: improve formatting --- src/patch_parse.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/patch_parse.c b/src/patch_parse.c index 647929fd5..3e78d8ca7 100644 --- a/src/patch_parse.c +++ b/src/patch_parse.c @@ -921,21 +921,15 @@ static int check_filenames(git_patch_parsed *patch) return git_parse_err("missing old path"); /* Ensure (non-renamed) paths match */ - if (check_header_names( - patch->header_old_path, patch->old_path, "old", added) < 0 || - check_header_names( - patch->header_new_path, patch->new_path, "new", deleted) < 0) + if (check_header_names(patch->header_old_path, patch->old_path, "old", added) < 0 || + check_header_names(patch->header_new_path, patch->new_path, "new", deleted) < 0) return -1; - prefixed_old = (!added && patch->old_path) ? patch->old_path : - patch->header_old_path; - prefixed_new = (!deleted && patch->new_path) ? patch->new_path : - patch->header_new_path; + prefixed_old = (!added && patch->old_path) ? patch->old_path : patch->header_old_path; + prefixed_new = (!deleted && patch->new_path) ? patch->new_path : patch->header_new_path; - if (check_prefix( - &patch->old_prefix, &old_prefixlen, patch, prefixed_old) < 0 || - check_prefix( - &patch->new_prefix, &new_prefixlen, patch, prefixed_new) < 0) + if (check_prefix(&patch->old_prefix, &old_prefixlen, patch, prefixed_old) < 0 || + check_prefix(&patch->new_prefix, &new_prefixlen, patch, prefixed_new) < 0) return -1; /* Prefer the rename filenames as they are unambiguous and unprefixed */ @@ -950,7 +944,7 @@ static int check_filenames(git_patch_parsed *patch) patch->base.delta->new_file.path = prefixed_new + new_prefixlen; if (!patch->base.delta->old_file.path && - !patch->base.delta->new_file.path) + !patch->base.delta->new_file.path) return git_parse_err("git diff header lacks old / new paths"); return 0; @@ -964,14 +958,14 @@ static int check_patch(git_patch_parsed *patch) return -1; if (delta->old_file.path && - delta->status != GIT_DELTA_DELETED && - !delta->new_file.mode) + delta->status != GIT_DELTA_DELETED && + !delta->new_file.mode) delta->new_file.mode = delta->old_file.mode; if (delta->status == GIT_DELTA_MODIFIED && - !(delta->flags & GIT_DIFF_FLAG_BINARY) && - delta->new_file.mode == delta->old_file.mode && - git_array_size(patch->base.hunks) == 0) + !(delta->flags & GIT_DIFF_FLAG_BINARY) && + delta->new_file.mode == delta->old_file.mode && + git_array_size(patch->base.hunks) == 0) return git_parse_err("patch with no hunks"); if (delta->status == GIT_DELTA_ADDED) { -- cgit v1.2.1 From b3497344fa591c98091bf2d68caed6d7edfa8d01 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 29 Mar 2019 12:15:20 +0100 Subject: patch_parse: fix parsing addition/deletion of file with space The diff header format is a strange beast in that it is inherently unparseable in an unambiguous way. While parsing a/file.txt b/file.txt is obvious and trivially doable, parsing a diff header of a/file b/file ab.txt b/file b/file ab.txt is not (but in fact valid and created by git.git). Due to that, we have relaxed our diff header parser in commit 80226b5f6 (patch_parse: allow parsing ambiguous patch headers, 2017-09-22), so that we started to bail out when seeing diff headers with spaces in their file names. Instead, we try to use the "---" and "+++" lines, which are unambiguous. In some cases, though, we neither have a useable file name from the header nor from the "---" or "+++" lines. This is the case when we have a deletion or addition of a file with spaces: the header is unparseable and the other lines will simply show "/dev/null". This trips our parsing logic when we try to extract the prefix (the "a/" part) that is being used in the path line, where we unconditionally try to dereference a NULL pointer in such a scenario. We can fix this by simply not trying to parse the prefix in cases where we have no useable path name. That'd leave the parsed patch without either `old_prefix` or `new_prefix` populated. But in fact such cases are already handled by users of the patch object, which simply opt to use the default prefixes in that case. --- src/patch_parse.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/patch_parse.c b/src/patch_parse.c index 3e78d8ca7..545f1ca9e 100644 --- a/src/patch_parse.c +++ b/src/patch_parse.c @@ -928,8 +928,8 @@ static int check_filenames(git_patch_parsed *patch) prefixed_old = (!added && patch->old_path) ? patch->old_path : patch->header_old_path; prefixed_new = (!deleted && patch->new_path) ? patch->new_path : patch->header_new_path; - if (check_prefix(&patch->old_prefix, &old_prefixlen, patch, prefixed_old) < 0 || - check_prefix(&patch->new_prefix, &new_prefixlen, patch, prefixed_new) < 0) + if ((prefixed_old && check_prefix(&patch->old_prefix, &old_prefixlen, patch, prefixed_old) < 0) || + (prefixed_new && check_prefix(&patch->new_prefix, &new_prefixlen, patch, prefixed_new) < 0)) return -1; /* Prefer the rename filenames as they are unambiguous and unprefixed */ -- cgit v1.2.1 From 9d65360b4eaa402c8f137684577d0131a861ce8b Mon Sep 17 00:00:00 2001 From: Erik Aigner Date: Fri, 29 Mar 2019 12:30:37 +0100 Subject: tests: diff: test parsing diffs with a new file with spaces in its path Add a test that verifies that we are able to parse patches which add a new file that has spaces in its path. --- tests/diff/parse.c | 18 ++++++++++++++++++ tests/patch/patch_common.h | 9 +++++++++ 2 files changed, 27 insertions(+) diff --git a/tests/diff/parse.c b/tests/diff/parse.c index 9cdaa92fb..7cc468245 100644 --- a/tests/diff/parse.c +++ b/tests/diff/parse.c @@ -359,3 +359,21 @@ void test_diff_parse__lineinfo(void) git_patch_free(patch); git_diff_free(diff); } + +void test_diff_parse__new_file_with_space(void) +{ + const char *content = PATCH_ORIGINAL_NEW_FILE_WITH_SPACE; + git_patch *patch; + git_diff *diff; + + cl_git_pass(git_diff_from_buffer(&diff, content, strlen(content))); + cl_git_pass(git_patch_from_diff((git_patch **) &patch, diff, 0)); + + cl_assert_equal_p(patch->diff_opts.old_prefix, NULL); + cl_assert_equal_p(patch->delta->old_file.path, NULL); + cl_assert_equal_s(patch->diff_opts.new_prefix, "b/"); + cl_assert_equal_s(patch->delta->new_file.path, "sp ace.txt"); + + git_patch_free(patch); + git_diff_free(diff); +} diff --git a/tests/patch/patch_common.h b/tests/patch/patch_common.h index 3f2668ddc..75aab4453 100644 --- a/tests/patch/patch_common.h +++ b/tests/patch/patch_common.h @@ -841,3 +841,12 @@ "diff --git a/binary.bin b/binary.bin\n" \ "index 27184d9..7c94f9e 100644\n" \ "Binary files a/binary.bin and b/binary.bin differ\n" + +#define PATCH_ORIGINAL_NEW_FILE_WITH_SPACE \ + "diff --git a/sp ace.txt b/sp ace.txt\n" \ + "new file mode 100644\n" \ + "index 000000000..789819226\n" \ + "--- /dev/null\n" \ + "+++ b/sp ace.txt\n" \ + "@@ -0,0 +1 @@\n" \ + "+a\n" -- cgit v1.2.1