[PATCH 0/3] lazily load commit->buffer
- From
Jeff King <peff@peff.net>
- Date
- Jan 26, 2013, 09:40 UTC
- Message-ID
- <20130126094026.GA9646@sigill.intra.peff.net>
- In-Reply-To
- <7vip6l5l71.fsf@alter.siamese.dyndns.org>
On Fri, Jan 25, 2013 at 07:36:18AM -0800, Junio C Hamano wrote:
Show 16 quoted lines
> Jonathon Mah <jmah@me.com> writes:
>
> > 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.Yeah, agreed. I started to fix this up with a use/unuse pattern and realized something: all of the call sites are calling logmsg_reencode anyway, because that is the next logical step in doing anything with the buffer that is not just parsing out the parent/timestamp/tree info. And since that function already might allocate (for the re-encoded copy), callers have to handle the maybe-borrowed-maybe-free situation already.
So I came up with this patch series, which I think should fix the problem, and actually makes the call-sites easier to read, rather than harder.
[1/3]: commit: drop useless xstrdup of commit message [2/3]: logmsg_reencode: never return NULL [3/3]: logmsg_reencode: lazily load missing commit buffers
Here's the diffstat:
builtin/blame.c | 22 ++------- builtin/commit.c | 14 +----- commit.h | 1 + pretty.c | 93 ++++++++++++++++++++++++++--------- t/t4042-diff-textconv-caching.sh | 8 +++ 5 files changed, 85 insertions(+), 53 deletions(-)
Not too bad, and 27 of the lines added in pretty.c are new comments explaining the flow of logmsg_reencode. So even if this doesn't get every case, I think it's a nice cleanup.
-Peff