[PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only

[PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only

[Michael J Gruber]

This mini series makes it so that --no-merges undoes --merges
and vice versa, as the user should be able to expect,
and that --merges-only is a separate option.

Michael J Gruber (2):
  revision.c: rename --merges to --merges-only
  revision.c: introduce --merges to undo --no-merges

 Documentation/git-rev-list.txt         |    1 +
 Documentation/git-stash.txt            |    2 +-
 Documentation/rev-list-options.txt     |    6 ++++--
 contrib/completion/git-completion.bash |    2 +-
 revision.c                             |    4 +++-
 submodule.c                            |    2 +-
 6 files changed, 11 insertions(+), 6 deletions(-)

-- 
1.7.4.1.464.gf81ff

[PATCH/RFD 1/2] revision.c: rename --merges to --merges-only

[Michael J Gruber]

Users have a right to expect that --no-foo is the opposite of --foo and
undoes its effect. Since "--no-merges" means "exclude merges" it is
natural to expect that "--merges" means "include merges", but in fact it
means "merges only". Rename it accordingly.

As a side effect, this makes the option name coincide with the rev->flag
(modulo - -> _).

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-rev-list.txt         |    2 +-
 Documentation/git-stash.txt            |    2 +-
 Documentation/rev-list-options.txt     |    2 +-
 contrib/completion/git-completion.bash |    2 +-
 revision.c                             |    2 +-
 submodule.c                            |    2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index b08dfbc..020e585 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 	     [ \--max-age=<timestamp> ]
 	     [ \--min-age=<timestamp> ]
 	     [ \--sparse ]
-	     [ \--merges ]
+	     [ \--merges-only ]
 	     [ \--no-merges ]
 	     [ \--first-parent ]
 	     [ \--remove-empty ]
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 79abc38..eccafcb 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -246,7 +246,7 @@ repository, but not reachable any more:
 ----------------------------------------------------------------
 git fsck --unreachable |
 grep commit | cut -d\  -f3 |
-xargs git log --merges --no-walk --grep=WIP
+xargs git log --merges-only --no-walk --grep=WIP
 ----------------------------------------------------------------
 
 
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 5c6850f..78ea305 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -70,7 +70,7 @@ endif::git-rev-list[]
 
 	Stop when a given path disappears from the tree.
 
---merges::
+--merges-only::
 
 	Print only merge commits.
 
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ccdc172..298dddc 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1570,7 +1570,7 @@ _git_ls_tree ()
 __git_log_common_options="
 	--not --all
 	--branches --tags --remotes
-	--first-parent --merges --no-merges
+	--first-parent --merges-only --no-merges
 	--max-count=
 	--max-age= --since= --after=
 	--min-age= --until= --before=
diff --git a/revision.c b/revision.c
index e96c281..bd2fedd 100644
--- a/revision.c
+++ b/revision.c
@@ -1276,7 +1276,7 @@ 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 (!strcmp(arg, "--merges")) {
+	} else if (!strcmp(arg, "--merges-only")) {
 		revs->merges_only = 1;
 	} else if (!strcmp(arg, "--no-merges")) {
 		revs->no_merges = 1;
diff --git a/submodule.c b/submodule.c
index e9f2b19..6cae159 100644
--- a/submodule.c
+++ b/submodule.c
@@ -423,7 +423,7 @@ static int find_first_merges(struct object_array *result, const char *path,
 	int contains_another;
 
 	char merged_revision[42];
-	const char *rev_args[] = { "rev-list", "--merges", "--ancestry-path",
+	const char *rev_args[] = { "rev-list", "--merges-only", "--ancestry-path",
 				   "--all", merged_revision, NULL };
 	struct rev_info revs;
 	struct setup_revision_opt rev_opts;
-- 
1.7.4.1.464.gf81ff

[PATCH/RFD 2/2] revision.c: introduce --merges to undo --no-merges

[Michael J Gruber]

Introduce "--merges" as an option to undo "--no-merges". This is useful
whenever you want to override a default such as those from
"format-patch", "--cherry" or aliases which may use "--no-merges".

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-rev-list.txt         |    1 +
 Documentation/rev-list-options.txt     |    4 +++-
 contrib/completion/git-completion.bash |    2 +-
 revision.c                             |    2 ++
 4 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 020e585..6379f21 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -15,6 +15,7 @@ SYNOPSIS
 	     [ \--min-age=<timestamp> ]
 	     [ \--sparse ]
 	     [ \--merges-only ]
+	     [ \--merges ]
 	     [ \--no-merges ]
 	     [ \--first-parent ]
 	     [ \--remove-empty ]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 78ea305..e11145b 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -74,9 +74,11 @@ endif::git-rev-list[]
 
 	Print only merge commits.
 
+--merges::
 --no-merges::
 
-	Do not print commits with more than one parent.
+	Do resp. do not print commits with more than one parent.
+	For most commands, `--merges` is the default.
 
 --first-parent::
 	Follow only the first parent commit upon seeing a merge
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 298dddc..db56153 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1570,7 +1570,7 @@ _git_ls_tree ()
 __git_log_common_options="
 	--not --all
 	--branches --tags --remotes
-	--first-parent --merges-only --no-merges
+	--first-parent --merges-only --merges --no-merges
 	--max-count=
 	--max-age= --since= --after=
 	--min-age= --until= --before=
diff --git a/revision.c b/revision.c
index bd2fedd..a9ec574 100644
--- a/revision.c
+++ b/revision.c
@@ -1278,6 +1278,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->remove_empty_trees = 1;
 	} else if (!strcmp(arg, "--merges-only")) {
 		revs->merges_only = 1;
+	} else if (!strcmp(arg, "--merges")) {
+		revs->no_merges = 0;
 	} else if (!strcmp(arg, "--no-merges")) {
 		revs->no_merges = 1;
 	} else if (!strcmp(arg, "--boundary")) {
-- 
1.7.4.1.464.gf81ff

Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> This mini series makes it so that --no-merges undoes --merges
> and vice versa, as the user should be able to expect,
> and that --merges-only is a separate option.

Nice to see the series marked as RFD, but it would have been much better
what aspect of the series you wanted to be discussed.

I know that you have been here long enough to know that an outright
breakage of backward compatibility does not deserve a discussion, and that
you are feeling to see if people consider it is worth to design and
implement a migration path that is not fully specified by this series to
eventually give us the end result that is hinted by this series.

But I suspect some other readers on the list don't (and that is why I am
spelling it out).

As to the eventual goal, I think two modes "show me only merge commits",
"show me only commits that are not merges" are absolute nesessity no
matter how they are named, and as a uniformity bonus, "earlier I might
have said 'show me only merges' but I take it back" and its counterpart
might be good things to have.

But.

If a script writer wants to futureproof his script against later
introduction of configuration variables to trigger 'no-merges' and/or
'merges-only', what should she say?  "git log --no-merges-only --merges"?

Shouldn't the choice offered by the eventual end result of this topic,
perhaps two years down the road after migration period, not two seemingly
independent options as hinted with the series, but an enum with three
values (any commit, non-merge commit only, merge commit only)?

IOW, wouldn't it be a more logical thing to do to add a new option with
three values --show-commit={all,merges,no-merges}, if we were adding the
feature to filter commits by number of parents?

If we come up with a nicer name for the --show-commit example above (both
the name of the variable and the enum value) and promote it, the migration
would go smoothly without giving much pain to the existing users and
scripts, as it will not _have to_ involve touching the existing merges and
no-merges options at all, even though we can make them less visible.

Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only

[Jeff King]

On Thu, Mar 17, 2011 at 12:33:56PM +0100, Michael J Gruber wrote:

> This mini series makes it so that --no-merges undoes --merges
> and vice versa, as the user should be able to expect,
> and that --merges-only is a separate option.

Having recently been confused by this (and frustrated at the lack of an
equivalent to your new "--merges"), I do think the result is better.

However, this is totally changing the meaning of an option to plumbing
like rev-list (among others). Is it worth the breakage? If so, what's
the migration plan? Did I miss a discussion somewhere?

-Peff

Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only

[Michael J Gruber]

Jeff King venit, vidit, dixit 17.03.2011 20:59:
> On Thu, Mar 17, 2011 at 12:33:56PM +0100, Michael J Gruber wrote:
> 
>> This mini series makes it so that --no-merges undoes --merges
>> and vice versa, as the user should be able to expect,
>> and that --merges-only is a separate option.
> 
> Having recently been confused by this (and frustrated at the lack of an
> equivalent to your new "--merges"), I do think the result is better.
> 
> However, this is totally changing the meaning of an option to plumbing
> like rev-list (among others). Is it worth the breakage? If so, what's
> the migration plan? Did I miss a discussion somewhere?

You missed only the "D" in RFD :)

The meaning of a plumbing option can't be changed light-heartedly, of
course. OTOH, the current design is *really bad* from the ui point of
view. The expectation that

"cmd --no-foo --foo" is eq. to "cmd --foo"

and

"cmd --foo --no-foo" is eq. to "cmd --no-foo"

should be valid universally. In the long run, we might even try and
convert revision.c to parse_options, thereby gaining --no-foo for every
--foo.

So, my RFD really consists of two things:

- provide a way to override --no-merges/no_merges
- sane naming

The first step + half of the second could be achieved by:

- provide "--merges-also" which does "no_merges = 0"
- provide "--merges-only" which does "merges_only = 1", i.e. an alias
for current "--merges"

"--merges-also --no-merges" would be "--no-merges" and vice versa.
"--merges-also --merges-only" would be "--merges-only"
"--merges-only --merges-also" would be "--merges-only"
which all make sense sematically

"--merges-only --no-merges" would be no commits with the current
implementation (just like "--merges --no-merges"), but we could catch
this case easily and make it do "--no-merges".

I don't think we need to care about keeping the current behavior of
"--merges --no-merges", do we?

So, no breakage here. Along with it we could issue deprecation notices
in doc and relnotes for "--merges" or just make them less visible. That
is, it would serve the same purpose as Junio's tri-state option.

In 2.0 or so, we could change "--merges" to be an alias for
"--merges-also" rather than "--merges-only" (but don't have to).

>From the ui perspective I'm somehow not a big fan of tri-state options
but can't give hard reasons why; maybe because they force you to use
option arguments. Also, the only reasonable name for a tri-state option
would be "--merges". Can a tri-state have a default? Then we could
default "--merges" to the current behavior, i.e. "--merges=only"
(assuming we have "--merges=only/also/no").

Junio: Sorry for making you explain "D" more rather than doing it myself
upfront.

Michael

Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> From the ui perspective I'm somehow not a big fan of tri-state options
> but can't give hard reasons why; maybe because they force you to use
> option arguments.

But in this particular case, you cannot really hack it with two options
that appear independent on surface but in reality are not. Logically, it
is an enum <everything, non-merges, merges-only> and you can choose only
one of them, and it is even worse from the UI perspective to use
combination of two not-quite-independent options.

Also I have a hidden agenda to add "because we could" --show=octopus to
the enum later perhaps only to my private edition ;-)

Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 18.03.2011 09:22:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> From the ui perspective I'm somehow not a big fan of tri-state options
>> but can't give hard reasons why; maybe because they force you to use
>> option arguments.
> 
> But in this particular case, you cannot really hack it with two options
> that appear independent on surface but in reality are not. Logically, it
> is an enum <everything, non-merges, merges-only> and you can choose only
> one of them, and it is even worse from the UI perspective to use
> combination of two not-quite-independent options.
> 
> Also I have a hidden agenda to add "because we could" --show=octopus to
> the enum later perhaps only to my private edition ;-)

Now you're selling me that (tri++)-state!

git config alias.calamari 'git log --merges=octopus'

Surprisingly few call sites deal with the corresponding rev.foo so that
even those could be enumified... Stay tuned :)

Michael

Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only

[Jeff King]

On Fri, Mar 18, 2011 at 01:22:20AM -0700, Junio C Hamano wrote:

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
> > From the ui perspective I'm somehow not a big fan of tri-state options
> > but can't give hard reasons why; maybe because they force you to use
> > option arguments.
> 
> But in this particular case, you cannot really hack it with two options
> that appear independent on surface but in reality are not. Logically, it
> is an enum <everything, non-merges, merges-only> and you can choose only
> one of them, and it is even worse from the UI perspective to use
> combination of two not-quite-independent options.
> 
> Also I have a hidden agenda to add "because we could" --show=octopus to
> the enum later perhaps only to my private edition ;-)

Yes, I read your other response to Michael and I think that makes the
most sense. In the most general form it is "--show=<num_parents>",
though I don't think there is really much point in values besides 0, 1,
2, or >2 (or combinations thereof, though again, probably only "this
particular value" or "any" is of interest).

So what you have said so far covers that, except for "--show=roots",
which I think would be a useful addition.

The over-engineer in me wants to have the general form
"--grep-by-parent-count=0,1" and "--show=no-merges" as a human-friendly
alias to that. But I can't think of a good reason to have the general
form, except for the sake of generalizing.

-Peff

[PATCH 0/3] rev-list and friends: --min-parents, --max-parents

[Michael J Gruber]

As opposed to the RFD it replaces, this is a real patch series with
documentation and tests, and it even comes with boiler plate. It should
make all of Jeff's and Junio's dreams come true (as far as revision
limiting by parent number goes).

1/3 introduces the new options (and has a proper commit message)
2/3 I noted along the way and could be applied earlier
3/3 depends on 1 and 2 and is the candy (doc, tests, completion)

*** BLURB HERE ***

Michael J Gruber (3):
  revision.c: introduce --min-parents and --max-parents
  t6009: use test_commit() from test-lib.sh
  rev-list --min-parents,--max-parents: doc and test and completion

 Documentation/git-rev-list.txt         |    2 +
 Documentation/rev-list-options.txt     |   13 +++++
 builtin/log.c                          |    2 +-
 builtin/rev-list.c                     |    2 +
 builtin/rev-parse.c                    |    2 +
 contrib/completion/git-completion.bash |    1 +
 revision.c                             |   23 ++++++---
 revision.h                             |    9 +++-
 t/t6009-rev-list-parent.sh             |   85 +++++++++++++++++++++++++++-----
 9 files changed, 117 insertions(+), 22 deletions(-)

-- 
1.7.4.1.464.gf81ff

[PATCH 1/3] revision.c: introduce --min-parents and --max-parents

[Michael J Gruber]

--merges and --no-merges are named confusingly and cannot be overridden
by each other, but they are there to stay for plumbers' sake.

Introduce --min-parents and --max-parents which take values 0,...,7 and
limit the revisions to those commits which have at least resp. at most
that many commits, where --max-parents=8 denotes --max-parents=infinity
(i.e. no upper limit). In fact, 7 (or any negative number) does, but 8
is infinity sideways 8-)

In particular:

--max-parents=1: no merges
--min-parents=2: merges only
--max-parents=0: only roots
--min-parents=3: only octopusses

--min-parents=n --max-parents=m with n>m gives you what you ask for
(nothing) just like --merges --no-merges does, but at least for an
obvious reason.

Implementation notes:

* We compute the number of parents only when we limit by that, so there
  is no performance impact when there are no limiters.

* This uses 6 bits in struct rev_info (rather than the previous 2). We
  need at least 4 to distinguish the 5 cases "all", "merges only", "no
  merges", "roots", "octopusses". 4 would get us to infinity == 3.
  I don't think two int's are warranted - they would also spoil the 8.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/log.c       |    2 +-
 builtin/rev-list.c  |    2 ++
 builtin/rev-parse.c |    2 ++
 revision.c          |   23 ++++++++++++++++-------
 revision.h          |    9 +++++++--
 5 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 796e9e5..9ffccc3 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1061,7 +1061,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.commit_format = CMIT_FMT_EMAIL;
 	rev.verbose_header = 1;
 	rev.diff = 1;
-	rev.no_merges = 1;
+	rev.max_parents = MAX_PARENTS(1);
 	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
 	rev.subject_prefix = fmt_patch_subject_prefix;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index f458cb7..7b276e0 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -16,6 +16,8 @@ static const char rev_list_usage[] =
 "    --min-age=<epoch>\n"
 "    --sparse\n"
 "    --no-merges\n"
+"    --min-parents=<n>\n"
+"    --max-parents=<n>\n"
 "    --remove-empty\n"
 "    --all\n"
 "    --branches\n"
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index a5a1c86..9940546 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -48,6 +48,8 @@ static int is_rev_argument(const char *arg)
 		"--max-count=",
 		"--min-age=",
 		"--no-merges",
+		"--min-parents=",
+		"--max-parents=",
 		"--objects",
 		"--objects-edge",
 		"--parents",
diff --git a/revision.c b/revision.c
index e96c281..6cfd091 100644
--- a/revision.c
+++ b/revision.c
@@ -1277,9 +1277,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--remove-empty")) {
 		revs->remove_empty_trees = 1;
 	} else if (!strcmp(arg, "--merges")) {
-		revs->merges_only = 1;
+		revs->min_parents = MIN_PARENTS(2);
 	} else if (!strcmp(arg, "--no-merges")) {
-		revs->no_merges = 1;
+		revs->max_parents = MAX_PARENTS(1);
+	} else if (!prefixcmp(arg, "--min-parents=")) {
+		revs->min_parents = MIN_PARENTS(atoi(arg+14));
+	} else if (!prefixcmp(arg, "--max-parents=")) {
+		revs->max_parents = MAX_PARENTS(atoi(arg+14));
 	} else if (!strcmp(arg, "--boundary")) {
 		revs->boundary = 1;
 	} else if (!strcmp(arg, "--left-right")) {
@@ -1298,7 +1302,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 			die("--cherry is incompatible with --left-only");
 		revs->cherry_mark = 1;
 		revs->right_only = 1;
-		revs->no_merges = 1;
+		revs->max_parents = MAX_PARENTS(1);
 		revs->limited = 1;
 	} else if (!strcmp(arg, "--count")) {
 		revs->count = 1;
@@ -2029,10 +2033,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi
 		return commit_ignore;
 	if (revs->min_age != -1 && (commit->date > revs->min_age))
 		return commit_ignore;
-	if (revs->no_merges && commit->parents && commit->parents->next)
-		return commit_ignore;
-	if (revs->merges_only && !(commit->parents && commit->parents->next))
-		return commit_ignore;
+	if (revs->min_parents || revs->max_parents) {
+		int n = 0;
+		struct commit_list *p;
+		for (p = commit->parents; p; p = p->next)
+			n++;
+		if ((MIN_PARENTS(n) < revs->min_parents) ||
+		    (MAX_PARENTS(n) < revs->max_parents)) /* max is inv. */
+			return commit_ignore;
+	}
 	if (!commit_match(commit, revs))
 		return commit_ignore;
 	if (revs->prune && revs->dense) {
diff --git a/revision.h b/revision.h
index ae94860..192b2d4 100644
--- a/revision.h
+++ b/revision.h
@@ -20,6 +20,11 @@
 #define DECORATE_SHORT_REFS	1
 #define DECORATE_FULL_REFS	2
 
+/* limit to used range */
+#define MIN_PARENTS(m)	({ unsigned int __n = (m); (__n < 0) ? 0 : (__n > 7) ? 7 : __n; })
+/* invert fox MAX so that default = 0 -> infinity */
+#define MAX_PARENTS(m)	({ unsigned int __n = (m); (__n < 0) ? 7 : (__n > 7) ? 0 : 7 - __n;})
+
 struct rev_info;
 struct log_info;
 struct string_list;
@@ -41,8 +46,8 @@ struct rev_info {
 	/* Traversal flags */
 	unsigned int	dense:1,
 			prune:1,
-			no_merges:1,
-			merges_only:1,
+			min_parents:3,
+			max_parents:3, /* MAX_PARENTS(n) */
 			no_walk:1,
 			show_all:1,
 			remove_empty_trees:1,
-- 
1.7.4.1.464.gf81ff

Re: [PATCH 1/3] revision.c: introduce --min-parents and --max-parents

[Jeff King]

On Fri, Mar 18, 2011 at 03:50:23PM +0100, Michael J Gruber wrote:

> Introduce --min-parents and --max-parents which take values 0,...,7 and
> limit the revisions to those commits which have at least resp. at most
> that many commits, where --max-parents=8 denotes --max-parents=infinity
> (i.e. no upper limit). In fact, 7 (or any negative number) does, but 8
> is infinity sideways 8-)

In practice, I don't think anybody is all that interested in
differentiating octopus merges by their numbers of parents. But the
choice of "7" as a maximum seems kind of arbitrary. There are commits in
linux-2.6 with up to 31 parents (once upon a time we had an arbitrary
limit of 16 parents, but that was lifted in v1.6.0).

You mention below that this fits in three 3 bits in rev_info. I don't
think bits are precious in rev_info, though, as they are in other
revision-related structs. We only have one such struct per walk.

If it were just an implementation issue, I would say fine, we can tweak
it later if somebody really cares. But we are cementing "7" as the value
for infinity in the user-facing interface. Wouldn't a value like "-1" or
"infinity" be more appropriate?

> In particular:
> 
> --max-parents=1: no merges
> --min-parents=2: merges only
> --max-parents=0: only roots
> --min-parents=3: only octopusses
> 
> --min-parents=n --max-parents=m with n>m gives you what you ask for
> (nothing) just like --merges --no-merges does, but at least for an
> obvious reason.

I like this. It's much more natural than the list syntax I suggested
earlier, and it handles ranges in an obvious way. It doesn't allow
selecting, e.g., root commits and merges, but not regular commits. But I
really don't see the point in supporting that.

> @@ -2029,10 +2033,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi
>  		return commit_ignore;
>  	if (revs->min_age != -1 && (commit->date > revs->min_age))
>  		return commit_ignore;
> -	if (revs->no_merges && commit->parents && commit->parents->next)
> -		return commit_ignore;
> -	if (revs->merges_only && !(commit->parents && commit->parents->next))
> -		return commit_ignore;
> +	if (revs->min_parents || revs->max_parents) {
> +		int n = 0;
> +		struct commit_list *p;
> +		for (p = commit->parents; p; p = p->next)
> +			n++;
> +		if ((MIN_PARENTS(n) < revs->min_parents) ||
> +		    (MAX_PARENTS(n) < revs->max_parents)) /* max is inv. */
> +			return commit_ignore;
> +	}

You did the obvious optimization not to count parents if we don't care
about min/max. You could also do something like:

  switch (revs->max_parents) {
  case 0:
          if (commit->parents)
                  return commit_ignore;
          break;
  case 1:
          if (commit->parents && commit->parents->next)
                  return commit_ignore;
          break;
  default:
          if (count_parents(commit) > commit->max_parents)
                  return commit_ignore;
          break;
  }

which more closely matches the original code (you would also need to do
the same for min_parents, and obviously this code ignores the
max-parents-is-inverted thing).

I'm not sure it would buy much in practice, though. Commits with more
than 2 parents are rare, so unnecessarily counting all of them just to
find out something is a merge is probably not going to create a
measurable slowdown.

> +/* limit to used range */
> +#define MIN_PARENTS(m)	({ unsigned int __n = (m); (__n < 0) ? 0 : (__n > 7) ? 7 : __n; })
> +/* invert fox MAX so that default = 0 -> infinity */
> +#define MAX_PARENTS(m)	({ unsigned int __n = (m); (__n < 0) ? 7 : (__n > 7) ? 0 : 7 - __n;})

Hmm. You assign the input to an unsigned int, and then compare it to 0.
Won't that always be false?

The inversion trick is clever to make a bzero'd rev_info work, but I
think the code would be more obvious without it. And you really have to
call init_revisions() anyway, so initializing it to a maximum there (or
-1) would be acceptable (e.g., see how max_age and min_age work).

-Peff

Re: [PATCH 1/3] revision.c: introduce --min-parents and --max-parents

[Michael J Gruber]

Jeff King venit, vidit, dixit 18.03.2011 20:34:
> On Fri, Mar 18, 2011 at 03:50:23PM +0100, Michael J Gruber wrote:
> 
>> Introduce --min-parents and --max-parents which take values 0,...,7 and
>> limit the revisions to those commits which have at least resp. at most
>> that many commits, where --max-parents=8 denotes --max-parents=infinity
>> (i.e. no upper limit). In fact, 7 (or any negative number) does, but 8
>> is infinity sideways 8-)
> 
> In practice, I don't think anybody is all that interested in
> differentiating octopus merges by their numbers of parents. But the
> choice of "7" as a maximum seems kind of arbitrary. There are commits in
> linux-2.6 with up to 31 parents (once upon a time we had an arbitrary
> limit of 16 parents, but that was lifted in v1.6.0).
> 
> You mention below that this fits in three 3 bits in rev_info. I don't
> think bits are precious in rev_info, though, as they are in other
> revision-related structs. We only have one such struct per walk.
> 
> If it were just an implementation issue, I would say fine, we can tweak
> it later if somebody really cares. But we are cementing "7" as the value
> for infinity in the user-facing interface. Wouldn't a value like "-1" or
> "infinity" be more appropriate?

-1 works and "infinity" is difficult to parse.

We could advocate "-1" only, rather than the max which is infinity.

Regarding using an "unbounded" int: I think it is difficult to
distinguish between an unspecified max (value 0 from the initialisation)
and a specified "unbounded" if I don't do the range inversion, which
obviously needs a bounded range. But I'll recheck.

Regarding the 8: Besides the connotation of being infinity sideways,
there's also the undeniable fact that everything with fewer than 8 isn't
really an octo-puss, so the choice of 3 bits is indeed natural!

>> In particular:
>>
>> --max-parents=1: no merges
>> --min-parents=2: merges only
>> --max-parents=0: only roots
>> --min-parents=3: only octopusses
>>
>> --min-parents=n --max-parents=m with n>m gives you what you ask for
>> (nothing) just like --merges --no-merges does, but at least for an
>> obvious reason.
> 
> I like this. It's much more natural than the list syntax I suggested
> earlier, and it handles ranges in an obvious way. It doesn't allow
> selecting, e.g., root commits and merges, but not regular commits. But I
> really don't see the point in supporting that.
> 
>> @@ -2029,10 +2033,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi
>>  		return commit_ignore;
>>  	if (revs->min_age != -1 && (commit->date > revs->min_age))
>>  		return commit_ignore;
>> -	if (revs->no_merges && commit->parents && commit->parents->next)
>> -		return commit_ignore;
>> -	if (revs->merges_only && !(commit->parents && commit->parents->next))
>> -		return commit_ignore;
>> +	if (revs->min_parents || revs->max_parents) {
>> +		int n = 0;
>> +		struct commit_list *p;
>> +		for (p = commit->parents; p; p = p->next)
>> +			n++;
>> +		if ((MIN_PARENTS(n) < revs->min_parents) ||
>> +		    (MAX_PARENTS(n) < revs->max_parents)) /* max is inv. */
>> +			return commit_ignore;
>> +	}
> 
> You did the obvious optimization not to count parents if we don't care
> about min/max. You could also do something like:
> 
>   switch (revs->max_parents) {
>   case 0:
>           if (commit->parents)
>                   return commit_ignore;
>           break;
>   case 1:
>           if (commit->parents && commit->parents->next)
>                   return commit_ignore;
>           break;
>   default:
>           if (count_parents(commit) > commit->max_parents)
>                   return commit_ignore;
>           break;
>   }
> 
> which more closely matches the original code (you would also need to do
> the same for min_parents, and obviously this code ignores the
> max-parents-is-inverted thing).
> 
> I'm not sure it would buy much in practice, though. Commits with more
> than 2 parents are rare, so unnecessarily counting all of them just to
> find out something is a merge is probably not going to create a
> measurable slowdown.

That was exactly my thinking.

> 
>> +/* limit to used range */
>> +#define MIN_PARENTS(m)	({ unsigned int __n = (m); (__n < 0) ? 0 : (__n > 7) ? 7 : __n; })
>> +/* invert fox MAX so that default = 0 -> infinity */
>> +#define MAX_PARENTS(m)	({ unsigned int __n = (m); (__n < 0) ? 7 : (__n > 7) ? 0 : 7 - __n;})
> 
> Hmm. You assign the input to an unsigned int, and then compare it to 0.
> Won't that always be false?

Yes, sorry. I had an int there first, the unsigned gives the "negative =
infinity" automatically.  I probably couldn't decide what
--min-parents=-1 should mean: should "-1" always denote "infinity", or
should it be equivalent to "0"? Switching from signed to unsigned
changed this.

> The inversion trick is clever to make a bzero'd rev_info work, but I
> think the code would be more obvious without it. And you really have to
> call init_revisions() anyway, so initializing it to a maximum there (or
> -1) would be acceptable (e.g., see how max_age and min_age work).

I think for all users of the code it doesn't make a difference since
they would call the macros anyways (for the range check), so it would be
only that single comparison in the walker (which has a comment) that
would become clearer.

Michael

Re: [PATCH 1/3] revision.c: introduce --min-parents and --max-parents

[Jonathan Nieder]

Hi,

Michael J Gruber wrote:

> --max-parents=1: no merges
> --min-parents=2: merges only
> --max-parents=0: only roots
> --min-parents=3: only octopusses

This is growing on me.  Thanks for inventing it.

> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1061,7 +1061,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	rev.commit_format = CMIT_FMT_EMAIL;
>  	rev.verbose_header = 1;
>  	rev.diff = 1;
> -	rev.no_merges = 1;
> +	rev.max_parents = MAX_PARENTS(1);

Is there a reason not to choose a convention for which

	rev.max_parents = 1;

works?

What does --no-merges --merges do?  I would find it most intuitive to
error out (since some people would want the last choice to win and
others want --merges-only --nonmerges-only to select the empty set),
but this patch does the backward-compatible thing, which is to show
zero commits.  Maybe it deserves a test case?

> --- a/revision.c
> +++ b/revision.c
> @@ -1277,9 +1277,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  	} else if (!strcmp(arg, "--remove-empty")) {
>  		revs->remove_empty_trees = 1;
>  	} else if (!strcmp(arg, "--merges")) {
> -		revs->merges_only = 1;
> +		revs->min_parents = MIN_PARENTS(2);

Why not "revs->min_parents = 2;"?

>  	} else if (!strcmp(arg, "--no-merges")) {
> -		revs->no_merges = 1;
> +		revs->max_parents = MAX_PARENTS(1);
> +	} else if (!prefixcmp(arg, "--min-parents=")) {
> +		revs->min_parents = MIN_PARENTS(atoi(arg+14));
> +	} else if (!prefixcmp(arg, "--max-parents=")) {
> +		revs->max_parents = MAX_PARENTS(atoi(arg+14));

It would be nicer to error out for malformed numbers.  That's
a separate topic, though --- you have plenty of company.

> @@ -2029,10 +2033,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi
>  		return commit_ignore;
>  	if (revs->min_age != -1 && (commit->date > revs->min_age))
>  		return commit_ignore;
> -	if (revs->no_merges && commit->parents && commit->parents->next)
> -		return commit_ignore;
> -	if (revs->merges_only && !(commit->parents && commit->parents->next))
> -		return commit_ignore;
> +	if (revs->min_parents || revs->max_parents) {
> +		int n = 0;
> +		struct commit_list *p;
> +		for (p = commit->parents; p; p = p->next)
> +			n++;
> +		if ((MIN_PARENTS(n) < revs->min_parents) ||
> +		    (MAX_PARENTS(n) < revs->max_parents)) /* max is inv. */
> +			return commit_ignore;

Sane.  If we feared enormous parent lists we could do

	for (p = commit->parents; p && n <= 7 - revs->max_parents; p = p->next)
		n++;

but I suspect that's slower.

> --- a/revision.h
> +++ b/revision.h
> @@ -20,6 +20,11 @@
>  #define DECORATE_SHORT_REFS	1
>  #define DECORATE_FULL_REFS	2
>  
> +/* limit to used range */
> +#define MIN_PARENTS(m)	({ unsigned int __n = (m); (__n < 0) ? 0 : (__n > 7) ? 7 : __n; })
> +/* invert fox MAX so that default = 0 -> infinity */
> +#define MAX_PARENTS(m)	({ unsigned int __n = (m); (__n < 0) ? 7 : (__n > 7) ? 0 : 7 - __n;})

Statement expressions don't work in most non-gcc compilers (but
inline functions do).

Hope that helps,
Jonathan

Re: [PATCH 1/3] revision.c: introduce --min-parents and --max-parents

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Is there a reason not to choose a convention for which
>
> 	rev.max_parents = 1;
>
> works?

I also hate the peculiar convention used in this patch.  Is it worth this
ugliness to avoid adding a line in init_revisions() to initialize it to
the infinity value?

Re: [PATCH 1/3] revision.c: introduce --min-parents and --max-parents

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 18.03.2011 21:48:
> Hi,
> 
> Michael J Gruber wrote:
> 
>> --max-parents=1: no merges
>> --min-parents=2: merges only
>> --max-parents=0: only roots
>> --min-parents=3: only octopusses
> 
> This is growing on me.  Thanks for inventing it.

Thanks for teaching me a new collocation (to grow on sb) ;)

> 
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -1061,7 +1061,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	rev.commit_format = CMIT_FMT_EMAIL;
>>  	rev.verbose_header = 1;
>>  	rev.diff = 1;
>> -	rev.no_merges = 1;
>> +	rev.max_parents = MAX_PARENTS(1);
> 
> Is there a reason not to choose a convention for which
> 
> 	rev.max_parents = 1;
> 
> works?

Yes.

Oh, you also want to know what it is? I was somehow fixed on using a
limited number of bits (probably because of no_merges etc. and the
mentioning of tri-state), therefore using a bounded range.

Also, I was keen of having "8" be infinity. (My fingers are trained to
enter "`8" to get "\infty".) But I've come to realise I'm the only one.

> 
> What does --no-merges --merges do?  I would find it most intuitive to
> error out (since some people would want the last choice to win and
> others want --merges-only --nonmerges-only to select the empty set),
> but this patch does the backward-compatible thing, which is to show

Yes, a true case of being "backward(!)-compatible"...

> zero commits.  Maybe it deserves a test case?
> 
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -1277,9 +1277,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>>  	} else if (!strcmp(arg, "--remove-empty")) {
>>  		revs->remove_empty_trees = 1;
>>  	} else if (!strcmp(arg, "--merges")) {
>> -		revs->merges_only = 1;
>> +		revs->min_parents = MIN_PARENTS(2);
> 
> Why not "revs->min_parents = 2;"?

For consistency.

> 
>>  	} else if (!strcmp(arg, "--no-merges")) {
>> -		revs->no_merges = 1;
>> +		revs->max_parents = MAX_PARENTS(1);
>> +	} else if (!prefixcmp(arg, "--min-parents=")) {
>> +		revs->min_parents = MIN_PARENTS(atoi(arg+14));
>> +	} else if (!prefixcmp(arg, "--max-parents=")) {
>> +		revs->max_parents = MAX_PARENTS(atoi(arg+14));
> 
> It would be nicer to error out for malformed numbers.  That's
> a separate topic, though --- you have plenty of company.

min_age etc., yes.

> 
>> @@ -2029,10 +2033,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi
>>  		return commit_ignore;
>>  	if (revs->min_age != -1 && (commit->date > revs->min_age))
>>  		return commit_ignore;
>> -	if (revs->no_merges && commit->parents && commit->parents->next)
>> -		return commit_ignore;
>> -	if (revs->merges_only && !(commit->parents && commit->parents->next))
>> -		return commit_ignore;
>> +	if (revs->min_parents || revs->max_parents) {
>> +		int n = 0;
>> +		struct commit_list *p;
>> +		for (p = commit->parents; p; p = p->next)
>> +			n++;
>> +		if ((MIN_PARENTS(n) < revs->min_parents) ||
>> +		    (MAX_PARENTS(n) < revs->max_parents)) /* max is inv. */
>> +			return commit_ignore;
> 
> Sane.  If we feared enormous parent lists we could do
> 
> 	for (p = commit->parents; p && n <= 7 - revs->max_parents; p = p->next)
> 		n++;
> 
> but I suspect that's slower.
> 
>> --- a/revision.h
>> +++ b/revision.h
>> @@ -20,6 +20,11 @@
>>  #define DECORATE_SHORT_REFS	1
>>  #define DECORATE_FULL_REFS	2
>>  
>> +/* limit to used range */
>> +#define MIN_PARENTS(m)	({ unsigned int __n = (m); (__n < 0) ? 0 : (__n > 7) ? 7 : __n; })
>> +/* invert fox MAX so that default = 0 -> infinity */
>> +#define MAX_PARENTS(m)	({ unsigned int __n = (m); (__n < 0) ? 7 : (__n > 7) ? 0 : 7 - __n;})
> 
> Statement expressions don't work in most non-gcc compilers (but
> inline functions do).
> 
> Hope that helps,
> Jonathan

Yes, that would have helped, although I'm going for simple "unbounded"
int's in v2 without the need for those range enforcing macros/inlines.

Am I allowed to do

unsigned max_parent = -1;

to get "infinity for practical purposes" on all compilers?

Michael

[PATCH 2/3] t6009: use test_commit() from test-lib.sh

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t6009-rev-list-parent.sh |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index 52f7b27..0f0e457 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -4,25 +4,18 @@ test_description='properly cull all ancestors'
 
 . ./test-lib.sh
 
-commit () {
-	test_tick &&
-	echo $1 >file &&
-	git commit -a -m $1 &&
-	git tag $1
-}
-
 test_expect_success setup '
 
 	touch file &&
 	git add file &&
 
-	commit one &&
+	test_commit one &&
 
 	test_tick=$(($test_tick - 2400)) &&
 
-	commit two &&
-	commit three &&
-	commit four &&
+	test_commit two &&
+	test_commit three &&
+	test_commit four &&
 
 	git log --pretty=oneline --abbrev-commit
 '
-- 
1.7.4.1.464.gf81ff

[PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Michael J Gruber]

This also adds test for "--merges" and "--no-merges" which we did not
have so far.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-rev-list.txt         |    2 +
 Documentation/rev-list-options.txt     |   13 ++++++
 contrib/completion/git-completion.bash |    1 +
 t/t6009-rev-list-parent.sh             |   70 +++++++++++++++++++++++++++++++-
 4 files changed, 85 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index b08dfbc..4d2b0c5 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -16,6 +16,8 @@ SYNOPSIS
 	     [ \--sparse ]
 	     [ \--merges ]
 	     [ \--no-merges ]
+	     [ \--min-parents ]
+	     [ \--max-parents ]
 	     [ \--first-parent ]
 	     [ \--remove-empty ]
 	     [ \--full-history ]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 5c6850f..104e644 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -78,6 +78,19 @@ endif::git-rev-list[]
 
 	Do not print commits with more than one parent.
 
+--min-parents::
+--max-parents::
+
+	Show only commits which have at least resp. at most that many
+	commits, where `--max-parents=8` denotes infinity (i.e. no upper
+	limit). In fact, 7 (or any negative number) does, but 8 is
+	infinity sideways 8-)
++
+In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is
+`--merges` (only), `--max-parents=0` gives all root commits and
+`--min-parents=3` all octopusses.
+
+
 --first-parent::
 	Follow only the first parent commit upon seeing a merge
 	commit.  This option can give a better overview when
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3b1cc83..4da087e 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1577,6 +1577,7 @@ __git_log_common_options="
 	--max-count=
 	--max-age= --since= --after=
 	--min-age= --until= --before=
+	--min-parents= --max-parents=
 "
 # Options that go well for log and gitk (not shortlog)
 __git_log_gitk_options="
diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index 0f0e457..792bbe9 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='properly cull all ancestors'
+test_description='ancestor culling and limitting by parent number'
 
 . ./test-lib.sh
 
@@ -28,4 +28,72 @@ test_expect_success 'one is ancestor of others and should not be shown' '
 
 '
 
+test_expect_success 'setup roots, merges and octopusses' '
+
+	git checkout --orphan newroot &&
+	test_commit five &&
+	git checkout -b sidebranch two &&
+	test_commit six &&
+	git checkout -b anotherbranch three &&
+	test_commit seven &&
+	git checkout master &&
+	test_merge normalmerge newroot &&
+	test_tick &&
+	git merge -m octopus sidebranch anotherbranch &&
+	git tag octopus
+'
+
+check_revlist () {
+	> expect &&
+	for c in $2; do echo "$c" >> expect; done &&
+	git rev-list $1 master | git name-rev --name-only --tags --stdin >actual &&
+	test_cmp expect actual
+}
+ 
+test_expect_success 'rev-list roots' '
+
+	check_revlist "--max-parents=0" "five one"
+'
+
+test_expect_success 'rev-list no merges' '
+
+	check_revlist "--max-parents=1" "seven six five four three two one" &&
+	check_revlist "--no-merges" "seven six five four three two one"
+'
+
+test_expect_success 'rev-list no octopusses' '
+
+	check_revlist "--max-parents=2" "normalmerge seven six five four three two one"
+'
+
+test_expect_success 'rev-list no roots' '
+
+	check_revlist "--min-parents=1" "octopus normalmerge seven six four three two"
+'
+
+test_expect_success 'rev-list merges' '
+
+	check_revlist "--min-parents=2" "octopus normalmerge" &&
+	check_revlist "--merges" "octopus normalmerge"
+'
+
+test_expect_success 'rev-list octopus' '
+
+	check_revlist "--min-parents=3" "octopus"
+'
+
+test_expect_success 'rev-list ordinary commits' '
+
+	check_revlist "--min-parents=1 --max-parents=1" "seven six four three two"
+'
+
+test_expect_success 'rev-list override and infinities' '
+
+	check_revlist "--min-parents=2 --no-merges" "" &&
+	check_revlist "--min-parents=2 --no-merges --max-parents=3" "octopus normalmerge" &&
+	check_revlist "--min-parents=2 --no-merges --max-parents=7" "octopus normalmerge" &&
+	check_revlist "--min-parents=2 --no-merges --max-parents=8" "octopus normalmerge" &&
+	check_revlist "--min-parents=2 --no-merges --max-parents=-1" "octopus normalmerge"
+'
+
 test_done
-- 
1.7.4.1.464.gf81ff

Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Jeff King]

On Fri, Mar 18, 2011 at 03:50:25PM +0100, Michael J Gruber wrote:

> +--min-parents::
> +--max-parents::
> +
> +	Show only commits which have at least resp. at most that many
> +	commits, where `--max-parents=8` denotes infinity (i.e. no upper
> +	limit). In fact, 7 (or any negative number) does, but 8 is
> +	infinity sideways 8-)

I didn't quite parse this "resp." in the middle.

> ++
> +In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is
> +`--merges` (only), `--max-parents=0` gives all root commits and
> +`--min-parents=3` all octopusses.

Spelling nit: the plural of octopus is "octopuses" (or "octopi" or
"octopodes", depending on which dictionary you consult).

I think it's good to equate --{no-,}merges with their
--{max,min}-parents counterparts here. We should probably do the same
for the reverse association, like:

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index fcc4c6c..f32509a 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -226,11 +226,13 @@ endif::git-rev-list[]
 
 --merges::
 
-	Print only merge commits.
+	Print only merge commits. This is equivalent to
+	`--min-parents=2`.
 
 --no-merges::
 
-	Do not print commits with more than one parent.
+	Do not print commits with more than one parent. This is
+	equivalent to `--max-parents=1`.
 
 --min-parents::
 --max-parents::

That way it is obvious that "--merges" cancels a previous --min-parents
on the command line (maybe the text should be "this is an alias for..."
to make it clear that doing it is exactly the same).

This provides a user-friendly form for the two common cases. Do we care
about the other cases, or adding a single multi-state flag like
--show-parents={all,merges,etc}? They other ones are rare enough and
particular enough that it is probably fine for people to just use
{min,max}-parents directly. So I would say what you have is good.

-Peff

Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Michael J Gruber]

(Working my way backwards through the responses.)

Jeff King venit, vidit, dixit 18.03.2011 20:48:
> On Fri, Mar 18, 2011 at 03:50:25PM +0100, Michael J Gruber wrote:
> 
>> +--min-parents::
>> +--max-parents::
>> +
>> +	Show only commits which have at least resp. at most that many
>> +	commits, where `--max-parents=8` denotes infinity (i.e. no upper
>> +	limit). In fact, 7 (or any negative number) does, but 8 is
>> +	infinity sideways 8-)
> 
> I didn't quite parse this "resp." in the middle.

Well, there are two options (--min-parents, --max-parents) which we
describe in one paragraph. Sooo...

> 
>> ++
>> +In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is
>> +`--merges` (only), `--max-parents=0` gives all root commits and
>> +`--min-parents=3` all octopusses.
> 
> Spelling nit: the plural of octopus is "octopuses" (or "octopi" or
> "octopodes", depending on which dictionary you consult).
> 
> I think it's good to equate --{no-,}merges with their
> --{max,min}-parents counterparts here. We should probably do the same
> for the reverse association, like:
> 
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index fcc4c6c..f32509a 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -226,11 +226,13 @@ endif::git-rev-list[]
>  
>  --merges::
>  
> -	Print only merge commits.
> +	Print only merge commits. This is equivalent to
> +	`--min-parents=2`.
>  
>  --no-merges::
>  
> -	Do not print commits with more than one parent.
> +	Do not print commits with more than one parent. This is
> +	equivalent to `--max-parents=1`.
>  
>  --min-parents::
>  --max-parents::
> 
> That way it is obvious that "--merges" cancels a previous --min-parents
> on the command line (maybe the text should be "this is an alias for..."
> to make it clear that doing it is exactly the same).

Yes, that is helpful. I have doubts about "alias" for. Without wanting
to sound elitist or something, I have the impression that we start
catering for users who understand "equivalent" more reliably than "alias".

> This provides a user-friendly form for the two common cases. Do we care
> about the other cases, or adding a single multi-state flag like
> --show-parents={all,merges,etc}? They other ones are rare enough and

I know that name is not a serious suggestion...

> particular enough that it is probably fine for people to just use
> {min,max}-parents directly. So I would say what you have is good.

Thanks :)

I would go for something multi-state like only If I were allowed to
stamp on the existing --merges/--no-merges, which I am not. ;)

Michael

Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Jeff King]

On Mon, Mar 21, 2011 at 10:01:53AM +0100, Michael J Gruber wrote:

> >> +	Show only commits which have at least resp. at most that many
> >> +	commits, where `--max-parents=8` denotes infinity (i.e. no upper
> >> +	limit). In fact, 7 (or any negative number) does, but 8 is
> >> +	infinity sideways 8-)
> > 
> > I didn't quite parse this "resp." in the middle.
> 
> Well, there are two options (--min-parents, --max-parents) which we
> describe in one paragraph. Sooo...

I figured out what you were trying to say. I just had never seen the
abbreviation "resp." before. I guessed it meant "respectively", but the
syntax is all wrong.

Digging around via google, I was able to find that it is a mathematical
term with a specific syntax, but one I had never seen before. Maybe I am
just clueless and sheltered, but after 30-odd years of reading English
(12 of which involved reading academic computer science papers!), I
can't help but think it is not all that common and may confuse other
readers. Add on top that it is usually used in parentheses, which helps
make it more obvious what is going on.

I really think "Show only commits which have at least (or at most,
respectively) that many commits" says the same thing, but is way more
accessible.

> > That way it is obvious that "--merges" cancels a previous --min-parents
> > on the command line (maybe the text should be "this is an alias for..."
> > to make it clear that doing it is exactly the same).
> 
> Yes, that is helpful. I have doubts about "alias" for. Without wanting
> to sound elitist or something, I have the impression that we start
> catering for users who understand "equivalent" more reliably than "alias".

I just wanted to make sure people didn't think "equivalent" meant "has a
similar effect to" as opposed to "is exactly as if you did". But reading
it again, I think "equivalent" is fine, and I see you picked it up in
the latest series.

-Peff

Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Michael J Gruber]

Jeff King venit, vidit, dixit 21.03.2011 11:54:
> On Mon, Mar 21, 2011 at 10:01:53AM +0100, Michael J Gruber wrote:
> 
>>>> +	Show only commits which have at least resp. at most that many
>>>> +	commits, where `--max-parents=8` denotes infinity (i.e. no upper
>>>> +	limit). In fact, 7 (or any negative number) does, but 8 is
>>>> +	infinity sideways 8-)
>>>
>>> I didn't quite parse this "resp." in the middle.
>>
>> Well, there are two options (--min-parents, --max-parents) which we
>> describe in one paragraph. Sooo...
> 
> I figured out what you were trying to say. I just had never seen the
> abbreviation "resp." before. I guessed it meant "respectively", but the
> syntax is all wrong.
> 
> Digging around via google, I was able to find that it is a mathematical
> term with a specific syntax, but one I had never seen before. Maybe I am
> just clueless and sheltered, but after 30-odd years of reading English
> (12 of which involved reading academic computer science papers!), I
> can't help but think it is not all that common and may confuse other
> readers. Add on top that it is usually used in parentheses, which helps
> make it more obvious what is going on.

In my community it is very common, which may partly be due to the fact
that there is a strong proportion of non-native speakers. It took it for
granted that it's a standard expression.

> 
> I really think "Show only commits which have at least (or at most,
> respectively) that many commits" says the same thing, but is way more
> accessible.

Sounds good, I'm happy with that. Resend or squash on apply?

> 
>>> That way it is obvious that "--merges" cancels a previous --min-parents
>>> on the command line (maybe the text should be "this is an alias for..."
>>> to make it clear that doing it is exactly the same).
>>
>> Yes, that is helpful. I have doubts about "alias" for. Without wanting
>> to sound elitist or something, I have the impression that we start
>> catering for users who understand "equivalent" more reliably than "alias".
> 
> I just wanted to make sure people didn't think "equivalent" meant "has a
> similar effect to" as opposed to "is exactly as if you did". But reading
> it again, I think "equivalent" is fine, and I see you picked it up in
> the latest series.

I may be wrong about what is common in this case, too. For me, "alias"
is foremost a technical term, and I would guess that many non-native
speaker know "alias" either in the technical sense or not at all, but
not so much in the common English sense. But either way is fine.

Michael

Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Jeff King venit, vidit, dixit 21.03.2011 11:54:
>> On Mon, Mar 21, 2011 at 10:01:53AM +0100, Michael J Gruber wrote:
>> 
> In my community it is very common, which may partly be due to the fact
> that there is a strong proportion of non-native speakers. It took it for
> granted that it's a standard expression.

Perhaps in math circles or something?

>> I really think "Show only commits which have at least (or at most,
>> respectively) that many commits" says the same thing, but is way more
>> accessible.
>
> Sounds good, I'm happy with that. Resend or squash on apply?

The "... A resp. B" composition did made me go "huh?", even though I
managed to guess what you meant; I agree Jeff's rewrite is much more
approacheable.

The code part of the patch needs a bit of touch-up anyway, so let's do a
v2.

It is not like you will have a million copies of "struct rev_info", while
you will be reading from and comparing with the field for each commit
during the traversal, so I'd rather see these max/min stored in the usual
"int", not squashed into bitfields.  Also initialize max to the magic
token that means "unlimited" in init_revisions() without swapping the
meaning of comparison.  One initialization assignment you can omit there
is not worth the resulting confusion.

>>>> That way it is obvious that "--merges" cancels a previous --min-parents
>>>> on the command line (maybe the text should be "this is an alias for..."
>>>> to make it clear that doing it is exactly the same).
>>>
>>> Yes, that is helpful. I have doubts about "alias" for. Without wanting
>>> to sound elitist or something, I have the impression that we start
>>> catering for users who understand "equivalent" more reliably than "alias".
>> 
>> I just wanted to make sure people didn't think "equivalent" meant "has a
>> similar effect to" as opposed to "is exactly as if you did". But reading
>> it again, I think "equivalent" is fine, and I see you picked it up in
>> the latest series.
>
> I may be wrong about what is common in this case, too. For me, "alias"
> is foremost a technical term, and I would guess that many non-native
> speaker know "alias" either in the technical sense or not at all, but
> not so much in the common English sense. But either way is fine.

We would want to make sure the reader understands that saying --no-merges
is _exactly the same_ (your words above) as saying --max-parents=1, so why
not spell that out, i.e. "This is exactly the same as `--max-parents=1`"?

Thanks.

Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 21.03.2011 15:54:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Jeff King venit, vidit, dixit 21.03.2011 11:54:
>>> On Mon, Mar 21, 2011 at 10:01:53AM +0100, Michael J Gruber wrote:
>>>
>> In my community it is very common, which may partly be due to the fact
>> that there is a strong proportion of non-native speakers. It took it for
>> granted that it's a standard expression.
> 
> Perhaps in math circles or something?

Yeah, those (us) weirdos ;)

> 
>>> I really think "Show only commits which have at least (or at most,
>>> respectively) that many commits" says the same thing, but is way more
>>> accessible.
>>
>> Sounds good, I'm happy with that. Resend or squash on apply?
> 
> The "... A resp. B" composition did made me go "huh?", even though I
> managed to guess what you meant; I agree Jeff's rewrite is much more
> approacheable.
> 
> The code part of the patch needs a bit of touch-up anyway, so let's do a
> v2.
> 
> It is not like you will have a million copies of "struct rev_info", while
> you will be reading from and comparing with the field for each commit
> during the traversal, so I'd rather see these max/min stored in the usual
> "int", not squashed into bitfields.  Also initialize max to the magic
> token that means "unlimited" in init_revisions() without swapping the
> meaning of comparison.  One initialization assignment you can omit there
> is not worth the resulting confusion.
> 
>>>>> That way it is obvious that "--merges" cancels a previous --min-parents
>>>>> on the command line (maybe the text should be "this is an alias for..."
>>>>> to make it clear that doing it is exactly the same).
>>>>
>>>> Yes, that is helpful. I have doubts about "alias" for. Without wanting
>>>> to sound elitist or something, I have the impression that we start
>>>> catering for users who understand "equivalent" more reliably than "alias".
>>>
>>> I just wanted to make sure people didn't think "equivalent" meant "has a
>>> similar effect to" as opposed to "is exactly as if you did". But reading
>>> it again, I think "equivalent" is fine, and I see you picked it up in
>>> the latest series.
>>
>> I may be wrong about what is common in this case, too. For me, "alias"
>> is foremost a technical term, and I would guess that many non-native
>> speaker know "alias" either in the technical sense or not at all, but
>> not so much in the common English sense. But either way is fine.
> 
> We would want to make sure the reader understands that saying --no-merges
> is _exactly the same_ (your words above) as saying --max-parents=1, so why
> not spell that out, i.e. "This is exactly the same as `--max-parents=1`"?
> 
> Thanks.

I guess you haven't seen v2 yet, where most of these changes are
incorporated.

For the doc, "exactly the same" is clean and simple and understandable
for both Jeff and myself, I guess ;)

Michael

Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> I guess you haven't seen v2 yet, where most of these changes are
> incorporated.

Yeah, I think that was before you wrote "resend or squash", so my
automatic reaction was to discard everything you sent on the topic before
the timestamp of that email.  Your v2 does seem to be in a very good shape.

> For the doc, "exactly the same" is clean and simple and understandable
> for both Jeff and myself, I guess ;)

Ok, will tweak v2 and apply.

Thanks.

Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Jonathan Nieder]

Hi,

Michael J Gruber wrote:

> [Subject: rev-list --min-parents,--max-parents: doc and test and completion]
>
> This also adds test for "--merges" and "--no-merges" which we did not
> have so far.

Minor nit: the life of reviewers is easier when the documentation and
tests are squashed with the implementation (or if anything, the
documentation comes first) so they can look at what the code tries to
do before seeing how it does it.

> --- a/t/t6009-rev-list-parent.sh
> +++ b/t/t6009-rev-list-parent.sh
> @@ -1,6 +1,6 @@
[...]
> -test_description='properly cull all ancestors'
> +test_description='ancestor culling and limitting by parent number'

sp: limiting

Thanks for updating the description.  It's easy to forget.

> @@ -28,4 +28,72 @@ test_expect_success 'one is ancestor of others and should not be shown' '
[...]
> +check_revlist () {
> +	> expect &&
> +	for c in $2; do echo "$c" >> expect; done &&
> +	git rev-list $1 master | git name-rev --name-only --tags --stdin >actual &&
> +	test_cmp expect actual
> +}

Pedantic nits:

 * if global functions are defined before all test_expect_success stanzas,
    (1) it is easier to reuse them in tests throughout the script;
    (2) it is easy to see at a glance what primitives a test script
        will be using when getting acquainted with it and what
        syntax might be worth lifting to test-lib or other test
        scripts when shopping for that
 * style: no space between >redirection operators and their argument
   for consistency (maybe because > &2 is a syntax error)
 * might be simpler to pass one rev per argument.  Like so:

	rev_list_args=$1 &&
	shift &&
	printf '%s\n' "$@" >expect &&
	git rev-list $rev_list_args master ...

   Avoiding the for loop means errors from 'echo' before the last
   iteration are not ignored; a more verbose way to write the same
   thing is

	for commit
	do
		printf '%s\n' "$commit" ||
		return
	done >expect &&
	git rev-list ...
 * this does not catch errors from rev-list!  how about

	git rev-list $rev_list_args master >rev-list &&
	git name-rev ... <rev-list >actual &&
	test_cmp expect actual

   Junio will probably complain that this is not meant to be a test
   for name-rev; indeed, filling "expect" by computing commit names
   might be even better (because faster).

> +test_expect_success 'rev-list roots' '
> +test_expect_success 'rev-list no merges' '
> +test_expect_success 'rev-list no octopusses' '
> +test_expect_success 'rev-list no roots' '
> +test_expect_success 'rev-list merges' '

Neat. :)


> +test_expect_success 'rev-list octopus' '
> +
> +	check_revlist "--min-parents=3" "octopus"
> +'

Might make sense to check a dodecapus (with --min-parents=3 still) as
well.

> +test_expect_success 'rev-list override and infinities' '

Ah, here's the test I suggested in reply to patch 1.  But this is not
about overriding but the lack thereof, no?  So I'd be happier reading
something like

	test_expect_success '--no-merges --merges yields the empty set' '
		check_revlist "--no-merges --merges" &&
		check_revlist "--min-parents=2 --no-merges"
	'

	test_expect_success 'last --max-parents wins' '
		check_revlist "--min-parents=2 --no-merges --max-parents=3" octopus normalmerge &&
		check_revlist "--min-parents=2 --max-parents=3 --no-merges" ...
	'

	test_expect_success '--max-parents=infinity' '
		...
	'

	test_expect_success '--min-parents=infinity' '
		...
	'

	test_expect_failure '--max-parents=gobbledegood errors out' '
		...
	'

Thanks for your attention to detail, and hope that helps.

Ciao,
Jonathan

Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 18.03.2011 22:14:
> Hi,
> 
> Michael J Gruber wrote:
> 
>> [Subject: rev-list --min-parents,--max-parents: doc and test and completion]
>>
>> This also adds test for "--merges" and "--no-merges" which we did not
>> have so far.
> 
> Minor nit: the life of reviewers is easier when the documentation and
> tests are squashed with the implementation (or if anything, the
> documentation comes first) so they can look at what the code tries to
> do before seeing how it does it.

Come on, with a cover-letter saying doc and tests are in 3/3, how much
effort is it to read that before 1/3 if you care?

The tests are bogus before the code and the doc pointless before it.

Squashing 1 and 3 is okay, of course. For my own digestion, smaller
bites are better.

>> --- a/t/t6009-rev-list-parent.sh
>> +++ b/t/t6009-rev-list-parent.sh
>> @@ -1,6 +1,6 @@
> [...]
>> -test_description='properly cull all ancestors'
>> +test_description='ancestor culling and limitting by parent number'
> 
> sp: limiting
> 
> Thanks for updating the description.  It's easy to forget.

I would have preferred to assign an extra test number (since the
relation between the above is a bit weak) but the numbers are all used
up in that range.

> 
>> @@ -28,4 +28,72 @@ test_expect_success 'one is ancestor of others and should not be shown' '
> [...]
>> +check_revlist () {
>> +	> expect &&
>> +	for c in $2; do echo "$c" >> expect; done &&
>> +	git rev-list $1 master | git name-rev --name-only --tags --stdin >actual &&
>> +	test_cmp expect actual
>> +}
> 
> Pedantic nits:
> 
>  * if global functions are defined before all test_expect_success stanzas,
>     (1) it is easier to reuse them in tests throughout the script;
>     (2) it is easy to see at a glance what primitives a test script
>         will be using when getting acquainted with it and what
>         syntax might be worth lifting to test-lib or other test
>         scripts when shopping for that
>  * style: no space between >redirection operators and their argument
>    for consistency (maybe because > &2 is a syntax error)
>  * might be simpler to pass one rev per argument.  Like so:
> 
> 	rev_list_args=$1 &&
> 	shift &&
> 	printf '%s\n' "$@" >expect &&
> 	git rev-list $rev_list_args master ...
> 
>    Avoiding the for loop means errors from 'echo' before the last
>    iteration are not ignored; a more verbose way to write the same

Do we really need to safe-guard echo and prints?

>    thing is
> 
> 	for commit
> 	do
> 		printf '%s\n' "$commit" ||
> 		return
> 	done >expect &&
> 	git rev-list ...
>  * this does not catch errors from rev-list!  how about
> 
> 	git rev-list $rev_list_args master >rev-list &&
> 	git name-rev ... <rev-list >actual &&
> 	test_cmp expect actual

Thanks for both of the above, that makes things much better. Although I
have to treat the case with empty rev-list specially now, or use the
verbose version.

> 
>    Junio will probably complain that this is not meant to be a test
>    for name-rev; indeed, filling "expect" by computing commit names
>    might be even better (because faster).

Well, we do this in other places as well, and it makes the test much
more readable. Alternatively, I could use rev-parse on the args, which
is faster than name-rev on the rev-list output. That makes it less
readable in case a test breaks, though. Anyways, I'll go for that.

> 
>> +test_expect_success 'rev-list roots' '
>> +test_expect_success 'rev-list no merges' '
>> +test_expect_success 'rev-list no octopusses' '
>> +test_expect_success 'rev-list no roots' '
>> +test_expect_success 'rev-list merges' '
> 
> Neat. :)
> 
> 
>> +test_expect_success 'rev-list octopus' '
>> +
>> +	check_revlist "--min-parents=3" "octopus"
>> +'
> 
> Might make sense to check a dodecapus (with --min-parents=3 still) as
> well.

Dodeka, really? I leave that to you.
I might add a tetrapus, though.

> 
>> +test_expect_success 'rev-list override and infinities' '
> 
> Ah, here's the test I suggested in reply to patch 1.  But this is not
> about overriding but the lack thereof, no?  So I'd be happier reading
> something like
> 
> 	test_expect_success '--no-merges --merges yields the empty set' '
> 		check_revlist "--no-merges --merges" &&
> 		check_revlist "--min-parents=2 --no-merges"
> 	'
> 
> 	test_expect_success 'last --max-parents wins' '

wins against? Against itself, i.e. it overrides previous occurences. But
I'll separate these.

> 		check_revlist "--min-parents=2 --no-merges --max-parents=3" octopus normalmerge &&
> 		check_revlist "--min-parents=2 --max-parents=3 --no-merges" ...
> 	'
> 
> 	test_expect_success '--max-parents=infinity' '
> 		...
> 	'
> 
> 	test_expect_success '--min-parents=infinity' '
> 		...
> 	'
> 
> 	test_expect_failure '--max-parents=gobbledegood errors out' '
> 		...
> 	'

I don't really want to parse for the string "infinity" nor go for strtol
instead of atoi. Why shouldn't something "unparseable" be 0?

Michael

Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Jonathan Nieder]

Michael J Gruber wrote:

> Come on, with a cover-letter saying doc and tests are in 3/3, how much
> effort is it to read that before 1/3 if you care?

Nonzero.  Now multiply that by the number of people who are going to
look at the history over the years.

> The tests are bogus before the code and the doc pointless before it.
>
> Squashing 1 and 3 is okay, of course. For my own digestion, smaller
> bites are better.

The reason I mentioned the possibility of docs and tests before
implementation is that that can be a good way to get feedback on the
design of something when the implementation is not even ready yet.
Which doesn't apply hear, of course.

Anyway, I don't care too much about this, but I wanted to make the
convention clear (if I have understood it correctly, at least).

>>    Avoiding the for loop means errors from 'echo' before the last
>>    iteration are not ignored; a more verbose way to write the same
>
> Do we really need to safe-guard echo and prints?

No; sorry about that.  I get worried about for loops because they
have a tendency to start using git commands later, but I shouldn't
have mentioned it.

> Thanks for both of the above, that makes things much better. Although I
> have to treat the case with empty rev-list specially now, or use the
> verbose version.

Sorry I missed it; good catch.  I don't recommend

	printf ${1+'%s\n' "$@"}

> Dodeka, really? I leave that to you.
> I might add a tetrapus, though.

I meant "greater than eight", since it seemed like a test that in
some alternate universe could fail.  I'll write a test on top.

(Dodecapus comes from [1].)
http://thread.gmane.org/gmane.comp.version-control.git/15486 )I mentioned dodecapus

> wins against? Against itself, i.e. it overrides previous occurences. But
> I'll separate these.

Thanks.  The details weren't important.

>> 	test_expect_failure '--max-parents=gobbledegood errors out' '
>> 		...
>> 	'
>
> I don't really want to parse for the string "infinity" nor go for strtol
> instead of atoi. Why shouldn't something "unparseable" be 0?

Three reasons:

 1) why shouldn't it be -1 or 27?  When in doubt, it's best to let
    the operator know so she can specify what is wanted unambiguously.
 2) to catch typos on the commandline and bugs in scripts
 3) less importantly, erroring out makes it less likely someone is
    going to rely on it (since we didn't point out their typo) and
    thus makes future extensions easier

Thanks again.  Now to look at v2. :)
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/15486

Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 21.03.2011 18:49:
> Michael J Gruber wrote:
> 
>> Come on, with a cover-letter saying doc and tests are in 3/3, how much
>> effort is it to read that before 1/3 if you care?
> 
> Nonzero.  Now multiply that by the number of people who are going to
> look at the history over the years.
> 
>> The tests are bogus before the code and the doc pointless before it.
>>
>> Squashing 1 and 3 is okay, of course. For my own digestion, smaller
>> bites are better.
> 
> The reason I mentioned the possibility of docs and tests before
> implementation is that that can be a good way to get feedback on the
> design of something when the implementation is not even ready yet.
> Which doesn't apply hear, of course.
> 
> Anyway, I don't care too much about this, but I wanted to make the
> convention clear (if I have understood it correctly, at least).

Maybe we should clarify this. No, I don't mean by taking it outside ;)

If there's a different convention I'll try and follow that, of course.
I'm very used to seeing "1/2 implementation" and "2/2 doc and tests",
the other way round you would introduce failing tests (for new
features). So I took that as a convention. You could always squash
these, of course (just like my 2 and 3 in v2).

I actually have to keep myself from feeling annoyed when I see
documentation patches without any implementation, because a patch
creates the impression that something has been done, but without any
attempt at implementation it's hard to tell whether it's doable at all.
This does not apply to pure RFD/design questions, of course.

Cheers,
Michael

Re: [PATCH 0/3] rev-list and friends: --min-parents, --max-parents

[Michael J Gruber]

Michael J Gruber venit, vidit, dixit 18.03.2011 15:50:
> As opposed to the RFD it replaces, this is a real patch series with
> documentation and tests, and it even comes with boiler plate. It should
> make all of Jeff's and Junio's dreams come true (as far as revision
> limiting by parent number goes).
> 
> 1/3 introduces the new options (and has a proper commit message)
> 2/3 I noted along the way and could be applied earlier
> 3/3 depends on 1 and 2 and is the candy (doc, tests, completion)
> 
> *** BLURB HERE ***

Hmpf. I guess this should say "NO MORE BLURB HERE". Gosh!

This comes from filling in the cover letter by using

:r! git notes show ref:HEAD

in vim and partially failing to delete the place holders. Longing for
that format-patch option "--note-cover".

> 
> Michael J Gruber (3):
>   revision.c: introduce --min-parents and --max-parents
>   t6009: use test_commit() from test-lib.sh
>   rev-list --min-parents,--max-parents: doc and test and completion
> 
>  Documentation/git-rev-list.txt         |    2 +
>  Documentation/rev-list-options.txt     |   13 +++++
>  builtin/log.c                          |    2 +-
>  builtin/rev-list.c                     |    2 +
>  builtin/rev-parse.c                    |    2 +
>  contrib/completion/git-completion.bash |    1 +
>  revision.c                             |   23 ++++++---
>  revision.h                             |    9 +++-
>  t/t6009-rev-list-parent.sh             |   85 +++++++++++++++++++++++++++-----
>  9 files changed, 117 insertions(+), 22 deletions(-)
> 

Re: [PATCH 0/3] rev-list and friends: --min-parents, --max-parents

[Jeff King]

On Fri, Mar 18, 2011 at 03:50:22PM +0100, Michael J Gruber wrote:

> As opposed to the RFD it replaces, this is a real patch series with
> documentation and tests, and it even comes with boiler plate. It should
> make all of Jeff's and Junio's dreams come true (as far as revision
> limiting by parent number goes).
> 
> 1/3 introduces the new options (and has a proper commit message)
> 2/3 I noted along the way and could be applied earlier
> 3/3 depends on 1 and 2 and is the candy (doc, tests, completion)

I didn't see it mentioned anywhere, but I assume this applies on
mg/rev-list-one-side-only (b1b4755). It has conflicts when applied on
master.

(I know you probably know that, but it is for the benefit of other
reviewers).

-Peff

Re: [PATCH 0/3] rev-list and friends: --min-parents, --max-parents

[Michael J Gruber]

Jeff King venit, vidit, dixit 18.03.2011 20:41:
> On Fri, Mar 18, 2011 at 03:50:22PM +0100, Michael J Gruber wrote:
> 
>> As opposed to the RFD it replaces, this is a real patch series with
>> documentation and tests, and it even comes with boiler plate. It should
>> make all of Jeff's and Junio's dreams come true (as far as revision
>> limiting by parent number goes).
>>
>> 1/3 introduces the new options (and has a proper commit message)
>> 2/3 I noted along the way and could be applied earlier
>> 3/3 depends on 1 and 2 and is the candy (doc, tests, completion)
> 
> I didn't see it mentioned anywhere, but I assume this applies on
> mg/rev-list-one-side-only (b1b4755). It has conflicts when applied on
> master.
> 
> (I know you probably know that, but it is for the benefit of other
> reviewers).

Well, yes, but that's in next already (and probably in master by the
time this is taking shape). If I had it based on something earlier it
would have applied cleanly but failed to compile which is fine for
something meant to go to maint but a nuisance for pu material.

Michael

[PATCHv2 0/3] rev-list and friends: --min-parents, --max-parents

[Michael J Gruber]

In v2, based on the input from Jeff, Junio and Jonathan, I switched to
ordinary signed integers for --min-parent and --max-parent. Negative
parameters to the latter mean infinity. (Thus, no more range/macro magic.)

Also, I optimized the tests and made the structure clearer (hopefully).

Lastly, I rearranged the tests so that the unrelated cleanup comes first.

Based on mg/rev-list-one-side-only (in next) to save Junio a build conflict
resolution.
Michael J Gruber (3):
  t6009: use test_commit() from test-lib.sh
  revision.c: introduce --min-parents and --max-parents
  rev-list --min-parents,--max-parents: doc and test and completion

 Documentation/git-rev-list.txt         |    2 +
 Documentation/rev-list-options.txt     |   17 +++++-
 builtin/log.c                          |    2 +-
 builtin/rev-list.c                     |    2 +
 builtin/rev-parse.c                    |    2 +
 contrib/completion/git-completion.bash |    1 +
 revision.c                             |   24 ++++++--
 revision.h                             |    4 +-
 t/t6009-rev-list-parent.sh             |   97 ++++++++++++++++++++++++++++----
 9 files changed, 128 insertions(+), 23 deletions(-)

-- 
1.7.4.1.511.g72e46

[PATCHv2 1/3] t6009: use test_commit() from test-lib.sh

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t6009-rev-list-parent.sh |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index 52f7b27..0f0e457 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -4,25 +4,18 @@ test_description='properly cull all ancestors'
 
 . ./test-lib.sh
 
-commit () {
-	test_tick &&
-	echo $1 >file &&
-	git commit -a -m $1 &&
-	git tag $1
-}
-
 test_expect_success setup '
 
 	touch file &&
 	git add file &&
 
-	commit one &&
+	test_commit one &&
 
 	test_tick=$(($test_tick - 2400)) &&
 
-	commit two &&
-	commit three &&
-	commit four &&
+	test_commit two &&
+	test_commit three &&
+	test_commit four &&
 
 	git log --pretty=oneline --abbrev-commit
 '
-- 
1.7.4.1.511.g72e46

[PATCHv2 2/3] revision.c: introduce --min-parents and --max-parents

[Michael J Gruber]

--merges and --no-merges are named confusingly and cannot be overridden
by each other, but they are there to stay for plumbers' sake.

Introduce --min-parents and --max-parents which limit the revisions to
those commits which have at least resp. at most that many commits, where
negative arguments for --max-parents= denote infinity (i.e. no upper
limit).

In particular:

--max-parents=1: no merges
--min-parents=2: merges only
--max-parents=0: only roots
--min-parents=3: only octopusses

--min-parents=n --max-parents=m with n>m gives you what you ask for
(nothing) just like --merges --no-merges does, but at least for an
obvious reason.

Implementation note:

We compute the number of parents only when we limit by that, so there
is no performance impact when there are no limiters.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/log.c       |    2 +-
 builtin/rev-list.c  |    2 ++
 builtin/rev-parse.c |    2 ++
 revision.c          |   24 +++++++++++++++++-------
 revision.h          |    4 ++--
 5 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 796e9e5..4a0f78d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1061,7 +1061,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.commit_format = CMIT_FMT_EMAIL;
 	rev.verbose_header = 1;
 	rev.diff = 1;
-	rev.no_merges = 1;
+	rev.max_parents = 1;
 	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
 	rev.subject_prefix = fmt_patch_subject_prefix;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index f458cb7..7b276e0 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -16,6 +16,8 @@ static const char rev_list_usage[] =
 "    --min-age=<epoch>\n"
 "    --sparse\n"
 "    --no-merges\n"
+"    --min-parents=<n>\n"
+"    --max-parents=<n>\n"
 "    --remove-empty\n"
 "    --all\n"
 "    --branches\n"
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index a5a1c86..9940546 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -48,6 +48,8 @@ static int is_rev_argument(const char *arg)
 		"--max-count=",
 		"--min-age=",
 		"--no-merges",
+		"--min-parents=",
+		"--max-parents=",
 		"--objects",
 		"--objects-edge",
 		"--parents",
diff --git a/revision.c b/revision.c
index e96c281..8d48870 100644
--- a/revision.c
+++ b/revision.c
@@ -941,6 +941,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->min_age = -1;
 	revs->skip_count = -1;
 	revs->max_count = -1;
+	revs->max_parents = -1;
 
 	revs->commit_format = CMIT_FMT_DEFAULT;
 
@@ -1277,9 +1278,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--remove-empty")) {
 		revs->remove_empty_trees = 1;
 	} else if (!strcmp(arg, "--merges")) {
-		revs->merges_only = 1;
+		revs->min_parents = 2;
 	} else if (!strcmp(arg, "--no-merges")) {
-		revs->no_merges = 1;
+		revs->max_parents = 1;
+	} else if (!prefixcmp(arg, "--min-parents=")) {
+		revs->min_parents = atoi(arg+14);
+	} else if (!prefixcmp(arg, "--max-parents=")) {
+		revs->max_parents = atoi(arg+14);
 	} else if (!strcmp(arg, "--boundary")) {
 		revs->boundary = 1;
 	} else if (!strcmp(arg, "--left-right")) {
@@ -1298,7 +1303,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 			die("--cherry is incompatible with --left-only");
 		revs->cherry_mark = 1;
 		revs->right_only = 1;
-		revs->no_merges = 1;
+		revs->max_parents = 1;
 		revs->limited = 1;
 	} else if (!strcmp(arg, "--count")) {
 		revs->count = 1;
@@ -2029,10 +2034,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi
 		return commit_ignore;
 	if (revs->min_age != -1 && (commit->date > revs->min_age))
 		return commit_ignore;
-	if (revs->no_merges && commit->parents && commit->parents->next)
-		return commit_ignore;
-	if (revs->merges_only && !(commit->parents && commit->parents->next))
-		return commit_ignore;
+	if (revs->min_parents || (revs->max_parents >= 0)) {
+		int n = 0;
+		struct commit_list *p;
+		for (p = commit->parents; p; p = p->next)
+			n++;
+		if ((n < revs->min_parents) ||
+		    ((revs->max_parents >= 0) && (n > revs->max_parents)))
+			return commit_ignore;
+	}
 	if (!commit_match(commit, revs))
 		return commit_ignore;
 	if (revs->prune && revs->dense) {
diff --git a/revision.h b/revision.h
index ae94860..9fd8f30 100644
--- a/revision.h
+++ b/revision.h
@@ -41,8 +41,6 @@ struct rev_info {
 	/* Traversal flags */
 	unsigned int	dense:1,
 			prune:1,
-			no_merges:1,
-			merges_only:1,
 			no_walk:1,
 			show_all:1,
 			remove_empty_trees:1,
@@ -126,6 +124,8 @@ struct rev_info {
 	int max_count;
 	unsigned long max_age;
 	unsigned long min_age;
+	int min_parents;
+	int max_parents;
 
 	/* diff info for patches and for paths limiting */
 	struct diff_options diffopt;
-- 
1.7.4.1.511.g72e46

Re: [PATCHv2 2/3] revision.c: introduce --min-parents and --max-parents

[Michael J Gruber]

Michael J Gruber venit, vidit, dixit 21.03.2011 11:14:
> --merges and --no-merges are named confusingly and cannot be overridden
> by each other, but they are there to stay for plumbers' sake.
> 
> Introduce --min-parents and --max-parents which limit the revisions to
> those commits which have at least resp. at most that many commits, where
> negative arguments for --max-parents= denote infinity (i.e. no upper
> limit).
> 
> In particular:
> 
> --max-parents=1: no merges
> --min-parents=2: merges only
> --max-parents=0: only roots
> --min-parents=3: only octopusses
> 
> --min-parents=n --max-parents=m with n>m gives you what you ask for
> (nothing) just like --merges --no-merges does, but at least for an
> obvious reason.
> 
> Implementation note:
> 
> We compute the number of parents only when we limit by that, so there
> is no performance impact when there are no limiters.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  builtin/log.c       |    2 +-
>  builtin/rev-list.c  |    2 ++
>  builtin/rev-parse.c |    2 ++
>  revision.c          |   24 +++++++++++++++++-------
>  revision.h          |    4 ++--
>  5 files changed, 24 insertions(+), 10 deletions(-)
...
> @@ -2029,10 +2034,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi
>  		return commit_ignore;
>  	if (revs->min_age != -1 && (commit->date > revs->min_age))
>  		return commit_ignore;
> -	if (revs->no_merges && commit->parents && commit->parents->next)
> -		return commit_ignore;
> -	if (revs->merges_only && !(commit->parents && commit->parents->next))
> -		return commit_ignore;
> +	if (revs->min_parents || (revs->max_parents >= 0)) {
> +		int n = 0;
> +		struct commit_list *p;
> +		for (p = commit->parents; p; p = p->next)
> +			n++;
> +		if ((n < revs->min_parents) ||
> +		    ((revs->max_parents >= 0) && (n > revs->max_parents)))
> +			return commit_ignore;
> +	}
>  	if (!commit_match(commit, revs))
>  		return commit_ignore;
>  	if (revs->prune && revs->dense) {

BTW: Version v2-eps had this conditional:

(n < revs->min_parents) || (n > (unsigned int) revs->max_parents)

I actually compared runs of "git rev-list origin/next >/dev/null"
without options and with "--merges" or "--no-merges" for git without
this patch and with it (v2) and do not see any measurable difference
(between without or with the patch; the runs with limiting options are
faster, so I redid it with --count, again no difference with or without
patch).

So, the clearer version above should be preferred over the unsigned
trick, and special casing seems not worth it.

Michael

Re: [PATCHv2 2/3] revision.c: introduce --min-parents and --max-parents

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Implementation note:
>
> We compute the number of parents only when we limit by that, so there
> is no performance impact when there are no limiters.

Only about 16% of the commits in git.git (and 7% in the kernel) are two
parent merges and the rest are mostly single strand of pearls, so even the
cost of counting all the parents in a loop that does not exit early when
it reaches max-parents limit should be negligible, and checking against
max-parents in each iteration would probably costs even more.

So I think this is a good to go.

Thanks.

Re: [PATCHv2 2/3] revision.c: introduce --min-parents and --max-parents

[Jonathan Nieder]

Michael J Gruber wrote:

> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> builtin/log.c       |    2 +-
> builtin/rev-list.c  |    2 ++
> builtin/rev-parse.c |    2 ++
> revision.c          |   24 +++++++++++++++++-------
> revision.h          |    4 ++--
> 5 files changed, 24 insertions(+), 10 deletions(-)

For what it's worth, the patch looks obviously good to me, too.

[PATCHv2 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Michael J Gruber]

This also adds test for "--merges" and "--no-merges" which we did not
have so far.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-rev-list.txt         |    2 +
 Documentation/rev-list-options.txt     |   17 ++++++-
 contrib/completion/git-completion.bash |    1 +
 t/t6009-rev-list-parent.sh             |   84 +++++++++++++++++++++++++++++++-
 4 files changed, 101 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index b08dfbc..4d2b0c5 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -16,6 +16,8 @@ SYNOPSIS
 	     [ \--sparse ]
 	     [ \--merges ]
 	     [ \--no-merges ]
+	     [ \--min-parents ]
+	     [ \--max-parents ]
 	     [ \--first-parent ]
 	     [ \--remove-empty ]
 	     [ \--full-history ]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 5c6850f..18d5534 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -72,11 +72,24 @@ endif::git-rev-list[]
 
 --merges::
 
-	Print only merge commits.
+	Print only merge commits. This is equivalent to `--min-parents=2`.
 
 --no-merges::
 
-	Do not print commits with more than one parent.
+	Do not print commits with more than one parent. This is
+	equivalent to `--max-parents=1`.
+
+--min-parents::
+--max-parents::
+
+	Show only commits which have at least resp. at most that many
+	commits, where negative parameters for `--max-parents=` denote
+	infinity (i.e. no upper	limit).
++
+In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is
+`--merges` (only), `--max-parents=0` gives all root commits and
+`--min-parents=3` all octopuses.
+
 
 --first-parent::
 	Follow only the first parent commit upon seeing a merge
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3b1cc83..4da087e 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1577,6 +1577,7 @@ __git_log_common_options="
 	--max-count=
 	--max-age= --since= --after=
 	--min-age= --until= --before=
+	--min-parents= --max-parents=
 "
 # Options that go well for log and gitk (not shortlog)
 __git_log_gitk_options="
diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index 0f0e457..d460369 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -1,9 +1,17 @@
 #!/bin/sh
 
-test_description='properly cull all ancestors'
+test_description='ancestor culling and limiting by parent number'
 
 . ./test-lib.sh
 
+check_revlist () {
+	rev_list_args="$1" &&
+	shift &&
+	git rev-parse "$@" >expect &&
+	git rev-list $rev_list_args --all >actual &&
+	test_cmp expect actual
+}
+ 
 test_expect_success setup '
 
 	touch file &&
@@ -28,4 +36,78 @@ test_expect_success 'one is ancestor of others and should not be shown' '
 
 '
 
+test_expect_success 'setup roots, merges and octopuses' '
+
+	git checkout --orphan newroot &&
+	test_commit five &&
+	git checkout -b sidebranch two &&
+	test_commit six &&
+	git checkout -b anotherbranch three &&
+	test_commit seven &&
+	git checkout -b yetanotherbranch four &&
+	test_commit eight &&
+	git checkout master &&
+	test_merge normalmerge newroot &&
+	test_tick &&
+	git merge -m tripus sidebranch anotherbranch &&
+	git tag tripus &&
+	git checkout -b tetrabranch normalmerge &&
+	test_tick &&
+	git merge -m tetrapus sidebranch anotherbranch yetanotherbranch &&
+	git tag tetrapus &&
+	git checkout master 
+'
+
+test_expect_success 'rev-list roots' '
+
+	check_revlist "--max-parents=0" one five
+'
+
+test_expect_success 'rev-list no merges' '
+
+	check_revlist "--max-parents=1" one eight seven six five four three two &&
+	check_revlist "--no-merges" one eight seven six five four three two
+'
+
+test_expect_success 'rev-list no octopuses' '
+
+	check_revlist "--max-parents=2" one normalmerge eight seven six five four three two
+'
+
+test_expect_success 'rev-list no roots' '
+
+	check_revlist "--min-parents=1" tetrapus tripus normalmerge eight seven six four three two
+'
+
+test_expect_success 'rev-list merges' '
+
+	check_revlist "--min-parents=2" tetrapus tripus normalmerge &&
+	check_revlist "--merges" tetrapus tripus normalmerge
+'
+
+test_expect_success 'rev-list octopus' '
+
+	check_revlist "--min-parents=3" tetrapus tripus
+'
+
+test_expect_success 'rev-list ordinary commits' '
+
+	check_revlist "--min-parents=1 --max-parents=1" eight seven six four three two
+'
+
+test_expect_success 'rev-list --merges --no-merges yields empty set' '
+
+	check_revlist "--min-parents=2 --no-merges" &&
+	check_revlist "--merges --no-merges" &&
+	check_revlist "--no-merges --merges"
+'
+
+test_expect_success 'rev-list override and infinities' '
+
+	check_revlist "--min-parents=2 --max-parents=1 --max-parents=3" tripus normalmerge &&
+	check_revlist "--min-parents=1 --min-parents=2 --max-parents=7" tetrapus tripus normalmerge &&
+	check_revlist "--min-parents=2 --max-parents=8" tetrapus tripus normalmerge &&
+	check_revlist "--min-parents=2 --max-parents=-1" tetrapus tripus normalmerge
+'
+
 test_done
-- 
1.7.4.1.511.g72e46

Re: [PATCHv2 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Jonathan Nieder]

Michael J Gruber wrote:

> Based on mg/rev-list-one-side-only (in next) to save Junio a build conflict
> resolution.

Not a serious problem, but I wish it hadn't been.  What particular
functionality from that branch does this use?

Ah, now that I check it seems that that is to change a use of
no_merges in the implementation of --cherry to use the new API?  Makes
sense (and good catch).  With that hunk skipped, the patches apply to
master.

> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -72,11 +72,24 @@ endif::git-rev-list[]
>  
>  --merges::
>  
> -	Print only merge commits.
> +	Print only merge commits. This is equivalent to `--min-parents=2`.
>  
>  --no-merges::
>  
> -	Do not print commits with more than one parent.
> +	Do not print commits with more than one parent. This is
> +	equivalent to `--max-parents=1`.
> +
> +--min-parents::
> +--max-parents::
> +
> +	Show only commits which have at least resp. at most that many

ENOPARSE.  I guess parentheses around "resp. at most" would work as
a minimal fix, but it might be better to say:

 --min-parents=<n>::
	Show only commits which have at least <n> parents.

 --max-parents=<n>::
	Show only commits which have at least <n> parents.

and perhaps to put

 git log --max-parents=0::
	Lists all root commits.

 git log --min-parents=3::
	Lists all octopus merges.

under EXAMPLES.

> +	commits, where negative parameters for `--max-parents=` denote
> +	infinity (i.e. no upper	limit).

Seems hackish.  Maybe --no-max-parents could denote infinity?

> ++
> +In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is
> +`--merges` (only), `--max-parents=0` gives all root commits and
> +`--min-parents=3` all octopuses.
> +
>  
>  --first-parent::

It seems there is an extra newline here.

> --- a/t/t6009-rev-list-parent.sh
> +++ b/t/t6009-rev-list-parent.sh
> @@ -1,9 +1,17 @@
>  #!/bin/sh
>  
> -test_description='properly cull all ancestors'
> +test_description='ancestor culling and limiting by parent number'
>  
>  . ./test-lib.sh
>  
> +check_revlist () {
> +	rev_list_args="$1" &&
> +	shift &&
> +	git rev-parse "$@" >expect &&
> +	git rev-list $rev_list_args --all >actual &&
> +	test_cmp expect actual
> +}
> + 

"git am" warns about trailing whitespace on the line after the closing
brace (nothing that --whitespace=fix can't fix, though).

Thanks for factoring this out btw.  It makes the tests themselves
very easy to read.

> +test_expect_success 'rev-list override and infinities' '
> +
> +	check_revlist "--min-parents=2 --max-parents=1 --max-parents=3" tripus normalmerge &&
> +	check_revlist "--min-parents=1 --min-parents=2 --max-parents=7" tetrapus tripus normalmerge &&
> +	check_revlist "--min-parents=2 --max-parents=8" tetrapus tripus normalmerge &&
> +	check_revlist "--min-parents=2 --max-parents=-1" tetrapus tripus normalmerge
> +'

7 and 8 don't mean infinity any more, do they?  What is this test
checking?

Re: [PATCHv2 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 21.03.2011 19:45:
> Michael J Gruber wrote:
> 
>> Based on mg/rev-list-one-side-only (in next) to save Junio a build conflict
>> resolution.
> 
> Not a serious problem, but I wish it hadn't been.  What particular

But why? Basing it on something earlier would have served no purpose
(that I know of) at all.

I should have mentioned the dependency in v1.

> functionality from that branch does this use?
> 
> Ah, now that I check it seems that that is to change a use of
> no_merges in the implementation of --cherry to use the new API?  Makes
> sense (and good catch).  With that hunk skipped, the patches apply to
> master.

Yes, revs->no_merges and revs->merges are gone, so a series based on
master would produce a compile failure when applied to next.

> 
>> --- a/Documentation/rev-list-options.txt
>> +++ b/Documentation/rev-list-options.txt
>> @@ -72,11 +72,24 @@ endif::git-rev-list[]
>>  
>>  --merges::
>>  
>> -	Print only merge commits.
>> +	Print only merge commits. This is equivalent to `--min-parents=2`.
>>  
>>  --no-merges::
>>  
>> -	Do not print commits with more than one parent.
>> +	Do not print commits with more than one parent. This is
>> +	equivalent to `--max-parents=1`.
>> +
>> +--min-parents::
>> +--max-parents::
>> +
>> +	Show only commits which have at least resp. at most that many
> 
> ENOPARSE.  I guess parentheses around "resp. at most" would work as
> a minimal fix, but it might be better to say:
> 
>  --min-parents=<n>::
> 	Show only commits which have at least <n> parents.
> 
>  --max-parents=<n>::
> 	Show only commits which have at least <n> parents.
> 
> and perhaps to put
> 
>  git log --max-parents=0::
> 	Lists all root commits.
> 
>  git log --min-parents=3::
> 	Lists all octopus merges.
> 
> under EXAMPLES.
> 

Well, we discussed this under the v1 thread (after v2 was sent). Junio,
should I do a v3 with that or this?

>> +	commits, where negative parameters for `--max-parents=` denote
>> +	infinity (i.e. no upper	limit).
> 
> Seems hackish.  Maybe --no-max-parents could denote infinity?

For me, "-1" is a quite natural way to reset a count type parameter, and
you don't even have to think "unsigned" or "mod max_int" for that.

There is no problem parsing for "--max-parents=infinity" and/or
"--no-max-parents" or even (better?) "--max-parents=" without number,
it's only a matter of bike shedding decisions.

>> ++
>> +In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is
>> +`--merges` (only), `--max-parents=0` gives all root commits and
>> +`--min-parents=3` all octopuses.
>> +
>>  
>>  --first-parent::
> 
> It seems there is an extra newline here.
> 
>> --- a/t/t6009-rev-list-parent.sh
>> +++ b/t/t6009-rev-list-parent.sh
>> @@ -1,9 +1,17 @@
>>  #!/bin/sh
>>  
>> -test_description='properly cull all ancestors'
>> +test_description='ancestor culling and limiting by parent number'
>>  
>>  . ./test-lib.sh
>>  
>> +check_revlist () {
>> +	rev_list_args="$1" &&
>> +	shift &&
>> +	git rev-parse "$@" >expect &&
>> +	git rev-list $rev_list_args --all >actual &&
>> +	test_cmp expect actual
>> +}
>> + 
> 
> "git am" warns about trailing whitespace on the line after the closing
> brace (nothing that --whitespace=fix can't fix, though).

Hmmm, are there whitespace issues which am warns about and diff does
not, or have I missed a warning?

> 
> Thanks for factoring this out btw.  It makes the tests themselves
> very easy to read.
> 
>> +test_expect_success 'rev-list override and infinities' '
>> +
>> +	check_revlist "--min-parents=2 --max-parents=1 --max-parents=3" tripus normalmerge &&
>> +	check_revlist "--min-parents=1 --min-parents=2 --max-parents=7" tetrapus tripus normalmerge &&
>> +	check_revlist "--min-parents=2 --max-parents=8" tetrapus tripus normalmerge &&
>> +	check_revlist "--min-parents=2 --max-parents=-1" tetrapus tripus normalmerge
>> +'
> 
> 7 and 8 don't mean infinity any more, do they?  What is this test
> checking?

The test checks "override and infinities", where the plural indicates
the fact that it tests different ways of spelling (practical) infinity
such as the very suggestive "8" which nobody cares about but me.

Michael

Re: [PATCHv2 3/3] rev-list --min-parents,--max-parents: doc and test and completion

[Jonathan Nieder]

Michael J Gruber wrote:
> Jonathan Nieder venit, vidit, dixit 21.03.2011 19:45:
>> Michael J Gruber wrote:

>>> Based on mg/rev-list-one-side-only (in next) to save Junio a build conflict
>>> resolution.
>>
>> Not a serious problem, but I wish it hadn't been.  What particular
>
> But why? Basing it on something earlier would have served no purpose
> (that I know of) at all.
>
> I should have mentioned the dependency in v1.

*nod*  Sorry, I was just confused before --- I hadn't understood why
you were doing it.  Giving a heads up like you did about interaction
between topics is indeed very useful.

To answer your question about why: basing it on master would mean
that Junio could merge it to master without merging
mg/rev-list-one-side-only first.  Similarly, I had wanted to test
against master because master is what I use day-to-day so applying
it there would avoid confusing unrelated changes.

>> Seems hackish.  Maybe --no-max-parents could denote infinity?
>
> For me, "-1" is a quite natural way to reset a count type parameter

Natural to me in code, but not on the command line.  I can write a
separate patch for --no-max-parents if you'd like.

> There is no problem parsing for "--max-parents=infinity" and/or
> "--no-max-parents" or even (better?) "--max-parents=" without number,
> it's only a matter of bike shedding decisions.

Well, if it doesn't matter what color it is, I guess I shouldn't have
mentioned it then?

> Hmmm, are there whitespace issues which am warns about and diff does
> not, or have I missed a warning?

Yep, diff doesn't warn about anything without --check.

>>> +test_expect_success 'rev-list override and infinities' '
>>> +
>>> +	check_revlist "--min-parents=2 --max-parents=1 --max-parents=3" tripus normalmerge &&
>>> +	check_revlist "--min-parents=1 --min-parents=2 --max-parents=7" tetrapus tripus normalmerge &&
>>> +	check_revlist "--min-parents=2 --max-parents=8" tetrapus tripus normalmerge &&
>>> +	check_revlist "--min-parents=2 --max-parents=-1" tetrapus tripus normalmerge
>>> +'
>> 
>> 7 and 8 don't mean infinity any more, do they?  What is this test
>> checking?
>
> The test checks "override and infinities", where the plural indicates
> the fact that it tests different ways of spelling (practical) infinity
> such as the very suggestive "8" which nobody cares about but me.

Ah, so that's what you meant.  git.git has very few octopus merges,
so even "4" is a practical infinity there (well, technically there's
an early 6-parent commit).

Regards,
Jonathan

Re: [PATCHv2 0/3] rev-list and friends: --min-parents, --max-parents

[Jeff King]

On Mon, Mar 21, 2011 at 11:14:04AM +0100, Michael J Gruber wrote:

> In v2, based on the input from Jeff, Junio and Jonathan, I switched to
> ordinary signed integers for --min-parent and --max-parent. Negative
> parameters to the latter mean infinity. (Thus, no more range/macro magic.)

FWIW, I like this version much better. Besides my "resp." nit, I have no
complaints.

-Peff

[PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents

[Michael J Gruber]

Compared to what is currently in pu (which is v2+eps), v3 has:

1/3 -> 1/5 unchanged

2/3 -> 2/5 unchanged

3/5 is !squash for 2/5 and introduces --no-min-parents and --no-max-parents
as natural ways to reset the limits

3/3 -> 4/5 with a fix to the notation in documentation (spell out =<number>)
and an additional dodecapus test

5/5 is !fixup for 4/5 and adds test, doc and completion for --no-min-parents
and --no-max-parents

Junio, please let me/us know whether sending an amended series in this way
(which I've seen before) is actually convenient for you or not. !squash
commits require a message edit, for example. OTOH, I don't know any (other?)
good inter diff solution.

Thanks for everyone's input which went into this (Junio, Jeff, Jonathan).

Michael J Gruber (5):
  t6009: use test_commit() from test-lib.sh
  revision.c: introduce --min-parents and --max-parents
  squash! revision.c: introduce --min-parents and --max-parents
  rev-list --min-parents,--max-parents: doc, test and completion
  fixup! rev-list --min-parents,--max-parents: doc, test and completion

 Documentation/git-rev-list.txt         |    4 +
 Documentation/rev-list-options.txt     |   19 +++++-
 builtin/log.c                          |    2 +-
 builtin/rev-list.c                     |    4 +
 builtin/rev-parse.c                    |    4 +
 contrib/completion/git-completion.bash |    2 +
 revision.c                             |   28 ++++++--
 revision.h                             |    4 +-
 t/t6009-rev-list-parent.sh             |  118 +++++++++++++++++++++++++++++---
 9 files changed, 163 insertions(+), 22 deletions(-)

-- 
1.7.4.1.511.g72e46

[PATCHv3 1/5] t6009: use test_commit() from test-lib.sh

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t6009-rev-list-parent.sh |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index 52f7b27..0f0e457 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -4,25 +4,18 @@ test_description='properly cull all ancestors'
 
 . ./test-lib.sh
 
-commit () {
-	test_tick &&
-	echo $1 >file &&
-	git commit -a -m $1 &&
-	git tag $1
-}
-
 test_expect_success setup '
 
 	touch file &&
 	git add file &&
 
-	commit one &&
+	test_commit one &&
 
 	test_tick=$(($test_tick - 2400)) &&
 
-	commit two &&
-	commit three &&
-	commit four &&
+	test_commit two &&
+	test_commit three &&
+	test_commit four &&
 
 	git log --pretty=oneline --abbrev-commit
 '
-- 
1.7.4.1.511.g72e46

[PATCHv3 2/5] revision.c: introduce --min-parents and --max-parents

[Michael J Gruber]

--merges and --no-merges are named confusingly and cannot be overridden
by each other, but they are there to stay for plumbers' sake.

Introduce --min-parents and --max-parents which limit the revisions to
those commits which have at least resp. at most that many commits, where
negative arguments for --max-parents= denote infinity (i.e. no upper
limit).

In particular:

--max-parents=1: no merges
--min-parents=2: merges only
--max-parents=0: only roots
--min-parents=3: only octopusses

--min-parents=n --max-parents=m with n>m gives you what you ask for
(nothing) just like --merges --no-merges does, but at least for an
obvious reason.

Implementation note:

We compute the number of parents only when we limit by that, so there
is no performance impact when there are no limiters.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/log.c       |    2 +-
 builtin/rev-list.c  |    2 ++
 builtin/rev-parse.c |    2 ++
 revision.c          |   24 +++++++++++++++++-------
 revision.h          |    4 ++--
 5 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 796e9e5..4a0f78d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1061,7 +1061,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.commit_format = CMIT_FMT_EMAIL;
 	rev.verbose_header = 1;
 	rev.diff = 1;
-	rev.no_merges = 1;
+	rev.max_parents = 1;
 	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
 	rev.subject_prefix = fmt_patch_subject_prefix;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index f458cb7..7b276e0 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -16,6 +16,8 @@ static const char rev_list_usage[] =
 "    --min-age=<epoch>\n"
 "    --sparse\n"
 "    --no-merges\n"
+"    --min-parents=<n>\n"
+"    --max-parents=<n>\n"
 "    --remove-empty\n"
 "    --all\n"
 "    --branches\n"
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index a5a1c86..9940546 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -48,6 +48,8 @@ static int is_rev_argument(const char *arg)
 		"--max-count=",
 		"--min-age=",
 		"--no-merges",
+		"--min-parents=",
+		"--max-parents=",
 		"--objects",
 		"--objects-edge",
 		"--parents",
diff --git a/revision.c b/revision.c
index e96c281..8d48870 100644
--- a/revision.c
+++ b/revision.c
@@ -941,6 +941,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->min_age = -1;
 	revs->skip_count = -1;
 	revs->max_count = -1;
+	revs->max_parents = -1;
 
 	revs->commit_format = CMIT_FMT_DEFAULT;
 
@@ -1277,9 +1278,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--remove-empty")) {
 		revs->remove_empty_trees = 1;
 	} else if (!strcmp(arg, "--merges")) {
-		revs->merges_only = 1;
+		revs->min_parents = 2;
 	} else if (!strcmp(arg, "--no-merges")) {
-		revs->no_merges = 1;
+		revs->max_parents = 1;
+	} else if (!prefixcmp(arg, "--min-parents=")) {
+		revs->min_parents = atoi(arg+14);
+	} else if (!prefixcmp(arg, "--max-parents=")) {
+		revs->max_parents = atoi(arg+14);
 	} else if (!strcmp(arg, "--boundary")) {
 		revs->boundary = 1;
 	} else if (!strcmp(arg, "--left-right")) {
@@ -1298,7 +1303,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 			die("--cherry is incompatible with --left-only");
 		revs->cherry_mark = 1;
 		revs->right_only = 1;
-		revs->no_merges = 1;
+		revs->max_parents = 1;
 		revs->limited = 1;
 	} else if (!strcmp(arg, "--count")) {
 		revs->count = 1;
@@ -2029,10 +2034,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi
 		return commit_ignore;
 	if (revs->min_age != -1 && (commit->date > revs->min_age))
 		return commit_ignore;
-	if (revs->no_merges && commit->parents && commit->parents->next)
-		return commit_ignore;
-	if (revs->merges_only && !(commit->parents && commit->parents->next))
-		return commit_ignore;
+	if (revs->min_parents || (revs->max_parents >= 0)) {
+		int n = 0;
+		struct commit_list *p;
+		for (p = commit->parents; p; p = p->next)
+			n++;
+		if ((n < revs->min_parents) ||
+		    ((revs->max_parents >= 0) && (n > revs->max_parents)))
+			return commit_ignore;
+	}
 	if (!commit_match(commit, revs))
 		return commit_ignore;
 	if (revs->prune && revs->dense) {
diff --git a/revision.h b/revision.h
index ae94860..9fd8f30 100644
--- a/revision.h
+++ b/revision.h
@@ -41,8 +41,6 @@ struct rev_info {
 	/* Traversal flags */
 	unsigned int	dense:1,
 			prune:1,
-			no_merges:1,
-			merges_only:1,
 			no_walk:1,
 			show_all:1,
 			remove_empty_trees:1,
@@ -126,6 +124,8 @@ struct rev_info {
 	int max_count;
 	unsigned long max_age;
 	unsigned long min_age;
+	int min_parents;
+	int max_parents;
 
 	/* diff info for patches and for paths limiting */
 	struct diff_options diffopt;
-- 
1.7.4.1.511.g72e46

[PATCHv3 3/5] squash! revision.c: introduce --min-parents and --max-parents

[Michael J Gruber]

Also, introduce --no-min-parents and --no-max-parents to do the obvious
thing for convenience.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/rev-list.c  |    2 ++
 builtin/rev-parse.c |    2 ++
 revision.c          |    4 ++++
 3 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 7b276e0..9bfb942 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -17,7 +17,9 @@ static const char rev_list_usage[] =
 "    --sparse\n"
 "    --no-merges\n"
 "    --min-parents=<n>\n"
+"    --no-min-parents\n"
 "    --max-parents=<n>\n"
+"    --no-max-parents\n"
 "    --remove-empty\n"
 "    --all\n"
 "    --branches\n"
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 9940546..adb1cae 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -49,7 +49,9 @@ static int is_rev_argument(const char *arg)
 		"--min-age=",
 		"--no-merges",
 		"--min-parents=",
+		"--no-min-parents",
 		"--max-parents=",
+		"--no-max-parents",
 		"--objects",
 		"--objects-edge",
 		"--parents",
diff --git a/revision.c b/revision.c
index 8d48870..0f38364 100644
--- a/revision.c
+++ b/revision.c
@@ -1283,8 +1283,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->max_parents = 1;
 	} else if (!prefixcmp(arg, "--min-parents=")) {
 		revs->min_parents = atoi(arg+14);
+	} else if (!prefixcmp(arg, "--no-min-parents")) {
+		revs->min_parents = 0;
 	} else if (!prefixcmp(arg, "--max-parents=")) {
 		revs->max_parents = atoi(arg+14);
+	} else if (!prefixcmp(arg, "--no-max-parents")) {
+		revs->max_parents = -1;
 	} else if (!strcmp(arg, "--boundary")) {
 		revs->boundary = 1;
 	} else if (!strcmp(arg, "--left-right")) {
-- 
1.7.4.1.511.g72e46

[PATCHv3 4/5] rev-list --min-parents,--max-parents: doc, test and completion

[Michael J Gruber]

This also adds test for "--merges" and "--no-merges" which we did not
have so far.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-rev-list.txt         |    2 +
 Documentation/rev-list-options.txt     |   16 ++++-
 contrib/completion/git-completion.bash |    1 +
 t/t6009-rev-list-parent.sh             |  105 +++++++++++++++++++++++++++++++-
 4 files changed, 121 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index b08dfbc..c5ea96f 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -16,6 +16,8 @@ SYNOPSIS
 	     [ \--sparse ]
 	     [ \--merges ]
 	     [ \--no-merges ]
+	     [ \--min-parents=<number> ]
+	     [ \--max-parents=<number> ]
 	     [ \--first-parent ]
 	     [ \--remove-empty ]
 	     [ \--full-history ]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 5c6850f..0bbf7da 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -72,11 +72,23 @@ endif::git-rev-list[]
 
 --merges::
 
-	Print only merge commits.
+	Print only merge commits. This is exactly the same as `--min-parents=2`.
 
 --no-merges::
 
-	Do not print commits with more than one parent.
+	Do not print commits with more than one parent. This is
+	exactly the same as `--max-parents=1`.
+
+--min-parents=<number>::
+--max-parents=<number>::
+
+	Show only commits which have at least (or at most) that many
+	commits, where negative parameters for `--max-parents=` denote
+	infinity (i.e. no upper	limit).
++
+In particular, `--max-parents=1` is the same as `--no-merges`,
+`--min-parents=2` is the same as `--merges`.  `--max-parents=0`
+gives all root commits and `--min-parents=3` all octopus merges.
 
 --first-parent::
 	Follow only the first parent commit upon seeing a merge
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3b1cc83..4da087e 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1577,6 +1577,7 @@ __git_log_common_options="
 	--max-count=
 	--max-age= --since= --after=
 	--min-age= --until= --before=
+	--min-parents= --max-parents=
 "
 # Options that go well for log and gitk (not shortlog)
 __git_log_gitk_options="
diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index 0f0e457..5309378 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -1,9 +1,17 @@
 #!/bin/sh
 
-test_description='properly cull all ancestors'
+test_description='ancestor culling and limiting by parent number'
 
 . ./test-lib.sh
 
+check_revlist () {
+	rev_list_args="$1" &&
+	shift &&
+	git rev-parse "$@" >expect &&
+	git rev-list $rev_list_args --all >actual &&
+	test_cmp expect actual
+}
+
 test_expect_success setup '
 
 	touch file &&
@@ -28,4 +36,99 @@ test_expect_success 'one is ancestor of others and should not be shown' '
 
 '
 
+test_expect_success 'setup roots, merges and octopuses' '
+
+	git checkout --orphan newroot &&
+	test_commit five &&
+	git checkout -b sidebranch two &&
+	test_commit six &&
+	git checkout -b anotherbranch three &&
+	test_commit seven &&
+	git checkout -b yetanotherbranch four &&
+	test_commit eight &&
+	git checkout master &&
+	test_merge normalmerge newroot &&
+	test_tick &&
+	git merge -m tripus sidebranch anotherbranch &&
+	git tag tripus &&
+	git checkout -b tetrabranch normalmerge &&
+	test_tick &&
+	git merge -m tetrapus sidebranch anotherbranch yetanotherbranch &&
+	git tag tetrapus &&
+	git checkout master
+'
+
+test_expect_success 'rev-list roots' '
+
+	check_revlist "--max-parents=0" one five
+'
+
+test_expect_success 'rev-list no merges' '
+
+	check_revlist "--max-parents=1" one eight seven six five four three two &&
+	check_revlist "--no-merges" one eight seven six five four three two
+'
+
+test_expect_success 'rev-list no octopuses' '
+
+	check_revlist "--max-parents=2" one normalmerge eight seven six five four three two
+'
+
+test_expect_success 'rev-list no roots' '
+
+	check_revlist "--min-parents=1" tetrapus tripus normalmerge eight seven six four three two
+'
+
+test_expect_success 'rev-list merges' '
+
+	check_revlist "--min-parents=2" tetrapus tripus normalmerge &&
+	check_revlist "--merges" tetrapus tripus normalmerge
+'
+
+test_expect_success 'rev-list octopus' '
+
+	check_revlist "--min-parents=3" tetrapus tripus
+'
+
+test_expect_success 'rev-list ordinary commits' '
+
+	check_revlist "--min-parents=1 --max-parents=1" eight seven six four three two
+'
+
+test_expect_success 'rev-list --merges --no-merges yields empty set' '
+
+	check_revlist "--min-parents=2 --no-merges" &&
+	check_revlist "--merges --no-merges" &&
+	check_revlist "--no-merges --merges"
+'
+
+test_expect_success 'rev-list override and infinities' '
+
+	check_revlist "--min-parents=2 --max-parents=1 --max-parents=3" tripus normalmerge &&
+	check_revlist "--min-parents=1 --min-parents=2 --max-parents=7" tetrapus tripus normalmerge &&
+	check_revlist "--min-parents=2 --max-parents=8" tetrapus tripus normalmerge &&
+	check_revlist "--min-parents=2 --max-parents=-1" tetrapus tripus normalmerge
+'
+
+test_expect_success 'set up dodecapus' '
+
+	for i in 1 2 3 4 5 6 7 8 9 10 11
+	do
+		git checkout -b root$i five || return
+		test_commit $i || return
+	done &&
+	git checkout master &&
+	test_tick &&
+	git merge -m dodecapus root{1,2,3,4,5,6,7,8,9,10,11} &&
+	git tag dodecapus
+'
+
+test_expect_success 'test with dodecapus' '
+
+	check_revlist "--min-parents=4" dodecapus tetrapus &&
+	check_revlist "--min-parents=8" dodecapus &&
+	check_revlist "--min-parents=12" dodecapus &&
+	check_revlist "--min-parents=13" &&
+	check_revlist "--min-parents=4 --max-parents=11" tetrapus
+'
 test_done
-- 
1.7.4.1.511.g72e46

[PATCHv3 5/5] fixup! rev-list --min-parents,--max-parents: doc, test and completion

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-rev-list.txt         |    2 ++
 Documentation/rev-list-options.txt     |   13 ++++++++-----
 contrib/completion/git-completion.bash |    1 +
 t/t6009-rev-list-parent.sh             |    4 +++-
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index c5ea96f..415f4f0 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -17,7 +17,9 @@ SYNOPSIS
 	     [ \--merges ]
 	     [ \--no-merges ]
 	     [ \--min-parents=<number> ]
+	     [ \--no-min-parents ]
 	     [ \--max-parents=<number> ]
+	     [ \--no-max-parents ]
 	     [ \--first-parent ]
 	     [ \--remove-empty ]
 	     [ \--full-history ]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 0bbf7da..ea5c6c4 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -81,14 +81,17 @@ endif::git-rev-list[]
 
 --min-parents=<number>::
 --max-parents=<number>::
+--no-min-parents::
+--no-max-parents::
 
 	Show only commits which have at least (or at most) that many
-	commits, where negative parameters for `--max-parents=` denote
-	infinity (i.e. no upper	limit).
+	commits. In particular, `--max-parents=1` is the same as `--no-merges`,
+	`--min-parents=2` is the same as `--merges`.  `--max-parents=0`
+	gives all root commits and `--min-parents=3` all octopus merges.
 +
-In particular, `--max-parents=1` is the same as `--no-merges`,
-`--min-parents=2` is the same as `--merges`.  `--max-parents=0`
-gives all root commits and `--min-parents=3` all octopus merges.
+`--no-min-parents` and `--no-max-parents` reset these limits (to no limit)
+again.  Equivalent forms are `--min-parents=0` (any commit has 0 or more
+parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 
 --first-parent::
 	Follow only the first parent commit upon seeing a merge
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4da087e..d5215e8 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1578,6 +1578,7 @@ __git_log_common_options="
 	--max-age= --since= --after=
 	--min-age= --until= --before=
 	--min-parents= --max-parents=
+	--no-min-parents --no-max-parents
 "
 # Options that go well for log and gitk (not shortlog)
 __git_log_gitk_options="
diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index 5309378..fc89d6d 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -107,7 +107,9 @@ test_expect_success 'rev-list override and infinities' '
 	check_revlist "--min-parents=2 --max-parents=1 --max-parents=3" tripus normalmerge &&
 	check_revlist "--min-parents=1 --min-parents=2 --max-parents=7" tetrapus tripus normalmerge &&
 	check_revlist "--min-parents=2 --max-parents=8" tetrapus tripus normalmerge &&
-	check_revlist "--min-parents=2 --max-parents=-1" tetrapus tripus normalmerge
+	check_revlist "--min-parents=2 --max-parents=-1" tetrapus tripus normalmerge &&
+	check_revlist "--min-parents=2 --no-max-parents" tetrapus tripus normalmerge &&
+	check_revlist "--max-parents=0 --min-parents=1 --no-min-parents" one five
 '
 
 test_expect_success 'set up dodecapus' '
-- 
1.7.4.1.511.g72e46

Re: [PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents

[Jeff King]

On Wed, Mar 23, 2011 at 10:38:47AM +0100, Michael J Gruber wrote:

> Compared to what is currently in pu (which is v2+eps), v3 has:

This one looks good to me (I like the --no-{min,max}-parents).

> 1/3 -> 1/5 unchanged
> 
> 2/3 -> 2/5 unchanged
> 
> 3/5 is !squash for 2/5 and introduces --no-min-parents and --no-max-parents
> as natural ways to reset the limits
> 
> 3/3 -> 4/5 with a fix to the notation in documentation (spell out =<number>)
> and an additional dodecapus test
> 
> 5/5 is !fixup for 4/5 and adds test, doc and completion for --no-min-parents
> and --no-max-parents
> 
> Junio, please let me/us know whether sending an amended series in this way
> (which I've seen before) is actually convenient for you or not. !squash
> commits require a message edit, for example. OTOH, I don't know any (other?)
> good inter diff solution.

As a reviewer, I find it is usually most convenient to just have the
submitter do the squash, and then write below the "---" (or in a cover
letter like this) a brief explanation of the differences.

Yeah, I end up reading the whole patch again, including bits I already
read, but that is probably a good thing. A good patch is hopefully not
too long, and it is easier (to me, anyway) to review it as a whole and
not as an interdiff.

Technically speaking, what you posted gives more information, and I
could always apply and squash myself to get the final form. But I'm lazy
and I tend to review straight from the mail reader or from $EDITOR while
replying. So to me it's just an extra step.

I have occasionally seen people with long patches reply to the patch
themselves and say "btw, it is hard to see what is changed here in v2,
so here is a much more readable interdiff from v1". And that can be
useful at times, but it's nice to have the patch author using their
judgement about when it will be useful or not.

Just my two cents.

> Thanks for everyone's input which went into this (Junio, Jeff, Jonathan).

Thank you for taking the lead on writing it. :)

-Peff

Re: [PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Junio, please let me/us know whether sending an amended series in this way
>> (which I've seen before) is actually convenient for you or not. !squash
>> commits require a message edit, for example. OTOH, I don't know any (other?)
>> good inter diff solution.
>
> As a reviewer, I find it is usually most convenient to just have the
> submitter do the squash, and then write below the "---" (or in a cover
> letter like this) a brief explanation of the differences.
>
> Yeah, I end up reading the whole patch again, including bits I already
> read, but that is probably a good thing. A good patch is hopefully not
> too long, and it is easier (to me, anyway) to review it as a whole and
> not as an interdiff.

Yes.  And that would also make it easier for people who didn't read the
earlier round to double check.

Re: [PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Compared to what is currently in pu (which is v2+eps), v3 has:
>
> 1/3 -> 1/5 unchanged
>
> 2/3 -> 2/5 unchanged
>
> 3/5 is !squash for 2/5 and introduces --no-min-parents and --no-max-parents
> as natural ways to reset the limits
>
> 3/3 -> 4/5 with a fix to the notation in documentation (spell out =<number>)
> and an additional dodecapus test
>
> 5/5 is !fixup for 4/5 and adds test, doc and completion for --no-min-parents
> and --no-max-parents

Looked good; will replace what is in 'pu' and advance the topic to 'next'.

Re: [PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents

[Jonathan Nieder]

Michael J Gruber wrote:

> Compared to what is currently in pu (which is v2+eps), v3 has:

Thanks.  I reviewed this by looking at what Junio queued in pu; it
looks very good.  With the following patch, it passes tests.

The use of "return" was surprising.  It seems this style has been
intended to work ever since v0.99.5~24^2~4 (Trapping exit in tests,
using return for errors, 2005-08-10).

It interacts poorly with test_when_finished but since these tests do
not use that function, they should be safe.  test_when_finished could
use some fixes to avoid future surprises but that's another story.

-- 8< --
Subject: tests: avoid nonportable {foo,bar} glob

Unlike bash and ksh, dash and busybox ash do not support brace
expansion (as in 'echo {hello,world}').  So when dash is sh,
t6009.13 (set up dodecapus) ends up pass a string beginning with
"root{1,2," to "git merge" verbatim and the test fails.

Fix it by introducing a variable to hold the list of parents for
the dodecapus and populating it in a more low-tech way.

While at it, simplify a little by combining this setup code with the
test it sets up for.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t6009-rev-list-parent.sh |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index fc89d6d..3050740 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -112,20 +112,20 @@ test_expect_success 'rev-list override and infinities' '
 	check_revlist "--max-parents=0 --min-parents=1 --no-min-parents" one five
 '
 
-test_expect_success 'set up dodecapus' '
+test_expect_success 'dodecapus' '
 
+	roots= &&
 	for i in 1 2 3 4 5 6 7 8 9 10 11
 	do
-		git checkout -b root$i five || return
-		test_commit $i || return
+		git checkout -b root$i five &&
+		test_commit $i &&
+		roots="$roots root$i" ||
+		return
 	done &&
 	git checkout master &&
 	test_tick &&
-	git merge -m dodecapus root{1,2,3,4,5,6,7,8,9,10,11} &&
-	git tag dodecapus
-'
-
-test_expect_success 'test with dodecapus' '
+	git merge -m dodecapus $roots &&
+	git tag dodecapus &&
 
 	check_revlist "--min-parents=4" dodecapus tetrapus &&
 	check_revlist "--min-parents=8" dodecapus &&
-- 
1.7.4.1

Re: [PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 24.03.2011 09:21:
> Michael J Gruber wrote:
> 
>> Compared to what is currently in pu (which is v2+eps), v3 has:
> 
> Thanks.  I reviewed this by looking at what Junio queued in pu; it

OK, no more !fixup and !squash next time ;)

> looks very good.  With the following patch, it passes tests.

Uhm, it does so even without (for me) for bash and ksh, as you point out
below. What's the simplest way to run the tests so that they catch
non-POSIXisms?

> The use of "return" was surprising.  It seems this style has been
> intended to work ever since v0.99.5~24^2~4 (Trapping exit in tests,
> using return for errors, 2005-08-10).

I learned that from other tests and considered it safe therefore.

> It interacts poorly with test_when_finished but since these tests do
> not use that function, they should be safe.  test_when_finished could
> use some fixes to avoid future surprises but that's another story.
> 
> -- 8< --
> Subject: tests: avoid nonportable {foo,bar} glob
> 
> Unlike bash and ksh, dash and busybox ash do not support brace
> expansion (as in 'echo {hello,world}').  So when dash is sh,
> t6009.13 (set up dodecapus) ends up pass a string beginning with
> "root{1,2," to "git merge" verbatim and the test fails.

Thanks, I was afraid of a problem like that but then forgot about it.

> Fix it by introducing a variable to hold the list of parents for
> the dodecapus and populating it in a more low-tech way.
> 
> While at it, simplify a little by combining this setup code with the
> test it sets up for.

I thought it's considered bad style to mix setup and actual test?
Personally, I don't care either way. I just didn't want to have these 12
extra commits in the initial setup (forcing me to adjust all tests).

Michael

Re: [PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents

[Jonathan Nieder]

Michael J Gruber wrote:

> What's the simplest way to run the tests so that they catch
> non-POSIXisms?

In theory: use posh from <git://git.debian.org/users/clint/posh.git>.

In practice, testing with dash seems to work well for catching these.
dash tends to be less buggy but more forgiving because some people
(like me :)) use it day-to-day.

> I thought it's considered bad style to mix setup and actual test?
> Personally, I don't care either way. I just didn't want to have these 12
> extra commits in the initial setup (forcing me to adjust all tests).

Other test frameworks (inspired by JUnit?) like to separate errors
from set-up, tear-down, and the meat of the tests, but the git's test
suite doesn't do that.  Its goal is to test git rather than to track
bugs in the harness.

Still, many scripts include a test that sets up some state to be
shared by later tests.  This way, if git is severely broken (e.g., it
segfaults), then even the setup will fail; and meanwhile, it means it
is still possible to safely skip other individual tests with
GIT_SKIP_TESTS.

Anyway, I shouldn't have included such a tiny nit with the brace
expansion fix.  Sorry about that.  Here's a more minimal patch.

-- 8< --
Subject: tests: avoid nonportable {foo,bar} glob

Unlike bash and ksh, dash and busybox ash do not support brace
expansion (as in 'echo {hello,world}').  So when dash is sh,
t6009.13 (set up dodecapus) ends up passing a string beginning with
"root{1,2," to "git merge" verbatim and the test fails.

Fix it by introducing a variable to hold the list of parents for
the dodecapus and populating it in a more low-tech way.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t6009-rev-list-parent.sh |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index fc89d6d..b30834d 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -114,14 +114,16 @@ test_expect_success 'rev-list override and infinities' '
 
 test_expect_success 'set up dodecapus' '
 
+	roots= &&
 	for i in 1 2 3 4 5 6 7 8 9 10 11
 	do
 		git checkout -b root$i five || return
 		test_commit $i || return
+		roots="$roots root$i" || return
 	done &&
 	git checkout master &&
 	test_tick &&
-	git merge -m dodecapus root{1,2,3,4,5,6,7,8,9,10,11} &&
+	git merge -m dodecapus $roots &&
 	git tag dodecapus
 '
 
-- 
1.7.4.1

[PATCH/RFC 0/2] test_when_finished and returning early

[Jonathan Nieder]

Jonathan Nieder wrote:

> The use of "return" was surprising.  It seems this style has been
> intended to work ever since v0.99.5~24^2~4 (Trapping exit in tests,
> using return for errors, 2005-08-10).
>
> It interacts poorly with test_when_finished but since these tests do
> not use that function, they should be safe.  test_when_finished could
> use some fixes to avoid future surprises but that's another story.

The above was about some code that looks like this:

|	for i in 1 2 3 4 5 6 7 8 9 10 11
| 	do
|		git checkout -b root$i five || return

The fixes alluded to might go something like this.  Thoughts?

Jonathan Nieder (2):
  test: simplify return value of test_run_
  test: cope better with use of return for errors

 t/test-lib.sh |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

[PATCH 1/2] test: simplify return value of test_run_

[Jonathan Nieder]

As v0.99.5~24^2~4 (Trapping exit in tests, using return for errors,
2005-08-10) explains, callers to test_run_ (such as test_expect_code)
used to check the result from eval and the return value separately so
tests that fail early could be distinguished from tests that completed
normally with successful (nonzero) status.  Eventually tests that
succeed with nonzero status were phased out (see v1.7.4-rc0~65^2~19,
2010-10-03 and especially v1.5.5-rc0~271, 2008-02-01) but the weird
two-return-value calling convention lives on.

Let's get rid of it.  The new rule: test_run_ succeeds (returns 0)
if and only if the test succeeded.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/test-lib.sh |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index df25f179..b16a9b98 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -457,7 +457,7 @@ test_run_ () {
 	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"; then
 		echo ""
 	fi
-	return 0
+	return "$eval_ret"
 }
 
 test_skip () {
@@ -502,8 +502,7 @@ test_expect_failure () {
 	if ! test_skip "$@"
 	then
 		say >&3 "checking known breakage: $2"
-		test_run_ "$2" expecting_failure
-		if [ "$?" = 0 -a "$eval_ret" = 0 ]
+		if test_run_ "$2" expecting_failure
 		then
 			test_known_broken_ok_ "$1"
 		else
@@ -521,8 +520,7 @@ test_expect_success () {
 	if ! test_skip "$@"
 	then
 		say >&3 "expecting success: $2"
-		test_run_ "$2"
-		if [ "$?" = 0 -a "$eval_ret" = 0 ]
+		if test_run_ "$2"
 		then
 			test_ok_ "$1"
 		else
-- 
1.7.6

[PATCH 2/2] test: cope better with use of return for errors

[Jonathan Nieder]

In olden times, tests would quietly exit the script when they failed
at an inconvenient moment, which was a little disconcerting.
Therefore v0.99.5~24^2~4 (Trapping exit in tests, using return for
errors, 2005-08-10) switched to an idiom of using "return" instead,
wrapping evaluation of test code in a function to make that safe:

	test_run_ () {
		eval >&3 2>&4 "$1"
		eval_ret="$?"
		return 0
	}

Years later, the implementation of test_when_finished (v1.7.1.1~95,
2010-05-02) and v1.7.2-rc2~1^2~13 (test-lib: output a newline before
"ok" under a TAP harness, 2010-06-24) took advantage of test_run_ as a
place to put code shared by all test assertion functions, without
paying attention to the function's former purpose:

	test_run_ () {
		...
		eval >&3 2>&4 "$1"
		eval_ret=$?

		if should run cleanup
		then
			eval >&3 2>&4 "$test_cleanup"
		fi
		if TAP format requires a newline here
		then
			echo
		fi
		return 0
	}

That means cleanup commands and the newline to put TAP output at
column 0 are skipped when tests use "return" to fail early.  Fix it by
introducing a test_eval_ function to catch the "return", with a
comment explaining the new function's purpose for the next person who
might touch this code.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/test-lib.sh |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index b16a9b98..57c3d532 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -444,15 +444,21 @@ test_debug () {
 	test "$debug" = "" || eval "$1"
 }
 
+test_eval_ () {
+	# This is a separate function because some tests use
+	# "return" to end a test_expect_success block early.
+	eval >&3 2>&4 "$*"
+}
+
 test_run_ () {
 	test_cleanup=:
 	expecting_failure=$2
-	eval >&3 2>&4 "$1"
+	test_eval_ "$1"
 	eval_ret=$?
 
 	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
 	then
-		eval >&3 2>&4 "$test_cleanup"
+		test_eval_ "$test_cleanup"
 	fi
 	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"; then
 		echo ""
-- 
1.7.6

Re: [PATCH 2/2] test: cope better with use of return for errors

[Johannes Sixt]

Am 8/8/2011 3:17, schrieb Jonathan Nieder:
> +test_eval_ () {
> +	# This is a separate function because some tests use
> +	# "return" to end a test_expect_success block early.
> +	eval >&3 2>&4 "$*"
> +}
> +
>  test_run_ () {
>  	test_cleanup=:
>  	expecting_failure=$2
> -	eval >&3 2>&4 "$1"
> +	test_eval_ "$1"
>  	eval_ret=$?
>  
>  	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
>  	then
> -		eval >&3 2>&4 "$test_cleanup"
> +		test_eval_ "$test_cleanup"
>  	fi
>  	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"; then
>  		echo ""

This invalidates at least t3900.29, which accesses an unexpanded $3
from the test script. The patch below fixes this case.

I tried to detect other cases by poisoning test_run_ like this:

-       test_eval_ "$1"
+       test_eval_ :\; :\; :\; :\; :\; "$1"

in the hopes that ":;" is an error at the place that uses $1, $2, etc.
t3900.2[89] are the only tests that are uncovered in this way.

I noticed this because I have a patched test-lib.sh that calls
test_eval_ in a similarly modified manner.

--- >8 ---
From: Johannes Sixt <j6t@kdbg.org>
Subject: [PATCH] t3900: do not reference numbered arguments from the test
 script

The call to test_expect_success is nested inside a function, whose
arguments the test code wants to access. But it is not specified that any
unexpanded $1, $2, $3, etc in the test code will access the surrounding
function's arguments. Rather, they will access the arguments of the
function that happens to eval the test code.

Play safe by placing the argument in a named variable.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t3900-i18n-commit.sh |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index c06a5ee..3265fac 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -136,6 +136,7 @@ done
 test_commit_autosquash_flags () {
 	H=$1
 	flag=$2
+	mopt=$3
 	test_expect_success "commit --$flag with $H encoding" '
 		git config i18n.commitencoding $H &&
 		git checkout -b $H-$flag C0 &&
@@ -147,7 +148,7 @@ test_commit_autosquash_flags () {
 		git commit -a -m "intermediate commit" &&
 		test_tick &&
 		echo $H $flag >>F &&
-		git commit -a --$flag HEAD~1 $3 &&
+		git commit -a --$flag HEAD~1 $mopt &&
 		E=$(git cat-file commit '$H-$flag' |
 			sed -ne "s/^encoding //p") &&
 		test "z$E" = "z$H" &&
@@ -160,6 +161,6 @@ test_commit_autosquash_flags () {
 
 test_commit_autosquash_flags eucJP fixup
 
-test_commit_autosquash_flags ISO-2022-JP squash '-m "squash message"'
+test_commit_autosquash_flags ISO-2022-JP squash '-m squash_message'
 
 test_done
-- 
1.7.6.1618.gc932c

Re: [PATCH 2/2] test: cope better with use of return for errors

[Jeff King]

On Tue, Aug 09, 2011 at 10:46:34AM +0200, Johannes Sixt wrote:

> Am 8/8/2011 3:17, schrieb Jonathan Nieder:
> > +test_eval_ () {
> > +	# This is a separate function because some tests use
> > +	# "return" to end a test_expect_success block early.
> > +	eval >&3 2>&4 "$*"
> > +}
> > +
> >  test_run_ () {
> >  	test_cleanup=:
> >  	expecting_failure=$2
> > -	eval >&3 2>&4 "$1"
> > +	test_eval_ "$1"
> >  	eval_ret=$?
> >  
> >  	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
> >  	then
> > -		eval >&3 2>&4 "$test_cleanup"
> > +		test_eval_ "$test_cleanup"
> >  	fi
> >  	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"; then
> >  		echo ""
> 
> This invalidates at least t3900.29, which accesses an unexpanded $3
> from the test script. The patch below fixes this case.

Hmm. Isn't t3900 already broken, even without this patch? It does
something like this:

  foo() {
    test_expect_success 'bar' 'echo $3'
  }

That can't possibly access the third positional parameter of foo, as we
are already inside the test_expect_success function, no matter what
functions we call after that.

If there is any regression here, it would be that we used to get the
positional parameters of test_run_, and now we get them from test_eval_.
So accessing "$2" used to get us $expecting_failure, but now gets
nothing. I doubt it matters, and tests would be insane to rely on
something internal like that. If it did, the right fix would be:

  -       test_eval_ "$1"
  +       test_eval_ "$@"

in test_run_.

So I don't see a problem in Jonathan's patches. But clearly t3900 is
poorly written and should be fixed.

But:

> diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
> index c06a5ee..3265fac 100755
> --- a/t/t3900-i18n-commit.sh
> +++ b/t/t3900-i18n-commit.sh
> @@ -136,6 +136,7 @@ done
>  test_commit_autosquash_flags () {
>  	H=$1
>  	flag=$2
> +	mopt=$3
>  	test_expect_success "commit --$flag with $H encoding" '
>  		git config i18n.commitencoding $H &&
>  		git checkout -b $H-$flag C0 &&
> @@ -147,7 +148,7 @@ test_commit_autosquash_flags () {
>  		git commit -a -m "intermediate commit" &&
>  		test_tick &&
>  		echo $H $flag >>F &&
> -		git commit -a --$flag HEAD~1 $3 &&
> +		git commit -a --$flag HEAD~1 $mopt &&
>  		E=$(git cat-file commit '$H-$flag' |
>  			sed -ne "s/^encoding //p") &&
>  		test "z$E" = "z$H" &&
> @@ -160,6 +161,6 @@ test_commit_autosquash_flags () {
>  
>  test_commit_autosquash_flags eucJP fixup
>  
> -test_commit_autosquash_flags ISO-2022-JP squash '-m "squash message"'
> +test_commit_autosquash_flags ISO-2022-JP squash '-m squash_message'

Can't we just drop $3 here? I don't see how it's actually doing
anything. It makes us call:

  git commit --squash HEAD~1 -m squash_message

instead of just:

  git commit --squash HEAD~1

but then we never actually check to see that the included message
becomes part of the squashed commit, anyway.

Also, a side note. These tests put their shell snippet inside single
quotes. And then access the variables sometimes directly (assuming they
are left unchanged inside the test functions), and sometimes using
single quotes. Which actually places them outside the snippet's single
quotes, and expands them in place. Which happens to be OK because they
don't contain any spaces, but would otherwise pass extra arguments to
test_expect_success.

So it's not a bug, but it does look unintentional and poor style.

Anyway. Your patch is fine and sufficient to solve the problem. Those
were all just thoughtst I had while trying to figure out what the test
was actually trying to do.

-Peff

[PATCH v2] t3900: do not reference numbered arguments from the test script

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

The call to test_expect_success is nested inside a function, whose
arguments the test code wants to access. But it is not specified that any
unexpanded $1, $2, $3, etc in the test code will access the surrounding
function's arguments. Rather, they will access the arguments of the
function that happens to eval the test code.

In this case, the reference is intended to supply '-m message' to a call of
'git commit --squash'. Remove it because -m is optional and the test case
does not check for it. There are tests in t7500 that check combinations of
--squash and -m.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 8/9/2011 17:36, schrieb Jeff King:
> Hmm. Isn't t3900 already broken, even without this patch? It does
> something like this:
> 
>   foo() {
>     test_expect_success 'bar' 'echo $3'
>   }

Yes, it's broken and this patch just removes the reference.

 t/t3900-i18n-commit.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index c06a5ee..1f62c15 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -147,7 +147,7 @@ test_commit_autosquash_flags () {
 		git commit -a -m "intermediate commit" &&
 		test_tick &&
 		echo $H $flag >>F &&
-		git commit -a --$flag HEAD~1 $3 &&
+		git commit -a --$flag HEAD~1 &&
 		E=$(git cat-file commit '$H-$flag' |
 			sed -ne "s/^encoding //p") &&
 		test "z$E" = "z$H" &&
@@ -160,6 +160,6 @@ test_commit_autosquash_flags () {
 
 test_commit_autosquash_flags eucJP fixup
 
-test_commit_autosquash_flags ISO-2022-JP squash '-m "squash message"'
+test_commit_autosquash_flags ISO-2022-JP squash
 
 test_done
-- 
1.7.6.1618.gc932c

Re: [PATCH v2] t3900: do not reference numbered arguments from the test script

[Jonathan Nieder]

Johannes Sixt wrote:

> Remove it because -m is optional and the test case
> does not check for it. There are tests in t7500 that check combinations of
> --squash and -m.

That's a comfort.  Looks obviously good to me, fwiw.

Re: [PATCH v2] t3900: do not reference numbered arguments from the test script

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Johannes Sixt wrote:
>
>> Remove it because -m is optional and the test case
>> does not check for it. There are tests in t7500 that check combinations of
>> --squash and -m.
>
> That's a comfort.  Looks obviously good to me, fwiw.

Yeah, looks sane. Thanks everybody.

Re: [PATCH/RFC 0/2] test_when_finished and returning early

[Jeff King]

On Mon, Aug 08, 2011 at 03:13:41AM +0200, Jonathan Nieder wrote:

> Jonathan Nieder wrote:
> 
> > The use of "return" was surprising.  It seems this style has been
> > intended to work ever since v0.99.5~24^2~4 (Trapping exit in tests,
> > using return for errors, 2005-08-10).
> >
> > It interacts poorly with test_when_finished but since these tests do
> > not use that function, they should be safe.  test_when_finished could
> > use some fixes to avoid future surprises but that's another story.
> 
> The above was about some code that looks like this:
> 
> |	for i in 1 2 3 4 5 6 7 8 9 10 11
> | 	do
> |		git checkout -b root$i five || return
> 
> The fixes alluded to might go something like this.  Thoughts?
> 
> Jonathan Nieder (2):
>   test: simplify return value of test_run_
>   test: cope better with use of return for errors

Both look sane to me. Thanks for a nicely written set of commit messages
explaining the rather subtle issue.

-Peff

Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only

[Jeff King]

On Fri, Mar 18, 2011 at 08:56:51AM +0100, Michael J Gruber wrote:

> > However, this is totally changing the meaning of an option to plumbing
> > like rev-list (among others). Is it worth the breakage? If so, what's
> > the migration plan? Did I miss a discussion somewhere?
> 
> You missed only the "D" in RFD :)

I saw it, I was just expecting you to start the "D" with some text. :)

> The meaning of a plumbing option can't be changed light-heartedly, of
> course. OTOH, the current design is *really bad* from the ui point of
> view. The expectation that
> 
> "cmd --no-foo --foo" is eq. to "cmd --foo"
> 
> and
> 
> "cmd --foo --no-foo" is eq. to "cmd --no-foo"

I absolutely agree it's bad.

> should be valid universally. In the long run, we might even try and
> convert revision.c to parse_options, thereby gaining --no-foo for every
> --foo.

Another nice side effect would be short-option bundling. Every once in a
while I try something like "git log -pb" and it fails (though it is very
rare to come across these days, since everything _except_ revision and
diff options uses parse_options, and those two have very few short
options).

> So, my RFD really consists of two things:
> 
> - provide a way to override --no-merges/no_merges

Having read Junio's much longer and more intelligent response (compared
to mine) to your initial mail, I think the multi-state selector is the
right way to do this. And it has makes the transition easy, since the
new option appears, then eventually the old options can go away or be
renamed.

> In 2.0 or so, we could change "--merges" to be an alias for
> "--merges-also" rather than "--merges-only" (but don't have to).

Right. My big question reading your patches was when this switch was
supposed to happen (in your series, --merges goes away in the first
patch :) ).

-Peff

Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only

[Michael J Gruber]

Jeff King venit, vidit, dixit 18.03.2011 10:07:
> On Fri, Mar 18, 2011 at 08:56:51AM +0100, Michael J Gruber wrote:
> 
>>> However, this is totally changing the meaning of an option to plumbing
>>> like rev-list (among others). Is it worth the breakage? If so, what's
>>> the migration plan? Did I miss a discussion somewhere?
>>
>> You missed only the "D" in RFD :)
> 
> I saw it, I was just expecting you to start the "D" with some text. :)

According to my expectation, I get many more responses doing it this way :)

I'll try to be good next RFD-time, and I'll reshape my series according
to n-state.

Maybe you can help me understand this one:

int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
{
        int *target = opt->value;
        *target = unset ? 2 : 1;
        return 0;
}

This is no tri-state, is it? (It's used only for the undocumented
--autoupdate option to rerere.) I guess it's more like unset/yes/no,
which, admittedly, counts to 3, but still.

Anyway, I won't try parse_optifying revision.c

Michael

Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only

[Jeff King]

On Fri, Mar 18, 2011 at 10:42:17AM +0100, Michael J Gruber wrote:

> > I saw it, I was just expecting you to start the "D" with some text. :)
> 
> According to my expectation, I get many more responses doing it this way :)

Heh, you are just trolling? :)

> Maybe you can help me understand this one:
> 
> int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
> {
>         int *target = opt->value;
>         *target = unset ? 2 : 1;
>         return 0;
> }
> 
> This is no tri-state, is it? (It's used only for the undocumented
> --autoupdate option to rerere.) I guess it's more like unset/yes/no,
> which, admittedly, counts to 3, but still.

Yeah, it is not what you want. It takes no argument at all, but is a
boolean that uses "1" and "2" for "yes" and "no" (with presumably
"0" as the "unset" version, though this callback doesn't care).

If you want a parse-options example of what you're trying to do, try
OPT__COLOR.

-Peff