Re: Regression in v2.23

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 8, 2019, 03:11 UTC
Message-ID: <xmqqh84knd7l.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191007134831.GA74671@cat>

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?