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

[Eric Sunshine <sunshine@sunshineco.com>]

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.

> +       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.

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).

> +
>  test_done
> --
> 2.7.0-rc3-219-g24972d4