Re: Regression in v2.23

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Tue, 8 Oct 2019, Junio C Hamano wrote:

> 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?

FWIW I totally agree.

Ciao,
Dscho