revision.c: --merges, --no-merges and --merges-only

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.

Quoted 4 lines

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?

From the ui perspective I'm somehow not a big fan of tri-state options

Quoted 3 lines

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

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

Quoted 6 lines

Quoted 4 lines

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. :)

Quoted 1 line

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

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.

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.

Quoted 3 lines · Michael J Gruber

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

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.

revision.c +0 -3

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

Quoted 6 lines

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?

Quoted 10 lines

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.

Quoted 7 lines

revision.c +0 -3

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

revision.c enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi · line 2044

Quoted 10 lines

revision.c +9 -1

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

revision.h line 26

Quoted 5 lines

revision.h +4 -0

+/* 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).

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

revision.c +0 -2

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

Is there a reason not to choose a convention for which

	rev.max_parents = 1;

works?

Hi,

Quoted 4 lines · Michael J Gruber

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

builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 1064

Quoted 9 lines

builtin/log.c +1 -1

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

revision.c static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg · line 1280

Quoted 8 lines

revision.c +1 -1

@@ -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;"?

revision.c static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg · line 1286

Quoted 8 lines

revision.c +5 -1

 	} 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.

Quoted 7 lines

revision.c +0 -2

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

revision.c enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi · line 2043

Quoted 10 lines

revision.c +8 -2

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

revision.h line 26

Quoted 10 lines

revision.h +4 -0

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

++
+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.

Review

Quoted 8 lines

+--min-parents::
+--max-parents::

(quoted diff) +5 -0

+
+	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.

Quoted 5 lines

++
+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:

Documentation/rev-list-options.txt +4 -2

index fcc4c6c..f32509a 100644
@@ -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.

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	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...

Quoted 3 lines

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".

Quoted 10 lines

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+	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.

Quoted 5 lines

Quoted 4 lines

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

Quoted 3 lines

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.

Jeff King venit, vidit, dixit 21.03.2011 11:54:

Quoted 2 lines · Michael J Gruber

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.

Quoted 3 lines

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?

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

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

Quoted 8 lines

Quoted 3 lines

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.

Quoted 6 lines · Michael J Gruber

Jeff King venit, vidit, dixit 21.03.2011 11:54:

Quoted 2 lines · Michael J Gruber

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?

Quoted 6 lines

Quoted 4 lines

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.

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

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

Quoted 8 lines

Quoted 3 lines

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.

Quoted 4 lines

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

[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.

t/t6009-rev-list-parent.sh +0 -0

@@ -1,6 +1,6 @@

t/t6009-rev-list-parent.sh +0 -0

@@ -28,4 +28,72 @@ test_expect_success 'one is ancestor of others and should not be shown' '

+test_expect_success 'rev-list override and infinities' '

Hi,

Quoted 2 lines · Michael J Gruber

[Subject: rev-list --min-parents,--max-parents: doc and test and completion]

Quoted 2 lines

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.

Quoted 3 lines

t/t6009-rev-list-parent.sh +0 -0

@@ -1,6 +1,6 @@

[...]

t/t6009-rev-list-parent.sh line 3

Quoted 2 lines

t/t6009-rev-list-parent.sh +1 -1

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

Quoted 2 lines

t/t6009-rev-list-parent.sh +0 -0

@@ -28,4 +28,72 @@ test_expect_success 'one is ancestor of others and should not be shown' '

[...]

t/t6009-rev-list-parent.sh test_expect_success 'one is ancestor of others and should not be shown' ' · line 51

Quoted 6 lines

t/t6009-rev-list-parent.sh +6 -0

+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).

t/t6009-rev-list-parent.sh test_expect_success 'one is ancestor of others and should not be shown' ' · line 74

Quoted 6 lines

t/t6009-rev-list-parent.sh +5 -0

+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. :)

t/t6009-rev-list-parent.sh test_expect_success 'one is ancestor of others and should not be shown' ' · line 83

Quoted 4 lines

t/t6009-rev-list-parent.sh +4 -0

+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.

Quoted 2 lines

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

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.

Quoted 2 lines

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?

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.

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

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

Quoted 3 lines

	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?

Quoted 2 lines · Michael J Gruber

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.

Quoted 4 lines

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

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)

Quoted 9 lines

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

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

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

Quoted 4 lines

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.

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

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

Quoted 1 line · Michael J Gruber

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.

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

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

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.

(quoted diff) +6 -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"

Am 8/8/2011 3:17, schrieb Jonathan Nieder:

Quoted 13 lines

(quoted diff) +7 -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=$?

Review

Quoted 7 lines

(quoted diff) +1 -1

 	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.

t/t3900-i18n-commit.sh +2 -1

index c06a5ee..3265fac 100755
@@ -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" &&

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

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

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.

Quoted 3 lines · Johannes Sixt

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.

Quoted 7 lines · Jonathan Nieder

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

Signed-off-by Michael J Gruber <[email protected]>

---

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

--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 <[email protected]>

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

revision.c +2 -4

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

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.

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

Documentation/rev-list-options.txt +1 -1

@@ -72,11 +72,24 @@ endif::git-rev-list[]
 
 --merges::
 
-	Print only merge commits.
+	Print only merge commits. This is equivalent to `--min-parents=2`.
 

t/t6009-rev-list-parent.sh +1 -1

@@ -1,9 +1,17 @@
 #!/bin/sh
 
-test_description='properly cull all ancestors'
+test_description='ancestor culling and limiting by parent number'
 

Quoted 2 lines · Michael J Gruber

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.

Quoted 10 lines

Documentation/rev-list-options.txt +1 -1

@@ -72,11 +72,24 @@ endif::git-rev-list[]
 
 --merges::
 
-	Print only merge commits.
+	Print only merge commits. This is equivalent to `--min-parents=2`.
 

Documentation/rev-list-options.txt endif::git-rev-list[] · line 85

Quoted 10 lines

Documentation/rev-list-options.txt +7 -1

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

Documentation/rev-list-options.txt endif::git-rev-list[] · line 87

Quoted 2 lines

Documentation/rev-list-options.txt +2 -0

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

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

Documentation/rev-list-options.txt endif::git-rev-list[] · line 94

Quoted 7 lines

Documentation/rev-list-options.txt +5 -0

++
+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.

Quoted 10 lines

t/t6009-rev-list-parent.sh +1 -1

@@ -1,9 +1,17 @@
 #!/bin/sh
 
-test_description='properly cull all ancestors'
+test_description='ancestor culling and limiting by parent number'
 
 . ./test-lib.sh
 

t/t6009-rev-list-parent.sh line 14

Quoted 8 lines

t/t6009-rev-list-parent.sh +8 -0

+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.

t/t6009-rev-list-parent.sh test_expect_success 'one is ancestor of others and should not be shown' ' · line 111

Quoted 7 lines

t/t6009-rev-list-parent.sh +7 -0

+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?

Jonathan Nieder venit, vidit, dixit 21.03.2011 19:45:

Quoted 5 lines

Quoted 2 lines · Michael J Gruber

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

Quoted 1 line

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.

Quoted 1 line

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

For me, "-1" is a quite natural way to reset a count type parameter

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.

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

Quoted 10 lines

t/t6009-rev-list-parent.sh test_expect_success 'one is ancestor of others and should not be shown' ' · line 111

Quoted 7 lines

t/t6009-rev-list-parent.sh +7 -0

+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.

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

Quoted 3 lines · Michael J Gruber

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

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.