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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 30, 2015, 22:42 UTC
Message-ID: <xmqqk2nvd0cz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20151230221705.GA4025@spirit>

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> The really correct way of fixing this bug may actually be a level higher, and
> making git reflog not rely on information about parent commits, whether they
> are fake or not.

I tend to agree.

The only common thing between "git reflog" wants to do (i.e. showing the objects referred to by reflog entries) and what the normal "git log" was/is designed to do is "we have many things, and we show them one by one". As the "many things" we have in the context of the normal "git log" are all commits, it is reasonable that the internal interface (i.e. revision.c::get_revision()) to iterate over these "many things" returns a "struct commit *" and it also is reasonable that "show them one by one" is done by calling log_tree_commit() in builtin/log.c::cmd_log_walk(). Neither is suitable to deal with series of reflog entries in general. A proper implementation of "git reflog" would have liked to be able to iterate over "many things" by returning "struct object *" one-by-one, and then do the equivalent of the switch() statement in builtin/log.c::cmd_show() to show these objects.

The way "git log" was abused and made to show entries from reflog is one of the ugly and unfortunate hacks in our codebase.

However, I see that there are one of two things that you could do to make this part of code do slightly better than stopping at the first non-commit object:

 - pretend that the non-commit entry never existed in the first
   place and return the commit that appears in the reflog next.

 - fabricate a fake "commit" object that says "I am not a commit;
   I merely exist to represent that the reflog you are walking has
   this non-commit object at this point in the sequence" and return
   it, instead of giving NULL in the error path.

Show 18 quoted lines

> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index b79049f..130d671 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs at BUFSIZ boundaries' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'no segfaults for reflog containing non-commit sha1s' '
> +	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
> +'
> +
> +test_expect_failure 'reflog containing non-commit sha1s displays fully' '
> +	git reflog refs/tests/tree-in-reflog > actual &&

Please write this without space after the redirection operator, i.e.

	git reflog refs/tests/tree-in-reflog >actual &&

> +	test_line_count = 3 actual
> +'
> +
>  test_done