[PATCH] builtin-commit: fix --signoff

[PATCH] builtin-commit: fix --signoff

[Johannes Schindelin]

The Signed-off-by: line contained a spurious timestamp.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-commit.c |   31 ++++++++++++++++++++-----------
 1 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index e8bc4c4..f79ad48 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -181,21 +181,30 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 		die("could not open %s\n", git_path(commit_editmsg));
 
 	stripspace(&sb, 0);
-	if (fwrite(sb.buf, 1, sb.len, fp) < sb.len)
-		die("could not write commit template: %s\n",
-		    strerror(errno));
 
 	if (signoff) {
-		const char *info, *bol;
-
-		info = git_committer_info(1);
-		strbuf_addch(&sb, '\0');
-		bol = strrchr(sb.buf + sb.len - 1, '\n');
-		if (!bol || prefixcmp(bol, sign_off_header))
-			fprintf(fp, "\n");
-		fprintf(fp, "%s%s\n", sign_off_header, git_committer_info(1));
+		struct strbuf sob;
+		const char *p;
+		int i;
+
+		strbuf_init(&sob, 0);
+		strbuf_addstr(&sob, sign_off_header);
+		strbuf_addstr(&sob, git_committer_info(1));
+		p = strrchr(sob.buf, '>');
+		if (p)
+			strbuf_setlen(&sob, p + 1 - sob.buf);
+		strbuf_addch(&sob, '\n');
+
+		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
+			; /* do nothing */
+		if (prefixcmp(sb.buf + i, sob.buf))
+			strbuf_addbuf(&sb, &sob);
 	}
 
+	if (fwrite(sb.buf, 1, sb.len, fp) < sb.len)
+		die("could not write commit template: %s\n",
+		    strerror(errno));
+
 	strbuf_release(&sb);
 
 	if (in_merge && !no_edit)
-- 
1.5.3.5.1674.g6e7f7

Re: [PATCH] builtin-commit: fix --signoff

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>  	if (signoff) {
> ...
> +		strbuf_init(&sob, 0);
> +		strbuf_addstr(&sob, sign_off_header);
> +		strbuf_addstr(&sob, git_committer_info(1));
> +		p = strrchr(sob.buf, '>');
> +		if (p)
> +			strbuf_setlen(&sob, p + 1 - sob.buf);
> +		strbuf_addch(&sob, '\n');
> +
> +		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
> +			; /* do nothing */
> +		if (prefixcmp(sb.buf + i, sob.buf))
> +			strbuf_addbuf(&sb, &sob);
>  	}

At this point doesn't this leak sob.buf?

Re: [PATCH] builtin-commit: fix --signoff

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

On Sat, Nov 10, 2007 at 09:06:28AM +0000, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >  	if (signoff) {
> > ...
> > +		strbuf_init(&sob, 0);
> > +		strbuf_addstr(&sob, sign_off_header);
> > +		strbuf_addstr(&sob, git_committer_info(1));
> > +		p = strrchr(sob.buf, '>');
> > +		if (p)
> > +			strbuf_setlen(&sob, p + 1 - sob.buf);
> > +		strbuf_addch(&sob, '\n');
> > +
> > +		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
> > +			; /* do nothing */
> > +		if (prefixcmp(sb.buf + i, sob.buf))
> > +			strbuf_addbuf(&sb, &sob);
> >  	}
> 
> At this point doesn't this leak sob.buf?

  It does, strbuf_addbuf copies `sob` but doesn't release resources.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] builtin-commit: fix --signoff

[Johannes Schindelin]

Hi,

On Sat, 10 Nov 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >  	if (signoff) {
> > ...
> > +		strbuf_init(&sob, 0);
> > +		strbuf_addstr(&sob, sign_off_header);
> > +		strbuf_addstr(&sob, git_committer_info(1));
> > +		p = strrchr(sob.buf, '>');
> > +		if (p)
> > +			strbuf_setlen(&sob, p + 1 - sob.buf);
> > +		strbuf_addch(&sob, '\n');
> > +
> > +		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
> > +			; /* do nothing */
> > +		if (prefixcmp(sb.buf + i, sob.buf))
> > +			strbuf_addbuf(&sb, &sob);
> >  	}
> 
> At this point doesn't this leak sob.buf?

Darn.  I had a "strbuf_release(&sob);" there, but at some stage removed it 
by mistake.

But there is more to be fixed.  Rather than let git_committer_info() add 
the timestamp, only to strip it away, is a waste.

Will redo the patch.

Ciao,
Dscho

[PATCH] builtin-commit: fix --signoff

[Johannes Schindelin]

The Signed-off-by: line contained a spurious timestamp.  The reason was
a call to git_committer_info(1), which automatically added the
timestamp.

Instead, fmt_ident() was taught to interpret an empty string for the
date (as opposed to NULL, which still triggers the default behavior)
as "do not bother with the timestamp", and builtin-commit.c uses it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Sat, 10 Nov 2007, Johannes Schindelin wrote:

	> Will redo the patch.

	And here it is.

 builtin-commit.c  |   29 ++++++++++++++++++-----------
 ident.c           |   10 +++++++---
 t/t7500-commit.sh |   13 +++++++++++++
 3 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index e8bc4c4..a4d69ad 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -181,21 +181,28 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 		die("could not open %s\n", git_path(commit_editmsg));
 
 	stripspace(&sb, 0);
-	if (fwrite(sb.buf, 1, sb.len, fp) < sb.len)
-		die("could not write commit template: %s\n",
-		    strerror(errno));
 
 	if (signoff) {
-		const char *info, *bol;
-
-		info = git_committer_info(1);
-		strbuf_addch(&sb, '\0');
-		bol = strrchr(sb.buf + sb.len - 1, '\n');
-		if (!bol || prefixcmp(bol, sign_off_header))
-			fprintf(fp, "\n");
-		fprintf(fp, "%s%s\n", sign_off_header, git_committer_info(1));
+		struct strbuf sob;
+		int i;
+
+		strbuf_init(&sob, 0);
+		strbuf_addstr(&sob, sign_off_header);
+		strbuf_addstr(&sob, fmt_ident(getenv("GIT_COMMITTER_NAME"),
+                         getenv("GIT_COMMITTER_EMAIL"), "", 1));
+		strbuf_addch(&sob, '\n');
+
+		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
+			; /* do nothing */
+		if (prefixcmp(sb.buf + i, sob.buf))
+			strbuf_addbuf(&sb, &sob);
+		strbuf_release(&sob);
 	}
 
+	if (fwrite(sb.buf, 1, sb.len, fp) < sb.len)
+		die("could not write commit template: %s\n",
+		    strerror(errno));
+
 	strbuf_release(&sb);
 
 	if (in_merge && !no_edit)
diff --git a/ident.c b/ident.c
index 9b2a852..5be7533 100644
--- a/ident.c
+++ b/ident.c
@@ -224,13 +224,17 @@ const char *fmt_ident(const char *name, const char *email,
 	}
 
 	strcpy(date, git_default_date);
-	if (date_str)
-		parse_date(date_str, date, sizeof(date));
+	if (date_str) {
+		if (*date_str)
+			parse_date(date_str, date, sizeof(date));
+		else
+			date[0] = '\0';
+	}
 
 	i = copy(buffer, sizeof(buffer), 0, name);
 	i = add_raw(buffer, sizeof(buffer), i, " <");
 	i = copy(buffer, sizeof(buffer), i, email);
-	i = add_raw(buffer, sizeof(buffer), i, "> ");
+	i = add_raw(buffer, sizeof(buffer), i, date[0] ? "> " : ">");
 	i = copy(buffer, sizeof(buffer), i, date);
 	if (i >= sizeof(buffer))
 		die("Impossibly long personal identifier");
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index abbf54b..13d5a0c 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -93,4 +93,17 @@ test_expect_success 'commit message from file should override template' '
 	commit_msg_is "standard input msg"
 '
 
+cat > expect << EOF
+zort
+Signed-off-by: C O Mitter <committer@example.com>
+EOF
+
+test_expect_success '--signoff' '
+	echo "yet another content *narf*" >> foo &&
+	echo "zort" |
+		GIT_EDITOR=../t7500/add-content git commit -s -F - foo &&
+	git cat-file commit HEAD | sed "1,/^$/d" > output &&
+	git diff expect output
+'
+
 test_done
-- 
1.5.3.5.1674.g6e7f7