Re: segfault for git log --graph --no-walk --grep a

From

Junio C Hamano <gitster@pobox.com>

Date

Feb 9, 2013, 00:29 UTC

Message-ID

<7vk3qixrc8.fsf@alter.siamese.dyndns.org>

In-Reply-To

<7vobfuxrns.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Thomas Haller <thom311@gmail.com> writes:
>>
>>> it happens in file revision.c:2306 because "commit->buffer" is zero:
>>>
>>>                 retval = grep_buffer(&opt->grep_filter,
>>>                                      commit->buffer, strlen(commit->buffer));
>>
>> I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily
>> load missing commit buffers, 2013-01-26); I haven't merged it to any
>> of the maintenance releases, but the tip of 'master' should have it
>> already.
>
> Ah, no, this shares the same roots as the breakage the said commit
> fixed, and the solution should use the same idea, but not fixed.

Perhaps something along this line...

-- >8 --
Subject: "log --grep": commit's buffer may already have been discarded

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 The reproduction recipe in original bug report looked like this:

  git commit -m 'text1' --allow-empty
  git commit -m 'text2' --allow-empty
  git log --graph --no-walk --grep 'text2'

 which does not make any sense to me.  We should simply forbid
 combination of --graph (which inherently wants a connected history)
 and --no-walk (which is a way to tell "This is not about history,
 this is about a single point").

 But that is a separate issue.

 commit.c   | 19 +++++++++++++++++++
 commit.h   |  1 +
 pretty.c   | 14 ++------------
 revision.c | 14 +++++++++++---
 4 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/commit.c b/commit.c
index e8eb0ae..c0acf0f 100644
--- a/commit.c
+++ b/commit.c
@@ -335,6 +335,25 @@ int parse_commit(struct commit *item)
 	return ret;
 }
 
+char *read_commit_object_data(const struct commit *commit, unsigned long *sizep)
+{
+	char *msg;
+	enum object_type type;
+	unsigned long size;
+
+	if (!sizep)
+		sizep = &size;
+
+	msg = read_sha1_file(commit->object.sha1, &type, sizep);
+	if (!msg)
+		die("Cannot read commit object %s",
+		    sha1_to_hex(commit->object.sha1));
+	if (type != OBJ_COMMIT)
+		die("Expected commit for '%s', got %s",
+		    sha1_to_hex(commit->object.sha1), typename(type));
+	return msg;
+}
+
 int find_commit_subject(const char *commit_buffer, const char **subject)
 {
 	const char *eol;
diff --git a/commit.h b/commit.h
index 4138bb4..e314149 100644
--- a/commit.h
+++ b/commit.h
@@ -102,6 +102,7 @@ struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
 extern char *logmsg_reencode(const struct commit *commit,
 			     const char *output_encoding);
 extern void logmsg_free(char *msg, const struct commit *commit);
+extern char *read_commit_object_data(const struct commit *commit, unsigned long *size);
 extern void get_commit_format(const char *arg, struct rev_info *);
 extern const char *format_subject(struct strbuf *sb, const char *msg,
 				  const char *line_separator);
diff --git a/pretty.c b/pretty.c
index eae57ad..004d16d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -592,18 +592,8 @@ char *logmsg_reencode(const struct commit *commit,
 	char *msg = commit->buffer;
 	char *out;
 
-	if (!msg) {
-		enum object_type type;
-		unsigned long size;
-
-		msg = read_sha1_file(commit->object.sha1, &type, &size);
-		if (!msg)
-			die("Cannot read commit object %s",
-			    sha1_to_hex(commit->object.sha1));
-		if (type != OBJ_COMMIT)
-			die("Expected commit for '%s', got %s",
-			    sha1_to_hex(commit->object.sha1), typename(type));
-	}
+	if (!msg)
+		msg = read_commit_object_data(commit, NULL);
 
 	if (!output_encoding || !*output_encoding)
 		return msg;
diff --git a/revision.c b/revision.c
index d7562ee..caf8ef3 100644
--- a/revision.c
+++ b/revision.c
@@ -2279,9 +2279,16 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
 		strbuf_addch(&buf, '\n');
 	}
 
-	/* Copy the commit to temporary if we are using "fake" headers */
-	if (buf.len)
+	if (!commit->buffer) {
+		/* we may not have commit->buffer */
+		unsigned long size;
+		char *msg = read_commit_object_data(commit, &size);
+		strbuf_add(&buf, msg, size);
+		free(msg);
+	} else if (buf.len) {
+		/* Copy the commit to temporary if we are using "fake" headers */
 		strbuf_addstr(&buf, commit->buffer);
+	}
 
 	if (opt->grep_filter.header_list && opt->mailmap) {
 		if (!buf.len)
@@ -2302,9 +2309,10 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
 	/* Find either in the commit object, or in the temporary */
 	if (buf.len)
 		retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
-	else
+	else {
 		retval = grep_buffer(&opt->grep_filter,
 				     commit->buffer, strlen(commit->buffer));
+	}
 	strbuf_release(&buf);
 	return retval;
 }