Hi Thomas, On Tue, 8 Oct 2019, Thomas Gummerer wrote: > 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 Acked-by: Johannes Schindelin Ciao, Dscho > Signed-off-by: Thomas Gummerer > --- > > Thanks Junio and Dscho for your reviews. I decided to lift the whole > error handling behaviour from find_header into parse_git_diff_header, > instead of just filling the two names with xstrdup(def_name) if > (!old_name && !new_name && !!def_name). I think the additional > information presented there can be useful. For example we would have > gotten some "error: git diff header lacks filename information" > instead of a segfault for the problem described in > https://public-inbox.org/git/20191002141615.GB17916@kitsune.suse.cz/T/#me576615d7a151cf2ed46186c482fbd88f9959914. > > Dscho, I didn't re-use your test case here as I had already written > one, and think what I have is slightly nicer in that it follows what > most other range-diff tests do in using the fast-exported history. It > also expands the test coverage slightly, as we currently don't have > any coverage of the mode-change header, but will with this test. > > The downside is of course that the fast export script is harder to > understand than the test you had, at least for me, but I think the > tradeoff of having the additional test coverage, and having it similar > to the rest of the test script is worth it. If you strongly prefer > your test though I'm not going to be unhappy to use that :) > > apply.c | 43 +++++++++++++++++++++--------------------- > t/t3206-range-diff.sh | 40 +++++++++++++++++++++++++++++++++++++++ > t/t3206/history.export | 31 +++++++++++++++++++++++++++++- > 3 files changed, 92 insertions(+), 22 deletions(-) > > diff --git a/apply.c b/apply.c > index 57a61f2881..f8a046a6a5 100644 > --- 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 ec548654ce..5b87fead2e 100755 > --- 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 > + 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 > + 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 7bb3814962..4c808e5b3b 100644 > --- 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 1374485014 +0200 > committer Thomas Rast 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 1374485024 +0200 > +committer Thomas Gummerer 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 1374485036 +0200 > +committer Thomas Gummerer 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 1374485044 +0200 > +committer Thomas Gummerer 1570473768 +0100 > +data 8 > +s/12/B/ > +from :58 > +M 100644 :9 file > + > -- > 2.23.0.501.gb744c3af07 > >