Re: Regression in v2.23

[Junio C Hamano <gitster@pobox.com>]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> We can however rely on 'patch.def_name' in that case, which is
> extracted from the 'diff --git' line and should be equal to
> 'patch.new_name'.  Use that instead to avoid the segfault.

This patch makes the way this function calls parse_git_diff_header()
more in line with the way how it is used by its original caller in
apply.c::find_header(), but not quite.

I have to wonder if we want to move a bit of code around so that
callers of parse_git_diff_header() do not have to worry about
def_name and can rely on new_name and old_name fields correctly
filled.

There was only one caller of the parse_git_diff_header() function
before range-diff.  The division of labour between find_header() and
parse_git_diff_header() did not make any difference to the consumers
of the new/old_name fields.  They only cared that they do not have
to worry about def_name.  But by calling parse_git_diff_header()
that forces the caller to worry about def_name (which is done by
find_header() to free its callers from doing so), range-diff took
responsibility of caring, which was suboptimal.  The interface could
have been a bit more cleaned up before we started to reuse it in the
new caller, and as this bug shows, it may be time to do so now, no?

Perhaps before returing, parse_git_diff_header() should fill the two
names with xstrdup() of def_name if (!old_name && !new_name &&
!!def_name); all other cases the existing caller and this new caller
would work unchanged correctly, no?