[PATCH v2 1/5] revision: add --merges={show|only|hide} option

[PATCH v2 1/5] revision: add --merges={show|only|hide} option

[Koosha Khajehmoogahi]

From: Junio C Hamano <gitster@pobox.com>

revision: add a new option 'merges=' with
possible values of 'only', 'show' and 'hide'.
The option is used when showing the list of commits.
The value 'only' lists only merges. The value 'show'
is the default behavior which shows the commits as well
as merges and the value 'hide' makes it just list commit
items.

[kk: chose names for options; wrote commit message]

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
---
 revision.c | 20 ++++++++++++++++++++
 revision.h |  1 +
 2 files changed, 21 insertions(+)

diff --git a/revision.c b/revision.c
index 6399a04..c3c3dcc 100644
--- a/revision.c
+++ b/revision.c
@@ -1678,6 +1678,23 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)
 	add_grep(revs, pattern, GREP_PATTERN_BODY);
 }
 
+int parse_merges_opt(struct rev_info *revs, const char *param)
+{
+	if (!strcmp(param, "show")) {
+		revs->min_parents = 0;
+		revs->max_parents = -1;
+	} else if (!strcmp(param, "only")) {
+		revs->min_parents = 2;
+		revs->max_parents = -1;
+	} else if (!strcmp(param, "hide")) {
+		revs->min_parents = 0;
+		revs->max_parents = 1;
+	} else {
+		return -1;
+	}
+	return 0;
+}
+
 static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,
 			       int *unkc, const char **unkv)
 {
@@ -1800,6 +1817,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->show_all = 1;
 	} else if (!strcmp(arg, "--remove-empty")) {
 		revs->remove_empty_trees = 1;
+	} else if (starts_with(arg, "--merges=")) {
+		if (parse_merges_opt(revs, arg + 9))
+			die("unknown option: %s", arg);
 	} else if (!strcmp(arg, "--merges")) {
 		revs->min_parents = 2;
 	} else if (!strcmp(arg, "--no-merges")) {
diff --git a/revision.h b/revision.h
index 0ea8b4e..f9df58c 100644
--- a/revision.h
+++ b/revision.h
@@ -240,6 +240,7 @@ extern int setup_revisions(int argc, const char **argv, struct rev_info *revs,
 extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 			       const struct option *options,
 			       const char * const usagestr[]);
+extern int parse_merges_opt(struct rev_info *, const char *);
 #define REVARG_CANNOT_BE_FILENAME 01
 #define REVARG_COMMITTISH 02
 extern int handle_revision_arg(const char *arg, struct rev_info *revs,
-- 
2.3.3.263.g095251d.dirty

[PATCH v2 2/5] log: honor log.merges= option

[Koosha Khajehmoogahi]

From: Junio C Hamano <gitster@pobox.com>

[kk: wrote commit message]

Helped-by: Eris Sunshine <sunshine@sunshineco.com>
Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
---
 builtin/log.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..c7a7aad 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static int decoration_given;
 static int use_mailmap_config;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
+static const char *log_merges;
 
 static const char * const builtin_log_usage[] = {
 	N_("git log [<options>] [<revision range>] [[--] <path>...]"),
@@ -386,6 +387,9 @@ static int git_log_config(const char *var, const char *value, void *cb)
 			decoration_style = 0; /* maybe warn? */
 		return 0;
 	}
+	if (!strcmp(var, "log.merges")) {
+		return git_config_string(&log_merges, var, value);
+	}
 	if (!strcmp(var, "log.showroot")) {
 		default_show_root = git_config_bool(var, value);
 		return 0;
@@ -628,6 +632,8 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 
 	init_revisions(&rev, prefix);
 	rev.always_show_header = 1;
+	if (log_merges && parse_merges_opt(&rev, log_merges))
+		die("unknown config value for log.merges: %s", log_merges);
 	memset(&opt, 0, sizeof(opt));
 	opt.def = "HEAD";
 	opt.revarg_opt = REVARG_COMMITTISH;
-- 
2.3.3.263.g095251d.dirty

[PATCH v2 3/5] Documentation: add git-log --merges= option and log.merges config. var

[Koosha Khajehmoogahi]

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
---
 Documentation/git-log.txt          |  3 +++
 Documentation/rev-list-options.txt | 18 +++++++++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 18bc716..e16f0f8 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -190,6 +190,9 @@ log.showRoot::
 	`git log -p` output would be shown without a diff attached.
 	The default is `true`.
 
+log.merges::
+    Default for `--merges=` option. Defaults to `show`.
+
 mailmap.*::
 	See linkgit:git-shortlog[1].
 
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index f620ee4..0bb2390 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -96,12 +96,24 @@ if it is part of the log message.
 --remove-empty::
 	Stop when a given path disappears from the tree.
 
+--merges={show|hide|only}::
++
+--
+`show`: show both merge and non-merge commits
+
+`hide`: only show non-merge commits; same as `--max-parents=1`
+
+`only`: only show merge commits; same as `--min-parents=2`
+
+If `--merges=` is not specified, default value is `show`.
+--
++
+
 --merges::
-	Print only merge commits. This is exactly the same as `--min-parents=2`.
+	Shorthand for `--merges=only`.
 
 --no-merges::
-	Do not print commits with more than one parent. This is
-	exactly the same as `--max-parents=1`.
+	Shorthand for `--merges=hide`.
 
 --min-parents=<number>::
 --max-parents=<number>::
-- 
2.3.3.263.g095251d.dirty

[PATCH v2 4/5] t4202-log: add tests for --merges=

[Koosha Khajehmoogahi]

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
---
 t/t4202-log.sh | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 1b2e981..ceaaf4e 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -270,6 +270,90 @@ cat > expect <<\EOF
 * initial
 EOF
 
+test_expect_success 'setup --merges=' '
+	git log --min-parents=2 --pretty=tformat:%s >expect_only &&
+	git log --max-parents=1 --pretty=tformat:%s >expect_hide &&
+	git log --min-parents=-1 --pretty=tformat:%s >expect_show
+'
+
+test_expect_success 'log with config log.merges=show' '
+	test_config log.merges show &&
+	git log --pretty=tformat:%s >actual &&
+	test_cmp expect_show actual
+'
+
+test_expect_success 'log with config log.merges=only' '
+	test_config log.merges only &&
+	git log --pretty=tformat:%s >actual &&
+	test_cmp expect_only actual
+'
+
+test_expect_success 'log with config log.merges=hide' '
+	test_config log.merges hide &&
+	git log --pretty=tformat:%s >actual &&
+	test_cmp expect_hide actual
+'
+
+test_expect_success 'log with config log.merges=show with git log --no-merges' '
+	test_config log.merges show &&
+	git log --no-merges --pretty=tformat:%s >actual &&
+	test_cmp expect_hide actual
+'
+
+test_expect_success 'log with config log.merges=hide with git log ---merges' '
+	test_config log.merges hide &&
+	git log --merges --pretty=tformat:%s >actual &&
+	test ! -s actual
+'
+
+test_expect_success 'log --merges=show' '
+	test_unconfig log.merges &&
+	git log --merges=show --pretty=tformat:%s >actual &&
+	test_cmp expect_show actual
+'
+
+test_expect_success 'log --merges=only' '
+	test_unconfig log.merges &&
+	git log --merges=only --pretty=tformat:%s >actual &&
+	test_cmp expect_only actual
+'
+
+test_expect_success 'log --merges=hide' '
+	test_unconfig log.merges &&
+	git log --merges=hide --pretty=tformat:%s >actual &&
+	test_cmp expect_hide actual
+'
+
+test_log_merges() {
+	test_config log.merges $2 &&
+	git log --merges=$1 --pretty=tformat:%s >actual &&
+	test_cmp $3 actual
+}
+
+test_expect_success 'log --merges=show with config log.merges=hide' '
+	test_log_merges show hide expect_show
+'
+
+test_expect_success 'log --merges=show with config log.merges=only' '
+	test_log_merges show only expect_show
+'
+
+test_expect_success 'log --merges=only with config log.merges=show' '
+	test_log_merges only show expect_only
+'
+
+test_expect_success 'log --merges=only with config log.merges=hide' '
+	test_log_merges only hide expect_only
+'
+
+test_expect_success 'log --merges=hide with config log.merges=show' '
+	test_log_merges hide show expect_hide
+'
+
+test_expect_success 'log --merges=hide with config log.merges=only' '
+	test_log_merges hide only expect_hide
+'
+
 test_expect_success 'log --graph with merge' '
 	git log --graph --date-order --pretty=tformat:%s |
 		sed "s/ *\$//" >actual &&
-- 
2.3.3.263.g095251d.dirty

[PATCH v2 5/5] bash-completion: add support for git-log --merges= and log.merges

[Koosha Khajehmoogahi]

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
---
 contrib/completion/git-completion.bash | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index fbe5972..a75d7f5 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1406,7 +1406,7 @@ _git_ls_tree ()
 __git_log_common_options="
 	--not --all
 	--branches --tags --remotes
-	--first-parent --merges --no-merges
+	--first-parent --merges --merges= --no-merges
 	--max-count=
 	--max-age= --since= --after=
 	--min-age= --until= --before=
@@ -1451,7 +1451,11 @@ _git_log ()
 		__gitcomp "long short" "" "${cur##--decorate=}"
 		return
 		;;
-	--*)
+	--merges=*)
+		__gitcomp "show hide only" "" "${cur##--merges=}"
+		return
+		;;
+		--*)
 		__gitcomp "
 			$__git_log_common_options
 			$__git_log_shortlog_options
@@ -1861,6 +1865,10 @@ _git_config ()
 		__gitcomp "$__git_log_date_formats"
 		return
 		;;
+	log.merges)
+		__gitcomp "show hide only"
+		return
+		;;
 	sendemail.aliasesfiletype)
 		__gitcomp "mutt mailrc pine elm gnus"
 		return
@@ -2150,6 +2158,7 @@ _git_config ()
 		interactive.singlekey
 		log.date
 		log.decorate
+		log.merges
 		log.showroot
 		mailmap.file
 		man.
-- 
2.3.3.263.g095251d.dirty

Re: [PATCH v2 2/5] log: honor log.merges= option

[Junio C Hamano]

Koosha Khajehmoogahi <koosha@posteo.de> writes:

> From: Junio C Hamano <gitster@pobox.com>
>
> [kk: wrote commit message]

Ehh, what exactly did you write ;-)?

I think the most important thing that needs to be explained by the
log message for this change is that the variable is honored only by
log and it needs to explain why other Porcelain commands in the same
"log" family, like "whatchanged", should ignore the variable.

I think that we must not to allow format-patch and show to be
affected by this variable, because it is silly if log.merges=only
broke format-patch output or made "git show" silent.  But I didn't
think about others.  Whoever is doing this change needs to explain
in the log message the reason why it was decided that only "git log"
should pay attention to it.

> Helped-by: Eris Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---


>  builtin/log.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index dd8f3fc..c7a7aad 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -36,6 +36,7 @@ static int decoration_given;
>  static int use_mailmap_config;
>  static const char *fmt_patch_subject_prefix = "PATCH";
>  static const char *fmt_pretty;
> +static const char *log_merges;

The variable name may want to be updated to mimic other variables
used in a similar way, e.g. default_show_root is used to store the
value from log.showroot.

Perhaps "default_show_merge" or something?

Thanks.

>  static const char * const builtin_log_usage[] = {
>  	N_("git log [<options>] [<revision range>] [[--] <path>...]"),
> @@ -386,6 +387,9 @@ static int git_log_config(const char *var, const char *value, void *cb)
>  			decoration_style = 0; /* maybe warn? */
>  		return 0;
>  	}
> +	if (!strcmp(var, "log.merges")) {
> +		return git_config_string(&log_merges, var, value);
> +	}
>  	if (!strcmp(var, "log.showroot")) {
>  		default_show_root = git_config_bool(var, value);
>  		return 0;
> @@ -628,6 +632,8 @@ int cmd_log(int argc, const char **argv, const char *prefix)
>  
>  	init_revisions(&rev, prefix);
>  	rev.always_show_header = 1;
> +	if (log_merges && parse_merges_opt(&rev, log_merges))
> +		die("unknown config value for log.merges: %s", log_merges);
>  	memset(&opt, 0, sizeof(opt));
>  	opt.def = "HEAD";
>  	opt.revarg_opt = REVARG_COMMITTISH;

Re: [PATCH v2 3/5] Documentation: add git-log --merges= option and log.merges config. var

[Junio C Hamano]

Koosha Khajehmoogahi <koosha@posteo.de> writes:

> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index f620ee4..0bb2390 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -96,12 +96,24 @@ if it is part of the log message.
>  --remove-empty::
>  	Stop when a given path disappears from the tree.
>  
> +--merges={show|hide|only}::
> ++
> +--
> +`show`: show both merge and non-merge commits
> +
> +`hide`: only show non-merge commits; same as `--max-parents=1`
> +
> +`only`: only show merge commits; same as `--min-parents=2`
> +
> +If `--merges=` is not specified, default value is `show`.
> +--
> ++
> +

I am not sure if the "default value is `show`" is something we would
even want to mention like this.  It does not tell the whole story
and may even confuse the users, who did

	git log --merge
	git log --max-parent=...

but did not say any "--merges=<something>".

I think the importat point we want to teach users is that this is an
option to use when you want to limit what is output (and by default,
we show all but nothing else in the manpage says we hide things,
so...).  And it would be beneficial to highlight that 'show' is only
there to defeat an unusual log.merges setting in users' config.

Also the formatting of this part looks rather unusual.  I would have
expected that these three items to be listed as a true AsciiDoc
enumeration, not three hand-crafted enumration-looking separate
paragraphs.

Taking both points together, we may want to do something more like
this, perhaps?

--merges={show|hide|only}::

	Limit the output by type of commits.

	`hide`;;
		Hide merge commits from the output.

	`only`;;
		Hide non-merge commits from the output (i.e showing
		only merge commits).

	`show`;;
		Do not hide either merge or non-merge commits.  This
		is primarily useful when the user has non-standard
		setting of `log.merges` configuration variable that
		needs to be overriden from the command line.


Thanks.

Re: [PATCH v2 1/5] revision: add --merges={show|only|hide} option

[Eric Sunshine]

On Fri, Apr 3, 2015 at 9:21 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> From: Junio C Hamano <gitster@pobox.com>
>
> revision: add a new option 'merges=' with
> possible values of 'only', 'show' and 'hide'.
> The option is used when showing the list of commits.
> The value 'only' lists only merges. The value 'show'
> is the default behavior which shows the commits as well
> as merges and the value 'hide' makes it just list commit
> items.

A couple very minor comments if you to re-roll this series, otherwise
keep them in mind for future submissions:

Since "revision:" already prefixes the first line of the commit
message (in the Subject:), its reiteration at the start of this
paragraph is unnecessary. It's sufficient to start the paragraph with
"Add new --merges= option...".

The excessively jagged right edge of the paragraph makes it slightly
difficult to read. Try to wrap paragraphs to about 70-72 characters.

> [kk: chose names for options; wrote commit message]
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
>  revision.c | 20 ++++++++++++++++++++
>  revision.h |  1 +
>  2 files changed, 21 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index 6399a04..c3c3dcc 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1678,6 +1678,23 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)
>         add_grep(revs, pattern, GREP_PATTERN_BODY);
>  }
>
> +int parse_merges_opt(struct rev_info *revs, const char *param)
> +{
> +       if (!strcmp(param, "show")) {
> +               revs->min_parents = 0;
> +               revs->max_parents = -1;
> +       } else if (!strcmp(param, "only")) {
> +               revs->min_parents = 2;
> +               revs->max_parents = -1;
> +       } else if (!strcmp(param, "hide")) {
> +               revs->min_parents = 0;
> +               revs->max_parents = 1;
> +       } else {
> +               return -1;
> +       }
> +       return 0;
> +}
> +
>  static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,
>                                int *unkc, const char **unkv)
>  {
> @@ -1800,6 +1817,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>                 revs->show_all = 1;
>         } else if (!strcmp(arg, "--remove-empty")) {
>                 revs->remove_empty_trees = 1;
> +       } else if (starts_with(arg, "--merges=")) {
> +               if (parse_merges_opt(revs, arg + 9))
> +                       die("unknown option: %s", arg);
>         } else if (!strcmp(arg, "--merges")) {
>                 revs->min_parents = 2;
>         } else if (!strcmp(arg, "--no-merges")) {
> diff --git a/revision.h b/revision.h
> index 0ea8b4e..f9df58c 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -240,6 +240,7 @@ extern int setup_revisions(int argc, const char **argv, struct rev_info *revs,
>  extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
>                                const struct option *options,
>                                const char * const usagestr[]);
> +extern int parse_merges_opt(struct rev_info *, const char *);
>  #define REVARG_CANNOT_BE_FILENAME 01
>  #define REVARG_COMMITTISH 02
>  extern int handle_revision_arg(const char *arg, struct rev_info *revs,
> --
> 2.3.3.263.g095251d.dirty

Re: [PATCH v2 2/5] log: honor log.merges= option

[Eric Sunshine]

On Fri, Apr 3, 2015 at 9:21 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> From: Junio C Hamano <gitster@pobox.com>
>
> [kk: wrote commit message]
>
> Helped-by: Eris Sunshine <sunshine@sunshineco.com>

s/Eris/Eric/

> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
>  builtin/log.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index dd8f3fc..c7a7aad 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -36,6 +36,7 @@ static int decoration_given;
>  static int use_mailmap_config;
>  static const char *fmt_patch_subject_prefix = "PATCH";
>  static const char *fmt_pretty;
> +static const char *log_merges;
>
>  static const char * const builtin_log_usage[] = {
>         N_("git log [<options>] [<revision range>] [[--] <path>...]"),
> @@ -386,6 +387,9 @@ static int git_log_config(const char *var, const char *value, void *cb)
>                         decoration_style = 0; /* maybe warn? */
>                 return 0;
>         }
> +       if (!strcmp(var, "log.merges")) {
> +               return git_config_string(&log_merges, var, value);
> +       }
>         if (!strcmp(var, "log.showroot")) {
>                 default_show_root = git_config_bool(var, value);
>                 return 0;
> @@ -628,6 +632,8 @@ int cmd_log(int argc, const char **argv, const char *prefix)
>
>         init_revisions(&rev, prefix);
>         rev.always_show_header = 1;
> +       if (log_merges && parse_merges_opt(&rev, log_merges))
> +               die("unknown config value for log.merges: %s", log_merges);
>         memset(&opt, 0, sizeof(opt));
>         opt.def = "HEAD";
>         opt.revarg_opt = REVARG_COMMITTISH;
> --
> 2.3.3.263.g095251d.dirty

Re: [PATCH v2 4/5] t4202-log: add tests for --merges=

[Eric Sunshine]

On Sat, Apr 04, 2015 at 03:22:00AM +0200, Koosha Khajehmoogahi wrote:
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 1b2e981..ceaaf4e 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -270,6 +270,90 @@ cat > expect <<\EOF
>  * initial
>  EOF
>  
> +test_expect_success 'setup --merges=' '
> +	git log --min-parents=2 --pretty=tformat:%s >expect_only &&
> +	git log --max-parents=1 --pretty=tformat:%s >expect_hide &&
> +	git log --min-parents=-1 --pretty=tformat:%s >expect_show
> +'
> +
> +test_expect_success 'log with config log.merges=show' '
> +	test_config log.merges show &&
> +	git log --pretty=tformat:%s >actual &&
> +	test_cmp expect_show actual
> +'
> +
[...much repetitious code omitted...]
> +test_expect_success 'log with config log.merges=show with git log --no-merges' '
> +	test_config log.merges show &&
> +	git log --no-merges --pretty=tformat:%s >actual &&
> +	test_cmp expect_hide actual
> +'
> +
[...much repetitious code omitted...]
> +test_log_merges() {
> +	test_config log.merges $2 &&
> +	git log --merges=$1 --pretty=tformat:%s >actual &&
> +	test_cmp $3 actual
> +}
> +
> +test_expect_success 'log --merges=show with config log.merges=hide' '
> +	test_log_merges show hide expect_show
> +'
[...much repetitious code omitted...]

In my previous review[1], I mentioned that the significant repetition
in the implementation of the new tests increased potential for errors
when composing them, and made it more difficult to review. To resolve
the issue, I suggested factoring out the repeated code into a helper
function, and gave an idea of its interface and how it could be
applied to eliminate all the repetition.

In this version of the patch, you've made some progress at
eliminating repetition, but much still remains. I had hoped to see
much more aggressive repetition elimination, collapsing the code to
the bare minimum while still testing all combinations of states.
Here's what I had in mind:

---- 8< ----
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 1b2e981..be6d34c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -270,6 +270,35 @@ cat > expect <<\EOF
 * initial
 EOF
 
+test_expect_success 'setup merges' '
+	git log --min-parents=2 --pretty=tformat:%s >expect_only &&
+	git log --max-parents=1 --pretty=tformat:%s >expect_hide &&
+	git log --min-parents=-1 --pretty=tformat:%s >expect_show
+'
+
+test_log_merges() {
+	expect=expect_$1
+	config=${2:+-c log.merges=$2}
+	option=${3:+--merges=$3}
+	option=${4:-$option}
+	test_expect_success "merges${config:+ $config}${option:+ $option}" "
+		git $config log $option --pretty=tformat:%s >actual &&
+		test_cmp $expect actual
+	"
+}
+
+states="show only hide"
+for c in '' $states
+do
+	for o in '' $states
+	do
+		test_log_merges ${o:-${c:-show}} "$c" "$o"
+	done
+done
+
+test_log_merges hide show '' --no-merges
+test_log_merges only hide '' '--merges --max-parents=2'
+
 test_expect_success 'log --graph with merge' '
 	git log --graph --date-order --pretty=tformat:%s |
 		sed "s/ *\$//" >actual &&
---- 8< ----

The output of the above test code looks like this:

---- 8< ----
ok 29 - setup merges
ok 30 - merges
ok 31 - merges --merges=show
ok 32 - merges --merges=only
ok 33 - merges --merges=hide
ok 34 - merges -c log.merges=show
ok 35 - merges -c log.merges=show --merges=show
ok 36 - merges -c log.merges=show --merges=only
ok 37 - merges -c log.merges=show --merges=hide
ok 38 - merges -c log.merges=only
ok 39 - merges -c log.merges=only --merges=show
ok 40 - merges -c log.merges=only --merges=only
ok 41 - merges -c log.merges=only --merges=hide
ok 42 - merges -c log.merges=hide
ok 43 - merges -c log.merges=hide --merges=show
ok 44 - merges -c log.merges=hide --merges=only
ok 45 - merges -c log.merges=hide --merges=hide
ok 46 - merges -c log.merges=show --no-merges
ok 47 - merges -c log.merges=hide --merges --max-parents=2
---- 8< ----

Hmm, it seems that I've rewritten pretty much the entire patch, so
perhaps the From: header ought to mention my name instead. Oops.

[1]: http://git.661346.n2.nabble.com/PATCH-1-5-Add-a-new-option-merges-to-revision-c-td7627662.html#a7627683

Re: [PATCH v2 2/5] log: honor log.merges= option

[Koosha Khajehmoogahi]

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



On 04/04/2015 10:00 PM, Junio C Hamano wrote:
> Koosha Khajehmoogahi <koosha@posteo.de> writes:
> 
>> From: Junio C Hamano <gitster@pobox.com>
>>
>> [kk: wrote commit message]
> 
> Ehh, what exactly did you write ;-)?
> 
> I think the most important thing that needs to be explained by the
> log message for this change is that the variable is honored only by
> log and it needs to explain why other Porcelain commands in the same
> "log" family, like "whatchanged", should ignore the variable.
>
So, what would be the reason? 
 
> I think that we must not to allow format-patch and show to be
> affected by this variable, because it is silly if log.merges=only
> broke format-patch output or made "git show" silent.  But I didn't
> think about others.  Whoever is doing this change needs to explain
> in the log message the reason why it was decided that only "git log"
> should pay attention to it.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/5] log: honor log.merges= option

[Junio C Hamano]

Koosha Khajehmoogahi <koosha@posteo.de> writes:

> On 04/04/2015 10:00 PM, Junio C Hamano wrote:
>> Koosha Khajehmoogahi <koosha@posteo.de> writes:
>> 
>>> From: Junio C Hamano <gitster@pobox.com>
>>>
>>> [kk: wrote commit message]
>> 
>> Ehh, what exactly did you write ;-)?
>> 
>> I think the most important thing that needs to be explained by the
>> log message for this change is that the variable is honored only by
>> log and it needs to explain why other Porcelain commands in the same
>> "log" family, like "whatchanged", should ignore the variable.
>>
> So, what would be the reason? 

It is strange that you have to ask me to give you the reason why you
chose it that way, isn't it?

>> I think that we must not to allow format-patch and show to be
>> affected by this variable, because it is silly if log.merges=only
>> broke format-patch output or made "git show" silent.  But I didn't
>> think about others.  Whoever is doing this change needs to explain
>> in the log message the reason why it was decided that only "git log"
>> should pay attention to it.
>> 

Re: [PATCH v2 2/5] log: honor log.merges= option

[Koosha Khajehmoogahi]

On 04/08/2015 04:28 AM, Junio C Hamano wrote:
> Koosha Khajehmoogahi <koosha@posteo.de> writes:
> 
>> On 04/04/2015 10:00 PM, Junio C Hamano wrote:
>>> Koosha Khajehmoogahi <koosha@posteo.de> writes:
>>>
>>>> From: Junio C Hamano <gitster@pobox.com>
>>>>
>>>> [kk: wrote commit message]
>>>
>>> Ehh, what exactly did you write ;-)?
>>>
>>> I think the most important thing that needs to be explained by the
>>> log message for this change is that the variable is honored only by
>>> log and it needs to explain why other Porcelain commands in the same
>>> "log" family, like "whatchanged", should ignore the variable.
>>>
>> So, what would be the reason? 
> 
> It is strange that you have to ask me to give you the reason why you
> chose it that way, isn't it?

AFAIK, the only other command that supports --merges and --no-merges options is
rev-list. This new feature aims to make a default behavior for the commands
that have these options. The command-line option is supported by the two commands.
However, the config var is only used by git-log and rev-list ignores it. I didn't
exclude rev-list for any particular reason. If we need, I could also handle it in
rev-list.

Re: [PATCH v2 2/5] log: honor log.merges= option

[Junio C Hamano]

Koosha Khajehmoogahi <koosha@posteo.de> writes:

> On 04/08/2015 04:28 AM, Junio C Hamano wrote:
>
>> It is strange that you have to ask me to give you the reason why you
>> chose it that way, isn't it?
>
> AFAIK, the only other command that supports --merges and --no-merges options is
> rev-list. This new feature aims to make a default behavior for the commands
> that have these options. The command-line option is supported by the two commands.
> However, the config var is only used by git-log and rev-list ignores it. I didn't
> exclude rev-list for any particular reason. If we need, I could also handle it in
> rev-list.

As rev-list is plumbing, it shouldn't be affected by the UI level
customization knobs like this one; otherwise you will break people's
scripts when end users choose to use the new knobs.

Historically, "whatchanged" has been the way to ask for "log", with
different default output format (but not different commit selection
logic).  I would think people who are used to "whatchanged" would
expect that the command would pay attention to what "log" would.

As I already mentioned with the reason why, I do not think "show"
and "format-patch" should pay attention to it.

There may be other commands from the "log" family (i.e. what is
defined in builtin/log.c and/or uses get_revision() API to walk the
commit graph), for which similar reasoning should be done to decide
if each of them should or should not pay attention to it.

Thanks.