[PATCH 0/7] find commit subject refactoring

[PATCH 0/7] find commit subject refactoring

[Christian Couder]

I found 4 different places where there was custom code to find the
subject (sometimes called title) of a commit in the commit buffer.
So the purpose of this series is to refactor this by using a
common function called find_commit_subject(), except for the first
patch that is bug fix.

Christian Couder (7):
  revert: fix off by one read when searching the end of a commit
    subject
  revert: refactor code to find commit subject in find_commit_subject()
  revert: rename subject related variables in get_message()
  commit: move find_commit_subject() into commit.{h,c}
  bisect: use find_commit_subject() instead of custom code
  merge-recursive: use find_commit_subject() instead of custom code
  blame: use find_commit_subject() instead of custom code

 bisect.c                     |   13 +++++--------
 builtin/blame.c              |   22 +++++++---------------
 builtin/revert.c             |   20 +++++---------------
 commit.c                     |   19 +++++++++++++++++++
 commit.h                     |    3 +++
 merge-recursive.c            |   14 ++++----------
 t/t3505-cherry-pick-empty.sh |   20 +++++++++++++++++++-
 7 files changed, 62 insertions(+), 49 deletions(-)

-- 
1.7.2.rc3.267.g400b3

[PATCH 1/7] revert: fix off by one read when searching the end of a commit subject

[Christian Couder]

A test case is added but the problem can only be seen when running
the test case with --valgrind.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c             |    2 +-
 t/t3505-cherry-pick-empty.sh |   20 +++++++++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 8b9d829..3092233 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -131,7 +131,7 @@ static int get_message(const char *raw_message, struct commit_message *out)
 		p++;
 	if (*p) {
 		p += 2;
-		for (eol = p + 1; *eol && *eol != '\n'; eol++)
+		for (eol = p; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
 	} else
 		eol = p;
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index e51e505..c10b28c 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -13,12 +13,30 @@ test_expect_success setup '
 
 	git checkout -b empty-branch &&
 	test_tick &&
-	git commit --allow-empty -m "empty"
+	git commit --allow-empty -m "empty" &&
+
+	echo third >> file1 &&
+	git add file1 &&
+	test_tick &&
+	git commit --allow-empty-message -m ""
 
 '
 
 test_expect_success 'cherry-pick an empty commit' '
 	git checkout master && {
+		git cherry-pick empty-branch^
+		test "$?" = 1
+	}
+'
+
+test_expect_success 'index lockfile was removed' '
+
+	test ! -f .git/index.lock
+
+'
+
+test_expect_success 'cherry-pick a commit with an empty message' '
+	git checkout master && {
 		git cherry-pick empty-branch
 		test "$?" = 1
 	}
-- 
1.7.2.rc3.267.g400b3

[PATCH 2/7] revert: refactor code to find commit subject in find_commit_subject()

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   34 ++++++++++++++++++++++------------
 1 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 3092233..ed89bba 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -99,10 +99,30 @@ struct commit_message {
 	const char *message;
 };
 
+/* Find beginning and length of commit subject. */
+static int find_commit_subject(const char *commit_buffer, const char **subject)
+{
+	const char *eol;
+	const char *p = commit_buffer;
+
+	while (*p && (*p != '\n' || p[1] != '\n'))
+		p++;
+	if (*p) {
+		p += 2;
+		for (eol = p; *eol && *eol != '\n'; eol++)
+			; /* do nothing */
+	} else
+		eol = p;
+
+	*subject = p;
+
+	return eol - p;
+}
+
 static int get_message(const char *raw_message, struct commit_message *out)
 {
 	const char *encoding;
-	const char *p, *abbrev, *eol;
+	const char *p, *abbrev;
 	char *q;
 	int abbrev_len, oneline_len;
 
@@ -125,17 +145,7 @@ static int get_message(const char *raw_message, struct commit_message *out)
 	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
 	abbrev_len = strlen(abbrev);
 
-	/* Find beginning and end of commit subject. */
-	p = out->message;
-	while (*p && (*p != '\n' || p[1] != '\n'))
-		p++;
-	if (*p) {
-		p += 2;
-		for (eol = p; *eol && *eol != '\n'; eol++)
-			; /* do nothing */
-	} else
-		eol = p;
-	oneline_len = eol - p;
+	oneline_len = find_commit_subject(out->message, &p);
 
 	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
 			      strlen("... ") + oneline_len + 1);
-- 
1.7.2.rc3.267.g400b3

[PATCH 3/7] revert: rename subject related variables in get_message()

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index ed89bba..44149b5 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -122,9 +122,9 @@ static int find_commit_subject(const char *commit_buffer, const char **subject)
 static int get_message(const char *raw_message, struct commit_message *out)
 {
 	const char *encoding;
-	const char *p, *abbrev;
+	const char *abbrev, *subject;
+	int abbrev_len, subject_len;
 	char *q;
-	int abbrev_len, oneline_len;
 
 	if (!raw_message)
 		return -1;
@@ -145,17 +145,17 @@ static int get_message(const char *raw_message, struct commit_message *out)
 	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
 	abbrev_len = strlen(abbrev);
 
-	oneline_len = find_commit_subject(out->message, &p);
+	subject_len = find_commit_subject(out->message, &subject);
 
 	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
-			      strlen("... ") + oneline_len + 1);
+			      strlen("... ") + subject_len + 1);
 	q = out->parent_label;
 	q = mempcpy(q, "parent of ", strlen("parent of "));
 	out->label = q;
 	q = mempcpy(q, abbrev, abbrev_len);
 	q = mempcpy(q, "... ", strlen("... "));
 	out->subject = q;
-	q = mempcpy(q, p, oneline_len);
+	q = mempcpy(q, subject, subject_len);
 	*q = '\0';
 	return 0;
 }
-- 
1.7.2.rc3.267.g400b3

[PATCH 4/7] commit: move find_commit_subject() into commit.{h,c}

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   20 --------------------
 commit.c         |   19 +++++++++++++++++++
 commit.h         |    3 +++
 3 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 44149b5..9215e66 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -99,26 +99,6 @@ struct commit_message {
 	const char *message;
 };
 
-/* Find beginning and length of commit subject. */
-static int find_commit_subject(const char *commit_buffer, const char **subject)
-{
-	const char *eol;
-	const char *p = commit_buffer;
-
-	while (*p && (*p != '\n' || p[1] != '\n'))
-		p++;
-	if (*p) {
-		p += 2;
-		for (eol = p; *eol && *eol != '\n'; eol++)
-			; /* do nothing */
-	} else
-		eol = p;
-
-	*subject = p;
-
-	return eol - p;
-}
-
 static int get_message(const char *raw_message, struct commit_message *out)
 {
 	const char *encoding;
diff --git a/commit.c b/commit.c
index e9b0750..0094ec1 100644
--- a/commit.c
+++ b/commit.c
@@ -315,6 +315,25 @@ int parse_commit(struct commit *item)
 	return ret;
 }
 
+int find_commit_subject(const char *commit_buffer, const char **subject)
+{
+	const char *eol;
+	const char *p = commit_buffer;
+
+	while (*p && (*p != '\n' || p[1] != '\n'))
+		p++;
+	if (*p) {
+		p += 2;
+		for (eol = p; *eol && *eol != '\n'; eol++)
+			; /* do nothing */
+	} else
+		eol = p;
+
+	*subject = p;
+
+	return eol - p;
+}
+
 struct commit_list *commit_list_insert(struct commit *item, struct commit_list **list_p)
 {
 	struct commit_list *new_list = xmalloc(sizeof(struct commit_list));
diff --git a/commit.h b/commit.h
index eb2b8ac..9113bbe 100644
--- a/commit.h
+++ b/commit.h
@@ -41,6 +41,9 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size);
 
 int parse_commit(struct commit *item);
 
+/* Find beginning and length of commit subject. */
+int find_commit_subject(const char *commit_buffer, const char **subject);
+
 struct commit_list * commit_list_insert(struct commit *item, struct commit_list **list_p);
 unsigned commit_list_count(const struct commit_list *l);
 struct commit_list * insert_by_date(struct commit *item, struct commit_list **list);
-- 
1.7.2.rc3.267.g400b3

[PATCH 5/7] bisect: use find_commit_subject() instead of custom code

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 bisect.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/bisect.c b/bisect.c
index b556b11..060c042 100644
--- a/bisect.c
+++ b/bisect.c
@@ -141,7 +141,8 @@ static void show_list(const char *debug, int counted, int nr,
 		enum object_type type;
 		unsigned long size;
 		char *buf = read_sha1_file(commit->object.sha1, &type, &size);
-		char *ep, *sp;
+		const char *subject_start;
+		int subject_len;
 
 		fprintf(stderr, "%c%c%c ",
 			(flags & TREESAME) ? ' ' : 'T',
@@ -156,13 +157,9 @@ static void show_list(const char *debug, int counted, int nr,
 			fprintf(stderr, " %.*s", 8,
 				sha1_to_hex(pp->item->object.sha1));
 
-		sp = strstr(buf, "\n\n");
-		if (sp) {
-			sp += 2;
-			for (ep = sp; *ep && *ep != '\n'; ep++)
-				;
-			fprintf(stderr, " %.*s", (int)(ep - sp), sp);
-		}
+		subject_len = find_commit_subject(buf, &subject_start);
+		if (subject_len)
+			fprintf(stderr, " %.*s", subject_len, subject_start);
 		fprintf(stderr, "\n");
 	}
 }
-- 
1.7.2.rc3.267.g400b3

[PATCH 6/7] merge-recursive: use find_commit_subject() instead of custom code

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 merge-recursive.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 529d345..08f666a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -136,16 +136,10 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)
 		if (parse_commit(commit) != 0)
 			printf("(bad commit)\n");
 		else {
-			const char *s;
-			int len;
-			for (s = commit->buffer; *s; s++)
-				if (*s == '\n' && s[1] == '\n') {
-					s += 2;
-					break;
-				}
-			for (len = 0; s[len] && '\n' != s[len]; len++)
-				; /* do nothing */
-			printf("%.*s\n", len, s);
+			const char *title;
+			int len = find_commit_subject(commit->buffer, &title);
+			if (len)
+				printf("%.*s\n", len, title);
 		}
 	}
 }
-- 
1.7.2.rc3.267.g400b3

[PATCH 7/7] blame: use find_commit_subject() instead of custom code

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/blame.c |   22 +++++++---------------
 1 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 01e62fd..437b1a4 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1407,7 +1407,8 @@ static void get_commit_info(struct commit *commit,
 			    int detailed)
 {
 	int len;
-	char *tmp, *endp, *reencoded, *message;
+	const char *subject;
+	char *reencoded, *message;
 	static char author_name[1024];
 	static char author_mail[1024];
 	static char committer_name[1024];
@@ -1449,22 +1450,13 @@ static void get_commit_info(struct commit *commit,
 		    &ret->committer_time, &ret->committer_tz);
 
 	ret->summary = summary_buf;
-	tmp = strstr(message, "\n\n");
-	if (!tmp) {
-	error_out:
+	len = find_commit_subject(message, &subject);
+	if (len && len < sizeof(summary_buf)) {
+		memcpy(summary_buf, subject, len);
+		summary_buf[len] = 0;
+	} else {
 		sprintf(summary_buf, "(%s)", sha1_to_hex(commit->object.sha1));
-		free(reencoded);
-		return;
 	}
-	tmp += 2;
-	endp = strchr(tmp, '\n');
-	if (!endp)
-		endp = tmp + strlen(tmp);
-	len = endp - tmp;
-	if (len >= sizeof(summary_buf) || len == 0)
-		goto error_out;
-	memcpy(summary_buf, tmp, len);
-	summary_buf[len] = 0;
 	free(reencoded);
 }
 
-- 
1.7.2.rc3.267.g400b3

Re: [PATCH 2/7] revert: refactor code to find commit subject in find_commit_subject()

[Peter Baumann]

Wouldn't it be better to merge this with  [PATCH 4/7], so we won't create
find_commit_subject in revert.c and then immediatly move it to commit.c?

Re: [PATCH 0/7] find commit subject refactoring

[Junio C Hamano]

Nice ;-)

Re: [PATCH 1/7] revert: fix off by one read when searching the end of a commit subject

[Jonathan Nieder]

Christian Couder wrote:

> +++ b/builtin/revert.c
> @@ -131,7 +131,7 @@ static int get_message(const char *raw_message, struct commit_message *out)
>  		p++;
>  	if (*p) {
>  		p += 2;
> -		for (eol = p + 1; *eol && *eol != '\n'; eol++)
> +		for (eol = p; *eol && *eol != '\n'; eol++)
>  			; /* do nothing */
>  	} else
>  		eol = p;

Good catch.  For what it’s worth, this and the rest of the series is

Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH 2/7] revert: refactor code to find commit subject in find_commit_subject()

[Christian Couder]

On Thursday 22 July 2010 18:50:12 Peter Baumann wrote:
> Wouldn't it be better to merge this with  [PATCH 4/7], so we won't create
> find_commit_subject in revert.c and then immediatly move it to commit.c?

Yeah, I could have merged those 2 patches, but except for the first one I 
developed them in the order I sent them. So I didn't think much about merging 
some as it felt natural to send them as is.

Best regards,
Christian.