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

This PR was inspired by this bug report: https://public-inbox.org/git/20191002141615.GB17916@kitsune.suse.cz/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

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Noticed-by: Michal Suchánek <msuchanek@suse.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 range-diff.c          | 3 ++-
 t/t3206-range-diff.sh | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

On Wed, Oct 2, 2019 at 2:05 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

s/all/call/

s/Noticed/Reported/ perhaps?

On Wed, Oct 02, 2019 at 11:05:13AM -0700, Johannes Schindelin via GitGitGadget wrote:

Tested-by: Michal Suchánek <msuchanek@suse.de>

Thanks

Michal

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

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,

On Thu, 3 Oct 2019, Junio C Hamano wrote:

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Junio C Hamano <gitster@pobox.com> writes:

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).

This PR was inspired by this bug report: https://public-inbox.org/git/20191002141615.GB17916@kitsune.suse.cz/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 <johannes.schindelin@gmx.de>
      
     -    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 <msuchanek@suse.de>
     +    Reported-by: Michal Suchánek <msuchanek@suse.de>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       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

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Reported-by: Michal Suchánek <msuchanek@suse.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 range-diff.c          | 3 ++-
 t/t3206-range-diff.sh | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Looks good.  Thanks.