[PATCH 1/5] Add a new option 'merges' to revision.c

[PATCH 1/5] Add a new option 'merges' to revision.c

[Koosha Khajehmoogahi]

revision.c: 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.

Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
---
 revision.c | 22 ++++++++++++++++++++++
 revision.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/revision.c b/revision.c
index 66520c6..edb7bed 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,9 +1817,14 @@ 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->max_parents = -1;
 		revs->min_parents = 2;
 	} else if (!strcmp(arg, "--no-merges")) {
+		revs->min_parents = 0;
 		revs->max_parents = 1;
 	} else if (starts_with(arg, "--min-parents=")) {
 		revs->min_parents = atoi(arg+14);
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 2/5] Make git-log honor log.merges option

[Koosha Khajehmoogahi]

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 2/5] Make git-log honor log.merges option

[Eric Sunshine]

On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> Subject: Make git-log honor log.merges option

Drop capitalization, mention area you're touching, followed by colon,
followed by short summary:

    log: honor log.merges option

> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>

As discussed in my review of patch 1/5, since Junio actually wrote
this patch[1], it would be more correct and considerate to attribute
it to him by ensuring that a "From:" header is at the very top of the
commit message before mailing out the patch:

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

(If Junio had also signed-off on his patch, you would also include his
sign-off just above yours. In this case, he didn't sign off[1], so
your sign-off is all that is needed; and Junio will add his own if he
picks up this series.)

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

> ---
> 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 3/5] Update documentations for git-log to include the new --merges option and also its corresponding config option.

[Koosha Khajehmoogahi]

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

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 1f7bc67..506125a 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 4ed8587..398e657 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -99,6 +99,12 @@ if it is part of the log message.
 --merges::
 	Print only merge commits. This is exactly the same as `--min-parents=2`.
 
+--merges=show|hide|only::
+	If show is specified, merge commits will be shown in conjunction with
+	other commits. If hide is specified, it will work like `--no-merges`.
+	If only is specified, it will work like `--merges`. The default option
+	is show.
+
 --no-merges::
 	Do not print commits with more than one parent. This is
 	exactly the same as `--max-parents=1`.
-- 
2.3.3.263.g095251d.dirty

Re: [PATCH 3/5] Update documentations for git-log to include the new --merges option and also its corresponding config option.

[Eric Sunshine]

On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> Subject: Update documentations for git-log to include the new
> --merges option and also its corresponding config option.

The Subject: should be a short summary of the change; ideally less
than 70 or 72 characters. The rest of the commit message can flesh out
the description if necessary. The subject on this patch is far too
long. Also, drop capitalization, mention the area you're touching, and
drop the final period. You might say instead:

    Documentation: mention git-log --merges= and log.merges

In this case, it's probably not necessary to write anything else in
the commit message. The summary says enough, despite its conciseness.

More below.

> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
> index 1f7bc67..506125a 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`.

To disambiguate this from the --merges option, you should probably
spell it out explicitly as `--merges=` rather than just `--merges`.

>  mailmap.*::
>         See linkgit:git-shortlog[1].
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 4ed8587..398e657 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -99,6 +99,12 @@ if it is part of the log message.
>  --merges::
>         Print only merge commits. This is exactly the same as `--min-parents=2`.
>
> +--merges=show|hide|only::
> +       If show is specified, merge commits will be shown in conjunction with
> +       other commits. If hide is specified, it will work like `--no-merges`.
> +       If only is specified, it will work like `--merges`. The default option
> +       is show.

By "The default option is show", do you mean when --merges= is not
specified? Perhaps it would be better to phrase it as:

    Default is `show` if `--merges=` is not specified.

> +
>  --no-merges::
>         Do not print commits with more than one parent. This is
>         exactly the same as `--max-parents=1`.

It might make more sense to rewrite the --merges and --no-merges
options in terms of the new --merges= option. For instance, something
like this:

     --merges::
        Shorthand for `--merges=only`.

    --merges={show|hide|only}::
        `show`: show merge and non-merge commits

        `hide`: omit merge commits; same as `--max-parents=1`

        `only`: show only merge commits; same as `--min-parents=2`

        Default is `show` if `--merges=` is not specified.

     --no-merges::
        Shorthand for `--merges=hide`.

Alternately, --merges and --merges= could be combined like this
(taking inspiration from --decorate[=]):

    --merges[=show|hide|only]::

But then you need additional explanation about what --merges means
without the '=' and following keyword.

> --
> 2.3.3.263.g095251d.dirty

[PATCH 4/5] Add tests for git-log --merges=show|hide|only

[Koosha Khajehmoogahi]

Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
---
 t/t4202-log.sh | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 141 insertions(+)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 5f2b290..ab6f371 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -428,6 +428,147 @@ cat > expect <<\EOF
 * initial
 EOF
 
+cat > only_merges <<EOF
+Merge tag 'reach'
+Merge tags 'octopus-a' and 'octopus-b'
+Merge branch 'tangle'
+Merge branch 'side' (early part) into tangle
+Merge branch 'master' (early part) into tangle
+Merge branch 'side'
+EOF
+
+cat > only_commits <<EOF
+seventh
+octopus-b
+octopus-a
+reach
+tangle-a
+side-2
+side-1
+Second
+sixth
+fifth
+fourth
+third
+second
+initial
+EOF
+
+cat > both_commits_merges <<EOF
+Merge tag 'reach'
+Merge tags 'octopus-a' and 'octopus-b'
+seventh
+octopus-b
+octopus-a
+reach
+Merge branch 'tangle'
+Merge branch 'side' (early part) into tangle
+Merge branch 'master' (early part) into tangle
+tangle-a
+Merge branch 'side'
+side-2
+side-1
+Second
+sixth
+fifth
+fourth
+third
+second
+initial
+EOF
+
+test_expect_success 'log with config log.merges=show' '
+	git config log.merges show &&
+    git log --pretty=tformat:%s >actual &&
+	test_cmp both_commits_merges actual &&
+    git config --unset log.merges
+'
+
+test_expect_success 'log with config log.merges=only' '
+	git config log.merges only &&
+    git log --pretty=tformat:%s >actual &&
+	test_cmp only_merges actual &&
+    git config --unset log.merges
+'
+
+test_expect_success 'log with config log.merges=hide' '
+	git config log.merges hide &&
+    git log --pretty=tformat:%s >actual &&
+	test_cmp only_commits actual &&
+    git config --unset log.merges
+'
+
+test_expect_success 'log with config log.merges=show with git log --no-merges' '
+	git config log.merges show &&
+    git log --no-merges --pretty=tformat:%s >actual &&
+	test_cmp only_commits actual &&
+    git config --unset log.merges
+'
+
+test_expect_success 'log with config log.merges=hide with git log ---merges' '
+	git config log.merges hide &&
+    git log --merges --pretty=tformat:%s >actual &&
+	test_cmp only_merges actual &&
+    git config --unset log.merges
+'
+
+test_expect_success 'log --merges=show' '
+    git log --merges=show --pretty=tformat:%s >actual &&
+	test_cmp both_commits_merges actual
+'
+
+test_expect_success 'log --merges=only' '
+    git log --merges=only --pretty=tformat:%s >actual &&
+	test_cmp only_merges actual
+'
+
+test_expect_success 'log --merges=hide' '
+    git log --merges=hide --pretty=tformat:%s >actual &&
+	test_cmp only_commits actual
+'
+
+test_expect_success 'log --merges=show with config log.merges=hide' '
+	git config log.merges hide &&
+    git log --merges=show --pretty=tformat:%s >actual &&
+	test_cmp both_commits_merges actual &&
+    git config --unset log.merges
+'
+
+test_expect_success 'log --merges=show with config log.merges=only' '
+	git config log.merges only &&
+    git log --merges=show --pretty=tformat:%s >actual &&
+	test_cmp both_commits_merges actual &&
+    git config --unset log.merges
+'
+
+test_expect_success 'log --merges=only with config log.merges=show' '
+	git config log.merges show &&
+    git log --merges=only --pretty=tformat:%s >actual &&
+	test_cmp only_merges actual &&
+    git config --unset log.merges
+'
+
+test_expect_success 'log --merges=only with config log.merges=hide' '
+	git config log.merges hide &&
+    git log --merges=only --pretty=tformat:%s >actual &&
+	test_cmp only_merges actual &&
+    git config --unset log.merges
+'
+
+test_expect_success 'log --merges=hide with config log.merges=show' '
+	git config log.merges show &&
+    git log --merges=hide --pretty=tformat:%s >actual &&
+	test_cmp only_commits actual &&
+    git config --unset log.merges
+'
+
+test_expect_success 'log --merges=hide with config log.merges=only' '
+	git config log.merges only &&
+    git log --merges=hide --pretty=tformat:%s >actual &&
+	test_cmp only_commits actual &&
+    git config --unset log.merges
+'
+
 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 4/5] Add tests for git-log --merges=show|hide|only

[Torsten Bögershausen]

On 22.03.15 19:28, Koosha Khajehmoogahi wrote:
> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
>  t/t4202-log.sh | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 141 insertions(+)
> 
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 5f2b290..ab6f371 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -428,6 +428,147 @@ cat > expect <<\EOF
>  * initial
>  EOF
>  
> +cat > only_merges <<EOF

- please no space after the ">"
- please add the && at the end of the line:
cat >only_merges <<EOF &&
(And the same further down)

> +test_expect_success 'log with config log.merges=show' '
> +	git config log.merges show &&
Indent with TAB is good
> +    git log --pretty=tformat:%s >actual &&
but indent with 4 spaces not ideal, please use a TAB as well.
> +	test_cmp both_commits_merges actual &&
> +    git config --unset log.merges
Do we need the unset here?
The log.merges is nicely set up before each test case, so can we drop the unset lines ?
(Or do I miss something ?)

Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only

[Eric Sunshine]

On Sun, Mar 22, 2015 at 3:57 PM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 22.03.15 19:28, Koosha Khajehmoogahi wrote:
>> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
>> ---
>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 5f2b290..ab6f371 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -428,6 +428,147 @@ cat > expect <<\EOF
>>  * initial
>>  EOF
>>
>> +cat > only_merges <<EOF
>
> - please no space after the ">"
> - please add the && at the end of the line:
> cat >only_merges <<EOF &&

This code is outside any test, so && has no impact (and would be
slightly confusing). Better would be to move this setup code into a
"setup --merges=" test, in which case the &&-chain would be
appropriate.

Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only

[Koosha Khajehmoogahi]

On 03/22/2015 08:57 PM, Torsten Bögershausen wrote:
> On 22.03.15 19:28, Koosha Khajehmoogahi wrote:
>> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
>> ---
>>  t/t4202-log.sh | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 141 insertions(+)
>>
>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 5f2b290..ab6f371 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -428,6 +428,147 @@ cat > expect <<\EOF
>>  * initial
>>  EOF
>>  
>> +cat > only_merges <<EOF
> 
> - please no space after the ">"
> - please add the && at the end of the line:
> cat >only_merges <<EOF &&
> (And the same further down)
> 
>> +test_expect_success 'log with config log.merges=show' '
>> +	git config log.merges show &&
> Indent with TAB is good
>> +    git log --pretty=tformat:%s >actual &&
> but indent with 4 spaces not ideal, please use a TAB as well.
>> +	test_cmp both_commits_merges actual &&
>> +    git config --unset log.merges
> Do we need the unset here?
> The log.merges is nicely set up before each test case, so can we drop the unset lines ?
> (Or do I miss something ?)
> 

Good point; we can drop only those unset lines whose next test sets the log.merges.
However, if the next test does not set it, we must unset it as it affects the
default behavior of git-log.

-- 
Koosha

Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only

[Eric Sunshine]

On Sun, Mar 22, 2015 at 6:07 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> On 03/22/2015 08:57 PM, Torsten Bögershausen wrote:
>> On 22.03.15 19:28, Koosha Khajehmoogahi wrote:
>>> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
>>> ---
>>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>>> index 5f2b290..ab6f371 100755
>>> --- a/t/t4202-log.sh
>>> +++ b/t/t4202-log.sh
>>> @@ -428,6 +428,147 @@ cat > expect <<\EOF
>>>  * initial
>>>  EOF
>>>
>>> +test_expect_success 'log with config log.merges=show' '
>>> +    git config log.merges show &&
>>> +    git log --pretty=tformat:%s >actual &&
>>> +    test_cmp both_commits_merges actual &&
>>> +    git config --unset log.merges
>> Do we need the unset here?
>> The log.merges is nicely set up before each test case, so can we drop the unset lines ?
>> (Or do I miss something ?)
>
> Good point; we can drop only those unset lines whose next test sets the log.merges.
> However, if the next test does not set it, we must unset it as it affects the
> default behavior of git-log.

Such an approach would be too fragile. Tests may be re-ordered, added,
or removed. Better is to make each test as self-contained as possible.
See my review[1] of this patch for alternate suggestions.

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

Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only

[Koosha Khajehmoogahi]

On 03/22/2015 11:40 PM, Eric Sunshine wrote:
> On Sun, Mar 22, 2015 at 6:07 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
>> On 03/22/2015 08:57 PM, Torsten Bögershausen wrote:
>>> On 22.03.15 19:28, Koosha Khajehmoogahi wrote:
>>>> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
>>>> ---
>>>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>>>> index 5f2b290..ab6f371 100755
>>>> --- a/t/t4202-log.sh
>>>> +++ b/t/t4202-log.sh
>>>> @@ -428,6 +428,147 @@ cat > expect <<\EOF
>>>>  * initial
>>>>  EOF
>>>>
>>>> +test_expect_success 'log with config log.merges=show' '
>>>> +    git config log.merges show &&
>>>> +    git log --pretty=tformat:%s >actual &&
>>>> +    test_cmp both_commits_merges actual &&
>>>> +    git config --unset log.merges
>>> Do we need the unset here?
>>> The log.merges is nicely set up before each test case, so can we drop the unset lines ?
>>> (Or do I miss something ?)
>>
>> Good point; we can drop only those unset lines whose next test sets the log.merges.
>> However, if the next test does not set it, we must unset it as it affects the
>> default behavior of git-log.
> 
> Such an approach would be too fragile. Tests may be re-ordered, added,
> or removed. Better is to make each test as self-contained as possible.
> See my review[1] of this patch for alternate suggestions.
> 
> [1]: http://article.gmane.org/gmane.comp.version-control.git/266099
> 

That's why I wrote them this way actually. Also, thanks for your
review. I will refactor my code to consider your suggestions.

Thanks.
-- 
Koosha

Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only

[Torsten Bögershausen]

Back to the original discussion:

+test_expect_success 'log with config log.merges=show' '
+    git config log.merges show &&
+    git log --pretty=tformat:%s >actual &&
+    test_cmp both_commits_merges actual &&
+    git config --unset log.merges

These days I would probably shorten the code, the review time and
the execution time of the test and increase the clean-ness with 50%
by simply writing

+test_expect_success 'log with config log.merges=show' '
+	git -c log.merges=show log --pretty=tformat:%s >actual &&
+	test_cmp both_commits_merges actual
+	'

Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only

[Eric Sunshine]

On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> Subject: Add tests for git-log --merges=show|hide|only

Drop capitalization, mention area you're touching, followed by colon,
followed by short summary:

    t4202-log: add --merges= tests

More below.

> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 5f2b290..ab6f371 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -428,6 +428,147 @@ cat > expect <<\EOF
>  * initial
>  EOF
>
> +cat > only_merges <<EOF
> +Merge tag 'reach'
> +Merge tags 'octopus-a' and 'octopus-b'
> +Merge branch 'tangle'
> +Merge branch 'side' (early part) into tangle
> +Merge branch 'master' (early part) into tangle
> +Merge branch 'side'
> +EOF

Current custom is to place this sort of setup code in its own test
rather than having it at top-level. So, the above and below 'cat's
should all go in a test named "setup --merges=" (or something better).
Prefixing EOF with '-' allows you to indent the here-doc content and
EOF so that it reads nicely inside a test. Also prefix EOF with '\' to
indicate that you do not expect interpolation within the here-doc.
Torsten already mentioned to drop the space following '>'. Finally,
within the setup test, chain them all together with &&. So:

    cat >only_merges <<-\EOF &&
    ...
    EOF

More below.

> +cat > only_commits <<EOF
> +seventh
> +octopus-b
> +octopus-a
> +reach
> +tangle-a
> +side-2
> +side-1
> +Second
> +sixth
> +fifth
> +fourth
> +third
> +second
> +initial
> +EOF
> +
> +cat > both_commits_merges <<EOF
> +Merge tag 'reach'
> +Merge tags 'octopus-a' and 'octopus-b'
> +seventh
> +octopus-b
> +octopus-a
> +reach
> +Merge branch 'tangle'
> +Merge branch 'side' (early part) into tangle
> +Merge branch 'master' (early part) into tangle
> +tangle-a
> +Merge branch 'side'
> +side-2
> +side-1
> +Second
> +sixth
> +fifth
> +fourth
> +third
> +second
> +initial
> +EOF

t4202 already does this sort of fragile hard-coding of "expected"
output, so this isn't a particularly strong objection, but it would be
more robust if you could create these "expected" lists dynamically by
issuing some git command other than the one you're testing. (That
could also be considered fodder for a follow-on patch if you don't
consider such a change worthwhile at this time.)

> +
> +test_expect_success 'log with config log.merges=show' '
> +       git config log.merges show &&
> +    git log --pretty=tformat:%s >actual &&

Torsten already mentioned botched indentation. Use (8-character wide)
tab for indentation everywhere.

> +       test_cmp both_commits_merges actual &&
> +    git config --unset log.merges

If the test_cmp fails (because the expected and actual output
differed), then the git-config --unset will never be invoked. To
ensure that the config setting is undone when the test finishes
(whether successful or not), use test_config().

The other option is to ensure that each test sets or clears log.merges
as appropriate at the start of the test, and then not bother about
resetting it. The shortcoming of this approach, however, is that any
tests added following these new tests won't necessarily know that
log.merges is set or that they may need to clear it. Consequently,
test_config() at the start of each of these tests is probably the
better approach.

More below.

> +'
> +
> +test_expect_success 'log with config log.merges=only' '
> +       git config log.merges only &&
> +    git log --pretty=tformat:%s >actual &&
> +       test_cmp only_merges actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log with config log.merges=hide' '
> +       git config log.merges hide &&
> +    git log --pretty=tformat:%s >actual &&
> +       test_cmp only_commits actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log with config log.merges=show with git log --no-merges' '
> +       git config log.merges show &&
> +    git log --no-merges --pretty=tformat:%s >actual &&
> +       test_cmp only_commits actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log with config log.merges=hide with git log ---merges' '
> +       git config log.merges hide &&
> +    git log --merges --pretty=tformat:%s >actual &&
> +       test_cmp only_merges actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log --merges=show' '

For these tests which expect log.merges to be unset, it would be more
robust, and the intent clearer, if you actually ensured that it was
unset. test_unconfig() at the start of the test would be the
appropriate way to unset the config. It would also make the tests more
self-contained, in case they are ever re-ordered.

More below.

> +    git log --merges=show --pretty=tformat:%s >actual &&
> +       test_cmp both_commits_merges actual
> +'
> +
> +test_expect_success 'log --merges=only' '
> +    git log --merges=only --pretty=tformat:%s >actual &&
> +       test_cmp only_merges actual
> +'
> +
> +test_expect_success 'log --merges=hide' '
> +    git log --merges=hide --pretty=tformat:%s >actual &&
> +       test_cmp only_commits actual
> +'
> +
> +test_expect_success 'log --merges=show with config log.merges=hide' '
> +       git config log.merges hide &&
> +    git log --merges=show --pretty=tformat:%s >actual &&
> +       test_cmp both_commits_merges actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log --merges=show with config log.merges=only' '
> +       git config log.merges only &&
> +    git log --merges=show --pretty=tformat:%s >actual &&
> +       test_cmp both_commits_merges actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log --merges=only with config log.merges=show' '
> +       git config log.merges show &&
> +    git log --merges=only --pretty=tformat:%s >actual &&
> +       test_cmp only_merges actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log --merges=only with config log.merges=hide' '
> +       git config log.merges hide &&
> +    git log --merges=only --pretty=tformat:%s >actual &&
> +       test_cmp only_merges actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log --merges=hide with config log.merges=show' '
> +       git config log.merges show &&
> +    git log --merges=hide --pretty=tformat:%s >actual &&
> +       test_cmp only_commits actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log --merges=hide with config log.merges=only' '
> +       git config log.merges only &&
> +    git log --merges=hide --pretty=tformat:%s >actual &&
> +       test_cmp only_commits actual &&
> +    git config --unset log.merges
> +'

There's an awful lot of repetition in the implementation these new
tests. Such repetition invites errors when composing the tests and
makes them difficult to review (since it's easy to be sloppy in the
review as well). The situation could be improved by introducing a
helper function which performs the tests, controlled by a few
parameters. For instance, the helper could accept three arguments: (1)
optional value of --merges=, (2) optional value of log.merges, (3)
name of "expected" file. It also suggests that you should normalize
the names of the "expected" files ("expect_show", "expect_only",
"expect_hide") so that the 3rd argument can be short and sweet. Let's
say that the helper is test_log_merges(), then you would invoke it as:

    test_log_merges "" "" show
    test_log_merges "" show show
    test_log_merges "" only only
    ...
    test_log_merges hide "" hide
    ...
    test_log_merges only hide only

for all combination of --merges= and log.merges.

> +
>  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 5/5] Update Bash completion script to include git log --merges option

[Koosha Khajehmoogahi]

Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
---
 contrib/completion/git-completion.bash | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 731c289..b63bb95 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,6 +1451,10 @@ _git_log ()
 		__gitcomp "long short" "" "${cur##--decorate=}"
 		return
 		;;
+    --merges=*)
+        __gitcomp "show hide only" "" "${cur##--merges=}"
+        return
+        ;;
 	--*)
 		__gitcomp "
 			$__git_log_common_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 5/5] Update Bash completion script to include git log --merges option

[Eric Sunshine]

On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> Subject: Update Bash completion script to include git log --merges option

Nice to see a patch covering this oft-overlooked corner of the project.

It's misleading to say that you're updating it to include the --merges
option, which it already knows about. Spell it out explicitly as
--merges=. Also, drop capitalization, mention area you're touching,
followed by colon, followed by short summary:

    completion: teach about git-log --merges= and log.merges

> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 731c289..b63bb95 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,6 +1451,10 @@ _git_log ()
>                 __gitcomp "long short" "" "${cur##--decorate=}"
>                 return
>                 ;;
> +    --merges=*)
> +        __gitcomp "show hide only" "" "${cur##--merges=}"
> +        return
> +        ;;
>         --*)
>                 __gitcomp "
>                         $__git_log_common_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 5/5] Update Bash completion script to include git log --merges option

[SZEDER Gábor]

Hi,

I agree with Eric's comment about the subject line.

Quoting Koosha Khajehmoogahi <koosha@posteo.de>:

> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
>   contrib/completion/git-completion.bash | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index 731c289..b63bb95 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,6 +1451,10 @@ _git_log ()
>   		__gitcomp "long short" "" "${cur##--decorate=}"
>   		return
>   		;;
> +    --merges=*)
> +        __gitcomp "show hide only" "" "${cur##--merges=}"
> +        return
> +        ;;

Funny indentation, please use tabs instead of spaces.

Otherwise this patch looks good.

>   	--*)
>   		__gitcomp "
>   			$__git_log_common_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 1/5] Add a new option 'merges' to revision.c

[Eric Sunshine]

On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> Subject: Add a new option 'merges' to revision.c

For the subject, mention the area you're touching, followed by a
colon, followed by a short but meaningful summary of the change. Drop
capitalization.

    revision: add --merges={show|only|hide} option

> revision.c: add a new option 'merges' with

No need to mention "revision.c" here since the patch itself shows this clearly.

Considering that there is already a --merges option, it is somewhat
misleading and not terribly clear to say only that you're adding a new
"option 'merges'". Better would be to spell out --merges= explicitly.

> 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.
>
> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>

Considering that Junio actually wrote this patch[1], it would be more
correct and considerate to attribute it to him. You can do so by
ensuring that there is a "From:" header at the very top of the commit
message before mailing out the patch:

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

The customary way to indicate that you wrote the commit message and
made a few small adjustments to Junio's patch would be with a
bracketed comment (starting with your initials) just before your
sign-off. Something like this:

    [kk: wrote commit message; added a couple missing
    {min|max}_parents assignments]

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

> ---
> diff --git a/revision.c b/revision.c
> index 66520c6..edb7bed 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,9 +1817,14 @@ 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->max_parents = -1;
>                 revs->min_parents = 2;
>         } else if (!strcmp(arg, "--no-merges")) {
> +               revs->min_parents = 0;
>                 revs->max_parents = 1;
>         } else if (starts_with(arg, "--min-parents=")) {
>                 revs->min_parents = atoi(arg+14);
> 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 1/5] Add a new option 'merges' to revision.c

[Junio C Hamano]

Koosha Khajehmoogahi <koosha@posteo.de> writes:

> @@ -1800,9 +1817,14 @@ 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);

This one makes sense to me (so does what the parse_merges_opt()
does).

>  	} else if (!strcmp(arg, "--merges")) {
> +		revs->max_parents = -1;
>  		revs->min_parents = 2;

But is this change warranted?  An existing user who is not at all
interested in the new --merges= option may be relying on the fact
that "--merges" does not affect the value of max_parents and she can
say "log --max-parents=2 --merges" to see only the two-way merges,
for example.  This change just broke her, and I do not see why it is
a good thing.

>  	} else if (!strcmp(arg, "--no-merges")) {
> +		revs->min_parents = 0;
>  		revs->max_parents = 1;

Likewise.

Re: [PATCH 1/5] Add a new option 'merges' to revision.c

[Koosha Khajehmoogahi]

On 03/23/2015 12:31 AM, Junio C Hamano wrote:
> Koosha Khajehmoogahi <koosha@posteo.de> writes:
> 
>> @@ -1800,9 +1817,14 @@ 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);
> 
> This one makes sense to me (so does what the parse_merges_opt()
> does).

In fact this change was written by you in your previous kind review :-)

I will add a 'From: Junio C Hamano <gitster@pobox.com>' header to my next patch.

> 
>>  	} else if (!strcmp(arg, "--merges")) {
>> +		revs->max_parents = -1;
>>  		revs->min_parents = 2;
> 
> But is this change warranted?  An existing user who is not at all
> interested in the new --merges= option may be relying on the fact
> that "--merges" does not affect the value of max_parents and she can
> say "log --max-parents=2 --merges" to see only the two-way merges,
> for example.  This change just broke her, and I do not see why it is
> a good thing.

The point is that if you have your log.merges conf option set to 'hide'
and you use git log --merges (two mutually conflicting options),
git will silently exit without anything to show. We need to clear the
max_parents set by parse_merges_opt() as the user should be able to continue
to use --merges without problem. But, your point is also valid.

> 
>>  	} else if (!strcmp(arg, "--no-merges")) {
>> +		revs->min_parents = 0;
>>  		revs->max_parents = 1;
> 
> Likewise.
> 

Similarly, without this, if log.merges is set to 'only' and you use git log --no-merges,
you will still see the merges in the output.

Re: [PATCH 1/5] Add a new option 'merges' to revision.c

[Junio C Hamano]

Koosha Khajehmoogahi <koosha@posteo.de> writes:

>>>  	} else if (!strcmp(arg, "--merges")) {
>>> +		revs->max_parents = -1;
>>>  		revs->min_parents = 2;
>> 
>> But is this change warranted?  An existing user who is not at all
>> interested in the new --merges= option may be relying on the fact
>> that "--merges" does not affect the value of max_parents and she can
>> say "log --max-parents=2 --merges" to see only the two-way merges,
>> for example.  This change just broke her, and I do not see why it is
>> a good thing.
>
> The point is that if you have your log.merges conf option set to 'hide'
> and you use git log --merges (two mutually conflicting options),
> git will silently exit without anything to show.

That is not an excuse to break "--merges" and "--no-merges" for
existing users who do not care about setting log.merges option to
anything.

The whole point of introducing a new "--merges=show" option was so
that people can sanely countermand log.merges configuration from the
command line without breaking --merges and --no-merges, wasn't it?