Re: segmentation fault (nullpointer) with git log --submodule -p

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 25, 2013, 15:36 UTC
Message-ID: <7vip6l5l71.fsf@alter.siamese.dyndns.org>
In-Reply-To: <FE6CC927-1915-4486-BBB8-4C109F7B5295@me.com>

Jonathon Mah <jmah@me.com> writes:

Show 9 quoted lines

> Just to note, the proposals so far don't prevent a "smart-ass"
> function from freeing the buffer when it's called underneath the
> use/release scope, as in:
>
> with_commit_buffer(commit); {
> 	fn1_needing_buffer(commit);
> 	walk_rev_tree_or_something();
> 	fn2_needing_buffer(commit);
> } done_with_commit_buffer(commit);

I think the goal of everybody discussing these ideas is to make sure that all code follows the simple ownership policy proposed at the beginning of this subthread: commit->buffer belongs to the revision traversal machinery, and other users could borrow it when available.

Even though your sample code will break, from that point of view, I do not think it is something worth worrying about. If the function "walk_rev_tree_or_something()" discards commit->buffer, it by definition must be a part of the revision traversal machinery, and any code that calls it inside with_commit_buffer() or uses the field after such a call without revalidating commit->buffer, is already in violation. With or without such a macro, we would need to be careful about enforcing the ownership rule, and I think a code structure like the above example is easier to spot problems in during the review than the current code.

Because retaining commit->buffer is done for the benefit of the next/future users of the data, and not for the users that _are_ using them right now, I do not think the usual refcounting that discards when nobody references the data is a good match to the problem we are discussing.