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

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

[Ramkumar Ramachandra]

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

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

[Ramkumar Ramachandra]

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(-)

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

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

[Ramkumar Ramachandra]

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(-)

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

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

[Ramkumar Ramachandra]

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(-)

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

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

[Ramkumar Ramachandra]

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(-)

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

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

[Ramkumar Ramachandra]

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(-)

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

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

[Jonathan Nieder]

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.

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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(-)

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");

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

[Jonathan Nieder]

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?
[...]
> --- 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 :)).

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

[Ramkumar Ramachandra]

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?

> @@ -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>

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' '

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

[Junio C Hamano]

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?

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

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> 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