Re: segmentation fault (nullpointer) with git log --submodule -p
- From
Jeff King <peff@peff.net>
- Date
- Jan 25, 2013, 04:08 UTC
- Message-ID
- <20130125040856.GA30533@sigill.intra.peff.net>
- In-Reply-To
- <7vzjzx7w01.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 07:59:58PM -0800, Junio C Hamano wrote:
Show 22 quoted lines
> Junio C Hamano <gitster@pobox.com> writes: > > > Jeff King <peff@peff.net> writes: > > > >> ... (e.g., how should "log" know that a submodule diff might later want > >> to see the same entry? Should we optimistically free and then make it > >> easier for the later user to reliably ensure the buffer is primed? Or > >> should we err on the side of keeping it in place?). > > > > My knee-jerk reaction is that we should consider that commit->buffer > > belongs to the revision traversal machinery. Any other uses bolted > > on later can borrow it if buffer still exists (I do not think pretty > > code rewrites the buffer contents in place in any way), or they can > > ask read_sha1_file() to read it themselves and free when they are > > done. > > I've been toying with an idea along this line. > > commit.h | 16 ++++++++++++++++ > builtin/blame.c | 27 ++++++++------------------- > commit.c | 20 ++++++++++++++++++++ > 3 files changed, 44 insertions(+), 19 deletions(-)
I think we are on the same page as far as what needs to happen at the call sites.
My suggested implementation had a separate buffer, but you are right that we may need to actually set "commit->buffer" because sub-functions expect to find it there (the alternative might be cleaning up the sub-function interfaces). I haven't looked at the call-sites yet.
This:
Show 15 quoted lines
> +extern int ensure_commit_buffer(struct commit *);
> +extern void discard_commit_buffer(struct commit *);
> +
> +#define with_commit_buffer(commit) \
> + do { \
> + int had_buffer_ = !!commit->buffer; \
> + if (!had_buffer_) \
> + ensure_commit_buffer(commit); \
> + do
> +
> +#define done_with_commit_buffer(commit) \
> + while (0); \
> + if (!had_buffer_) \
> + discard_commit_buffer(commit); \
> + } while (0)is pretty nasty, though. I know it gets the job done, but in my experience, macros which do not behave syntactically like functions are usually a good sign that you are doing something gross and unmaintainable.
I dunno.
-Peff