Re: [PATCH] blame.c: fix garbled error message

From: Lukas Fleischer <git@cryptocrack.de>
Date: Jan 12, 2015, 23:18 UTC
Message-ID: <20150112231849.4992.72982@typhoon.lan>
In-Reply-To: <xmqqzj9n623h.fsf@gitster.dls.corp.google.com>

On Mon, 12 Jan 2015 at 23:55:30, Junio C Hamano wrote:

Show 28 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Lukas Fleischer <git@cryptocrack.de> writes:
> >
> >> The helper functions prepare_final() and prepare_initial() return a
> >> pointer to a string that is a member of an object in the revs->pending
> >> array. This array is later rebuilt when running prepare_revision_walk()
> >> which potentially transforms the pointer target into a bogus string. Fix
> >> this by maintaining a copy of the original string.
> >>
> >> Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
> >> ---
> >> The bug manifests when running `git blame HEAD^ -- nonexistent.file`.
> >
> > Before 1da1e07c (clean up name allocation in prepare_revision_walk,
> > 2014-10-15), these strings used to be non-volatile; they were instead
> > leaked more or less deliberately.  But these days, these strings are
> > cleared, so your patch is absolutely the right thing to do.
> >
> > Thanks for catching and fixing.  This fix needs to go to the 2.2.x
> > maintenance track.
> 
> Sigh, but not so fast.
> 
> With the patch applied on top of 1da1e07c (or the result merged to
> 'next' for that matter), I see test breakages in many places "git
> blame" is used, e.g. t7010.  Did you run the test suite?
>

No, I didn't.

> This is because it is perfectly normal for prepare_final() to return
> NULL.  Unconditionally running xstrdup() would of course fail.
> [...]

Something like

    return final_commit_name ? xstrdup(final_commit_name) : NULL;

should work, though, right? Calling free() with a null pointer is fine, so there is nothing else to do here. You can either amend those two lines or wait for me to resubmit v2 of the patch tomorrow :)