[PATCH 0/6] Various (replacement) patches to builtin-commit

[PATCH 0/6] Various (replacement) patches to builtin-commit

[Johannes Schindelin]

Hi,

I did some more work on top of kh/commit (rebased to next, + Kristian's 
refresh_cache() patch which is still waiting to be enhanced with 
refresh_cache()-after-commit-with-paths).

They are in chronological order.  In particular, all patches except 2/6 
resurrect old behaviour.

Ciao,
Dscho

[PATCH 1/6] builtin-commit: fix author date with --amend --author=<author>

[Johannes Schindelin]

When amending a commit with a new author, the author date is taken
from the original commit.

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

diff --git a/builtin-commit.c b/builtin-commit.c
index a84a729..6be6def 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -527,6 +527,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	} else if (amend) {
 		struct commit_list *c;
 		struct commit *commit;
+		const char *author;
 
 		reflog_msg = "commit (amend)";
 		commit = lookup_commit(head_sha1);
@@ -536,6 +537,21 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		for (c = commit->parents; c; c = c->next)
 			strbuf_addf(&sb, "parent %s\n",
 				      sha1_to_hex(c->item->object.sha1));
+
+		/* determine author date */
+		author = strstr(commit->buffer, "\nauthor");
+		if (author && !memmem(commit->buffer,
+					author + 1 - commit->buffer,
+					"\n\n", 2)) {
+			const char *email_end = strchr(author + 1, '>');
+			const char *line_end = strchr(author + 1, '\n');
+			if (email_end && line_end && email_end < line_end) {
+				char *date = xstrndup(email_end + 1,
+					line_end - email_end - 1);
+				setenv("GIT_AUTHOR_DATE", date, 1);
+				free(date);
+			}
+		}
 	} else if (in_merge) {
 		struct strbuf m;
 		FILE *fp;
-- 
1.5.3.5.1693.g26ed

Re: [PATCH 1/6] builtin-commit: fix author date with --amend --author=<author>

[Kristian Høgsberg]

On Sun, 2007-11-11 at 17:35 +0000, Johannes Schindelin wrote:
> When amending a commit with a new author, the author date is taken
> from the original commit.

The new determine_author_info() should fix this.  There was a problem
earlier that Junio pointed out and I sent a patch to update
determine_author_info() to do the right thing for amend commits.  The
test suite still pass without this patch, and if you look carefully a
determine_author_info(), you can see it does the right thing:

  1) Default to getenv for name, email and date

  2) If a commit has been specified (-c, -C or --amend), we parse the
author name, email and date from use_message_buffer, which holds the
commit, overriding the values from getenv.

  3) If --author has be passed, we parse name and email from the
argument and override whatever name and email the two previous steps
came up with.

Then we add the author line to the commit buffer under construction
based on these values.

I suggest we back this patch out.

cheers,
Kristian

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin-commit.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/builtin-commit.c b/builtin-commit.c
> index a84a729..6be6def 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -527,6 +527,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	} else if (amend) {
>  		struct commit_list *c;
>  		struct commit *commit;
> +		const char *author;
>  
>  		reflog_msg = "commit (amend)";
>  		commit = lookup_commit(head_sha1);
> @@ -536,6 +537,21 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		for (c = commit->parents; c; c = c->next)
>  			strbuf_addf(&sb, "parent %s\n",
>  				      sha1_to_hex(c->item->object.sha1));
> +
> +		/* determine author date */
> +		author = strstr(commit->buffer, "\nauthor");
> +		if (author && !memmem(commit->buffer,
> +					author + 1 - commit->buffer,
> +					"\n\n", 2)) {
> +			const char *email_end = strchr(author + 1, '>');
> +			const char *line_end = strchr(author + 1, '\n');
> +			if (email_end && line_end && email_end < line_end) {
> +				char *date = xstrndup(email_end + 1,
> +					line_end - email_end - 1);
> +				setenv("GIT_AUTHOR_DATE", date, 1);
> +				free(date);
> +			}
> +		}
>  	} else if (in_merge) {
>  		struct strbuf m;
>  		FILE *fp;

Re: [PATCH 1/6] builtin-commit: fix author date with --amend --author=<author>

[Johannes Schindelin]

Hi,

On Mon, 12 Nov 2007, Kristian H?gsberg wrote:

> On Sun, 2007-11-11 at 17:35 +0000, Johannes Schindelin wrote:
> > When amending a commit with a new author, the author date is taken
> > from the original commit.
> 
> The new determine_author_info() should fix this.  There was a problem
> earlier that Junio pointed out and I sent a patch to update
> determine_author_info() to do the right thing for amend commits.  The
> test suite still pass without this patch, and if you look carefully a
> determine_author_info(), you can see it does the right thing:
> 
>   1) Default to getenv for name, email and date
> 
>   2) If a commit has been specified (-c, -C or --amend), we parse the
> author name, email and date from use_message_buffer, which holds the
> commit, overriding the values from getenv.
> 
>   3) If --author has be passed, we parse name and email from the
> argument and override whatever name and email the two previous steps
> came up with.
> 
> Then we add the author line to the commit buffer under construction
> based on these values.
> 
> I suggest we back this patch out.

I agree.

Ciao,
Dscho

Re: [PATCH 1/6] builtin-commit: fix author date with --amend --author=<author>

[Junio C Hamano]

Kristian Høgsberg <krh@redhat.com> writes:

> I suggest we back this patch out.

Ok.  My mistake.  Your "Add testcase for amending and fixing
author in git commit" does contain test for this.

I need to amend the commit log message of that one, though ;-)

[PATCH 2/6] git status: show relative paths when run in a subdirectory

[Johannes Schindelin]

To show the relative paths, the function formerly called quote_crlf()
(now called quote_path()) takes the prefix as an additional argument.

While at it, the static buffers were replaced by strbufs.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-commit.c    |   13 ++++---
 builtin-runstatus.c |    1 +
 t/t7502-status.sh   |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c         |   69 ++++++++++++++++++++++++++-------------
 wt-status.h         |    1 +
 5 files changed, 146 insertions(+), 29 deletions(-)
 create mode 100755 t/t7502-status.sh

diff --git a/builtin-commit.c b/builtin-commit.c
index 6be6def..5c7d4df 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -118,11 +118,12 @@ static char *prepare_index(const char **files, const char *prefix)
 	return next_index_lock->filename;
 }
 
-static int run_status(FILE *fp, const char *index_file)
+static int run_status(FILE *fp, const char *index_file, const char *prefix)
 {
 	struct wt_status s;
 
 	wt_status_prepare(&s);
+	s.prefix = prefix;
 
 	if (amend) {
 		s.amend = 1;
@@ -140,7 +141,7 @@ static int run_status(FILE *fp, const char *index_file)
 
 static const char sign_off_header[] = "Signed-off-by: ";
 
-static int prepare_log_message(const char *index_file)
+static int prepare_log_message(const char *index_file, const char *prefix)
 {
 	struct stat statbuf;
 	int commitable;
@@ -216,7 +217,7 @@ static int prepare_log_message(const char *index_file)
 	if (only_include_assumed)
 		fprintf(fp, "# %s\n", only_include_assumed);
 
-	commitable = run_status(fp, index_file);
+	commitable = run_status(fp, index_file, prefix);
 
 	fclose(fp);
 
@@ -409,7 +410,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	index_file = prepare_index(argv, prefix);
 
-	commitable = run_status(stdout, index_file);
+	commitable = run_status(stdout, index_file, prefix);
 
 	rollback_lock_file(&lock_file);
 
@@ -503,8 +504,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
 		exit(1);
 
-	if (!prepare_log_message(index_file) && !in_merge) {
-		run_status(stdout, index_file);
+	if (!prepare_log_message(index_file, prefix) && !in_merge) {
+		run_status(stdout, index_file, prefix);
 		unlink(commit_editmsg);
 		return 1;
 	}
diff --git a/builtin-runstatus.c b/builtin-runstatus.c
index 2db25c8..8d167a9 100644
--- a/builtin-runstatus.c
+++ b/builtin-runstatus.c
@@ -14,6 +14,7 @@ int cmd_runstatus(int argc, const char **argv, const char *prefix)
 
 	git_config(git_status_config);
 	wt_status_prepare(&s);
+	s.prefix = prefix;
 
 	for (i = 1; i < argc; i++) {
 		if (!strcmp(argv[i], "--color"))
diff --git a/t/t7502-status.sh b/t/t7502-status.sh
new file mode 100755
index 0000000..269b334
--- /dev/null
+++ b/t/t7502-status.sh
@@ -0,0 +1,91 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Johannes E. Schindelin
+#
+
+test_description='git-status'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	: > tracked &&
+	: > modified &&
+	mkdir dir1 &&
+	: > dir1/tracked &&
+	: > dir1/modified &&
+	mkdir dir2 &&
+	: > dir1/tracked &&
+	: > dir1/modified &&
+	git add . &&
+	test_tick &&
+	git commit -m initial &&
+	: > untracked &&
+	: > dir1/untracked &&
+	: > dir2/untracked &&
+	echo 1 > dir1/modified &&
+	echo 2 > dir2/modified &&
+	echo 3 > dir2/added &&
+	git add dir2/added
+'
+
+cat > expect << \EOF
+# On branch master
+# Changes to be committed:
+#   (use "git reset HEAD <file>..." to unstage)
+#
+#	new file:   dir2/added
+#
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#	modified:   dir1/modified
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	dir1/untracked
+#	dir2/modified
+#	dir2/untracked
+#	expect
+#	output
+#	untracked
+EOF
+
+test_expect_success 'status' '
+
+	git status > output &&
+	git diff expect output
+
+'
+
+cat > expect << \EOF
+# On branch master
+# Changes to be committed:
+#   (use "git reset HEAD <file>..." to unstage)
+#
+#	new file:   ../dir2/added
+#
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#	modified:   ../dir1/modified
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	untracked
+#	../dir2/modified
+#	../dir2/untracked
+#	../expect
+#	../output
+#	../untracked
+EOF
+
+test_expect_success 'status with relative paths' '
+
+	(cd dir1 && git status) > output &&
+	git diff expect output
+
+'
+
+test_done
diff --git a/wt-status.c b/wt-status.c
index 03b5ec4..0d25362 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -82,33 +82,46 @@ static void wt_status_print_trailer(struct wt_status *s)
 	color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "#");
 }
 
-static const char *quote_crlf(const char *in, char *buf, size_t sz)
+static char *quote_path(const char *in, int len,
+		struct strbuf *out, const char *prefix)
 {
-	const char *scan;
-	char *out;
-	const char *ret = in;
+	if (len > 0)
+		strbuf_grow(out, len);
+	strbuf_setlen(out, 0);
+
+	if (prefix) {
+		int off = 0;
+		while (prefix[off] && off < len && prefix[off] == in[off])
+			if (prefix[off] == '/') {
+				prefix += off + 1;
+				in += off + 1;
+				len -= off + 1;
+				off = 0;
+			} else
+				off++;
+
+		for (; *prefix; prefix++)
+			if (*prefix == '/')
+				strbuf_addstr(out, "../");
+	}
 
-	for (scan = in, out = buf; *scan; scan++) {
-		int ch = *scan;
-		int quoted;
+	for (; (len < 0 && *in) || len > 0; in++, len--) {
+		int ch = *in;
 
 		switch (ch) {
 		case '\n':
-			quoted = 'n';
+			strbuf_addstr(out, "\\n");
 			break;
 		case '\r':
-			quoted = 'r';
+			strbuf_addstr(out, "\\r");
 			break;
 		default:
-			*out++ = ch;
+			strbuf_addch(out, ch);
 			continue;
 		}
-		*out++ = '\\';
-		*out++ = quoted;
-		ret = buf;
 	}
-	*out = '\0';
-	return ret;
+
+	return out->buf;
 }
 
 static void wt_status_print_filepair(struct wt_status *s,
@@ -116,10 +129,12 @@ static void wt_status_print_filepair(struct wt_status *s,
 {
 	const char *c = color(t);
 	const char *one, *two;
-	char onebuf[PATH_MAX], twobuf[PATH_MAX];
+	struct strbuf onebuf, twobuf;
 
-	one = quote_crlf(p->one->path, onebuf, sizeof(onebuf));
-	two = quote_crlf(p->two->path, twobuf, sizeof(twobuf));
+	strbuf_init(&onebuf, 0);
+	strbuf_init(&twobuf, 0);
+	one = quote_path(p->one->path, -1, &onebuf, s->prefix);
+	two = quote_path(p->two->path, -1, &twobuf, s->prefix);
 
 	color_fprintf(s->fp, color(WT_STATUS_HEADER), "#\t");
 	switch (p->status) {
@@ -151,6 +166,8 @@ static void wt_status_print_filepair(struct wt_status *s,
 		die("bug: unhandled diff status %c", p->status);
 	}
 	fprintf(s->fp, "\n");
+	strbuf_release(&onebuf);
+	strbuf_release(&twobuf);
 }
 
 static void wt_status_print_updated_cb(struct diff_queue_struct *q,
@@ -205,8 +222,9 @@ static void wt_read_cache(struct wt_status *s)
 static void wt_status_print_initial(struct wt_status *s)
 {
 	int i;
-	char buf[PATH_MAX];
+	struct strbuf buf;
 
+	strbuf_init(&buf, 0);
 	wt_read_cache(s);
 	if (active_nr) {
 		s->commitable = 1;
@@ -215,11 +233,12 @@ static void wt_status_print_initial(struct wt_status *s)
 	for (i = 0; i < active_nr; i++) {
 		color_fprintf(s->fp, color(WT_STATUS_HEADER), "#\t");
 		color_fprintf_ln(s->fp, color(WT_STATUS_UPDATED), "new file: %s",
-				quote_crlf(active_cache[i]->name,
-					   buf, sizeof(buf)));
+				quote_path(active_cache[i]->name, -1,
+					   &buf, s->prefix));
 	}
 	if (active_nr)
 		wt_status_print_trailer(s);
+	strbuf_release(&buf);
 }
 
 static void wt_status_print_updated(struct wt_status *s)
@@ -254,7 +273,9 @@ static void wt_status_print_untracked(struct wt_status *s)
 	const char *x;
 	int i;
 	int shown_header = 0;
+	struct strbuf buf;
 
+	strbuf_init(&buf, 0);
 	memset(&dir, 0, sizeof(dir));
 
 	dir.exclude_per_dir = ".gitignore";
@@ -291,9 +312,11 @@ static void wt_status_print_untracked(struct wt_status *s)
 			shown_header = 1;
 		}
 		color_fprintf(s->fp, color(WT_STATUS_HEADER), "#\t");
-		color_fprintf_ln(s->fp, color(WT_STATUS_UNTRACKED), "%.*s",
-				ent->len, ent->name);
+		color_fprintf_ln(s->fp, color(WT_STATUS_UNTRACKED), "%s",
+				quote_path(ent->name, ent->len,
+					&buf, s->prefix));
 	}
+	strbuf_release(&buf);
 }
 
 static void wt_status_print_verbose(struct wt_status *s)
diff --git a/wt-status.h b/wt-status.h
index 7744932..f58ebcb 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -23,6 +23,7 @@ struct wt_status {
 	int workdir_untracked;
 	const char *index_file;
 	FILE *fp;
+	const char *prefix;
 };
 
 int git_status_config(const char *var, const char *value);
-- 
1.5.3.5.1693.g26ed

[PATCH 3/6] 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>
---
 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 5c7d4df..6b1507d 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -183,21 +183,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.1693.g26ed

[PATCH 4/6] builtin-commit --s: add a newline if the last line was no S-O-B

[Johannes Schindelin]

The rule is this: if the last line already contains the sign off by the
current committer, do nothing.  If it contains another sign off, just
add the sign off of the current committer.  If the last line does not
contain a sign off, add a new line before adding the sign off.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-commit.c  |    5 ++++-
 t/t7500-commit.sh |    1 +
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 6b1507d..66d7e5e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -196,8 +196,11 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 
 		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
 			; /* do nothing */
-		if (prefixcmp(sb.buf + i, sob.buf))
+		if (prefixcmp(sb.buf + i, sob.buf)) {
+			if (prefixcmp(sb.buf + i, sign_off_header))
+				strbuf_addch(&sb, '\n');
 			strbuf_addbuf(&sb, &sob);
+		}
 		strbuf_release(&sob);
 	}
 
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 13d5a0c..e0be2ed 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -95,6 +95,7 @@ test_expect_success 'commit message from file should override template' '
 
 cat > expect << EOF
 zort
+
 Signed-off-by: C O Mitter <committer@example.com>
 EOF
 
-- 
1.5.3.5.1693.g26ed

[PATCH 5/6] builtin-commit: resurrect behavior for multiple -m options

[Johannes Schindelin]

When more than one -m option is given, the message does not replace
the previous, but is appended.

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

diff --git a/builtin-commit.c b/builtin-commit.c
index 66d7e5e..069d180 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -30,13 +30,27 @@ static char *use_message_buffer;
 static const char commit_editmsg[] = "COMMIT_EDITMSG";
 static struct lock_file lock_file;
 
-static char *logfile, *force_author, *message, *template_file;
+static char *logfile, *force_author, *template_file;
 static char *edit_message, *use_message;
 static int all, edit_flag, also, interactive, only, amend, signoff;
 static int quiet, verbose, untracked_files, no_verify;
 
 static int no_edit, initial_commit, in_merge;
 const char *only_include_assumed;
+struct strbuf message;
+
+static int opt_parse_m(const struct option *opt, const char *arg, int unset)
+{
+	struct strbuf *buf = opt->value;
+	if (unset)
+		strbuf_setlen(buf, 0);
+	else {
+		strbuf_addstr(buf, arg);
+		strbuf_addch(buf, '\n');
+		strbuf_addch(buf, '\n');
+	}
+	return 0;
+}
 
 static struct option builtin_commit_options[] = {
 	OPT__QUIET(&quiet),
@@ -45,7 +59,7 @@ static struct option builtin_commit_options[] = {
 
 	OPT_STRING('F', "file", &logfile, "FILE", "read log from file"),
 	OPT_STRING(0, "author", &force_author, "AUTHOR", "override author for commit"),
-	OPT_STRING('m', "message", &message, "MESSAGE", "specify commit message"),
+	OPT_CALLBACK('m', "message", &message, "MESSAGE", "specify commit message", opt_parse_m),
 	OPT_STRING('c', "reedit-message", &edit_message, "COMMIT", "reuse and edit message from specified commit "),
 	OPT_STRING('C', "reuse-message", &use_message, "COMMIT", "reuse message from specified commit"),
 	OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by: header"),
@@ -150,8 +164,8 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 	FILE *fp;
 
 	strbuf_init(&sb, 0);
-	if (message) {
-		strbuf_add(&sb, message, strlen(message));
+	if (message.len) {
+		strbuf_addbuf(&sb, &message);
 	} else if (logfile && !strcmp(logfile, "-")) {
 		if (isatty(0))
 			fprintf(stderr, "(reading log message from standard input)\n");
@@ -321,7 +335,7 @@ static int parse_and_validate_options(int argc, const char *argv[])
 	argc = parse_options(argc, argv, builtin_commit_options,
 			     builtin_commit_usage, 0);
 
-	if (logfile || message || use_message)
+	if (logfile || message.len || use_message)
 		no_edit = 1;
 	if (edit_flag)
 		no_edit = 0;
@@ -346,7 +360,7 @@ static int parse_and_validate_options(int argc, const char *argv[])
 		f++;
 	if (f > 1)
 		die("Only one of -c/-C/-F can be used.");
-	if (message && f > 0)
+	if (message.len && f > 0)
 		die("Option -m cannot be combined with -c/-C/-F.");
 	if (edit_message)
 		use_message = edit_message;
-- 
1.5.3.5.1693.g26ed

Re: [PATCH 5/6] builtin-commit: resurrect behavior for multiple -m options

[Pierre Habouzit]

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

On Sun, Nov 11, 2007 at 05:36:39PM +0000, Johannes Schindelin wrote:
> 
> When more than one -m option is given, the message does not replace
> the previous, but is appended.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin-commit.c |   26 ++++++++++++++++++++------
>  1 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin-commit.c b/builtin-commit.c
> index 66d7e5e..069d180 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -30,13 +30,27 @@ static char *use_message_buffer;
>  static const char commit_editmsg[] = "COMMIT_EDITMSG";
>  static struct lock_file lock_file;
>  
> -static char *logfile, *force_author, *message, *template_file;
> +static char *logfile, *force_author, *template_file;
>  static char *edit_message, *use_message;
>  static int all, edit_flag, also, interactive, only, amend, signoff;
>  static int quiet, verbose, untracked_files, no_verify;
>  
>  static int no_edit, initial_commit, in_merge;
>  const char *only_include_assumed;
> +struct strbuf message;

  Unless I'm mistaken `static` keywords are missign for`message` and
`only_include_assumed`.

  And you _have_ to initialize message with STRBUF_INIT (remember of the
slop).

> +static int opt_parse_m(const struct option *opt, const char *arg, int unset)
> +{
> +	struct strbuf *buf = opt->value;
> +	if (unset)
> +		strbuf_setlen(buf, 0);
> +	else {
> +		strbuf_addstr(buf, arg);
> +		strbuf_addch(buf, '\n');
> +		strbuf_addch(buf, '\n');
> +	}
> +	return 0;
> +}

  I believe such a callback could live in parse-options.[hc]. The need
to aggregate all string arguments into a strbuf looks generic enough to
me. Why are you adding two '\n' btw ? Isn't one enough ?

  Oh and last nitpicking, strbuf_addstr(buf, "\n\n"); is more efficient
than the two addchar (the strlen it generates is inlined).

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

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

Re: [PATCH 5/6] builtin-commit: resurrect behavior for multiple -m options

[Johannes Schindelin]

Hi,

On Sun, 11 Nov 2007, Pierre Habouzit wrote:

> On Sun, Nov 11, 2007 at 05:36:39PM +0000, Johannes Schindelin wrote:
> > 
> > When more than one -m option is given, the message does not replace
> > the previous, but is appended.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin-commit.c |   26 ++++++++++++++++++++------
> >  1 files changed, 20 insertions(+), 6 deletions(-)
> > 
> > diff --git a/builtin-commit.c b/builtin-commit.c
> > index 66d7e5e..069d180 100644
> > --- a/builtin-commit.c
> > +++ b/builtin-commit.c
> > @@ -30,13 +30,27 @@ static char *use_message_buffer;
> >  static const char commit_editmsg[] = "COMMIT_EDITMSG";
> >  static struct lock_file lock_file;
> >  
> > -static char *logfile, *force_author, *message, *template_file;
> > +static char *logfile, *force_author, *template_file;
> >  static char *edit_message, *use_message;
> >  static int all, edit_flag, also, interactive, only, amend, signoff;
> >  static int quiet, verbose, untracked_files, no_verify;
> >  
> >  static int no_edit, initial_commit, in_merge;
> >  const char *only_include_assumed;
> > +struct strbuf message;
> 
>   Unless I'm mistaken `static` keywords are missign for`message` and
> `only_include_assumed`.

Oh yeah.  Will fix.

>   And you _have_ to initialize message with STRBUF_INIT (remember of the
> slop).

Not in this case, since I do not use message.buf as long as message.len == 
0.  But I agree it would be cleaner to just use STRBUF_INIT.

> > +static int opt_parse_m(const struct option *opt, const char *arg, int unset)
> > +{
> > +	struct strbuf *buf = opt->value;
> > +	if (unset)
> > +		strbuf_setlen(buf, 0);
> > +	else {
> > +		strbuf_addstr(buf, arg);
> > +		strbuf_addch(buf, '\n');
> > +		strbuf_addch(buf, '\n');
> > +	}
> > +	return 0;
> > +}
> 
>   I believe such a callback could live in parse-options.[hc]. The need
> to aggregate all string arguments into a strbuf looks generic enough to
> me. Why are you adding two '\n' btw ? Isn't one enough ?

Well, this empty line is needed to stay backwards compatible.  It was 
added to pass the test that Junio added to 'next'.  As such, this function 
is not really generic enough, right?

>   Oh and last nitpicking, strbuf_addstr(buf, "\n\n"); is more efficient
> than the two addchar (the strlen it generates is inlined).

Well, I meant to mention it in the cover letter.  My preference is to do 
away with the extra empty line.  But this might break existing setups 
depending on that behaviour.

Ciao,
Dscho

Re: [PATCH 5/6] builtin-commit: resurrect behavior for multiple -m options

[Pierre Habouzit]

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

On Sun, Nov 11, 2007 at 08:42:48PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 11 Nov 2007, Pierre Habouzit wrote:
> 
> > On Sun, Nov 11, 2007 at 05:36:39PM +0000, Johannes Schindelin wrote:
> > > 
> > > When more than one -m option is given, the message does not replace
> > > the previous, but is appended.
> > > 
> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > ---
> > >  builtin-commit.c |   26 ++++++++++++++++++++------
> > >  1 files changed, 20 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/builtin-commit.c b/builtin-commit.c
> > > index 66d7e5e..069d180 100644
> > > --- a/builtin-commit.c
> > > +++ b/builtin-commit.c
> > > @@ -30,13 +30,27 @@ static char *use_message_buffer;
> > >  static const char commit_editmsg[] = "COMMIT_EDITMSG";
> > >  static struct lock_file lock_file;
> > >  
> > > -static char *logfile, *force_author, *message, *template_file;
> > > +static char *logfile, *force_author, *template_file;
> > >  static char *edit_message, *use_message;
> > >  static int all, edit_flag, also, interactive, only, amend, signoff;
> > >  static int quiet, verbose, untracked_files, no_verify;
> > >  
> > >  static int no_edit, initial_commit, in_merge;
> > >  const char *only_include_assumed;
> > > +struct strbuf message;
> > 
> >   Unless I'm mistaken `static` keywords are missign for`message` and
> > `only_include_assumed`.
> 
> Oh yeah.  Will fix.
> 
> >   And you _have_ to initialize message with STRBUF_INIT (remember of the
> > slop).
> 
> Not in this case, since I do not use message.buf as long as message.len == 
> 0.  But I agree it would be cleaner to just use STRBUF_INIT.

  I know that you don't use it, but who knows where this code will be
headed in its future right ? :) It's just good practice.

> > > +static int opt_parse_m(const struct option *opt, const char *arg, int unset)
> > > +{
> > > +	struct strbuf *buf = opt->value;
> > > +	if (unset)
> > > +		strbuf_setlen(buf, 0);
> > > +	else {
> > > +		strbuf_addstr(buf, arg);
> > > +		strbuf_addch(buf, '\n');
> > > +		strbuf_addch(buf, '\n');
> > > +	}
> > > +	return 0;
> > > +}
> > 
> >   I believe such a callback could live in parse-options.[hc]. The need
> > to aggregate all string arguments into a strbuf looks generic enough to
> > me. Why are you adding two '\n' btw ? Isn't one enough ?
> 
> Well, this empty line is needed to stay backwards compatible.  It was 
> added to pass the test that Junio added to 'next'.  As such, this function 
> is not really generic enough, right?

  Well, you can make it generic enough using the defval to point to a
delimiter to use between lines :) But it's probably an overkill, and yes
if the "\n\n" is to be kept, then it's not generic "enough".

> Well, I meant to mention it in the cover letter.  My preference is to do 
> away with the extra empty line.  But this might break existing setups 
> depending on that behaviour.

  Right.

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

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

Re: [PATCH 5/6] builtin-commit: resurrect behavior for multiple -m options

[Pierre Habouzit]

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

On Sun, Nov 11, 2007 at 08:42:48PM +0000, Johannes Schindelin wrote:
> Well, I meant to mention it in the cover letter.  My preference is to do 
> away with the extra empty line.  But this might break existing setups 
> depending on that behaviour.

  In fact I believe what matters is that if there is more than one -m,
you have a \n\n between the first and the second one, else it'll break
subjects, and that sucks, so I'd say we have to stay with "\n\n" at
least for the first aggregation, and I'm unsure if it's worth the hassle
to count how many times we aggregated to use '\n' then.

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

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

[PATCH 6/6] builtin-commit: Add newline when showing which commit was created

[Johannes Schindelin]

The function log_tree_commit() does not break the line, so we have to
do it ourselves.

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

diff --git a/builtin-commit.c b/builtin-commit.c
index 069d180..3739bfc 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -493,6 +493,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	printf("Created %scommit ", initial_commit ? "initial " : "");
 
 	log_tree_commit(&rev, commit);
+	printf("\n");
 }
 
 int git_commit_config(const char *k, const char *v)
-- 
1.5.3.5.1693.g26ed

Re: [PATCH 6/6] builtin-commit: Add newline when showing which commit was created

[Jeff King]

On Sun, Nov 11, 2007 at 05:36:52PM +0000, Johannes Schindelin wrote:

> The function log_tree_commit() does not break the line, so we have to
> do it ourselves.

Johannes,

Can you explain the rationale for this change in more detail?

When I run builtin-commit from the tip of next, I always get an extra
newline (as compared to the shell behavior):

-- >8 --
$ git version &&
  mkdir test && cd test && git init &&
  touch file && git add file && git commit -m added &&
  echo content >file && git commit -a -m updated &&
  echo done
git version 1.5.3.6.2090.g4ece0
Initialized empty Git repository in .git/
Created initial commit b3cbe63: added
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 file

Created commit 7a6b446: updated
 1 files changed, 1 insertions(+), 0 deletions(-)

done
-- 8< --

where the shell behavior omits the extra newlines. Is there some other
input for which log_tree_commit actually needs the newline?

-Peff

Re: [PATCH 6/6] builtin-commit: Add newline when showing which commit was created

[Johannes Schindelin]

Hi,

On Sat, 1 Dec 2007, Jeff King wrote:

> On Sun, Nov 11, 2007 at 05:36:52PM +0000, Johannes Schindelin wrote:
> 
> > The function log_tree_commit() does not break the line, so we have to
> > do it ourselves.
> 
> Johannes,
> 
> Can you explain the rationale for this change in more detail?

Basically, I ran a test case in which the shell script was different from 
the builtin version, and this was the patch that fixed it for me.

Maybe it should have been

	if (log_tree_commit(&rev, commit))
		printf("\n");

at the end of print_summary() instead.  Can you try if that fixes it for 
you?

Ciao,
Dscho

Re: [PATCH 6/6] builtin-commit: Add newline when showing which commit was created

[Jeff King]

On Sat, Dec 01, 2007 at 10:41:47PM +0000, Johannes Schindelin wrote:

> Basically, I ran a test case in which the shell script was different from 
> the builtin version, and this was the patch that fixed it for me.
> 
> Maybe it should have been
> 
> 	if (log_tree_commit(&rev, commit))
> 		printf("\n");
> 
> at the end of print_summary() instead.  Can you try if that fixes it for 
> you?

No, it doesn't, unless you meant "if (!log_tree_commit(...". Of course,
I can't get log_tree_commit to return anything but true anyway (either
log_tree_diff shows something, or since we have set always_show_header,
we end up calling show_log).

It would be helpful if you could remember the test case, but perhaps
that is not an option at this point.

-Peff

Re: [PATCH 6/6] builtin-commit: Add newline when showing which commit was created

[Johannes Schindelin]

Hi,

On Sun, 2 Dec 2007, Jeff King wrote:

> On Sat, Dec 01, 2007 at 10:41:47PM +0000, Johannes Schindelin wrote:
> 
> > Basically, I ran a test case in which the shell script was different from 
> > the builtin version, and this was the patch that fixed it for me.
> > 
> > Maybe it should have been
> > 
> > 	if (log_tree_commit(&rev, commit))
> > 		printf("\n");
> > 
> > at the end of print_summary() instead.  Can you try if that fixes it for 
> > you?
> 
> No, it doesn't, unless you meant "if (!log_tree_commit(...". Of course,
> I can't get log_tree_commit to return anything but true anyway (either
> log_tree_diff shows something, or since we have set always_show_header,
> we end up calling show_log).
> 
> It would be helpful if you could remember the test case, but perhaps
> that is not an option at this point.

IIRC it was "git commit -m bla".

Hth,
Dscho

Re: [PATCH 6/6] builtin-commit: Add newline when showing which commit was created

[Jeff King]

On Sun, Dec 02, 2007 at 12:13:07PM +0000, Johannes Schindelin wrote:

> > It would be helpful if you could remember the test case, but perhaps
> > that is not an option at this point.
> 
> IIRC it was "git commit -m bla".

I have made several attempts to reproduce the problem, looked a bit
through the log-tree code, and checked the results of the t750* series
of tests; but I have found nothing. I think we are better off reverting,
which fixes every case I have seen; if the problem behavior comes back,
we will have figured out what causes it. And if it doesn't come back,
then the revert is the right thing. :)

-- >8 --
Revert "builtin-commit: Add newline when showing which commit was created"

This reverts commit 129fa606365c172d07a5d98bea9345277f221363.

We end up in most cases with an undesired extra newline. It
is possible that there is some corner case that requires the
newline, but there is no published test case. So let's fix
the known problem, and we can deal with the corner case if
and when there is a bug report.
---
 builtin-commit.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index f6e8e44..118853d 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -660,7 +660,6 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	printf("Created %scommit ", initial_commit ? "initial " : "");
 
 	log_tree_commit(&rev, commit);
-	printf("\n");
 }
 
 int git_commit_config(const char *k, const char *v)
-- 
1.5.3.6.2094.g3713

Re: [PATCH 6/6] builtin-commit: Add newline when showing which commit was created

[Johannes Schindelin]

Hi,

On Sun, 2 Dec 2007, Jeff King wrote:

> On Sun, Dec 02, 2007 at 12:13:07PM +0000, Johannes Schindelin wrote:
> 
> > > It would be helpful if you could remember the test case, but perhaps
> > > that is not an option at this point.
> > 
> > IIRC it was "git commit -m bla".
> 
> I have made several attempts to reproduce the problem, looked a bit
> through the log-tree code, and checked the results of the t750* series
> of tests; but I have found nothing.

I remember again.  When I did "commit -s -m bla" the  empty line between 
the oneline and the signoff would be missing.  But in the meantime, the 
signoff was dragged into the strbuf and all is well.

ACK.

Ciao,
Dscho

Re: [PATCH 6/6] builtin-commit: Add newline when showing which commit was created

[Junio C Hamano]

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

>> I have made several attempts to reproduce the problem, looked a bit
>> through the log-tree code, and checked the results of the t750* series
>> of tests; but I have found nothing.
>
> I remember again.  When I did "commit -s -m bla" the  empty line between 
> the oneline and the signoff would be missing.  But in the meantime, the 
> signoff was dragged into the strbuf and all is well.

Sorry, now I am confused.  Building the version before that change and
doing "./git-commit -a -s -m bla", I do not see the extra blank line in
the "Created commit" response, and I see a blank line before and after
the sign-off in the "git show" output for the resulting commit.

Was this unnecessary change from the beginning?  I am inclined to think
so...

Re: [PATCH 6/6] builtin-commit: Add newline when showing which commit was created

[Johannes Schindelin]

Hi,

On Sun, 2 Dec 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I have made several attempts to reproduce the problem, looked a bit
> >> through the log-tree code, and checked the results of the t750* series
> >> of tests; but I have found nothing.
> >
> > I remember again.  When I did "commit -s -m bla" the  empty line between 
> > the oneline and the signoff would be missing.  But in the meantime, the 
> > signoff was dragged into the strbuf and all is well.
> 
> Sorry, now I am confused.  Building the version before that change and
> doing "./git-commit -a -s -m bla", I do not see the extra blank line in
> the "Created commit" response, and I see a blank line before and after
> the sign-off in the "git show" output for the resulting commit.
> 
> Was this unnecessary change from the beginning?  I am inclined to think
> so...

IIRC I did 2150554b0ed60356d8918b610834c04ad2eecdec(builtin-commit --s: 
add a newline if the last line was no S-O-B) after the commit that 
Peff wants to undo.

So yes, my mistake.

Ciao,
Dscho

Re: [PATCH 6/6] builtin-commit: Add newline when showing which commit was created

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Nov 11, 2007 at 05:36:52PM +0000, Johannes Schindelin wrote:
>
>> The function log_tree_commit() does not break the line, so we have to
>> do it ourselves.
>
> Johannes,
>
> Can you explain the rationale for this change in more detail?
>
> When I run builtin-commit from the tip of next, I always get an extra
> newline (as compared to the shell behavior):

After reverting this, recording a merge commit seems to have lost the
newline.  Can be easily reproduced with:

	$ git merge --no-commit some-branch
        $ git commit -a -m 'foo'

Re: [PATCH 6/6] builtin-commit: Add newline when showing which commit was created

[Jeff King]

On Sun, Dec 02, 2007 at 11:33:28PM -0800, Junio C Hamano wrote:

> After reverting this, recording a merge commit seems to have lost the
> newline.  Can be easily reproduced with:
> 
> 	$ git merge --no-commit some-branch
>   $ git commit -a -m 'foo'

See, I knew reverting would flush it out. ;)

Unfortunately, the fix isn't terribly obvious. log_tree_commit produces
output either from log_tree_diff, which appends a newline, or from
show_log, which doesn't, and returns the same value in either case.

I tried adding a '%n' to the format specifier (which is "%h: %s"),
but that has inconsistent results, since another newline is always
placed between the diffstat and the log, anyway. The design of
--pretty=format is a bit inflexible here, since you are stuck with the
"$log\n$diff" format. But changing it would impact users, and is likely
to require a lot of surgery in the log machinery.

So short of munging log_tree_commit to add the newline, I'm not sure how
to fix it, and I'm a little wary of messing with that function.

-Peff