On Fri, Jan 25, 2013 at 07:36:18AM -0800, Junio C Hamano wrote:

> Jonathon Mah <j...@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

  [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.

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

Reply via email to