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.gb17...@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 -
