[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v2] reflog-walk: don't segfault on non-commit sha1's in the reflog
From:       Junio C Hamano <gitster () pobox ! com>
Date:       2015-12-30 22:42:52
Message-ID: xmqqk2nvd0cz.fsf () gitster ! mtv ! corp ! google ! com
[Download RAW message or body]

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.

> 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]