patch, 2 partsrevision.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.

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

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

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

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

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.

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

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

Jeff King venit, vidit, dixit 17.03.2011 20:59:

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

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

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

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

Junio C Hamano venit, vidit, dixit 18.03.2011 09:22:

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

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

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

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

Michael J Gruber venit, vidit, dixit 18.03.2011 15:50:

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

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

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

Jeff King venit, vidit, dixit 18.03.2011 20:41:

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

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

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

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

-Peff

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

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

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

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.

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

-Peff

Jeff King <peff@peff.net> writes:

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

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

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

Michael J Gruber wrote:

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

Jonathan Nieder venit, vidit, dixit 24.03.2011 09:21:

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

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?

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

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

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

Michael J Gruber wrote:

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.

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

Jonathan Nieder wrote:

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

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

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

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

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

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

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:

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

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:

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

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

Johannes Sixt wrote:

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

Jonathan Nieder <jrnieder@gmail.com> writes:

Yeah, looks sane. Thanks everybody.

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

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

-Peff

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

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

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

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

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

Michael J Gruber venit, vidit, dixit 21.03.2011 11:14:

...

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

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

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.

Michael J Gruber wrote:

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

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

Michael J Gruber wrote:

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.

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.

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

It seems there is an extra newline here.

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

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:

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.

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

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

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.

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

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

Michael J Gruber wrote:

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

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.

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

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

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

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

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

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?

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.

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.

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

Jeff King venit, vidit, dixit 18.03.2011 20:34:

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

That was exactly my thinking.

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.

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

Hi,

Michael J Gruber wrote:

This is growing on me.  Thanks for inventing it.

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?

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

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

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.

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

Hope that helps, Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

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?

Jonathan Nieder venit, vidit, dixit 18.03.2011 21:48:

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

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.

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

For consistency.

min_age etc., yes.

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

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

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

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

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

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:

(Working my way backwards through the responses.)

Jeff King venit, vidit, dixit 18.03.2011 20:48:

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

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 know that name is not a serious suggestion...

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

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

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.

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

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

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.

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

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

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

Perhaps in math circles or something?

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.

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.

Junio C Hamano venit, vidit, dixit 21.03.2011 15:54:

Yeah, those (us) weirdos ;)

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

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

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.

Ok, will tweak v2 and apply.

Thanks.

Hi,

Michael J Gruber wrote:

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.

[...]

sp: limiting

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

[...]

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

Neat. :)

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

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

Jonathan Nieder venit, vidit, dixit 18.03.2011 22:14:

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.

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.

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.

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.

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.

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

Michael J Gruber wrote:

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

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

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.

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

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

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

Thanks.  The details weren't important.

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

Jonathan Nieder venit, vidit, dixit 21.03.2011 18:49:

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

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

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

I absolutely agree it's bad.

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

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.

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

Jeff King venit, vidit, dixit 18.03.2011 10:07:

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

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

Heh, you are just trolling? :)

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