Dennis Kaarsemaker <den...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html