Re: [PATCH v4] reflog-walk: don't segfault on non-commit sha1's in the reflog

From: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Date: Jan 6, 2016, 01:20 UTC
Message-ID: <1452043212.5562.18.camel@kaarsemaker.net>
In-Reply-To: <CAPig+cRufd4qOwZRpw2TR39npkRGg=7S+7YwfSu6EvRR95kRSA@mail.gmail.com>

On di, 2016-01-05 at 20:05 -0500, Eric Sunshine wrote:

Show 34 quoted lines

> On Tue, Jan 5, 2016 at 4:12 PM, Dennis Kaarsemaker
> <dennis@kaarsemaker.net> wrote:
> > git reflog (ab)uses the log machinery to display its list of log
> > entries. To do so it must fake commit parent information for the
> > log
> > walker.
> > 
> > For refs in refs/heads this is no problem, as they should only ever
> > point to commits. Tags and other refs however can point to
> > anything,
> > thus their reflog may contain non-commit objects.
> > 
> > To avoid segfaulting, we check whether reflog entries are commits
> > before
> > feeding them to the log walker and skip any non-commits. This means
> > that
> > git reflog output will be incomplete for such refs, but that's one
> > step
> > up from segfaulting. A more complete solution would be to decouple
> > git
> > reflog from the log walker machinery.
> > 
> > Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> > ---
> > diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> > @@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs
> > at BUFSIZ boundaries' '
> > +test_expect_success 'no segfaults for reflog containing non-commit
> > sha1s' '
> 
> Nit: It's kind of strange for a test title to talk about not
> segfaulting; that's behavior you'd expect to be true for all tests.
> Perhaps describe it as "non-commit reflog entries handled sanely" or
> something.

To paraphrase what Junio said earlier in this thread: tests determine what is sane behavior, so using the word 'sanely' isn't really appropriate. This is a regression test to make sure we don't accidentally reintroduce behavior that segfaults, which I think is an easy mistake to make with the current code, so I think the title is appropriate.

Show 21 quoted lines

> > +       git update-ref --create-reflog -m "Creating ref" \
> > +               refs/tests/tree-in-reflog HEAD &&
> > +       git update-ref -m "Forcing tree" refs/tests/tree-in-reflog
> > HEAD^{tree} &&
> > +       git update-ref -m "Restoring to commit" refs/tests/tree-in
> > -reflog HEAD &&
> > +       git reflog refs/tests/tree-in-reflog
> > +'
> 
> Hmm, this test is successful for me on OS X even without the
> reflog-walk.c changes applied.
> 
> > +test_expect_failure 'reflog with non-commit entries displays all
> > entries' '
> > +       git reflog refs/tests/tree-in-reflog >actual &&
> > +       test_line_count = 3 actual
> > +'
> 
> And this test actually fails (inversely) because it's expecting a
> failure, but doesn't get one since the command produces the expected
> output.

That's... surprising to say the least. What's the content of 'actual', and which git.git commit are you on?

> By the way, it may make sense to combine these two tests. If a
> segfault occurs, the actual output likely will not match the expected
> output, thus the test will fail anyhow (unless the segfault occurs
> after all output).

I kept them separate to show that while this no longer segfaults, it's still not the correct output, but showing correct output is a much bigger project.

-- 
Dennis Kaarsemaker
www.kaarsemaker.net