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

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

[Koosha Khajehmoogahi]

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

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 v3 2/5] log: honor log.merges= option

[Koosha Khajehmoogahi]

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

The config. variable is honored only by log. Other log family commands
including show, shortlog and rev-list are affected only from the
equivalent command line option (i.e. --merges=). Since these commands
are somehow different representations of log command, we like to have
the same behavior from them as we do from log itself. However, to not
break the default output of them, we do not consider the the config.
var. Other log-family commands format-patch and cherry ignore the option
altogether (i.e. both on command line and on config settings).

[kk: wrote commit message]

Helped-by: Eric 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

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

[Junio C Hamano]

On Mon, Apr 13, 2015 at 8:29 AM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> From: Junio C Hamano <gitster@pobox.com>

Let's not call this *my* change. The mechanism I did write, but I think the
most important part of the change is to decide which commands in the
log family should honor the configuration variable and which ones should
not. As I said in the beginning, I am not convinced that letting "git log" and
"git log" alone is the right choice. Make it "Helped-by" or something instead,
to make it clear that I helped the implementation but not the design.

> The config. variable is honored only by log. Other log family commands
> including show, shortlog and rev-list are affected only from the
> equivalent command line option (i.e. --merges=). Since these commands
> are somehow different representations of log command, we like to have
> the same behavior from them as we do from log itself. However, to not
> break the default output of them, we do not consider the the config.

That is a weak argument, isn't it? If we really wanted not to break the
default output, then "git log" should not honor the configuration variable,
either. And I do not agree with "we like to have the same behaviour" in
the first place.

My preference, as I already hinted in previous review rounds, is:

 - "log" and "whatchanged" are the same things and should behave the
   same way with respect to this configuration variable;

 - "rev-list" is plumbing and should never pay attention to a UI level
   configuration variable like this one;

 - "show" is given exact commit object(s) and is asked to show them;
   not showing what was explicitly asked to be shown only because
   the user has "do not show merges" configuration does not make
   much sense, so the configuration variable should be ignored;

 - The revision graph traversal done by "format-patch" and "cherry-pick"
   is primarily to determine the set of commits to process and of a
   different nature from the traversal done by "log", even though they
   share the same underlying code. It is not appropriate to pay attention
   to this configuration variable.

 - I am neutral as to "shortlog". As long as it pays attention to the
   new command line override, I think it is OK if it paid attention to
   the configuration variable, primarily because people most of the
   time use "shortlog --no-merges" with the current version of Git
   _anyway_.

Thanks.

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

[Koosha Khajehmoogahi]

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

[kk: added documentation in git-log.txt]

Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
---
 Documentation/git-log.txt          |  3 +++
 Documentation/rev-list-options.txt | 17 +++++++++++++++++
 2 files changed, 20 insertions(+)

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..88f152f 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -96,6 +96,23 @@ if it is part of the log message.
 --remove-empty::
 	Stop when a given path disappears from the tree.
 
+--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.
+
 --merges::
 	Print only merge commits. This is exactly the same as `--min-parents=2`.
 
-- 
2.3.3.263.g095251d.dirty

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

[Eric Sunshine]

On Mon, Apr 13, 2015 at 11:29 AM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> From: Junio C Hamano <gitster@pobox.com>
>
> [kk: added documentation in git-log.txt]
>
> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index f620ee4..88f152f 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -96,6 +96,23 @@ if it is part of the log message.
>  --remove-empty::
>         Stop when a given path disappears from the tree.
>
> +--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.

s/overriden/overridden/

>  --merges::
>         Print only merge commits. This is exactly the same as `--min-parents=2`.
>
> --
> 2.3.3.263.g095251d.dirty

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

[Koosha Khajehmoogahi]

On 04/14/2015 08:42 AM, Eric Sunshine wrote:
> On Mon, Apr 13, 2015 at 11:29 AM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
>> From: Junio C Hamano <gitster@pobox.com>
>>
>> [kk: added documentation in git-log.txt]
>>
>> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
>> ---
>> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
>> index f620ee4..88f152f 100644
>> --- a/Documentation/rev-list-options.txt
>> +++ b/Documentation/rev-list-options.txt
>> @@ -96,6 +96,23 @@ if it is part of the log message.
>>  --remove-empty::
>>         Stop when a given path disappears from the tree.
>>
>> +--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.
> 
> s/overriden/overridden/
> 
>>  --merges::
>>         Print only merge commits. This is exactly the same as `--min-parents=2`.
>>
>> --
>> 2.3.3.263.g095251d.dirty

Should I send a new reroll or wait for reviews on my other commits.
I have not received any review on other patches of this series yet.

-- 
Koosha

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

[Eric Sunshine]

On Wed, Apr 15, 2015 at 6:27 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> On 04/14/2015 08:42 AM, Eric Sunshine wrote:
>> On Mon, Apr 13, 2015 at 11:29 AM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
>>> From: Junio C Hamano <gitster@pobox.com>
>>>
>>> [kk: added documentation in git-log.txt]
>>>
>>> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
>>> ---
>>> @@ -96,6 +96,23 @@ if it is part of the log message.
>>>  --remove-empty::
>>>         Stop when a given path disappears from the tree.
>>>
>>> +--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.
>>
>> s/overriden/overridden/
>
> Should I send a new reroll or wait for reviews on my other commits.
> I have not received any review on other patches of this series yet.

Lack of review comments does not necessarily mean that people did not
review the patches; it could mean that reviewers had nothing more to
say. In addition to pointing out the typo above, I also made a small
comment[1] on patch 4/5, but otherwise did not find anything else
about which to comment.

Whether or not you should re-send is something Junio can answer. He
will sometimes silently pick up a series as-is or sometimes silently
make minor fixes based upon review comments. Check his 'pu' branch
periodically to see if your series appears there and if it has those
minor adjustments. If not, and if he doesn't answer this question,
then you may need to re-roll.

[1]: http://article.gmane.org/gmane.comp.version-control.git/267118

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

[Koosha Khajehmoogahi]

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

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 1b2e981..3edcd81 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 &&
-- 
2.3.3.263.g095251d.dirty

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

[Eric Sunshine]

On Mon, Apr 13, 2015 at 11:29 AM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

> ---
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 1b2e981..3edcd81 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=' '

This differs slightly from the version I posted[1], in which I had
intentionally dropped the '=' from "merges=" in the description to
normalize the output. (I think it looks slightly nicer without the
'='.)

[1]: http://article.gmane.org/gmane.comp.version-control.git/266902/

> +       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 &&
> --
> 2.3.3.263.g095251d.dirty

[PATCH v3 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

[PATCH] squash! bash-completion: add support for git-log --merges= and log.merges

[SZEDER Gábor]

---
The patch at the tip of kk/log-merges-config misindented one of the case
arms, this one amends it.

 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a75d7f5..83b2359 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1454,9 +1454,9 @@ _git_log ()
 	--merges=*)
 		__gitcomp "show hide only" "" "${cur##--merges=}"
 		return
 		;;
-		--*)
+	--*)
 		__gitcomp "
 			$__git_log_common_options
 			$__git_log_shortlog_options
 			$__git_log_gitk_options
-- 
1.9.5.msysgit.0

Re: [PATCH] squash! bash-completion: add support for git-log --merges= and log.merges

[Junio C Hamano]

Thanks.