Merge pull request #5159 from pks-t/pks/patch-parse-old-missing-nl

patch_parse: handle missing newline indicator in old file

6 files changed, 109 insertions, 20 deletions

diff --git a/src/apply.c b/src/apply.c
index f876e437e..9d607dd50 100644
--- a/src/apply.c
+++ b/src/apply.c
@@ -199,23 +199,34 @@ static int apply_hunk(
 
 	for (i = 0; i < hunk->line_count; i++) {
 		size_t linenum = hunk->line_start + i;
-		git_diff_line *line = git_array_get(patch->lines, linenum);
+		git_diff_line *line = git_array_get(patch->lines, linenum), *prev;
 
 		if (!line) {
 			error = apply_err("preimage does not contain line %"PRIuZ, linenum);
 			goto done;
 		}
 
-		if (line->origin == GIT_DIFF_LINE_CONTEXT ||
-			line->origin == GIT_DIFF_LINE_DELETION) {
-			if ((error = git_vector_insert(&preimage.lines, line)) < 0)
-				goto done;
-		}
-
-		if (line->origin == GIT_DIFF_LINE_CONTEXT ||
-			line->origin == GIT_DIFF_LINE_ADDITION) {
-			if ((error = git_vector_insert(&postimage.lines, line)) < 0)
-				goto done;
+		switch (line->origin) {
+			case GIT_DIFF_LINE_CONTEXT_EOFNL:
+			case GIT_DIFF_LINE_DEL_EOFNL:
+			case GIT_DIFF_LINE_ADD_EOFNL:
+				prev = i ? git_array_get(patch->lines, i - 1) : NULL;
+				if (prev && prev->content[prev->content_len - 1] == '\n')
+					prev->content_len -= 1;
+				break;
+			case GIT_DIFF_LINE_CONTEXT:
+				if ((error = git_vector_insert(&preimage.lines, line)) < 0 ||
+				    (error = git_vector_insert(&postimage.lines, line)) < 0)
+					goto done;
+				break;
+			case GIT_DIFF_LINE_DELETION:
+				if ((error = git_vector_insert(&preimage.lines, line)) < 0)
+					goto done;
+				break;
+			case GIT_DIFF_LINE_ADDITION:
+				if ((error = git_vector_insert(&postimage.lines, line)) < 0)
+					goto done;
+				break;
 		}
 	}
 
diff --git a/src/diff.c b/src/diff.c
index 6c2592137..3c6a0f242 100644
--- a/src/diff.c
+++ b/src/diff.c
@@ -460,7 +460,7 @@ out:
 	return error;
 }
 
-static int line_cb(
+static int patchid_line_cb(
 	const git_diff_delta *delta,
 	const git_diff_hunk *hunk,
 	const git_diff_line *line,
@@ -482,6 +482,14 @@ static int line_cb(
 		break;
 	    case GIT_DIFF_LINE_CONTEXT:
 		break;
+	    case GIT_DIFF_LINE_CONTEXT_EOFNL:
+	    case GIT_DIFF_LINE_ADD_EOFNL:
+	    case GIT_DIFF_LINE_DEL_EOFNL:
+		/*
+		 * Ignore EOF without newlines for patch IDs as whitespace is
+		 * not supposed to be significant.
+		 */
+		return 0;
 	    default:
 		git_error_set(GIT_ERROR_PATCH, "invalid line origin for patch");
 		return -1;
@@ -518,7 +526,7 @@ int git_diff_patchid(git_oid *out, git_diff *diff, git_diff_patchid_options *opt
 	if ((error = git_hash_ctx_init(&args.ctx)) < 0)
 		goto out;
 
-	if ((error = git_diff_foreach(diff, file_cb, NULL, NULL, line_cb, &args)) < 0)
+	if ((error = git_diff_foreach(diff, file_cb, NULL, NULL, patchid_line_cb, &args)) < 0)
 		goto out;
 
 	if ((error = (flush_hunk(&args.result, &args.ctx))) < 0)
diff --git a/src/patch_generate.c b/src/patch_generate.c
index 80033ee53..5023bfe28 100644
--- a/src/patch_generate.c
+++ b/src/patch_generate.c
@@ -836,7 +836,7 @@ static int patch_generated_line_cb(
 {
 	git_patch_generated  *patch = payload;
 	git_patch_hunk *hunk;
-	git_diff_line   *line;
+	git_diff_line *line;
 
 	GIT_UNUSED(delta);
 	GIT_UNUSED(hunk_);
diff --git a/src/patch_parse.c b/src/patch_parse.c
index 5e5e5d941..70c33ed7d 100644
--- a/src/patch_parse.c
+++ b/src/patch_parse.c
@@ -524,6 +524,14 @@ fail:
 	return -1;
 }
 
+static int eof_for_origin(int origin) {
+	if (origin == GIT_DIFF_LINE_ADDITION)
+		return GIT_DIFF_LINE_ADD_EOFNL;
+	if (origin == GIT_DIFF_LINE_DELETION)
+		return GIT_DIFF_LINE_DEL_EOFNL;
+	return GIT_DIFF_LINE_CONTEXT_EOFNL;
+}
+
 static int parse_hunk_body(
 	git_patch_parsed *patch,
 	git_patch_hunk *hunk,
@@ -534,6 +542,7 @@ static int parse_hunk_body(
 
 	int oldlines = hunk->hunk.old_lines;
 	int newlines = hunk->hunk.new_lines;
+	int last_origin = 0;
 
 	for (;
 		ctx->parse_ctx.remain_len > 1 &&
@@ -578,6 +587,21 @@ static int parse_hunk_body(
 			old_lineno = -1;
 			break;
 
+		case '\\':
+			/*
+			 * If there are no oldlines left, then this is probably
+			 * the "\ No newline at end of file" marker. Do not
+			 * verify its format, as it may be localized.
+			 */
+			if (!oldlines) {
+				prefix = 0;
+				origin = eof_for_origin(last_origin);
+				old_lineno = -1;
+				new_lineno = -1;
+				break;
+			}
+			/* fall through */
+
 		default:
 			error = git_parse_err("invalid patch hunk at line %"PRIuZ, ctx->parse_ctx.line_num);
 			goto done;
@@ -597,6 +621,8 @@ static int parse_hunk_body(
 		line->new_lineno = new_lineno;
 
 		hunk->line_count++;
+
+		last_origin = origin;
 	}
 
 	if (oldlines || newlines) {
@@ -606,7 +632,8 @@ static int parse_hunk_body(
 		goto done;
 	}
 
-	/* Handle "\ No newline at end of file".  Only expect the leading
+	/*
+	 * Handle "\ No newline at end of file". Only expect the leading
 	 * backslash, though, because the rest of the string could be
 	 * localized.  Because `diff` optimizes for the case where you
 	 * want to apply the patch by hand.
@@ -617,11 +644,24 @@ static int parse_hunk_body(
 		line = git_array_get(patch->base.lines, git_array_size(patch->base.lines) - 1);
 
 		if (line->content_len < 1) {
-			error = git_parse_err("cannot trim trailing newline of empty line");
+			error = git_parse_err("last line has no trailing newline");
 			goto done;
 		}
 
-		line->content_len--;
+		line = git_array_alloc(patch->base.lines);
+		GIT_ERROR_CHECK_ALLOC(line);
+
+		memset(line, 0x0, sizeof(git_diff_line));
+
+		line->content = ctx->parse_ctx.line;
+		line->content_len = ctx->parse_ctx.line_len;
+		line->content_offset = ctx->parse_ctx.content_len - ctx->parse_ctx.remain_len;
+		line->origin = eof_for_origin(last_origin);
+		line->num_lines = 1;
+		line->old_lineno = -1;
+		line->new_lineno = -1;
+
+		hunk->line_count++;
 
 		git_parse_advance_line(&ctx->parse_ctx);
 	}
diff --git a/tests/patch/parse.c b/tests/patch/parse.c
index 7eb987920..b89322ff3 100644
--- a/tests/patch/parse.c
+++ b/tests/patch/parse.c
@@ -27,6 +27,18 @@ static void ensure_patch_validity(git_patch *patch)
 	cl_assert_equal_i(0, delta->new_file.size);
 }
 
+static void ensure_identical_patch_inout(const char *content) {
+	git_buf buf = GIT_BUF_INIT;
+	git_patch *patch;
+
+	cl_git_pass(git_patch_from_buffer(&patch, content, strlen(content), NULL));
+	cl_git_pass(git_patch_to_buf(&buf, patch));
+	cl_assert_equal_strn(git_buf_cstr(&buf), content, strlen(content));
+
+	git_patch_free(patch);
+	git_buf_dispose(&buf);
+}
+
 void test_patch_parse__original_to_change_middle(void)
 {
 	git_patch *patch;
@@ -102,11 +114,19 @@ void test_patch_parse__invalid_patches_fails(void)
 		strlen(PATCH_CORRUPT_MISSING_HUNK_HEADER), NULL));
 }
 
+void test_patch_parse__no_newline_at_end_of_new_file(void)
+{
+	ensure_identical_patch_inout(PATCH_APPEND_NO_NL);
+}
+
+void test_patch_parse__no_newline_at_end_of_old_file(void)
+{
+	ensure_identical_patch_inout(PATCH_APPEND_NO_NL_IN_OLD_FILE);
+}
+
 void test_patch_parse__files_with_whitespaces_succeeds(void)
 {
-	git_patch *patch;
-	cl_git_pass(git_patch_from_buffer(&patch, PATCH_NAME_WHITESPACE, strlen(PATCH_NAME_WHITESPACE), NULL));
-	git_patch_free(patch);
+	ensure_identical_patch_inout(PATCH_NAME_WHITESPACE);
 }
 
 void test_patch_parse__lifetime_of_patch_does_not_depend_on_buffer(void)
diff --git a/tests/patch/patch_common.h b/tests/patch/patch_common.h
index 291ece9eb..2db8d933f 100644
--- a/tests/patch/patch_common.h
+++ b/tests/patch/patch_common.h
@@ -681,6 +681,16 @@
 	"+added line with no nl\n" \
 	"\\ No newline at end of file\n"
 
+#define PATCH_APPEND_NO_NL_IN_OLD_FILE \
+	"diff --git a/file.txt b/file.txt\n" \
+	"index 9432026..83759c0 100644\n" \
+	"--- a/file.txt\n" \
+	"+++ b/file.txt\n" \
+	"@@ -1,1 +1,1 @@\n" \
+	"-foo\n" \
+	"\\ No newline at end of file\n" \
+	"+foo\n"
+
 #define PATCH_NAME_WHITESPACE \
 	"diff --git a/file with spaces.txt b/file with spaces.txt\n" \
 	"index 9432026..83759c0 100644\n" \