range-diff: don't segfault with mode-only changes

In ef283b3699 ("apply: make parse_git_diff_header public", 2019-07-11)
the 'parse_git_diff_header' function was made public and useable by
callers outside of apply.c.

However it was missed that its (then) only caller, 'find_header' did
some error handling, and completing 'struct patch' appropriately.

range-diff then started using this function, and tried to handle this
appropriately itself, but fell short in some cases.  This in turn
would lead to range-diff segfaulting when there are mode-only changes
in a range.

Move the error handling and completing of the struct into the
'parse_git_diff_header' function, so other callers can take advantage
of it.  This fixes the segfault in 'git range-diff'.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/apply.c b/apply.c
index cde95369bb3f3a9c763108fd91e1e48e0e455e29..5f06a1460cc286246db6d8d8422f570c6f891d12 100644 (file)
--- a/apply.c
+++ b/apply.c
@@ -1361,11 +1361,32 @@ int parse_git_diff_header(struct strbuf *root,
                        if (check_header_line(*linenr, patch))
                                return -1;
                        if (res > 0)
-                               return offset;
+                               goto done;
                        break;
                }
        }
 
+done:
+       if (!patch->old_name && !patch->new_name) {
+               if (!patch->def_name) {
+                       error(Q_("git diff header lacks filename information when removing "
+                                "%d leading pathname component (line %d)",
+                                "git diff header lacks filename information when removing "
+                                "%d leading pathname components (line %d)",
+                                parse_hdr_state.p_value),
+                             parse_hdr_state.p_value, *linenr);
+                       return -128;
+               }
+               patch->old_name = xstrdup(patch->def_name);
+               patch->new_name = xstrdup(patch->def_name);
+       }
+       if ((!patch->new_name && !patch->is_delete) ||
+           (!patch->old_name && !patch->is_new)) {
+               error(_("git diff header lacks filename information "
+                       "(line %d)"), *linenr);
+               return -128;
+       }
+       patch->is_toplevel_relative = 1;
        return offset;
 }
 
@@ -1546,26 +1567,6 @@ static int find_header(struct apply_state *state,
                                return -128;
                        if (git_hdr_len <= len)
                                continue;
-                       if (!patch->old_name && !patch->new_name) {
-                               if (!patch->def_name) {
-                                       error(Q_("git diff header lacks filename information when removing "
-                                                       "%d leading pathname component (line %d)",
-                                                       "git diff header lacks filename information when removing "
-                                                       "%d leading pathname components (line %d)",
-                                                       state->p_value),
-                                                    state->p_value, state->linenr);
-                                       return -128;
-                               }
-                               patch->old_name = xstrdup(patch->def_name);
-                               patch->new_name = xstrdup(patch->def_name);
-                       }
-                       if ((!patch->new_name && !patch->is_delete) ||
-                           (!patch->old_name && !patch->is_new)) {
-                               error(_("git diff header lacks filename information "
-                                            "(line %d)"), state->linenr);
-                               return -128;
-                       }
-                       patch->is_toplevel_relative = 1;
                        *hdrsize = git_hdr_len;
                        return offset;
                }

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index ec548654ce1cae8773ad642877e525896a6b54f4..5b87fead2ef8b10103e4f86e0f89e0c2d8c516fa 100755 (executable)
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -226,6 +226,46 @@ test_expect_success 'renamed file' '
        test_cmp expected actual
 '
 
+test_expect_success 'file with mode only change' '
+       git range-diff --no-color --submodule=log topic...mode-only-change >actual &&
+       sed s/Z/\ /g >expected <<-EOF &&
+       1:  fccce22 ! 1:  4d39cb3 s/4/A/
+           @@ Metadata
+           ZAuthor: Thomas Rast <trast@inf.ethz.ch>
+           Z
+           Z ## Commit message ##
+           -    s/4/A/
+           +    s/4/A/ + add other-file
+           Z
+           Z ## file ##
+           Z@@
+           @@ file
+           Z A
+           Z 6
+           Z 7
+           +
+           + ## other-file (new) ##
+       2:  147e64e ! 2:  26c107f s/11/B/
+           @@ Metadata
+           ZAuthor: Thomas Rast <trast@inf.ethz.ch>
+           Z
+           Z ## Commit message ##
+           -    s/11/B/
+           +    s/11/B/ + mode change other-file
+           Z
+           Z ## file ##
+           Z@@ file: A
+           @@ file: A
+           Z 12
+           Z 13
+           Z 14
+           +
+           + ## other-file (mode change 100644 => 100755) ##
+       3:  a63e992 = 3:  4c1e0f5 s/12/B/
+       EOF
+       test_cmp expected actual
+'
+
 test_expect_success 'file added and later removed' '
        git range-diff --no-color --submodule=log topic...added-removed >actual &&
        sed s/Z/\ /g >expected <<-EOF &&

diff --git a/t/t3206/history.export b/t/t3206/history.export
index 7bb38149622737ac65a05b5b8105360640e0aa6e..4c808e5b3b8d507468f50b64dc6414998c5ab813 100644 (file)
--- a/t/t3206/history.export
+++ b/t/t3206/history.export
@@ -55,7 +55,7 @@ A
 19
 20
 
-commit refs/heads/topic
+commit refs/heads/mode-only-change
 mark :4
 author Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200
 committer Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200
@@ -678,3 +678,32 @@ s/12/B/
 from :55
 M 100644 :9 renamed-file
 
+commit refs/heads/mode-only-change
+mark :57
+author Thomas Rast <trast@inf.ethz.ch> 1374485024 +0200
+committer Thomas Gummerer <t.gummerer@gmail.com> 1570473767 +0100
+data 24
+s/4/A/ + add other-file
+from :4
+M 100644 :5 file
+M 100644 :49 other-file
+
+commit refs/heads/mode-only-change
+mark :58
+author Thomas Rast <trast@inf.ethz.ch> 1374485036 +0200
+committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100
+data 33
+s/11/B/ + mode change other-file
+from :57
+M 100644 :7 file
+M 100755 :49 other-file
+
+commit refs/heads/mode-only-change
+mark :59
+author Thomas Rast <trast@inf.ethz.ch> 1374485044 +0200
+committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100
+data 8
+s/12/B/
+from :58
+M 100644 :9 file
+