threads / patch / 24892

patch, 3 partsrr/fmt-merge-msg replacements

Subject: [PATCH 0/3] rr/fmt-merge-msg replacements

## tl;dr

12 messages between Aug 28, 2010 and Sep 1, 2010. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Ramkumar Ramachandra· Aug 28, 2010, 10:35 UTC · lore

I found a problem while writing the test for -1: the first patch is a correction for that; sorry I didn't detect it earlier.

The second and third patches are rewrites of the tests I submitted earlier- the rest of the file needs to be cleaned up to match this style. Thanks Jonathan.

Ramkumar Ramachandra (3):
  fmt-merge-msg: Disallow '--log', 'merge.log' < 0
  t6200-fmt-merge-msg: Exercise 'merge.log' to configure shortlog
    length
  t6200-fmt-merge-msg: Exercise '--log' to configure shortlog length
 builtin/fmt-merge-msg.c |    4 +++-
 builtin/merge.c         |    2 +-
 t/t6200-fmt-merge-msg.sh |  123 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 2 deletions(-)
-- 
1.7.2.2.409.gdbb11.dirty
Ramkumar Ramachandra· Aug 28, 2010, 10:35 UTC · re: Ramkumar Ramachandra · lore

[PATCH] fmt-merge-msg: Disallow '--log', 'merge.log' < 0

Explicitly check for the case when '--log' or 'merge.log' is passed a negative integer argument. Handle the case by making it look as if such an argument was never parsed.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/fmt-merge-msg.c |    4 +++-
 builtin/merge.c         |    2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)
Show changes to 2 files +4 −2

builtin/fmt-merge-msg.c, builtin/merge.c

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index d3b8297..0684179 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -18,7 +18,7 @@ static int fmt_merge_msg_config(const char *key, const char *value, void *cb)
 	if (!strcmp(key, "merge.log") || !strcmp(key, "merge.summary")) {
 		int is_bool;
 		shortlog_len = git_config_bool_or_int(key, value, &is_bool);
-		if (is_bool && shortlog_len)
+		if ((is_bool && shortlog_len) || shortlog_len < 0)
 			shortlog_len = DEFAULT_MERGE_LOG_LEN;
 	}
 	return 0;
@@ -345,6 +345,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 		write_in_full(STDOUT_FILENO, &nl, 1);
 		return 0;
 	}
+	if (shortlog_len < 0)
+		shortlog_len = DEFAULT_MERGE_LOG_LEN;
 
 	if (inpath && strcmp(inpath, "-")) {
 		in = fopen(inpath, "r");
diff --git a/builtin/merge.c b/builtin/merge.c
index 55dc571..affa197 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -510,7 +510,7 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 	else if (!strcmp(k, "merge.log") || !strcmp(k, "merge.summary")) {
 		int is_bool;
 		shortlog_len = git_config_bool_or_int(k, v, &is_bool);
-		if (is_bool && shortlog_len)
+		if ((is_bool && shortlog_len) || shortlog_len < 0)
 			shortlog_len = DEFAULT_MERGE_LOG_LEN;
 		return 0;
 	} else if (!strcmp(k, "merge.renormalize"))
-- 
1.7.2.2.409.gdbb11.dirty
Ramkumar Ramachandra· Aug 28, 2010, 10:35 UTC · re: Ramkumar Ramachandra · lore

[PATCH 2/3] t6200-fmt-merge-msg: Exercise 'merge.log' to configure shortlog length

Add a test to exercise the 'merge.log' configuration option of 'git fmt-merge-msg'. It controls the number of shortlog entries to display in merge commit messages.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t6200-fmt-merge-msg.sh |   64 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 64 insertions(+), 0 deletions(-)
Show changes to t/t6200-fmt-merge-msg.sh +64 −0
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 71f6cad..528fdf9 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -129,6 +129,70 @@ test_expect_success '[merge] summary/log configuration' '
 	test_cmp expected actual2
 '
 
+test_expect_success 'setup: clear [merge] configuration' '
+	test_might_fail git config --unset-all merge.log &&
+	test_might_fail git config --unset-all merge.summary
+'
+
+test_expect_success 'setup FETCH_HEAD' '
+	git checkout master &&
+	test_tick &&
+	git fetch . left
+'
+
+test_expect_success 'merge.log=3 limits shortlog length' '
+	cat >expected <<-EOF &&
+	Merge branch ${apos}left${apos}
+
+	* left: (5 commits)
+	  Left #5
+	  Left #4
+	  Left #3
+	  ...
+	EOF
+
+	git -c merge.log=3 fmt-merge-msg <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'merge.log=5 shows all 5 commits' '
+	cat >expected <<-EOF &&
+	Merge branch ${apos}left${apos}
+
+	* left:
+	  Left #5
+	  Left #4
+	  Left #3
+	  Common #2
+	  Common #1
+	EOF
+
+	git -c merge.log=5 fmt-merge-msg <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'merge.log=0 disables shortlog' '
+	echo "Merge branch ${apos}left${apos}" >expected
+	git -c merge.log=0 fmt-merge-msg <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'merge.log=-1 does something sane' '
+	cat >expected <<-EOF &&
+	Merge branch ${apos}left${apos}
+
+	* left:
+	  Left #5
+	  Left #4
+	  Left #3
+	  Common #2
+	  Common #1
+	EOF
+
+	git -c merge.log=-1 fmt-merge-msg <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'fmt-merge-msg -m' '
 	echo "Sync with left" >expected &&
 	cat >expected.log <<-EOF &&
-- 
1.7.2.2.409.gdbb11.dirty
Ramkumar Ramachandra· Aug 28, 2010, 10:35 UTC · re: Ramkumar Ramachandra · lore

[PATCH 3/3] t6200-fmt-merge-msg: Exercise '--log' to configure shortlog length

Add a test to exercise the '--log' command-line option of 'git fmt-merge-msg'. It controls the number of shortlog entries to display in merge commit messages.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t6200-fmt-merge-msg.sh |   59 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 59 insertions(+), 0 deletions(-)
Show changes to t/t6200-fmt-merge-msg.sh +59 −0
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 528fdf9..395c23e 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -193,6 +193,65 @@ test_expect_success 'merge.log=-1 does something sane' '
 	test_cmp expected actual
 '
 
+test_expect_success '--log=3 limits shortlog length' '
+	cat >expected <<-EOF &&
+	Merge branch ${apos}left${apos}
+
+	* left: (5 commits)
+	  Left #5
+	  Left #4
+	  Left #3
+	  ...
+	EOF
+
+	git fmt-merge-msg --log=3 <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--log=5 shows all 5 commits' '
+	cat >expected <<-EOF &&
+	Merge branch ${apos}left${apos}
+
+	* left:
+	  Left #5
+	  Left #4
+	  Left #3
+	  Common #2
+	  Common #1
+	EOF
+
+	git fmt-merge-msg --log=5 <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--no-log disables shortlog' '
+	echo "Merge branch ${apos}left${apos}" >expected &&
+	git fmt-merge-msg --no-log <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--log=0 disables shortlog' '
+	echo "Merge branch ${apos}left${apos}" >expected &&
+	git fmt-merge-msg --no-log <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--log=-1 does something sane' '
+	cat >expected <<-EOF &&
+	Merge branch ${apos}left${apos}
+
+	* left:
+	  Left #5
+	  Left #4
+	  Left #3
+	  Common #2
+	  Common #1
+	EOF
+
+	git fmt-merge-msg --log=-1 <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'fmt-merge-msg -m' '
 	echo "Sync with left" >expected &&
 	cat >expected.log <<-EOF &&
-- 
1.7.2.2.409.gdbb11.dirty
Ramkumar Ramachandra· Aug 31, 2010, 03:53 UTC · re: Ramkumar Ramachandra · lore

[PATCH 1/2] merge: Make '--log' an integer option for number of shortlog entries

Change the command-line '--log' option from a boolean option to an integer option, and parse the optional integer provided on the command-line into the 'shortlog_len' variable. Also update the documentation accordingly.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Reported-by: Yaroslav Halchenko <debian@onerussian.com>
Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
Thanks-to: Johannes Sixt <j.sixt@viscovery.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Replacement for b5cdf2a.
 Documentation/git-fmt-merge-msg.txt |   10 ++++++----
 Documentation/merge-options.txt     |    6 +++---
 builtin/fmt-merge-msg.c             |   28 ++++++++++++++++++----------
 builtin/merge.c                     |   17 ++++++++++-------
 4 files changed, 37 insertions(+), 24 deletions(-)
Show changes to 4 files +37 −22

Documentation/git-fmt-merge-msg.txt, Documentation/merge-options.txt, builtin/fmt-merge-msg.c, builtin/merge.c

diff --git a/Documentation/git-fmt-merge-msg.txt b/Documentation/git-fmt-merge-msg.txt
index 302f56b..f04a9ff 100644
--- a/Documentation/git-fmt-merge-msg.txt
+++ b/Documentation/git-fmt-merge-msg.txt
@@ -9,8 +9,8 @@ git-fmt-merge-msg - Produce a merge commit message
 SYNOPSIS
 --------
 [verse]
-'git fmt-merge-msg' [-m <message>] [--log | --no-log] <$GIT_DIR/FETCH_HEAD
-'git fmt-merge-msg' [-m <message>] [--log | --no-log] -F <file>
+'git fmt-merge-msg' [-m <message>] [--log[=<n>] | --no-log] <$GIT_DIR/FETCH_HEAD
+'git fmt-merge-msg' [-m <message>] [--log[=<n>] | --no-log] -F <file>
 
 DESCRIPTION
 -----------
@@ -24,10 +24,12 @@ automatically invoking 'git merge'.
 OPTIONS
 -------
 
---log::
+--log[=<n>]::
 	In addition to branch names, populate the log message with
 	one-line descriptions from the actual commits that are being
-	merged.
+	merged.  At most <n> commits from each merge parent will be
+	used (20 if <n> is omitted).  This overrides the `merge.log`
+	configuration variable.
 
 --no-log::
 	Do not list one-line descriptions from the actual commits being
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 722d704..e33e0f8 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -16,11 +16,11 @@ inspect and further tweak the merge result before committing.
 With --no-ff Generate a merge commit even if the merge
 resolved as a fast-forward.
 
---log::
+--log[=<n>]::
 --no-log::
 	In addition to branch names, populate the log message with
-	one-line descriptions from the actual commits that are being
-	merged.
+	one-line descriptions from at most <n> actual commits that are being
+	merged. See also linkgit:git-fmt-merge-msg[1].
 +
 With --no-log do not list one-line descriptions from the
 actual commits being merged.
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 42021d3..0997c26 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -7,21 +7,24 @@
 #include "string-list.h"
 
 static const char * const fmt_merge_msg_usage[] = {
-	"git fmt-merge-msg [-m <message>] [--log|--no-log] [--file <file>]",
+	"git fmt-merge-msg [-m <message>] [--log[=<n>]|--no-log] [--file <file>]",
 	NULL
 };
 
-static int merge_summary;
+static int shortlog_len;
 
 static int fmt_merge_msg_config(const char *key, const char *value, void *cb)
 {
 	static int found_merge_log = 0;
 	if (!strcmp("merge.log", key)) {
 		found_merge_log = 1;
-		merge_summary = git_config_bool(key, value);
+		shortlog_len = git_config_bool(key, value) ? DEFAULT_MERGE_LOG_LEN : 0;
+		return 0;
+	}
+	if (!found_merge_log && !strcmp("merge.summary", key)) {
+		shortlog_len = git_config_bool(key, value) ? DEFAULT_MERGE_LOG_LEN : 0;
+		return 0;
 	}
-	if (!found_merge_log && !strcmp("merge.summary", key))
-		merge_summary = git_config_bool(key, value);
 	return 0;
 }
 
@@ -318,10 +321,13 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 	const char *inpath = NULL;
 	const char *message = NULL;
 	struct option options[] = {
-		OPT_BOOLEAN(0, "log",     &merge_summary, "populate log with the shortlog"),
-		{ OPTION_BOOLEAN, 0, "summary", &merge_summary, NULL,
+		{ OPTION_INTEGER, 0, "log", &shortlog_len, "n",
+		  "populate log with <n> entries from shortlog",
+		  PARSE_OPT_OPTARG, NULL, DEFAULT_MERGE_LOG_LEN },
+		{ OPTION_INTEGER, 0, "summary", &shortlog_len, "n",
 		  "alias for --log (deprecated)",
-		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
+		  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, NULL,
+		  DEFAULT_MERGE_LOG_LEN },
 		OPT_STRING('m', "message", &message, "text",
 			"use <text> as start of message"),
 		OPT_FILENAME('F', "file", &inpath, "file to read from"),
@@ -337,12 +343,14 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 			     0);
 	if (argc > 0)
 		usage_with_options(fmt_merge_msg_usage, options);
-	if (message && !merge_summary) {
+	if (message && !shortlog_len) {
 		char nl = '\n';
 		write_in_full(STDOUT_FILENO, message, strlen(message));
 		write_in_full(STDOUT_FILENO, &nl, 1);
 		return 0;
 	}
+	if (shortlog_len < 0)
+		shortlog_len = DEFAULT_MERGE_LOG_LEN;
 
 	if (inpath && strcmp(inpath, "-")) {
 		in = fopen(inpath, "r");
@@ -357,7 +365,7 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 		strbuf_addstr(&output, message);
 	ret = fmt_merge_msg(&input, &output,
 			    message ? 0 : 1,
-			    merge_summary ? DEFAULT_MERGE_LOG_LEN : 0);
+			    shortlog_len);
 
 	if (ret)
 		return ret;
diff --git a/builtin/merge.c b/builtin/merge.c
index b2c0984..9e4733d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -42,7 +42,7 @@ static const char * const builtin_merge_usage[] = {
 	NULL
 };
 
-static int show_diffstat = 1, option_log, squash;
+static int show_diffstat = 1, shortlog_len, squash;
 static int option_commit = 1, allow_fast_forward = 1;
 static int fast_forward_only;
 static int allow_trivial = 1, have_message;
@@ -175,8 +175,9 @@ static struct option builtin_merge_options[] = {
 	OPT_BOOLEAN(0, "stat", &show_diffstat,
 		"show a diffstat at the end of the merge"),
 	OPT_BOOLEAN(0, "summary", &show_diffstat, "(synonym to --stat)"),
-	OPT_BOOLEAN(0, "log", &option_log,
-		"add list of one-line log to merge commit message"),
+	{ OPTION_INTEGER, 0, "log", &shortlog_len, "n",
+	  "add (at most <n>) entries from shortlog to merge commit message",
+	  PARSE_OPT_OPTARG, NULL, DEFAULT_MERGE_LOG_LEN },
 	OPT_BOOLEAN(0, "squash", &squash,
 		"create a single commit instead of doing a merge"),
 	OPT_BOOLEAN(0, "commit", &option_commit,
@@ -501,8 +502,10 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		return git_config_string(&pull_twohead, k, v);
 	else if (!strcmp(k, "pull.octopus"))
 		return git_config_string(&pull_octopus, k, v);
-	else if (!strcmp(k, "merge.log") || !strcmp(k, "merge.summary"))
-		option_log = git_config_bool(k, v);
+	else if (!strcmp(k, "merge.log") || !strcmp(k, "merge.summary")) {
+		shortlog_len = git_config_bool(k, v) ? DEFAULT_MERGE_LOG_LEN : 0;
+		return 0;
+	}
 	return git_diff_ui_config(k, v, cb);
 }
 
@@ -998,9 +1001,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		for (i = 0; i < argc; i++)
 			merge_name(argv[i], &merge_names);
 
-		if (!have_message || option_log) {
+		if (!have_message || shortlog_len) {
 			fmt_merge_msg(&merge_names, &merge_msg, !have_message,
-				      option_log ? DEFAULT_MERGE_LOG_LEN : 0);
+				      shortlog_len);
 			if (merge_msg.len)
 				strbuf_setlen(&merge_msg, merge_msg.len - 1);
 		}
-- 
1.7.2.2.409.gdbb11.dirty
Jonathan Nieder· Aug 31, 2010, 14:32 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 1/2] merge: Make '--log' an integer option for number of shortlog entries

Ramkumar Ramachandra wrote:
> Change the command-line '--log' option from a boolean option to an
> integer option
What does "git merge --log=-1" do?

It looks like you have made "git fmt-merge-msg --log=-1" equivalent to --log=20, but it might be better to error out or use INT_MAX or something (especially because of plans to make -1 mean "infinity" later).

Thanks for your perseverance. Aside from those two nits, the patch looks good.

Junio C Hamano· Aug 31, 2010, 18:17 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/2] merge: Make '--log' an integer option for number of shortlog entries

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 11 quoted lines
> Ramkumar Ramachandra wrote:
>
>> Change the command-line '--log' option from a boolean option to an
>> integer option
>
> What does "git merge --log=-1" do?
>
> It looks like you have made "git fmt-merge-msg --log=-1" equivalent to
> --log=20, but it might be better to error out or use INT_MAX or
> something (especially because of plans to make -1 mean "infinity"
> later).

Hmm, do we plan on that? It sounds a little bit insane. Erroring it out would be Ok. How about doing this on top?

 builtin/fmt-merge-msg.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/fmt-merge-msg.c +2 −2
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 0997c26..7c6e226 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -322,7 +322,7 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 	const char *message = NULL;
 	struct option options[] = {
 		{ OPTION_INTEGER, 0, "log", &shortlog_len, "n",
-		  "populate log with <n> entries from shortlog",
+		  "populate log with at most <n> entries from shortlog",
 		  PARSE_OPT_OPTARG, NULL, DEFAULT_MERGE_LOG_LEN },
 		{ OPTION_INTEGER, 0, "summary", &shortlog_len, "n",
 		  "alias for --log (deprecated)",
@@ -350,7 +350,7 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 	if (shortlog_len < 0)
-		shortlog_len = DEFAULT_MERGE_LOG_LEN;
+		die("Negative --log=%d???", shortlog_len);
 
 	if (inpath && strcmp(inpath, "-")) {
 		in = fopen(inpath, "r");
Jonathan Nieder· Aug 31, 2010, 18:21 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] merge: Make '--log' an integer option for number of shortlog entries

Junio C Hamano wrote:
> Hmm, do we plan on that?  It sounds a little bit insane.

I sent an RFC patch with -1 as internal representation for "git grep -Cinfinity". Infinity was the length Yaroslav was most interested in for merge.log (not sure why, really).

Regardless, it's true that that would probably not be a detail to expose.

> How about doing this on top?
[...]
Show 17 quoted lines
> --- a/builtin/fmt-merge-msg.c
> +++ b/builtin/fmt-merge-msg.c
> @@ -322,7 +322,7 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
>  	const char *message = NULL;
>  	struct option options[] = {
>  		{ OPTION_INTEGER, 0, "log", &shortlog_len, "n",
> -		  "populate log with <n> entries from shortlog",
> +		  "populate log with at most <n> entries from shortlog",
>  		  PARSE_OPT_OPTARG, NULL, DEFAULT_MERGE_LOG_LEN },
>  		{ OPTION_INTEGER, 0, "summary", &shortlog_len, "n",
>  		  "alias for --log (deprecated)",
> @@ -350,7 +350,7 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
>  		return 0;
>  	}
>  	if (shortlog_len < 0)
> -		shortlog_len = DEFAULT_MERGE_LOG_LEN;
> +		die("Negative --log=%d???", shortlog_len);
Looks good to me (modulo punctuation :)).
Ramkumar Ramachandra· Aug 31, 2010, 19:13 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] merge: Make '--log' an integer option for number of shortlog entries

Hi,
Junio C Hamano writes:
> Hmm, do we plan on that?  It sounds a little bit insane.  Erroring it out
> would be Ok.  How about doing this on top?
Show 9 quoted lines
> @@ -350,7 +350,7 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
>  		return 0;
>  	}
>  	if (shortlog_len < 0)
> -		shortlog_len = DEFAULT_MERGE_LOG_LEN;
> +		die("Negative --log=%d???", shortlog_len);
>  
>  	if (inpath && strcmp(inpath, "-")) {
>  		in = fopen(inpath, "r");

It's a little asymmetric- the program won't error out when 'merge.log' is set to -1. Then again, it's probably nicer to simply pretend that we didn't see that invalid configuration.

Also, to avoid breaking the '--log=-1' test, either drop the test altogether or use something like this:

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Show changes to t/t6200-fmt-merge-msg.sh +2 −3
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 395c23e..3bfdebc 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -236,7 +236,7 @@ test_expect_success '--log=0 disables shortlog' '
 	test_cmp expected actual
 '
 
-test_expect_success '--log=-1 does something sane' '
+test_expect_code 128 '--log=-1 errors out' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}
 
@@ -248,8 +248,7 @@ test_expect_success '--log=-1 does something sane' '
 	  Common #1
 	EOF
 
-	git fmt-merge-msg --log=-1 <.git/FETCH_HEAD >actual &&
-	test_cmp expected actual
+	git fmt-merge-msg --log=-1 <.git/FETCH_HEAD >actual
 '
 
 test_expect_success 'fmt-merge-msg -m' '
Junio C Hamano· Aug 31, 2010, 19:53 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 1/2] merge: Make '--log' an integer option for number of shortlog entries

Ramkumar Ramachandra <artagnon@gmail.com> writes:
> It's a little asymmetric- the program won't error out when 'merge.log'
> is set to -1.
And what is the reason you do not want to diagnose that as an error?
Ramkumar Ramachandra· Sep 1, 2010, 04:22 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] merge: Make '--log' an integer option for number of shortlog entries

Hi Junio,
Junio C Hamano writes:
Show 6 quoted lines
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> 
> > It's a little asymmetric- the program won't error out when 'merge.log'
> > is set to -1.
> 
> And what is the reason you do not want to diagnose that as an error?

Oh, I'm just saying I'm not sure what the behavior should be. Okay- if erroring out is the desired result, I'll fixup and re-roll in a bit.

-- Ram
Ramkumar Ramachandra· Aug 31, 2010, 03:53 UTC · re: Ramkumar Ramachandra · lore

[PATCH 2/2] merge: Make 'merge.log' an integer or boolean option

Make 'merge.log' an integer or boolean option to set the number of shortlog entries to display in the merge commit. Note that it defaults to false, and that true means a default value of 20. Also update corresponding documentation.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
Thanks-to: Johannes Sixt <j.sixt@viscovery.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Replacement for afdcbdc.
 Documentation/git-fmt-merge-msg.txt |    6 ++++--
 Documentation/merge-config.txt      |    6 ++++--
 builtin/fmt-merge-msg.c             |   14 +++++---------
 builtin/merge.c                     |    5 ++++-
 4 files changed, 17 insertions(+), 14 deletions(-)
Show changes to 4 files +17 −14

Documentation/git-fmt-merge-msg.txt, Documentation/merge-config.txt, builtin/fmt-merge-msg.c, builtin/merge.c

diff --git a/Documentation/git-fmt-merge-msg.txt b/Documentation/git-fmt-merge-msg.txt
index f04a9ff..40dba8c 100644
--- a/Documentation/git-fmt-merge-msg.txt
+++ b/Documentation/git-fmt-merge-msg.txt
@@ -54,8 +54,10 @@ CONFIGURATION
 -------------
 
 merge.log::
-	Whether to include summaries of merged commits in newly
-	merge commit messages. False by default.
+	In addition to branch names, populate the log message with at
+	most the specified number of one-line descriptions from the
+	actual commits that are being merged.  Defaults to false, and
+	true is a synoym for 20.
 
 merge.summary::
 	Synonym to `merge.log`; this is deprecated and will be removed in
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index a403155..acbe1e1 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -7,8 +7,10 @@ merge.conflictstyle::
 	marker and the original text before the `=======` marker.
 
 merge.log::
-	Whether to include summaries of merged commits in newly created
-	merge commit messages. False by default.
+	In addition to branch names, populate the log message with at
+	most the specified number of one-line descriptions from the
+	actual commits that are being merged.  Defaults to false, and
+	true is a synoym for 20.
 
 merge.renameLimit::
 	The number of files to consider when performing rename detection
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 0997c26..975ef6a 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -15,15 +15,11 @@ static int shortlog_len;
 
 static int fmt_merge_msg_config(const char *key, const char *value, void *cb)
 {
-	static int found_merge_log = 0;
-	if (!strcmp("merge.log", key)) {
-		found_merge_log = 1;
-		shortlog_len = git_config_bool(key, value) ? DEFAULT_MERGE_LOG_LEN : 0;
-		return 0;
-	}
-	if (!found_merge_log && !strcmp("merge.summary", key)) {
-		shortlog_len = git_config_bool(key, value) ? DEFAULT_MERGE_LOG_LEN : 0;
-		return 0;
+	if (!strcmp(key, "merge.log") || !strcmp(key, "merge.summary")) {
+		int is_bool;
+		shortlog_len = git_config_bool_or_int(key, value, &is_bool);
+		if (is_bool && shortlog_len || shortlog_len < 0)
+			shortlog_len = DEFAULT_MERGE_LOG_LEN;
 	}
 	return 0;
 }
diff --git a/builtin/merge.c b/builtin/merge.c
index 9e4733d..57ea8d7 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -503,7 +503,10 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 	else if (!strcmp(k, "pull.octopus"))
 		return git_config_string(&pull_octopus, k, v);
 	else if (!strcmp(k, "merge.log") || !strcmp(k, "merge.summary")) {
-		shortlog_len = git_config_bool(k, v) ? DEFAULT_MERGE_LOG_LEN : 0;
+		int is_bool;
+		shortlog_len = git_config_bool_or_int(k, v, &is_bool);
+		if (is_bool && shortlog_len || shortlog_len < 0)
+			shortlog_len = DEFAULT_MERGE_LOG_LEN;
 		return 0;
 	}
 	return git_diff_ui_config(k, v, cb);
-- 
1.7.2.2.409.gdbb11.dirty

← back to recent threads