Avoid segmentation fault in git range-diff when diff.noprefix=true

This PR was inspired by this bug report:
https://public-inbox.org/git/[email protected]/T/#me576615d7a151cf2ed46186c482fbd88f9959914

Johannes Schindelin (1):
range-diff: internally force diff.noprefix=false

 range-diff.c          | 3 ++-
 t/t3206-range-diff.sh | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

base-commit: bc12974a897308fd3254cf0cc90319078fe45eea
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-373%2Fdscho%2Frange-diff-requires-diff-prefix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-373/dscho/range-diff-requires-diff-prefix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/373

gitgitgadget

From: Johannes Schindelin <[email protected]>

When parsing the diffs, range-diff expects to see the prefixes a/ and b/ in the diff headers.

These prefixes can be forced off via the config setting diff.noprefix=true. As range-diff is not prepared for that situation, this will cause a segmentation fault.

Let's avoid that by forcing diff.noprefix=false just for that all to git log that generates the diffs that range-diff wants to parse.

---
 range-diff.c          | 3 ++-
 t/t3206-range-diff.sh | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

index ba1e9a4265..8cc348b4cb 100644
@@ -50,7 +50,8 @@ static int read_patches(const char *range, struct string_list *list)
 	int offset, len;
 	size_t size;
 
-	argv_array_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
+	argv_array_pushl(&cp.args, "-c", "diff.noprefix=false",
+			"log", "--no-color", "-p", "--no-merges",
 			"--reverse", "--date-order", "--decorate=no",
 			/*
 			 * Choose indicators that are not used anywhere

index 0120f769f1..64b66f2094 100755
@@ -461,4 +461,8 @@ test_expect_success 'format-patch --range-diff as commentary' '
 	grep "> 1: .* new message" 0001-*
 '
 
+test_expect_success 'range-diff overrides diff.noprefix internally' '
+	git -c diff.noprefix=true range-diff HEAD^...
+'
+
 test_done

gitgitgadget

s/all/call/

s/Noticed/Reported/ perhaps?

Thanks

Michal

"Johannes Schindelin via GitGitGadget" <[email protected]>

If so, passing src/dst prefix as command line option is a much better solution, I think. diff.noprefix may not stay to be (or it may already not to be) the only thing how the prefix gets chosen.

Hi Junio,

Good point.

While at it, I invert the logic in v2: instead of forcing a prefix, I now force no prefix (and reduce the strip level from 1 to 0 when parsing the diff header).

Thanks,
Dscho

This PR was inspired by this bug report:
https://public-inbox.org/git/[email protected]/T/#me576615d7a151cf2ed46186c482fbd88f9959914

Changes since v1:

* Use a command-line option instead of a command-line config setting.
* Instead of forcing a prefix, force no prefix (and adjust the strip level).
* Fix typo "all" -> "call" in the commit message (and adjust it to reflect the other changes since v1, too).

Johannes Schindelin (1):
range-diff: internally force diff.noprefix=true

 range-diff.c          | 3 ++-
 t/t3206-range-diff.sh | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

base-commit: bc12974a897308fd3254cf0cc90319078fe45eea
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-373%2Fdscho%2Frange-diff-requires-diff-prefix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-373/dscho/range-diff-requires-diff-prefix-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/373

Range-diff vs v1:

1: 1f84f92846 ! 1: 9993a607be range-diff: internally force diff.noprefix=false
@@ -1,6 +1,6 @@
Author: Johannes Schindelin <[email protected]>

- range-diff: internally force diff.noprefix=false
+ range-diff: internally force diff.noprefix=true

When parsing the diffs, range-diff expects to see the prefixes a/ and b/ in the diff headers.
@@ -9,25 +9,33 @@
diff.noprefix=true. As range-diff is not prepared for that situation, this will cause a segmentation fault.

- Let's avoid that by forcing diff.noprefix=false just for that all to
- git log that generates the diffs that range-diff wants to parse.
+ Let's avoid that by passing the --no-prefix option to the git log
+ process that generates the diffs that range-diff wants to parse.
+ And of course expect the output to have no prefixes, then.

- Noticed-by: Michal Suchánek <[email protected]>
+ Reported-by: Michal Suchánek <[email protected]>

diff --git a/range-diff.c b/range-diff.c
--- a/range-diff.c
+++ b/range-diff.c
@@
- int offset, len;
- size_t size;

-- argv_array_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
-+ argv_array_pushl(&cp.args, "-c", "diff.noprefix=false",
-+ "log", "--no-color", "-p", "--no-merges",
+ argv_array_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
"--reverse", "--date-order", "--decorate=no",
++ "--no-prefix",
/*
* Choose indicators that are not used anywhere
+ * else in diffs, but still look reasonable
+@@
+ if (!util->diff_offset)
+ util->diff_offset = buf.len;
+ line[len - 1] = '\n';
+- len = parse_git_diff_header(&root, &linenr, 1, line,
++ len = parse_git_diff_header(&root, &linenr, 0, line,
+ len, size, &patch);
+ if (len < 0)
+ die(_("could not parse git header '%.*s'"), (int)len, line);

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh --- a/t/t3206-range-diff.sh

gitgitgadget

From: Johannes Schindelin <[email protected]>

When parsing the diffs, range-diff expects to see the prefixes a/ and b/ in the diff headers.

These prefixes can be forced off via the config setting diff.noprefix=true. As range-diff is not prepared for that situation, this will cause a segmentation fault.

Let's avoid that by passing the --no-prefix option to the git log process that generates the diffs that range-diff wants to parse. And of course expect the output to have no prefixes, then.

---
 range-diff.c          | 3 ++-
 t/t3206-range-diff.sh | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

index ba1e9a4265..f2fc1e02c2 100644
@@ -52,6 +52,7 @@ static int read_patches(const char *range, struct string_list *list)
 
 	argv_array_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
 			"--reverse", "--date-order", "--decorate=no",
+			"--no-prefix",
 			/*
 			 * Choose indicators that are not used anywhere
 			 * else in diffs, but still look reasonable

@@ -111,7 +112,7 @@ static int read_patches(const char *range, struct string_list *list)
 			if (!util->diff_offset)
 				util->diff_offset = buf.len;
 			line[len - 1] = '\n';
-			len = parse_git_diff_header(&root, &linenr, 1, line,
+			len = parse_git_diff_header(&root, &linenr, 0, line,
 						    len, size, &patch);
 			if (len < 0)
 				die(_("could not parse git header '%.*s'"), (int)len, line);

index 0120f769f1..64b66f2094 100755
@@ -461,4 +461,8 @@ test_expect_success 'format-patch --range-diff as commentary' '
 	grep "> 1: .* new message" 0001-*
 '
 
+test_expect_success 'range-diff overrides diff.noprefix internally' '
+	git -c diff.noprefix=true range-diff HEAD^...
+'
+
 test_done

gitgitgadget

That is, "--src-prefix=a/ --dst-prefix=b/", so that configuration variables yet to be invented for setting prefixes different from a/ and b/ can also be overridden, not just diff.noprefix (ancient Git used l/ and k/ as src and dst prefix, IIRC).

Ah, that approach would also work and may be conceptually cleaner than requiring a/ and b/ prefix and forcing them.

"Johannes Schindelin via GitGitGadget" <[email protected]>

Looks good. Thanks.