From: Junio C Hamano <gitster@pobox•com>
To: Dennis Kaarsemaker <dennis@kaarsemaker•net>
Cc: git@vger•kernel.org, pclouds@gmail•com
Subject: Re: [PATCH v2] reflog-walk: don't segfault on non-commit sha1's in the reflog
Date: Wed, 30 Dec 2015 14:42:52 -0800 [thread overview]
Message-ID: <xmqqk2nvd0cz.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20151230221705.GA4025@spirit> (Dennis Kaarsemaker's message of "Wed, 30 Dec 2015 23:17:08 +0100")
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
next prev parent reply other threads:[~2015-12-30 22:42 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-30 9:24 Segfault in git reflog Dennis Kaarsemaker
2015-12-30 10:31 ` Duy Nguyen
2015-12-30 11:17 ` Dennis Kaarsemaker
2015-12-30 11:26 ` Duy Nguyen
2015-12-30 11:28 ` Duy Nguyen
2015-12-30 12:28 ` Dennis Kaarsemaker
2015-12-30 13:19 ` Duy Nguyen
2015-12-30 15:22 ` [PATCH] reflog-walk: don't segfault on non-commit sha1's in the reflog Dennis Kaarsemaker
2015-12-30 21:20 ` Junio C Hamano
2015-12-30 21:33 ` Dennis Kaarsemaker
2015-12-30 21:41 ` Junio C Hamano
2015-12-30 21:49 ` Dennis Kaarsemaker
2015-12-30 22:17 ` [PATCH v2] " Dennis Kaarsemaker
2015-12-30 22:42 ` Junio C Hamano [this message]
2015-12-30 23:33 ` [PATCH v3] " Dennis Kaarsemaker
2015-12-31 0:02 ` Junio C Hamano
2015-12-31 8:57 ` Dennis Kaarsemaker
2015-12-31 15:43 ` Dennis Kaarsemaker
2016-01-05 21:12 ` [PATCH v4] " Dennis Kaarsemaker
2016-01-06 1:05 ` Eric Sunshine
2016-01-06 1:20 ` Dennis Kaarsemaker
2016-01-06 1:28 ` Eric Sunshine
2016-01-06 1:52 ` Eric Sunshine
2016-01-06 9:13 ` Dennis Kaarsemaker
2016-01-06 9:30 ` Duy Nguyen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqk2nvd0cz.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=dennis@kaarsemaker$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=pclouds@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox