{"thread":{"id":"26762","subject":"[PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only","startedAt":"2011-03-17T11:33:56Z","lastAt":"2011-08-11T21:49:19Z","messageCount":67,"participants":["Michael J Gruber","Junio C Hamano","Jeff King","Jonathan Nieder","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"163554","messageId":"cover.1300359256.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":null,"subject":"[PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-17T11:33:56Z","receivedAt":"2011-03-17T11:33:56Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"This mini series makes it so that --no-merges undoes --merges\nand vice versa, as the user should be able to expect,\nand that --merges-only is a separate option.\n\nMichael J Gruber (2):\n revision.c: rename --merges to --merges-only\n revision.c: introduce --merges to undo --no-merges\n\n Documentation/git-rev-list.txt | 1 +\n Documentation/git-stash.txt | 2 +-\n Documentation/rev-list-options.txt | 6 ++++--\n contrib/completion/git-completion.bash | 2 +-\n revision.c | 4 +++-\n submodule.c | 2 +-\n 6 files changed, 11 insertions(+), 6 deletions(-)\n\n-- \n1.7.4.1.464.gf81ff\n"},{"id":"163555","messageId":"2505df2aeeedd22a9918bf242a9ebeada9fe8adb.1300359256.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"cover.1300359256.git.git@drmicha.warpmail.net","subject":"[PATCH/RFD 1/2] revision.c: rename --merges to --merges-only","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-17T11:33:57Z","receivedAt":"2011-03-17T11:33:57Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Users have a right to expect that --no-foo is the opposite of --foo and\nundoes its effect. Since \"--no-merges\" means \"exclude merges\" it is\nnatural to expect that \"--merges\" means \"include merges\", but in fact it\nmeans \"merges only\". Rename it accordingly.\n\nAs a side effect, this makes the option name coincide with the rev->flag\n(modulo - -> _).\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-rev-list.txt | 2 +-\n Documentation/git-stash.txt | 2 +-\n Documentation/rev-list-options.txt | 2 +-\n contrib/completion/git-completion.bash | 2 +-\n revision.c | 2 +-\n submodule.c | 2 +-\n 6 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt\nindex b08dfbc..020e585 100644\n--- a/Documentation/git-rev-list.txt\n+++ b/Documentation/git-rev-list.txt\n@@ -14,7 +14,7 @@ SYNOPSIS\n \t [ \\--max-age= ]\n \t [ \\--min-age= ]\n \t [ \\--sparse ]\n-\t [ \\--merges ]\n+\t [ \\--merges-only ]\n \t [ \\--no-merges ]\n \t [ \\--first-parent ]\n \t [ \\--remove-empty ]\ndiff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt\nindex 79abc38..eccafcb 100644\n--- a/Documentation/git-stash.txt\n+++ b/Documentation/git-stash.txt\n@@ -246,7 +246,7 @@ repository, but not reachable any more:\n ----------------------------------------------------------------\n git fsck --unreachable |\n grep commit | cut -d\\ -f3 |\n-xargs git log --merges --no-walk --grep=WIP\n+xargs git log --merges-only --no-walk --grep=WIP\n ----------------------------------------------------------------\n \n \ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 5c6850f..78ea305 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -70,7 +70,7 @@ endif::git-rev-list[]\n \n \tStop when a given path disappears from the tree.\n \n---merges::\n+--merges-only::\n \n \tPrint only merge commits.\n \ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex ccdc172..298dddc 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1570,7 +1570,7 @@ _git_ls_tree ()\n __git_log_common_options=\"\n \t--not --all\n \t--branches --tags --remotes\n-\t--first-parent --merges --no-merges\n+\t--first-parent --merges-only --no-merges\n \t--max-count=\n \t--max-age= --since= --after=\n \t--min-age= --until= --before=\ndiff --git a/revision.c b/revision.c\nindex e96c281..bd2fedd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1276,7 +1276,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->show_all = 1;\n \t} else if (!strcmp(arg, \"--remove-empty\")) {\n \t\trevs->remove_empty_trees = 1;\n-\t} else if (!strcmp(arg, \"--merges\")) {\n+\t} else if (!strcmp(arg, \"--merges-only\")) {\n \t\trevs->merges_only = 1;\n \t} else if (!strcmp(arg, \"--no-merges\")) {\n \t\trevs->no_merges = 1;\ndiff --git a/submodule.c b/submodule.c\nindex e9f2b19..6cae159 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -423,7 +423,7 @@ static int find_first_merges(struct object_array *result, const char *path,\n \tint contains_another;\n \n \tchar merged_revision[42];\n-\tconst char *rev_args[] = { \"rev-list\", \"--merges\", \"--ancestry-path\",\n+\tconst char *rev_args[] = { \"rev-list\", \"--merges-only\", \"--ancestry-path\",\n \t\t\t\t \"--all\", merged_revision, NULL };\n \tstruct rev_info revs;\n \tstruct setup_revision_opt rev_opts;\n-- \n1.7.4.1.464.gf81ff\n"},{"id":"163556","messageId":"4033daed8fe7990d7ff5334fcbb5235d86e39b3b.1300359256.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"cover.1300359256.git.git@drmicha.warpmail.net","subject":"[PATCH/RFD 2/2] revision.c: introduce --merges to undo --no-merges","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-17T11:33:58Z","receivedAt":"2011-03-17T11:33:58Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Introduce \"--merges\" as an option to undo \"--no-merges\". This is useful\nwhenever you want to override a default such as those from\n\"format-patch\", \"--cherry\" or aliases which may use \"--no-merges\".\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-rev-list.txt | 1 +\n Documentation/rev-list-options.txt | 4 +++-\n contrib/completion/git-completion.bash | 2 +-\n revision.c | 2 ++\n 4 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt\nindex 020e585..6379f21 100644\n--- a/Documentation/git-rev-list.txt\n+++ b/Documentation/git-rev-list.txt\n@@ -15,6 +15,7 @@ SYNOPSIS\n \t [ \\--min-age= ]\n \t [ \\--sparse ]\n \t [ \\--merges-only ]\n+\t [ \\--merges ]\n \t [ \\--no-merges ]\n \t [ \\--first-parent ]\n \t [ \\--remove-empty ]\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 78ea305..e11145b 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -74,9 +74,11 @@ endif::git-rev-list[]\n \n \tPrint only merge commits.\n \n+--merges::\n --no-merges::\n \n-\tDo not print commits with more than one parent.\n+\tDo resp. do not print commits with more than one parent.\n+\tFor most commands, `--merges` is the default.\n \n --first-parent::\n \tFollow only the first parent commit upon seeing a merge\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 298dddc..db56153 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1570,7 +1570,7 @@ _git_ls_tree ()\n __git_log_common_options=\"\n \t--not --all\n \t--branches --tags --remotes\n-\t--first-parent --merges-only --no-merges\n+\t--first-parent --merges-only --merges --no-merges\n \t--max-count=\n \t--max-age= --since= --after=\n \t--min-age= --until= --before=\ndiff --git a/revision.c b/revision.c\nindex bd2fedd..a9ec574 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1278,6 +1278,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->remove_empty_trees = 1;\n \t} else if (!strcmp(arg, \"--merges-only\")) {\n \t\trevs->merges_only = 1;\n+\t} else if (!strcmp(arg, \"--merges\")) {\n+\t\trevs->no_merges = 0;\n \t} else if (!strcmp(arg, \"--no-merges\")) {\n \t\trevs->no_merges = 1;\n \t} else if (!strcmp(arg, \"--boundary\")) {\n-- \n1.7.4.1.464.gf81ff\n"},{"id":"163577","messageId":"7vwrjx361y.fsf@alter.siamese.dyndns.org","threadId":"26762","inReplyTo":"cover.1300359256.git.git@drmicha.warpmail.net","subject":"Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-17T19:23:05Z","receivedAt":"2011-03-17T19:23:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> This mini series makes it so that --no-merges undoes --merges\n> and vice versa, as the user should be able to expect,\n> and that --merges-only is a separate option.\n\nNice to see the series marked as RFD, but it would have been much better\nwhat aspect of the series you wanted to be discussed.\n\nI know that you have been here long enough to know that an outright\nbreakage of backward compatibility does not deserve a discussion, and that\nyou are feeling to see if people consider it is worth to design and\nimplement a migration path that is not fully specified by this series to\neventually give us the end result that is hinted by this series.\n\nBut I suspect some other readers on the list don't (and that is why I am\nspelling it out).\n\nAs to the eventual goal, I think two modes \"show me only merge commits\",\n\"show me only commits that are not merges\" are absolute nesessity no\nmatter how they are named, and as a uniformity bonus, \"earlier I might\nhave said 'show me only merges' but I take it back\" and its counterpart\nmight be good things to have.\n\nBut.\n\nIf a script writer wants to futureproof his script against later\nintroduction of configuration variables to trigger 'no-merges' and/or\n'merges-only', what should she say? \"git log --no-merges-only --merges\"?\n\nShouldn't the choice offered by the eventual end result of this topic,\nperhaps two years down the road after migration period, not two seemingly\nindependent options as hinted with the series, but an enum with three\nvalues (any commit, non-merge commit only, merge commit only)?\n\nIOW, wouldn't it be a more logical thing to do to add a new option with\nthree values --show-commit={all,merges,no-merges}, if we were adding the\nfeature to filter commits by number of parents?\n\nIf we come up with a nicer name for the --show-commit example above (both\nthe name of the variable and the enum value) and promote it, the migration\nwould go smoothly without giving much pain to the existing users and\nscripts, as it will not _have to_ involve touching the existing merges and\nno-merges options at all, even though we can make them less visible.\n"},{"id":"163588","messageId":"20110317195905.GG20508@sigill.intra.peff.net","threadId":"26762","inReplyTo":"cover.1300359256.git.git@drmicha.warpmail.net","subject":"Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-17T19:59:05Z","receivedAt":"2011-03-17T19:59:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 17, 2011 at 12:33:56PM +0100, Michael J Gruber wrote:\n\n> This mini series makes it so that --no-merges undoes --merges\n> and vice versa, as the user should be able to expect,\n> and that --merges-only is a separate option.\n\nHaving recently been confused by this (and frustrated at the lack of an\nequivalent to your new \"--merges\"), I do think the result is better.\n\nHowever, this is totally changing the meaning of an option to plumbing\nlike rev-list (among others). Is it worth the breakage? If so, what's\nthe migration plan? Did I miss a discussion somewhere?\n\n-Peff\n"},{"id":"163615","messageId":"4D831043.1060709@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110317195905.GG20508@sigill.intra.peff.net","subject":"Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-18T07:56:51Z","receivedAt":"2011-03-18T07:56:51Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 17.03.2011 20:59:\n> On Thu, Mar 17, 2011 at 12:33:56PM +0100, Michael J Gruber wrote:\n> \n>> This mini series makes it so that --no-merges undoes --merges\n>> and vice versa, as the user should be able to expect,\n>> and that --merges-only is a separate option.\n> \n> Having recently been confused by this (and frustrated at the lack of an\n> equivalent to your new \"--merges\"), I do think the result is better.\n> \n> However, this is totally changing the meaning of an option to plumbing\n> like rev-list (among others). Is it worth the breakage? If so, what's\n> the migration plan? Did I miss a discussion somewhere?\n\nYou missed only the \"D\" in RFD :)\n\nThe meaning of a plumbing option can't be changed light-heartedly, of\ncourse. OTOH, the current design is *really bad* from the ui point of\nview. The expectation that\n\n\"cmd --no-foo --foo\" is eq. to \"cmd --foo\"\n\nand\n\n\"cmd --foo --no-foo\" is eq. to \"cmd --no-foo\"\n\nshould be valid universally. In the long run, we might even try and\nconvert revision.c to parse_options, thereby gaining --no-foo for every\n--foo.\n\nSo, my RFD really consists of two things:\n\n- provide a way to override --no-merges/no_merges\n- sane naming\n\nThe first step + half of the second could be achieved by:\n\n- provide \"--merges-also\" which does \"no_merges = 0\"\n- provide \"--merges-only\" which does \"merges_only = 1\", i.e. an alias\nfor current \"--merges\"\n\n\"--merges-also --no-merges\" would be \"--no-merges\" and vice versa.\n\"--merges-also --merges-only\" would be \"--merges-only\"\n\"--merges-only --merges-also\" would be \"--merges-only\"\nwhich all make sense sematically\n\n\"--merges-only --no-merges\" would be no commits with the current\nimplementation (just like \"--merges --no-merges\"), but we could catch\nthis case easily and make it do \"--no-merges\".\n\nI don't think we need to care about keeping the current behavior of\n\"--merges --no-merges\", do we?\n\nSo, no breakage here. Along with it we could issue deprecation notices\nin doc and relnotes for \"--merges\" or just make them less visible. That\nis, it would serve the same purpose as Junio's tri-state option.\n\nIn 2.0 or so, we could change \"--merges\" to be an alias for\n\"--merges-also\" rather than \"--merges-only\" (but don't have to).\n\n>From the ui perspective I'm somehow not a big fan of tri-state options\nbut can't give hard reasons why; maybe because they force you to use\noption arguments. Also, the only reasonable name for a tri-state option\nwould be \"--merges\". Can a tri-state have a default? Then we could\ndefault \"--merges\" to the current behavior, i.e. \"--merges=only\"\n(assuming we have \"--merges=only/also/no\").\n\nJunio: Sorry for making you explain \"D\" more rather than doing it myself\nupfront.\n\nMichael\n"},{"id":"163617","messageId":"7vbp18yh1f.fsf@alter.siamese.dyndns.org","threadId":"26762","inReplyTo":"4D831043.1060709@drmicha.warpmail.net","subject":"Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-18T08:22:20Z","receivedAt":"2011-03-18T08:22:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> From the ui perspective I'm somehow not a big fan of tri-state options\n> but can't give hard reasons why; maybe because they force you to use\n> option arguments.\n\nBut in this particular case, you cannot really hack it with two options\nthat appear independent on surface but in reality are not. Logically, it\nis an enum and you can choose only\none of them, and it is even worse from the UI perspective to use\ncombination of two not-quite-independent options.\n\nAlso I have a hidden agenda to add \"because we could\" --show=octopus to\nthe enum later perhaps only to my private edition ;-)\n"},{"id":"163618","messageId":"4D831AD4.5020709@drmicha.warpmail.net","threadId":"26762","inReplyTo":"7vbp18yh1f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-18T08:41:56Z","receivedAt":"2011-03-18T08:41:56Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 18.03.2011 09:22:\n> Michael J Gruber writes:\n> \n>> From the ui perspective I'm somehow not a big fan of tri-state options\n>> but can't give hard reasons why; maybe because they force you to use\n>> option arguments.\n> \n> But in this particular case, you cannot really hack it with two options\n> that appear independent on surface but in reality are not. Logically, it\n> is an enum and you can choose only\n> one of them, and it is even worse from the UI perspective to use\n> combination of two not-quite-independent options.\n> \n> Also I have a hidden agenda to add \"because we could\" --show=octopus to\n> the enum later perhaps only to my private edition ;-)\n\nNow you're selling me that (tri++)-state!\n\ngit config alias.calamari 'git log --merges=octopus'\n\nSurprisingly few call sites deal with the corresponding rev.foo so that\neven those could be enumified... Stay tuned :)\n\nMichael\n"},{"id":"163619","messageId":"20110318085616.GA16703@sigill.intra.peff.net","threadId":"26762","inReplyTo":"7vbp18yh1f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-18T08:56:16Z","receivedAt":"2011-03-18T08:56:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 18, 2011 at 01:22:20AM -0700, Junio C Hamano wrote:\n\n> Michael J Gruber writes:\n> \n> > From the ui perspective I'm somehow not a big fan of tri-state options\n> > but can't give hard reasons why; maybe because they force you to use\n> > option arguments.\n> \n> But in this particular case, you cannot really hack it with two options\n> that appear independent on surface but in reality are not. Logically, it\n> is an enum and you can choose only\n> one of them, and it is even worse from the UI perspective to use\n> combination of two not-quite-independent options.\n> \n> Also I have a hidden agenda to add \"because we could\" --show=octopus to\n> the enum later perhaps only to my private edition ;-)\n\nYes, I read your other response to Michael and I think that makes the\nmost sense. In the most general form it is \"--show=\",\nthough I don't think there is really much point in values besides 0, 1,\n2, or >2 (or combinations thereof, though again, probably only \"this\nparticular value\" or \"any\" is of interest).\n\nSo what you have said so far covers that, except for \"--show=roots\",\nwhich I think would be a useful addition.\n\nThe over-engineer in me wants to have the general form\n\"--grep-by-parent-count=0,1\" and \"--show=no-merges\" as a human-friendly\nalias to that. But I can't think of a good reason to have the general\nform, except for the sake of generalizing.\n\n-Peff\n"},{"id":"163620","messageId":"20110318090724.GB16703@sigill.intra.peff.net","threadId":"26762","inReplyTo":"4D831043.1060709@drmicha.warpmail.net","subject":"Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-18T09:07:24Z","receivedAt":"2011-03-18T09:07:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 18, 2011 at 08:56:51AM +0100, Michael J Gruber wrote:\n\n> > However, this is totally changing the meaning of an option to plumbing\n> > like rev-list (among others). Is it worth the breakage? If so, what's\n> > the migration plan? Did I miss a discussion somewhere?\n> \n> You missed only the \"D\" in RFD :)\n\nI saw it, I was just expecting you to start the \"D\" with some text. :)\n\n> The meaning of a plumbing option can't be changed light-heartedly, of\n> course. OTOH, the current design is *really bad* from the ui point of\n> view. The expectation that\n> \n> \"cmd --no-foo --foo\" is eq. to \"cmd --foo\"\n> \n> and\n> \n> \"cmd --foo --no-foo\" is eq. to \"cmd --no-foo\"\n\nI absolutely agree it's bad.\n\n> should be valid universally. In the long run, we might even try and\n> convert revision.c to parse_options, thereby gaining --no-foo for every\n> --foo.\n\nAnother nice side effect would be short-option bundling. Every once in a\nwhile I try something like \"git log -pb\" and it fails (though it is very\nrare to come across these days, since everything _except_ revision and\ndiff options uses parse_options, and those two have very few short\noptions).\n\n> So, my RFD really consists of two things:\n> \n> - provide a way to override --no-merges/no_merges\n\nHaving read Junio's much longer and more intelligent response (compared\nto mine) to your initial mail, I think the multi-state selector is the\nright way to do this. And it has makes the transition easy, since the\nnew option appears, then eventually the old options can go away or be\nrenamed.\n\n> In 2.0 or so, we could change \"--merges\" to be an alias for\n> \"--merges-also\" rather than \"--merges-only\" (but don't have to).\n\nRight. My big question reading your patches was when this switch was\nsupposed to happen (in your series, --merges goes away in the first\npatch :) ).\n\n-Peff\n"},{"id":"163622","messageId":"4D8328F9.9050100@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110318090724.GB16703@sigill.intra.peff.net","subject":"Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-18T09:42:17Z","receivedAt":"2011-03-18T09:42:17Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 18.03.2011 10:07:\n> On Fri, Mar 18, 2011 at 08:56:51AM +0100, Michael J Gruber wrote:\n> \n>>> However, this is totally changing the meaning of an option to plumbing\n>>> like rev-list (among others). Is it worth the breakage? If so, what's\n>>> the migration plan? Did I miss a discussion somewhere?\n>>\n>> You missed only the \"D\" in RFD :)\n> \n> I saw it, I was just expecting you to start the \"D\" with some text. :)\n\nAccording to my expectation, I get many more responses doing it this way :)\n\nI'll try to be good next RFD-time, and I'll reshape my series according\nto n-state.\n\nMaybe you can help me understand this one:\n\nint parse_opt_tertiary(const struct option *opt, const char *arg, int unset)\n{\n int *target = opt->value;\n *target = unset ? 2 : 1;\n return 0;\n}\n\nThis is no tri-state, is it? (It's used only for the undocumented\n--autoupdate option to rerere.) I guess it's more like unset/yes/no,\nwhich, admittedly, counts to 3, but still.\n\nAnyway, I won't try parse_optifying revision.c\n\nMichael\n"},{"id":"163624","messageId":"20110318095417.GA18526@sigill.intra.peff.net","threadId":"26762","inReplyTo":"4D8328F9.9050100@drmicha.warpmail.net","subject":"Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-18T09:54:17Z","receivedAt":"2011-03-18T09:54:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 18, 2011 at 10:42:17AM +0100, Michael J Gruber wrote:\n\n> > I saw it, I was just expecting you to start the \"D\" with some text. :)\n> \n> According to my expectation, I get many more responses doing it this way :)\n\nHeh, you are just trolling? :)\n\n> Maybe you can help me understand this one:\n> \n> int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)\n> {\n> int *target = opt->value;\n> *target = unset ? 2 : 1;\n> return 0;\n> }\n> \n> This is no tri-state, is it? (It's used only for the undocumented\n> --autoupdate option to rerere.) I guess it's more like unset/yes/no,\n> which, admittedly, counts to 3, but still.\n\nYeah, it is not what you want. It takes no argument at all, but is a\nboolean that uses \"1\" and \"2\" for \"yes\" and \"no\" (with presumably\n\"0\" as the \"unset\" version, though this callback doesn't care).\n\nIf you want a parse-options example of what you're trying to do, try\nOPT__COLOR.\n\n-Peff\n"},{"id":"163640","messageId":"cover.1300459016.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110318085616.GA16703@sigill.intra.peff.net","subject":"[PATCH 0/3] rev-list and friends: --min-parents, --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-18T14:50:22Z","receivedAt":"2011-03-18T14:50:22Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"As opposed to the RFD it replaces, this is a real patch series with\ndocumentation and tests, and it even comes with boiler plate. It should\nmake all of Jeff's and Junio's dreams come true (as far as revision\nlimiting by parent number goes).\n\n1/3 introduces the new options (and has a proper commit message)\n2/3 I noted along the way and could be applied earlier\n3/3 depends on 1 and 2 and is the candy (doc, tests, completion)\n\n*** BLURB HERE ***\n\nMichael J Gruber (3):\n revision.c: introduce --min-parents and --max-parents\n t6009: use test_commit() from test-lib.sh\n rev-list --min-parents,--max-parents: doc and test and completion\n\n Documentation/git-rev-list.txt | 2 +\n Documentation/rev-list-options.txt | 13 +++++\n builtin/log.c | 2 +-\n builtin/rev-list.c | 2 +\n builtin/rev-parse.c | 2 +\n contrib/completion/git-completion.bash | 1 +\n revision.c | 23 ++++++---\n revision.h | 9 +++-\n t/t6009-rev-list-parent.sh | 85 +++++++++++++++++++++++++++-----\n 9 files changed, 117 insertions(+), 22 deletions(-)\n\n-- \n1.7.4.1.464.gf81ff\n"},{"id":"163639","messageId":"7f2a7d979cd8f6b9fce577994c82f50421575d58.1300459017.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110318085616.GA16703@sigill.intra.peff.net","subject":"[PATCH 1/3] revision.c: introduce --min-parents and --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-18T14:50:23Z","receivedAt":"2011-03-18T14:50:23Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"--merges and --no-merges are named confusingly and cannot be overridden\nby each other, but they are there to stay for plumbers' sake.\n\nIntroduce --min-parents and --max-parents which take values 0,...,7 and\nlimit the revisions to those commits which have at least resp. at most\nthat many commits, where --max-parents=8 denotes --max-parents=infinity\n(i.e. no upper limit). In fact, 7 (or any negative number) does, but 8\nis infinity sideways 8-)\n\nIn particular:\n\n--max-parents=1: no merges\n--min-parents=2: merges only\n--max-parents=0: only roots\n--min-parents=3: only octopusses\n\n--min-parents=n --max-parents=m with n>m gives you what you ask for\n(nothing) just like --merges --no-merges does, but at least for an\nobvious reason.\n\nImplementation notes:\n\n* We compute the number of parents only when we limit by that, so there\n is no performance impact when there are no limiters.\n\n* This uses 6 bits in struct rev_info (rather than the previous 2). We\n need at least 4 to distinguish the 5 cases \"all\", \"merges only\", \"no\n merges\", \"roots\", \"octopusses\". 4 would get us to infinity == 3.\n I don't think two int's are warranted - they would also spoil the 8.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/log.c | 2 +-\n builtin/rev-list.c | 2 ++\n builtin/rev-parse.c | 2 ++\n revision.c | 23 ++++++++++++++++-------\n revision.h | 9 +++++++--\n 5 files changed, 28 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 796e9e5..9ffccc3 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1061,7 +1061,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \trev.commit_format = CMIT_FMT_EMAIL;\n \trev.verbose_header = 1;\n \trev.diff = 1;\n-\trev.no_merges = 1;\n+\trev.max_parents = MAX_PARENTS(1);\n \tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n \trev.subject_prefix = fmt_patch_subject_prefix;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex f458cb7..7b276e0 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -16,6 +16,8 @@ static const char rev_list_usage[] =\n \" --min-age=\\n\"\n \" --sparse\\n\"\n \" --no-merges\\n\"\n+\" --min-parents=\\n\"\n+\" --max-parents=\\n\"\n \" --remove-empty\\n\"\n \" --all\\n\"\n \" --branches\\n\"\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex a5a1c86..9940546 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -48,6 +48,8 @@ static int is_rev_argument(const char *arg)\n \t\t\"--max-count=\",\n \t\t\"--min-age=\",\n \t\t\"--no-merges\",\n+\t\t\"--min-parents=\",\n+\t\t\"--max-parents=\",\n \t\t\"--objects\",\n \t\t\"--objects-edge\",\n \t\t\"--parents\",\ndiff --git a/revision.c b/revision.c\nindex e96c281..6cfd091 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1277,9 +1277,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--remove-empty\")) {\n \t\trevs->remove_empty_trees = 1;\n \t} else if (!strcmp(arg, \"--merges\")) {\n-\t\trevs->merges_only = 1;\n+\t\trevs->min_parents = MIN_PARENTS(2);\n \t} else if (!strcmp(arg, \"--no-merges\")) {\n-\t\trevs->no_merges = 1;\n+\t\trevs->max_parents = MAX_PARENTS(1);\n+\t} else if (!prefixcmp(arg, \"--min-parents=\")) {\n+\t\trevs->min_parents = MIN_PARENTS(atoi(arg+14));\n+\t} else if (!prefixcmp(arg, \"--max-parents=\")) {\n+\t\trevs->max_parents = MAX_PARENTS(atoi(arg+14));\n \t} else if (!strcmp(arg, \"--boundary\")) {\n \t\trevs->boundary = 1;\n \t} else if (!strcmp(arg, \"--left-right\")) {\n@@ -1298,7 +1302,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\t\tdie(\"--cherry is incompatible with --left-only\");\n \t\trevs->cherry_mark = 1;\n \t\trevs->right_only = 1;\n-\t\trevs->no_merges = 1;\n+\t\trevs->max_parents = MAX_PARENTS(1);\n \t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--count\")) {\n \t\trevs->count = 1;\n@@ -2029,10 +2033,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi\n \t\treturn commit_ignore;\n \tif (revs->min_age != -1 && (commit->date > revs->min_age))\n \t\treturn commit_ignore;\n-\tif (revs->no_merges && commit->parents && commit->parents->next)\n-\t\treturn commit_ignore;\n-\tif (revs->merges_only && !(commit->parents && commit->parents->next))\n-\t\treturn commit_ignore;\n+\tif (revs->min_parents || revs->max_parents) {\n+\t\tint n = 0;\n+\t\tstruct commit_list *p;\n+\t\tfor (p = commit->parents; p; p = p->next)\n+\t\t\tn++;\n+\t\tif ((MIN_PARENTS(n) < revs->min_parents) ||\n+\t\t (MAX_PARENTS(n) < revs->max_parents)) /* max is inv. */\n+\t\t\treturn commit_ignore;\n+\t}\n \tif (!commit_match(commit, revs))\n \t\treturn commit_ignore;\n \tif (revs->prune && revs->dense) {\ndiff --git a/revision.h b/revision.h\nindex ae94860..192b2d4 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -20,6 +20,11 @@\n #define DECORATE_SHORT_REFS\t1\n #define DECORATE_FULL_REFS\t2\n \n+/* limit to used range */\n+#define MIN_PARENTS(m)\t({ unsigned int __n = (m); (__n < 0) ? 0 : (__n > 7) ? 7 : __n; })\n+/* invert fox MAX so that default = 0 -> infinity */\n+#define MAX_PARENTS(m)\t({ unsigned int __n = (m); (__n < 0) ? 7 : (__n > 7) ? 0 : 7 - __n;})\n+\n struct rev_info;\n struct log_info;\n struct string_list;\n@@ -41,8 +46,8 @@ struct rev_info {\n \t/* Traversal flags */\n \tunsigned int\tdense:1,\n \t\t\tprune:1,\n-\t\t\tno_merges:1,\n-\t\t\tmerges_only:1,\n+\t\t\tmin_parents:3,\n+\t\t\tmax_parents:3, /* MAX_PARENTS(n) */\n \t\t\tno_walk:1,\n \t\t\tshow_all:1,\n \t\t\tremove_empty_trees:1,\n-- \n1.7.4.1.464.gf81ff\n"},{"id":"163642","messageId":"959b1f0c5afb87bde9fefbbe7c0b0947fb6780a7.1300459017.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110318085616.GA16703@sigill.intra.peff.net","subject":"[PATCH 2/3] t6009: use test_commit() from test-lib.sh","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-18T14:50:24Z","receivedAt":"2011-03-18T14:50:24Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Signed-off-by: Michael J Gruber \n---\n t/t6009-rev-list-parent.sh | 15 ++++-----------\n 1 files changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\nindex 52f7b27..0f0e457 100755\n--- a/t/t6009-rev-list-parent.sh\n+++ b/t/t6009-rev-list-parent.sh\n@@ -4,25 +4,18 @@ test_description='properly cull all ancestors'\n \n . ./test-lib.sh\n \n-commit () {\n-\ttest_tick &&\n-\techo $1 >file &&\n-\tgit commit -a -m $1 &&\n-\tgit tag $1\n-}\n-\n test_expect_success setup '\n \n \ttouch file &&\n \tgit add file &&\n \n-\tcommit one &&\n+\ttest_commit one &&\n \n \ttest_tick=$(($test_tick - 2400)) &&\n \n-\tcommit two &&\n-\tcommit three &&\n-\tcommit four &&\n+\ttest_commit two &&\n+\ttest_commit three &&\n+\ttest_commit four &&\n \n \tgit log --pretty=oneline --abbrev-commit\n '\n-- \n1.7.4.1.464.gf81ff\n"},{"id":"163641","messageId":"182c5ece22835559a9762e242adb701547cf6575.1300459017.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110318085616.GA16703@sigill.intra.peff.net","subject":"[PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-18T14:50:25Z","receivedAt":"2011-03-18T14:50:25Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"This also adds test for \"--merges\" and \"--no-merges\" which we did not\nhave so far.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-rev-list.txt | 2 +\n Documentation/rev-list-options.txt | 13 ++++++\n contrib/completion/git-completion.bash | 1 +\n t/t6009-rev-list-parent.sh | 70 +++++++++++++++++++++++++++++++-\n 4 files changed, 85 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt\nindex b08dfbc..4d2b0c5 100644\n--- a/Documentation/git-rev-list.txt\n+++ b/Documentation/git-rev-list.txt\n@@ -16,6 +16,8 @@ SYNOPSIS\n \t [ \\--sparse ]\n \t [ \\--merges ]\n \t [ \\--no-merges ]\n+\t [ \\--min-parents ]\n+\t [ \\--max-parents ]\n \t [ \\--first-parent ]\n \t [ \\--remove-empty ]\n \t [ \\--full-history ]\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 5c6850f..104e644 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -78,6 +78,19 @@ endif::git-rev-list[]\n \n \tDo not print commits with more than one parent.\n \n+--min-parents::\n+--max-parents::\n+\n+\tShow only commits which have at least resp. at most that many\n+\tcommits, where `--max-parents=8` denotes infinity (i.e. no upper\n+\tlimit). In fact, 7 (or any negative number) does, but 8 is\n+\tinfinity sideways 8-)\n++\n+In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is\n+`--merges` (only), `--max-parents=0` gives all root commits and\n+`--min-parents=3` all octopusses.\n+\n+\n --first-parent::\n \tFollow only the first parent commit upon seeing a merge\n \tcommit. This option can give a better overview when\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 3b1cc83..4da087e 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1577,6 +1577,7 @@ __git_log_common_options=\"\n \t--max-count=\n \t--max-age= --since= --after=\n \t--min-age= --until= --before=\n+\t--min-parents= --max-parents=\n \"\n # Options that go well for log and gitk (not shortlog)\n __git_log_gitk_options=\"\ndiff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\nindex 0f0e457..792bbe9 100755\n--- a/t/t6009-rev-list-parent.sh\n+++ b/t/t6009-rev-list-parent.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='properly cull all ancestors'\n+test_description='ancestor culling and limitting by parent number'\n \n . ./test-lib.sh\n \n@@ -28,4 +28,72 @@ test_expect_success 'one is ancestor of others and should not be shown' '\n \n '\n \n+test_expect_success 'setup roots, merges and octopusses' '\n+\n+\tgit checkout --orphan newroot &&\n+\ttest_commit five &&\n+\tgit checkout -b sidebranch two &&\n+\ttest_commit six &&\n+\tgit checkout -b anotherbranch three &&\n+\ttest_commit seven &&\n+\tgit checkout master &&\n+\ttest_merge normalmerge newroot &&\n+\ttest_tick &&\n+\tgit merge -m octopus sidebranch anotherbranch &&\n+\tgit tag octopus\n+'\n+\n+check_revlist () {\n+\t> expect &&\n+\tfor c in $2; do echo \"$c\" >> expect; done &&\n+\tgit rev-list $1 master | git name-rev --name-only --tags --stdin >actual &&\n+\ttest_cmp expect actual\n+}\n+ \n+test_expect_success 'rev-list roots' '\n+\n+\tcheck_revlist \"--max-parents=0\" \"five one\"\n+'\n+\n+test_expect_success 'rev-list no merges' '\n+\n+\tcheck_revlist \"--max-parents=1\" \"seven six five four three two one\" &&\n+\tcheck_revlist \"--no-merges\" \"seven six five four three two one\"\n+'\n+\n+test_expect_success 'rev-list no octopusses' '\n+\n+\tcheck_revlist \"--max-parents=2\" \"normalmerge seven six five four three two one\"\n+'\n+\n+test_expect_success 'rev-list no roots' '\n+\n+\tcheck_revlist \"--min-parents=1\" \"octopus normalmerge seven six four three two\"\n+'\n+\n+test_expect_success 'rev-list merges' '\n+\n+\tcheck_revlist \"--min-parents=2\" \"octopus normalmerge\" &&\n+\tcheck_revlist \"--merges\" \"octopus normalmerge\"\n+'\n+\n+test_expect_success 'rev-list octopus' '\n+\n+\tcheck_revlist \"--min-parents=3\" \"octopus\"\n+'\n+\n+test_expect_success 'rev-list ordinary commits' '\n+\n+\tcheck_revlist \"--min-parents=1 --max-parents=1\" \"seven six four three two\"\n+'\n+\n+test_expect_success 'rev-list override and infinities' '\n+\n+\tcheck_revlist \"--min-parents=2 --no-merges\" \"\" &&\n+\tcheck_revlist \"--min-parents=2 --no-merges --max-parents=3\" \"octopus normalmerge\" &&\n+\tcheck_revlist \"--min-parents=2 --no-merges --max-parents=7\" \"octopus normalmerge\" &&\n+\tcheck_revlist \"--min-parents=2 --no-merges --max-parents=8\" \"octopus normalmerge\" &&\n+\tcheck_revlist \"--min-parents=2 --no-merges --max-parents=-1\" \"octopus normalmerge\"\n+'\n+\n test_done\n-- \n1.7.4.1.464.gf81ff\n"},{"id":"163643","messageId":"4D837228.4070102@drmicha.warpmail.net","threadId":"26762","inReplyTo":"cover.1300459016.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 0/3] rev-list and friends: --min-parents, --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-18T14:54:32Z","receivedAt":"2011-03-18T14:54:32Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Michael J Gruber venit, vidit, dixit 18.03.2011 15:50:\n> As opposed to the RFD it replaces, this is a real patch series with\n> documentation and tests, and it even comes with boiler plate. It should\n> make all of Jeff's and Junio's dreams come true (as far as revision\n> limiting by parent number goes).\n> \n> 1/3 introduces the new options (and has a proper commit message)\n> 2/3 I noted along the way and could be applied earlier\n> 3/3 depends on 1 and 2 and is the candy (doc, tests, completion)\n> \n> *** BLURB HERE ***\n\nHmpf. I guess this should say \"NO MORE BLURB HERE\". Gosh!\n\nThis comes from filling in the cover letter by using\n\n:r! git notes show ref:HEAD\n\nin vim and partially failing to delete the place holders. Longing for\nthat format-patch option \"--note-cover\".\n\n> \n> Michael J Gruber (3):\n> revision.c: introduce --min-parents and --max-parents\n> t6009: use test_commit() from test-lib.sh\n> rev-list --min-parents,--max-parents: doc and test and completion\n> \n> Documentation/git-rev-list.txt | 2 +\n> Documentation/rev-list-options.txt | 13 +++++\n> builtin/log.c | 2 +-\n> builtin/rev-list.c | 2 +\n> builtin/rev-parse.c | 2 +\n> contrib/completion/git-completion.bash | 1 +\n> revision.c | 23 ++++++---\n> revision.h | 9 +++-\n> t/t6009-rev-list-parent.sh | 85 +++++++++++++++++++++++++++-----\n> 9 files changed, 117 insertions(+), 22 deletions(-)\n> \n"},{"id":"163663","messageId":"20110318193401.GA27825@sigill.intra.peff.net","threadId":"26762","inReplyTo":"7f2a7d979cd8f6b9fce577994c82f50421575d58.1300459017.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 1/3] revision.c: introduce --min-parents and --max-parents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-18T19:34:02Z","receivedAt":"2011-03-18T19:34:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 18, 2011 at 03:50:23PM +0100, Michael J Gruber wrote:\n\n> Introduce --min-parents and --max-parents which take values 0,...,7 and\n> limit the revisions to those commits which have at least resp. at most\n> that many commits, where --max-parents=8 denotes --max-parents=infinity\n> (i.e. no upper limit). In fact, 7 (or any negative number) does, but 8\n> is infinity sideways 8-)\n\nIn practice, I don't think anybody is all that interested in\ndifferentiating octopus merges by their numbers of parents. But the\nchoice of \"7\" as a maximum seems kind of arbitrary. There are commits in\nlinux-2.6 with up to 31 parents (once upon a time we had an arbitrary\nlimit of 16 parents, but that was lifted in v1.6.0).\n\nYou mention below that this fits in three 3 bits in rev_info. I don't\nthink bits are precious in rev_info, though, as they are in other\nrevision-related structs. We only have one such struct per walk.\n\nIf it were just an implementation issue, I would say fine, we can tweak\nit later if somebody really cares. But we are cementing \"7\" as the value\nfor infinity in the user-facing interface. Wouldn't a value like \"-1\" or\n\"infinity\" be more appropriate?\n\n> In particular:\n> \n> --max-parents=1: no merges\n> --min-parents=2: merges only\n> --max-parents=0: only roots\n> --min-parents=3: only octopusses\n> \n> --min-parents=n --max-parents=m with n>m gives you what you ask for\n> (nothing) just like --merges --no-merges does, but at least for an\n> obvious reason.\n\nI like this. It's much more natural than the list syntax I suggested\nearlier, and it handles ranges in an obvious way. It doesn't allow\nselecting, e.g., root commits and merges, but not regular commits. But I\nreally don't see the point in supporting that.\n\n> @@ -2029,10 +2033,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi\n> \t\treturn commit_ignore;\n> \tif (revs->min_age != -1 && (commit->date > revs->min_age))\n> \t\treturn commit_ignore;\n> -\tif (revs->no_merges && commit->parents && commit->parents->next)\n> -\t\treturn commit_ignore;\n> -\tif (revs->merges_only && !(commit->parents && commit->parents->next))\n> -\t\treturn commit_ignore;\n> +\tif (revs->min_parents || revs->max_parents) {\n> +\t\tint n = 0;\n> +\t\tstruct commit_list *p;\n> +\t\tfor (p = commit->parents; p; p = p->next)\n> +\t\t\tn++;\n> +\t\tif ((MIN_PARENTS(n) < revs->min_parents) ||\n> +\t\t (MAX_PARENTS(n) < revs->max_parents)) /* max is inv. */\n> +\t\t\treturn commit_ignore;\n> +\t}\n\nYou did the obvious optimization not to count parents if we don't care\nabout min/max. You could also do something like:\n\n switch (revs->max_parents) {\n case 0:\n if (commit->parents)\n return commit_ignore;\n break;\n case 1:\n if (commit->parents && commit->parents->next)\n return commit_ignore;\n break;\n default:\n if (count_parents(commit) > commit->max_parents)\n return commit_ignore;\n break;\n }\n\nwhich more closely matches the original code (you would also need to do\nthe same for min_parents, and obviously this code ignores the\nmax-parents-is-inverted thing).\n\nI'm not sure it would buy much in practice, though. Commits with more\nthan 2 parents are rare, so unnecessarily counting all of them just to\nfind out something is a merge is probably not going to create a\nmeasurable slowdown.\n\n> +/* limit to used range */\n> +#define MIN_PARENTS(m)\t({ unsigned int __n = (m); (__n < 0) ? 0 : (__n > 7) ? 7 : __n; })\n> +/* invert fox MAX so that default = 0 -> infinity */\n> +#define MAX_PARENTS(m)\t({ unsigned int __n = (m); (__n < 0) ? 7 : (__n > 7) ? 0 : 7 - __n;})\n\nHmm. You assign the input to an unsigned int, and then compare it to 0.\nWon't that always be false?\n\nThe inversion trick is clever to make a bzero'd rev_info work, but I\nthink the code would be more obvious without it. And you really have to\ncall init_revisions() anyway, so initializing it to a maximum there (or\n-1) would be acceptable (e.g., see how max_age and min_age work).\n\n-Peff\n"},{"id":"163665","messageId":"20110318194113.GA2215@sigill.intra.peff.net","threadId":"26762","inReplyTo":"cover.1300459016.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 0/3] rev-list and friends: --min-parents, --max-parents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-18T19:41:13Z","receivedAt":"2011-03-18T19:41:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 18, 2011 at 03:50:22PM +0100, Michael J Gruber wrote:\n\n> As opposed to the RFD it replaces, this is a real patch series with\n> documentation and tests, and it even comes with boiler plate. It should\n> make all of Jeff's and Junio's dreams come true (as far as revision\n> limiting by parent number goes).\n> \n> 1/3 introduces the new options (and has a proper commit message)\n> 2/3 I noted along the way and could be applied earlier\n> 3/3 depends on 1 and 2 and is the candy (doc, tests, completion)\n\nI didn't see it mentioned anywhere, but I assume this applies on\nmg/rev-list-one-side-only (b1b4755). It has conflicts when applied on\nmaster.\n\n(I know you probably know that, but it is for the benefit of other\nreviewers).\n\n-Peff\n"},{"id":"163666","messageId":"20110318194802.GB27825@sigill.intra.peff.net","threadId":"26762","inReplyTo":"182c5ece22835559a9762e242adb701547cf6575.1300459017.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-18T19:48:02Z","receivedAt":"2011-03-18T19:48:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 18, 2011 at 03:50:25PM +0100, Michael J Gruber wrote:\n\n> +--min-parents::\n> +--max-parents::\n> +\n> +\tShow only commits which have at least resp. at most that many\n> +\tcommits, where `--max-parents=8` denotes infinity (i.e. no upper\n> +\tlimit). In fact, 7 (or any negative number) does, but 8 is\n> +\tinfinity sideways 8-)\n\nI didn't quite parse this \"resp.\" in the middle.\n\n> ++\n> +In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is\n> +`--merges` (only), `--max-parents=0` gives all root commits and\n> +`--min-parents=3` all octopusses.\n\nSpelling nit: the plural of octopus is \"octopuses\" (or \"octopi\" or\n\"octopodes\", depending on which dictionary you consult).\n\nI think it's good to equate --{no-,}merges with their\n--{max,min}-parents counterparts here. We should probably do the same\nfor the reverse association, like:\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex fcc4c6c..f32509a 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -226,11 +226,13 @@ endif::git-rev-list[]\n \n --merges::\n \n-\tPrint only merge commits.\n+\tPrint only merge commits. This is equivalent to\n+\t`--min-parents=2`.\n \n --no-merges::\n \n-\tDo not print commits with more than one parent.\n+\tDo not print commits with more than one parent. This is\n+\tequivalent to `--max-parents=1`.\n \n --min-parents::\n --max-parents::\n\nThat way it is obvious that \"--merges\" cancels a previous --min-parents\non the command line (maybe the text should be \"this is an alias for...\"\nto make it clear that doing it is exactly the same).\n\nThis provides a user-friendly form for the two common cases. Do we care\nabout the other cases, or adding a single multi-state flag like\n--show-parents={all,merges,etc}? They other ones are rare enough and\nparticular enough that it is probably fine for people to just use\n{min,max}-parents directly. So I would say what you have is good.\n\n-Peff\n"},{"id":"163672","messageId":"20110318204854.GA23331@elie","threadId":"26762","inReplyTo":"7f2a7d979cd8f6b9fce577994c82f50421575d58.1300459017.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 1/3] revision.c: introduce --min-parents and --max-parents","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-18T20:48:54Z","receivedAt":"2011-03-18T20:48:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael J Gruber wrote:\n\n> --max-parents=1: no merges\n> --min-parents=2: merges only\n> --max-parents=0: only roots\n> --min-parents=3: only octopusses\n\nThis is growing on me. Thanks for inventing it.\n\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1061,7 +1061,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \trev.commit_format = CMIT_FMT_EMAIL;\n> \trev.verbose_header = 1;\n> \trev.diff = 1;\n> -\trev.no_merges = 1;\n> +\trev.max_parents = MAX_PARENTS(1);\n\nIs there a reason not to choose a convention for which\n\n\trev.max_parents = 1;\n\nworks?\n\nWhat does --no-merges --merges do? I would find it most intuitive to\nerror out (since some people would want the last choice to win and\nothers want --merges-only --nonmerges-only to select the empty set),\nbut this patch does the backward-compatible thing, which is to show\nzero commits. Maybe it deserves a test case?\n\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1277,9 +1277,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> \t} else if (!strcmp(arg, \"--remove-empty\")) {\n> \t\trevs->remove_empty_trees = 1;\n> \t} else if (!strcmp(arg, \"--merges\")) {\n> -\t\trevs->merges_only = 1;\n> +\t\trevs->min_parents = MIN_PARENTS(2);\n\nWhy not \"revs->min_parents = 2;\"?\n\n> \t} else if (!strcmp(arg, \"--no-merges\")) {\n> -\t\trevs->no_merges = 1;\n> +\t\trevs->max_parents = MAX_PARENTS(1);\n> +\t} else if (!prefixcmp(arg, \"--min-parents=\")) {\n> +\t\trevs->min_parents = MIN_PARENTS(atoi(arg+14));\n> +\t} else if (!prefixcmp(arg, \"--max-parents=\")) {\n> +\t\trevs->max_parents = MAX_PARENTS(atoi(arg+14));\n\nIt would be nicer to error out for malformed numbers. That's\na separate topic, though --- you have plenty of company.\n\n> @@ -2029,10 +2033,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi\n> \t\treturn commit_ignore;\n> \tif (revs->min_age != -1 && (commit->date > revs->min_age))\n> \t\treturn commit_ignore;\n> -\tif (revs->no_merges && commit->parents && commit->parents->next)\n> -\t\treturn commit_ignore;\n> -\tif (revs->merges_only && !(commit->parents && commit->parents->next))\n> -\t\treturn commit_ignore;\n> +\tif (revs->min_parents || revs->max_parents) {\n> +\t\tint n = 0;\n> +\t\tstruct commit_list *p;\n> +\t\tfor (p = commit->parents; p; p = p->next)\n> +\t\t\tn++;\n> +\t\tif ((MIN_PARENTS(n) < revs->min_parents) ||\n> +\t\t (MAX_PARENTS(n) < revs->max_parents)) /* max is inv. */\n> +\t\t\treturn commit_ignore;\n\nSane. If we feared enormous parent lists we could do\n\n\tfor (p = commit->parents; p && n <= 7 - revs->max_parents; p = p->next)\n\t\tn++;\n\nbut I suspect that's slower.\n\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -20,6 +20,11 @@\n> #define DECORATE_SHORT_REFS\t1\n> #define DECORATE_FULL_REFS\t2\n> \n> +/* limit to used range */\n> +#define MIN_PARENTS(m)\t({ unsigned int __n = (m); (__n < 0) ? 0 : (__n > 7) ? 7 : __n; })\n> +/* invert fox MAX so that default = 0 -> infinity */\n> +#define MAX_PARENTS(m)\t({ unsigned int __n = (m); (__n < 0) ? 7 : (__n > 7) ? 0 : 7 - __n;})\n\nStatement expressions don't work in most non-gcc compilers (but\ninline functions do).\n\nHope that helps,\nJonathan\n"},{"id":"163676","messageId":"20110318211418.GA23407@elie","threadId":"26762","inReplyTo":"182c5ece22835559a9762e242adb701547cf6575.1300459017.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-18T21:14:19Z","receivedAt":"2011-03-18T21:14:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael J Gruber wrote:\n\n> [Subject: rev-list --min-parents,--max-parents: doc and test and completion]\n>\n> This also adds test for \"--merges\" and \"--no-merges\" which we did not\n> have so far.\n\nMinor nit: the life of reviewers is easier when the documentation and\ntests are squashed with the implementation (or if anything, the\ndocumentation comes first) so they can look at what the code tries to\ndo before seeing how it does it.\n\n> --- a/t/t6009-rev-list-parent.sh\n> +++ b/t/t6009-rev-list-parent.sh\n> @@ -1,6 +1,6 @@\n[...]\n> -test_description='properly cull all ancestors'\n> +test_description='ancestor culling and limitting by parent number'\n\nsp: limiting\n\nThanks for updating the description. It's easy to forget.\n\n> @@ -28,4 +28,72 @@ test_expect_success 'one is ancestor of others and should not be shown' '\n[...]\n> +check_revlist () {\n> +\t> expect &&\n> +\tfor c in $2; do echo \"$c\" >> expect; done &&\n> +\tgit rev-list $1 master | git name-rev --name-only --tags --stdin >actual &&\n> +\ttest_cmp expect actual\n> +}\n\nPedantic nits:\n\n * if global functions are defined before all test_expect_success stanzas,\n (1) it is easier to reuse them in tests throughout the script;\n (2) it is easy to see at a glance what primitives a test script\n will be using when getting acquainted with it and what\n syntax might be worth lifting to test-lib or other test\n scripts when shopping for that\n * style: no space between >redirection operators and their argument\n for consistency (maybe because > &2 is a syntax error)\n * might be simpler to pass one rev per argument. Like so:\n\n\trev_list_args=$1 &&\n\tshift &&\n\tprintf '%s\\n' \"$@\" >expect &&\n\tgit rev-list $rev_list_args master ...\n\n Avoiding the for loop means errors from 'echo' before the last\n iteration are not ignored; a more verbose way to write the same\n thing is\n\n\tfor commit\n\tdo\n\t\tprintf '%s\\n' \"$commit\" ||\n\t\treturn\n\tdone >expect &&\n\tgit rev-list ...\n * this does not catch errors from rev-list! how about\n\n\tgit rev-list $rev_list_args master >rev-list &&\n\tgit name-rev ... actual &&\n\ttest_cmp expect actual\n\n Junio will probably complain that this is not meant to be a test\n for name-rev; indeed, filling \"expect\" by computing commit names\n might be even better (because faster).\n\n> +test_expect_success 'rev-list roots' '\n> +test_expect_success 'rev-list no merges' '\n> +test_expect_success 'rev-list no octopusses' '\n> +test_expect_success 'rev-list no roots' '\n> +test_expect_success 'rev-list merges' '\n\nNeat. :)\n\n\n> +test_expect_success 'rev-list octopus' '\n> +\n> +\tcheck_revlist \"--min-parents=3\" \"octopus\"\n> +'\n\nMight make sense to check a dodecapus (with --min-parents=3 still) as\nwell.\n\n> +test_expect_success 'rev-list override and infinities' '\n\nAh, here's the test I suggested in reply to patch 1. But this is not\nabout overriding but the lack thereof, no? So I'd be happier reading\nsomething like\n\n\ttest_expect_success '--no-merges --merges yields the empty set' '\n\t\tcheck_revlist \"--no-merges --merges\" &&\n\t\tcheck_revlist \"--min-parents=2 --no-merges\"\n\t'\n\n\ttest_expect_success 'last --max-parents wins' '\n\t\tcheck_revlist \"--min-parents=2 --no-merges --max-parents=3\" octopus normalmerge &&\n\t\tcheck_revlist \"--min-parents=2 --max-parents=3 --no-merges\" ...\n\t'\n\n\ttest_expect_success '--max-parents=infinity' '\n\t\t...\n\t'\n\n\ttest_expect_success '--min-parents=infinity' '\n\t\t...\n\t'\n\n\ttest_expect_failure '--max-parents=gobbledegood errors out' '\n\t\t...\n\t'\n\nThanks for your attention to detail, and hope that helps.\n\nCiao,\nJonathan\n"},{"id":"163678","messageId":"7v1v24w2dq.fsf@alter.siamese.dyndns.org","threadId":"26762","inReplyTo":"20110318204854.GA23331@elie","subject":"Re: [PATCH 1/3] revision.c: introduce --min-parents and --max-parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-18T21:21:53Z","receivedAt":"2011-03-18T21:21:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Is there a reason not to choose a convention for which\n>\n> \trev.max_parents = 1;\n>\n> works?\n\nI also hate the peculiar convention used in this patch. Is it worth this\nugliness to avoid adding a line in init_revisions() to initialize it to\nthe infinity value?\n"},{"id":"163868","messageId":"4D86FEED.2030204@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110318193401.GA27825@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] revision.c: introduce --min-parents and --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-21T07:31:57Z","receivedAt":"2011-03-21T07:31:57Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 18.03.2011 20:34:\n> On Fri, Mar 18, 2011 at 03:50:23PM +0100, Michael J Gruber wrote:\n> \n>> Introduce --min-parents and --max-parents which take values 0,...,7 and\n>> limit the revisions to those commits which have at least resp. at most\n>> that many commits, where --max-parents=8 denotes --max-parents=infinity\n>> (i.e. no upper limit). In fact, 7 (or any negative number) does, but 8\n>> is infinity sideways 8-)\n> \n> In practice, I don't think anybody is all that interested in\n> differentiating octopus merges by their numbers of parents. But the\n> choice of \"7\" as a maximum seems kind of arbitrary. There are commits in\n> linux-2.6 with up to 31 parents (once upon a time we had an arbitrary\n> limit of 16 parents, but that was lifted in v1.6.0).\n> \n> You mention below that this fits in three 3 bits in rev_info. I don't\n> think bits are precious in rev_info, though, as they are in other\n> revision-related structs. We only have one such struct per walk.\n> \n> If it were just an implementation issue, I would say fine, we can tweak\n> it later if somebody really cares. But we are cementing \"7\" as the value\n> for infinity in the user-facing interface. Wouldn't a value like \"-1\" or\n> \"infinity\" be more appropriate?\n\n-1 works and \"infinity\" is difficult to parse.\n\nWe could advocate \"-1\" only, rather than the max which is infinity.\n\nRegarding using an \"unbounded\" int: I think it is difficult to\ndistinguish between an unspecified max (value 0 from the initialisation)\nand a specified \"unbounded\" if I don't do the range inversion, which\nobviously needs a bounded range. But I'll recheck.\n\nRegarding the 8: Besides the connotation of being infinity sideways,\nthere's also the undeniable fact that everything with fewer than 8 isn't\nreally an octo-puss, so the choice of 3 bits is indeed natural!\n\n>> In particular:\n>>\n>> --max-parents=1: no merges\n>> --min-parents=2: merges only\n>> --max-parents=0: only roots\n>> --min-parents=3: only octopusses\n>>\n>> --min-parents=n --max-parents=m with n>m gives you what you ask for\n>> (nothing) just like --merges --no-merges does, but at least for an\n>> obvious reason.\n> \n> I like this. It's much more natural than the list syntax I suggested\n> earlier, and it handles ranges in an obvious way. It doesn't allow\n> selecting, e.g., root commits and merges, but not regular commits. But I\n> really don't see the point in supporting that.\n> \n>> @@ -2029,10 +2033,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi\n>> \t\treturn commit_ignore;\n>> \tif (revs->min_age != -1 && (commit->date > revs->min_age))\n>> \t\treturn commit_ignore;\n>> -\tif (revs->no_merges && commit->parents && commit->parents->next)\n>> -\t\treturn commit_ignore;\n>> -\tif (revs->merges_only && !(commit->parents && commit->parents->next))\n>> -\t\treturn commit_ignore;\n>> +\tif (revs->min_parents || revs->max_parents) {\n>> +\t\tint n = 0;\n>> +\t\tstruct commit_list *p;\n>> +\t\tfor (p = commit->parents; p; p = p->next)\n>> +\t\t\tn++;\n>> +\t\tif ((MIN_PARENTS(n) < revs->min_parents) ||\n>> +\t\t (MAX_PARENTS(n) < revs->max_parents)) /* max is inv. */\n>> +\t\t\treturn commit_ignore;\n>> +\t}\n> \n> You did the obvious optimization not to count parents if we don't care\n> about min/max. You could also do something like:\n> \n> switch (revs->max_parents) {\n> case 0:\n> if (commit->parents)\n> return commit_ignore;\n> break;\n> case 1:\n> if (commit->parents && commit->parents->next)\n> return commit_ignore;\n> break;\n> default:\n> if (count_parents(commit) > commit->max_parents)\n> return commit_ignore;\n> break;\n> }\n> \n> which more closely matches the original code (you would also need to do\n> the same for min_parents, and obviously this code ignores the\n> max-parents-is-inverted thing).\n> \n> I'm not sure it would buy much in practice, though. Commits with more\n> than 2 parents are rare, so unnecessarily counting all of them just to\n> find out something is a merge is probably not going to create a\n> measurable slowdown.\n\nThat was exactly my thinking.\n\n> \n>> +/* limit to used range */\n>> +#define MIN_PARENTS(m)\t({ unsigned int __n = (m); (__n < 0) ? 0 : (__n > 7) ? 7 : __n; })\n>> +/* invert fox MAX so that default = 0 -> infinity */\n>> +#define MAX_PARENTS(m)\t({ unsigned int __n = (m); (__n < 0) ? 7 : (__n > 7) ? 0 : 7 - __n;})\n> \n> Hmm. You assign the input to an unsigned int, and then compare it to 0.\n> Won't that always be false?\n\nYes, sorry. I had an int there first, the unsigned gives the \"negative =\ninfinity\" automatically. I probably couldn't decide what\n--min-parents=-1 should mean: should \"-1\" always denote \"infinity\", or\nshould it be equivalent to \"0\"? Switching from signed to unsigned\nchanged this.\n\n> The inversion trick is clever to make a bzero'd rev_info work, but I\n> think the code would be more obvious without it. And you really have to\n> call init_revisions() anyway, so initializing it to a maximum there (or\n> -1) would be acceptable (e.g., see how max_age and min_age work).\n\nI think for all users of the code it doesn't make a difference since\nthey would call the macros anyways (for the range check), so it would be\nonly that single comparison in the walker (which has a comment) that\nwould become clearer.\n\nMichael\n"},{"id":"163869","messageId":"4D870157.2070309@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110318194113.GA2215@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] rev-list and friends: --min-parents, --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-21T07:42:15Z","receivedAt":"2011-03-21T07:42:15Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 18.03.2011 20:41:\n> On Fri, Mar 18, 2011 at 03:50:22PM +0100, Michael J Gruber wrote:\n> \n>> As opposed to the RFD it replaces, this is a real patch series with\n>> documentation and tests, and it even comes with boiler plate. It should\n>> make all of Jeff's and Junio's dreams come true (as far as revision\n>> limiting by parent number goes).\n>>\n>> 1/3 introduces the new options (and has a proper commit message)\n>> 2/3 I noted along the way and could be applied earlier\n>> 3/3 depends on 1 and 2 and is the candy (doc, tests, completion)\n> \n> I didn't see it mentioned anywhere, but I assume this applies on\n> mg/rev-list-one-side-only (b1b4755). It has conflicts when applied on\n> master.\n> \n> (I know you probably know that, but it is for the benefit of other\n> reviewers).\n\nWell, yes, but that's in next already (and probably in master by the\ntime this is taking shape). If I had it based on something earlier it\nwould have applied cleanly but failed to compile which is fine for\nsomething meant to go to maint but a nuisance for pu material.\n\nMichael\n"},{"id":"163874","messageId":"4D8711DD.2040407@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110318211418.GA23407@elie","subject":"Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-21T08:52:45Z","receivedAt":"2011-03-21T08:52:45Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jonathan Nieder venit, vidit, dixit 18.03.2011 22:14:\n> Hi,\n> \n> Michael J Gruber wrote:\n> \n>> [Subject: rev-list --min-parents,--max-parents: doc and test and completion]\n>>\n>> This also adds test for \"--merges\" and \"--no-merges\" which we did not\n>> have so far.\n> \n> Minor nit: the life of reviewers is easier when the documentation and\n> tests are squashed with the implementation (or if anything, the\n> documentation comes first) so they can look at what the code tries to\n> do before seeing how it does it.\n\nCome on, with a cover-letter saying doc and tests are in 3/3, how much\neffort is it to read that before 1/3 if you care?\n\nThe tests are bogus before the code and the doc pointless before it.\n\nSquashing 1 and 3 is okay, of course. For my own digestion, smaller\nbites are better.\n\n>> --- a/t/t6009-rev-list-parent.sh\n>> +++ b/t/t6009-rev-list-parent.sh\n>> @@ -1,6 +1,6 @@\n> [...]\n>> -test_description='properly cull all ancestors'\n>> +test_description='ancestor culling and limitting by parent number'\n> \n> sp: limiting\n> \n> Thanks for updating the description. It's easy to forget.\n\nI would have preferred to assign an extra test number (since the\nrelation between the above is a bit weak) but the numbers are all used\nup in that range.\n\n> \n>> @@ -28,4 +28,72 @@ test_expect_success 'one is ancestor of others and should not be shown' '\n> [...]\n>> +check_revlist () {\n>> +\t> expect &&\n>> +\tfor c in $2; do echo \"$c\" >> expect; done &&\n>> +\tgit rev-list $1 master | git name-rev --name-only --tags --stdin >actual &&\n>> +\ttest_cmp expect actual\n>> +}\n> \n> Pedantic nits:\n> \n> * if global functions are defined before all test_expect_success stanzas,\n> (1) it is easier to reuse them in tests throughout the script;\n> (2) it is easy to see at a glance what primitives a test script\n> will be using when getting acquainted with it and what\n> syntax might be worth lifting to test-lib or other test\n> scripts when shopping for that\n> * style: no space between >redirection operators and their argument\n> for consistency (maybe because > &2 is a syntax error)\n> * might be simpler to pass one rev per argument. Like so:\n> \n> \trev_list_args=$1 &&\n> \tshift &&\n> \tprintf '%s\\n' \"$@\" >expect &&\n> \tgit rev-list $rev_list_args master ...\n> \n> Avoiding the for loop means errors from 'echo' before the last\n> iteration are not ignored; a more verbose way to write the same\n\nDo we really need to safe-guard echo and prints?\n\n> thing is\n> \n> \tfor commit\n> \tdo\n> \t\tprintf '%s\\n' \"$commit\" ||\n> \t\treturn\n> \tdone >expect &&\n> \tgit rev-list ...\n> * this does not catch errors from rev-list! how about\n> \n> \tgit rev-list $rev_list_args master >rev-list &&\n> \tgit name-rev ... actual &&\n> \ttest_cmp expect actual\n\nThanks for both of the above, that makes things much better. Although I\nhave to treat the case with empty rev-list specially now, or use the\nverbose version.\n\n> \n> Junio will probably complain that this is not meant to be a test\n> for name-rev; indeed, filling \"expect\" by computing commit names\n> might be even better (because faster).\n\nWell, we do this in other places as well, and it makes the test much\nmore readable. Alternatively, I could use rev-parse on the args, which\nis faster than name-rev on the rev-list output. That makes it less\nreadable in case a test breaks, though. Anyways, I'll go for that.\n\n> \n>> +test_expect_success 'rev-list roots' '\n>> +test_expect_success 'rev-list no merges' '\n>> +test_expect_success 'rev-list no octopusses' '\n>> +test_expect_success 'rev-list no roots' '\n>> +test_expect_success 'rev-list merges' '\n> \n> Neat. :)\n> \n> \n>> +test_expect_success 'rev-list octopus' '\n>> +\n>> +\tcheck_revlist \"--min-parents=3\" \"octopus\"\n>> +'\n> \n> Might make sense to check a dodecapus (with --min-parents=3 still) as\n> well.\n\nDodeka, really? I leave that to you.\nI might add a tetrapus, though.\n\n> \n>> +test_expect_success 'rev-list override and infinities' '\n> \n> Ah, here's the test I suggested in reply to patch 1. But this is not\n> about overriding but the lack thereof, no? So I'd be happier reading\n> something like\n> \n> \ttest_expect_success '--no-merges --merges yields the empty set' '\n> \t\tcheck_revlist \"--no-merges --merges\" &&\n> \t\tcheck_revlist \"--min-parents=2 --no-merges\"\n> \t'\n> \n> \ttest_expect_success 'last --max-parents wins' '\n\nwins against? Against itself, i.e. it overrides previous occurences. But\nI'll separate these.\n\n> \t\tcheck_revlist \"--min-parents=2 --no-merges --max-parents=3\" octopus normalmerge &&\n> \t\tcheck_revlist \"--min-parents=2 --max-parents=3 --no-merges\" ...\n> \t'\n> \n> \ttest_expect_success '--max-parents=infinity' '\n> \t\t...\n> \t'\n> \n> \ttest_expect_success '--min-parents=infinity' '\n> \t\t...\n> \t'\n> \n> \ttest_expect_failure '--max-parents=gobbledegood errors out' '\n> \t\t...\n> \t'\n\nI don't really want to parse for the string \"infinity\" nor go for strtol\ninstead of atoi. Why shouldn't something \"unparseable\" be 0?\n\nMichael\n"},{"id":"163875","messageId":"4D871401.6020900@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110318194802.GB27825@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-21T09:01:53Z","receivedAt":"2011-03-21T09:01:53Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"(Working my way backwards through the responses.)\n\nJeff King venit, vidit, dixit 18.03.2011 20:48:\n> On Fri, Mar 18, 2011 at 03:50:25PM +0100, Michael J Gruber wrote:\n> \n>> +--min-parents::\n>> +--max-parents::\n>> +\n>> +\tShow only commits which have at least resp. at most that many\n>> +\tcommits, where `--max-parents=8` denotes infinity (i.e. no upper\n>> +\tlimit). In fact, 7 (or any negative number) does, but 8 is\n>> +\tinfinity sideways 8-)\n> \n> I didn't quite parse this \"resp.\" in the middle.\n\nWell, there are two options (--min-parents, --max-parents) which we\ndescribe in one paragraph. Sooo...\n\n> \n>> ++\n>> +In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is\n>> +`--merges` (only), `--max-parents=0` gives all root commits and\n>> +`--min-parents=3` all octopusses.\n> \n> Spelling nit: the plural of octopus is \"octopuses\" (or \"octopi\" or\n> \"octopodes\", depending on which dictionary you consult).\n> \n> I think it's good to equate --{no-,}merges with their\n> --{max,min}-parents counterparts here. We should probably do the same\n> for the reverse association, like:\n> \n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index fcc4c6c..f32509a 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -226,11 +226,13 @@ endif::git-rev-list[]\n> \n> --merges::\n> \n> -\tPrint only merge commits.\n> +\tPrint only merge commits. This is equivalent to\n> +\t`--min-parents=2`.\n> \n> --no-merges::\n> \n> -\tDo not print commits with more than one parent.\n> +\tDo not print commits with more than one parent. This is\n> +\tequivalent to `--max-parents=1`.\n> \n> --min-parents::\n> --max-parents::\n> \n> That way it is obvious that \"--merges\" cancels a previous --min-parents\n> on the command line (maybe the text should be \"this is an alias for...\"\n> to make it clear that doing it is exactly the same).\n\nYes, that is helpful. I have doubts about \"alias\" for. Without wanting\nto sound elitist or something, I have the impression that we start\ncatering for users who understand \"equivalent\" more reliably than \"alias\".\n\n> This provides a user-friendly form for the two common cases. Do we care\n> about the other cases, or adding a single multi-state flag like\n> --show-parents={all,merges,etc}? They other ones are rare enough and\n\nI know that name is not a serious suggestion...\n\n> particular enough that it is probably fine for people to just use\n> {min,max}-parents directly. So I would say what you have is good.\n\nThanks :)\n\nI would go for something multi-state like only If I were allowed to\nstamp on the existing --merges/--no-merges, which I am not. ;)\n\nMichael\n"},{"id":"163877","messageId":"4D8719C7.7020905@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110318204854.GA23331@elie","subject":"Re: [PATCH 1/3] revision.c: introduce --min-parents and --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-21T09:26:31Z","receivedAt":"2011-03-21T09:26:31Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jonathan Nieder venit, vidit, dixit 18.03.2011 21:48:\n> Hi,\n> \n> Michael J Gruber wrote:\n> \n>> --max-parents=1: no merges\n>> --min-parents=2: merges only\n>> --max-parents=0: only roots\n>> --min-parents=3: only octopusses\n> \n> This is growing on me. Thanks for inventing it.\n\nThanks for teaching me a new collocation (to grow on sb) ;)\n\n> \n>> --- a/builtin/log.c\n>> +++ b/builtin/log.c\n>> @@ -1061,7 +1061,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>> \trev.commit_format = CMIT_FMT_EMAIL;\n>> \trev.verbose_header = 1;\n>> \trev.diff = 1;\n>> -\trev.no_merges = 1;\n>> +\trev.max_parents = MAX_PARENTS(1);\n> \n> Is there a reason not to choose a convention for which\n> \n> \trev.max_parents = 1;\n> \n> works?\n\nYes.\n\nOh, you also want to know what it is? I was somehow fixed on using a\nlimited number of bits (probably because of no_merges etc. and the\nmentioning of tri-state), therefore using a bounded range.\n\nAlso, I was keen of having \"8\" be infinity. (My fingers are trained to\nenter \"`8\" to get \"\\infty\".) But I've come to realise I'm the only one.\n\n> \n> What does --no-merges --merges do? I would find it most intuitive to\n> error out (since some people would want the last choice to win and\n> others want --merges-only --nonmerges-only to select the empty set),\n> but this patch does the backward-compatible thing, which is to show\n\nYes, a true case of being \"backward(!)-compatible\"...\n\n> zero commits. Maybe it deserves a test case?\n> \n>> --- a/revision.c\n>> +++ b/revision.c\n>> @@ -1277,9 +1277,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>> \t} else if (!strcmp(arg, \"--remove-empty\")) {\n>> \t\trevs->remove_empty_trees = 1;\n>> \t} else if (!strcmp(arg, \"--merges\")) {\n>> -\t\trevs->merges_only = 1;\n>> +\t\trevs->min_parents = MIN_PARENTS(2);\n> \n> Why not \"revs->min_parents = 2;\"?\n\nFor consistency.\n\n> \n>> \t} else if (!strcmp(arg, \"--no-merges\")) {\n>> -\t\trevs->no_merges = 1;\n>> +\t\trevs->max_parents = MAX_PARENTS(1);\n>> +\t} else if (!prefixcmp(arg, \"--min-parents=\")) {\n>> +\t\trevs->min_parents = MIN_PARENTS(atoi(arg+14));\n>> +\t} else if (!prefixcmp(arg, \"--max-parents=\")) {\n>> +\t\trevs->max_parents = MAX_PARENTS(atoi(arg+14));\n> \n> It would be nicer to error out for malformed numbers. That's\n> a separate topic, though --- you have plenty of company.\n\nmin_age etc., yes.\n\n> \n>> @@ -2029,10 +2033,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi\n>> \t\treturn commit_ignore;\n>> \tif (revs->min_age != -1 && (commit->date > revs->min_age))\n>> \t\treturn commit_ignore;\n>> -\tif (revs->no_merges && commit->parents && commit->parents->next)\n>> -\t\treturn commit_ignore;\n>> -\tif (revs->merges_only && !(commit->parents && commit->parents->next))\n>> -\t\treturn commit_ignore;\n>> +\tif (revs->min_parents || revs->max_parents) {\n>> +\t\tint n = 0;\n>> +\t\tstruct commit_list *p;\n>> +\t\tfor (p = commit->parents; p; p = p->next)\n>> +\t\t\tn++;\n>> +\t\tif ((MIN_PARENTS(n) < revs->min_parents) ||\n>> +\t\t (MAX_PARENTS(n) < revs->max_parents)) /* max is inv. */\n>> +\t\t\treturn commit_ignore;\n> \n> Sane. If we feared enormous parent lists we could do\n> \n> \tfor (p = commit->parents; p && n <= 7 - revs->max_parents; p = p->next)\n> \t\tn++;\n> \n> but I suspect that's slower.\n> \n>> --- a/revision.h\n>> +++ b/revision.h\n>> @@ -20,6 +20,11 @@\n>> #define DECORATE_SHORT_REFS\t1\n>> #define DECORATE_FULL_REFS\t2\n>> \n>> +/* limit to used range */\n>> +#define MIN_PARENTS(m)\t({ unsigned int __n = (m); (__n < 0) ? 0 : (__n > 7) ? 7 : __n; })\n>> +/* invert fox MAX so that default = 0 -> infinity */\n>> +#define MAX_PARENTS(m)\t({ unsigned int __n = (m); (__n < 0) ? 7 : (__n > 7) ? 0 : 7 - __n;})\n> \n> Statement expressions don't work in most non-gcc compilers (but\n> inline functions do).\n> \n> Hope that helps,\n> Jonathan\n\nYes, that would have helped, although I'm going for simple \"unbounded\"\nint's in v2 without the need for those range enforcing macros/inlines.\n\nAm I allowed to do\n\nunsigned max_parent = -1;\n\nto get \"infinity for practical purposes\" on all compilers?\n\nMichael\n"},{"id":"163887","messageId":"cover.1300702130.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"4D870157.2070309@drmicha.warpmail.net","subject":"[PATCHv2 0/3] rev-list and friends: --min-parents, --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-21T10:14:04Z","receivedAt":"2011-03-21T10:14:04Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"In v2, based on the input from Jeff, Junio and Jonathan, I switched to\nordinary signed integers for --min-parent and --max-parent. Negative\nparameters to the latter mean infinity. (Thus, no more range/macro magic.)\n\nAlso, I optimized the tests and made the structure clearer (hopefully).\n\nLastly, I rearranged the tests so that the unrelated cleanup comes first.\n\nBased on mg/rev-list-one-side-only (in next) to save Junio a build conflict\nresolution.\nMichael J Gruber (3):\n t6009: use test_commit() from test-lib.sh\n revision.c: introduce --min-parents and --max-parents\n rev-list --min-parents,--max-parents: doc and test and completion\n\n Documentation/git-rev-list.txt | 2 +\n Documentation/rev-list-options.txt | 17 +++++-\n builtin/log.c | 2 +-\n builtin/rev-list.c | 2 +\n builtin/rev-parse.c | 2 +\n contrib/completion/git-completion.bash | 1 +\n revision.c | 24 ++++++--\n revision.h | 4 +-\n t/t6009-rev-list-parent.sh | 97 ++++++++++++++++++++++++++++----\n 9 files changed, 128 insertions(+), 23 deletions(-)\n\n-- \n1.7.4.1.511.g72e46\n"},{"id":"163888","messageId":"1f39cb4a4fc02a50fa869bef412ea0f2a527e480.1300702130.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"4D870157.2070309@drmicha.warpmail.net","subject":"[PATCHv2 1/3] t6009: use test_commit() from test-lib.sh","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-21T10:14:05Z","receivedAt":"2011-03-21T10:14:05Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Signed-off-by: Michael J Gruber \n---\n t/t6009-rev-list-parent.sh | 15 ++++-----------\n 1 files changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\nindex 52f7b27..0f0e457 100755\n--- a/t/t6009-rev-list-parent.sh\n+++ b/t/t6009-rev-list-parent.sh\n@@ -4,25 +4,18 @@ test_description='properly cull all ancestors'\n \n . ./test-lib.sh\n \n-commit () {\n-\ttest_tick &&\n-\techo $1 >file &&\n-\tgit commit -a -m $1 &&\n-\tgit tag $1\n-}\n-\n test_expect_success setup '\n \n \ttouch file &&\n \tgit add file &&\n \n-\tcommit one &&\n+\ttest_commit one &&\n \n \ttest_tick=$(($test_tick - 2400)) &&\n \n-\tcommit two &&\n-\tcommit three &&\n-\tcommit four &&\n+\ttest_commit two &&\n+\ttest_commit three &&\n+\ttest_commit four &&\n \n \tgit log --pretty=oneline --abbrev-commit\n '\n-- \n1.7.4.1.511.g72e46\n"},{"id":"163889","messageId":"41d12c53966faa9c9cb5acc7646e9098dcd911ea.1300702130.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"4D870157.2070309@drmicha.warpmail.net","subject":"[PATCHv2 2/3] revision.c: introduce --min-parents and --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-21T10:14:06Z","receivedAt":"2011-03-21T10:14:06Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"--merges and --no-merges are named confusingly and cannot be overridden\nby each other, but they are there to stay for plumbers' sake.\n\nIntroduce --min-parents and --max-parents which limit the revisions to\nthose commits which have at least resp. at most that many commits, where\nnegative arguments for --max-parents= denote infinity (i.e. no upper\nlimit).\n\nIn particular:\n\n--max-parents=1: no merges\n--min-parents=2: merges only\n--max-parents=0: only roots\n--min-parents=3: only octopusses\n\n--min-parents=n --max-parents=m with n>m gives you what you ask for\n(nothing) just like --merges --no-merges does, but at least for an\nobvious reason.\n\nImplementation note:\n\nWe compute the number of parents only when we limit by that, so there\nis no performance impact when there are no limiters.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/log.c | 2 +-\n builtin/rev-list.c | 2 ++\n builtin/rev-parse.c | 2 ++\n revision.c | 24 +++++++++++++++++-------\n revision.h | 4 ++--\n 5 files changed, 24 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 796e9e5..4a0f78d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1061,7 +1061,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \trev.commit_format = CMIT_FMT_EMAIL;\n \trev.verbose_header = 1;\n \trev.diff = 1;\n-\trev.no_merges = 1;\n+\trev.max_parents = 1;\n \tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n \trev.subject_prefix = fmt_patch_subject_prefix;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex f458cb7..7b276e0 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -16,6 +16,8 @@ static const char rev_list_usage[] =\n \" --min-age=\\n\"\n \" --sparse\\n\"\n \" --no-merges\\n\"\n+\" --min-parents=\\n\"\n+\" --max-parents=\\n\"\n \" --remove-empty\\n\"\n \" --all\\n\"\n \" --branches\\n\"\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex a5a1c86..9940546 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -48,6 +48,8 @@ static int is_rev_argument(const char *arg)\n \t\t\"--max-count=\",\n \t\t\"--min-age=\",\n \t\t\"--no-merges\",\n+\t\t\"--min-parents=\",\n+\t\t\"--max-parents=\",\n \t\t\"--objects\",\n \t\t\"--objects-edge\",\n \t\t\"--parents\",\ndiff --git a/revision.c b/revision.c\nindex e96c281..8d48870 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -941,6 +941,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n \trevs->min_age = -1;\n \trevs->skip_count = -1;\n \trevs->max_count = -1;\n+\trevs->max_parents = -1;\n \n \trevs->commit_format = CMIT_FMT_DEFAULT;\n \n@@ -1277,9 +1278,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--remove-empty\")) {\n \t\trevs->remove_empty_trees = 1;\n \t} else if (!strcmp(arg, \"--merges\")) {\n-\t\trevs->merges_only = 1;\n+\t\trevs->min_parents = 2;\n \t} else if (!strcmp(arg, \"--no-merges\")) {\n-\t\trevs->no_merges = 1;\n+\t\trevs->max_parents = 1;\n+\t} else if (!prefixcmp(arg, \"--min-parents=\")) {\n+\t\trevs->min_parents = atoi(arg+14);\n+\t} else if (!prefixcmp(arg, \"--max-parents=\")) {\n+\t\trevs->max_parents = atoi(arg+14);\n \t} else if (!strcmp(arg, \"--boundary\")) {\n \t\trevs->boundary = 1;\n \t} else if (!strcmp(arg, \"--left-right\")) {\n@@ -1298,7 +1303,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\t\tdie(\"--cherry is incompatible with --left-only\");\n \t\trevs->cherry_mark = 1;\n \t\trevs->right_only = 1;\n-\t\trevs->no_merges = 1;\n+\t\trevs->max_parents = 1;\n \t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--count\")) {\n \t\trevs->count = 1;\n@@ -2029,10 +2034,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi\n \t\treturn commit_ignore;\n \tif (revs->min_age != -1 && (commit->date > revs->min_age))\n \t\treturn commit_ignore;\n-\tif (revs->no_merges && commit->parents && commit->parents->next)\n-\t\treturn commit_ignore;\n-\tif (revs->merges_only && !(commit->parents && commit->parents->next))\n-\t\treturn commit_ignore;\n+\tif (revs->min_parents || (revs->max_parents >= 0)) {\n+\t\tint n = 0;\n+\t\tstruct commit_list *p;\n+\t\tfor (p = commit->parents; p; p = p->next)\n+\t\t\tn++;\n+\t\tif ((n < revs->min_parents) ||\n+\t\t ((revs->max_parents >= 0) && (n > revs->max_parents)))\n+\t\t\treturn commit_ignore;\n+\t}\n \tif (!commit_match(commit, revs))\n \t\treturn commit_ignore;\n \tif (revs->prune && revs->dense) {\ndiff --git a/revision.h b/revision.h\nindex ae94860..9fd8f30 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -41,8 +41,6 @@ struct rev_info {\n \t/* Traversal flags */\n \tunsigned int\tdense:1,\n \t\t\tprune:1,\n-\t\t\tno_merges:1,\n-\t\t\tmerges_only:1,\n \t\t\tno_walk:1,\n \t\t\tshow_all:1,\n \t\t\tremove_empty_trees:1,\n@@ -126,6 +124,8 @@ struct rev_info {\n \tint max_count;\n \tunsigned long max_age;\n \tunsigned long min_age;\n+\tint min_parents;\n+\tint max_parents;\n \n \t/* diff info for patches and for paths limiting */\n \tstruct diff_options diffopt;\n-- \n1.7.4.1.511.g72e46\n"},{"id":"163890","messageId":"8bad49d4e4897be623b7af3096498a5803dbbd89.1300702130.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"4D870157.2070309@drmicha.warpmail.net","subject":"[PATCHv2 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-21T10:14:07Z","receivedAt":"2011-03-21T10:14:07Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"This also adds test for \"--merges\" and \"--no-merges\" which we did not\nhave so far.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-rev-list.txt | 2 +\n Documentation/rev-list-options.txt | 17 ++++++-\n contrib/completion/git-completion.bash | 1 +\n t/t6009-rev-list-parent.sh | 84 +++++++++++++++++++++++++++++++-\n 4 files changed, 101 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt\nindex b08dfbc..4d2b0c5 100644\n--- a/Documentation/git-rev-list.txt\n+++ b/Documentation/git-rev-list.txt\n@@ -16,6 +16,8 @@ SYNOPSIS\n \t [ \\--sparse ]\n \t [ \\--merges ]\n \t [ \\--no-merges ]\n+\t [ \\--min-parents ]\n+\t [ \\--max-parents ]\n \t [ \\--first-parent ]\n \t [ \\--remove-empty ]\n \t [ \\--full-history ]\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 5c6850f..18d5534 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -72,11 +72,24 @@ endif::git-rev-list[]\n \n --merges::\n \n-\tPrint only merge commits.\n+\tPrint only merge commits. This is equivalent to `--min-parents=2`.\n \n --no-merges::\n \n-\tDo not print commits with more than one parent.\n+\tDo not print commits with more than one parent. This is\n+\tequivalent to `--max-parents=1`.\n+\n+--min-parents::\n+--max-parents::\n+\n+\tShow only commits which have at least resp. at most that many\n+\tcommits, where negative parameters for `--max-parents=` denote\n+\tinfinity (i.e. no upper\tlimit).\n++\n+In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is\n+`--merges` (only), `--max-parents=0` gives all root commits and\n+`--min-parents=3` all octopuses.\n+\n \n --first-parent::\n \tFollow only the first parent commit upon seeing a merge\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 3b1cc83..4da087e 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1577,6 +1577,7 @@ __git_log_common_options=\"\n \t--max-count=\n \t--max-age= --since= --after=\n \t--min-age= --until= --before=\n+\t--min-parents= --max-parents=\n \"\n # Options that go well for log and gitk (not shortlog)\n __git_log_gitk_options=\"\ndiff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\nindex 0f0e457..d460369 100755\n--- a/t/t6009-rev-list-parent.sh\n+++ b/t/t6009-rev-list-parent.sh\n@@ -1,9 +1,17 @@\n #!/bin/sh\n \n-test_description='properly cull all ancestors'\n+test_description='ancestor culling and limiting by parent number'\n \n . ./test-lib.sh\n \n+check_revlist () {\n+\trev_list_args=\"$1\" &&\n+\tshift &&\n+\tgit rev-parse \"$@\" >expect &&\n+\tgit rev-list $rev_list_args --all >actual &&\n+\ttest_cmp expect actual\n+}\n+ \n test_expect_success setup '\n \n \ttouch file &&\n@@ -28,4 +36,78 @@ test_expect_success 'one is ancestor of others and should not be shown' '\n \n '\n \n+test_expect_success 'setup roots, merges and octopuses' '\n+\n+\tgit checkout --orphan newroot &&\n+\ttest_commit five &&\n+\tgit checkout -b sidebranch two &&\n+\ttest_commit six &&\n+\tgit checkout -b anotherbranch three &&\n+\ttest_commit seven &&\n+\tgit checkout -b yetanotherbranch four &&\n+\ttest_commit eight &&\n+\tgit checkout master &&\n+\ttest_merge normalmerge newroot &&\n+\ttest_tick &&\n+\tgit merge -m tripus sidebranch anotherbranch &&\n+\tgit tag tripus &&\n+\tgit checkout -b tetrabranch normalmerge &&\n+\ttest_tick &&\n+\tgit merge -m tetrapus sidebranch anotherbranch yetanotherbranch &&\n+\tgit tag tetrapus &&\n+\tgit checkout master \n+'\n+\n+test_expect_success 'rev-list roots' '\n+\n+\tcheck_revlist \"--max-parents=0\" one five\n+'\n+\n+test_expect_success 'rev-list no merges' '\n+\n+\tcheck_revlist \"--max-parents=1\" one eight seven six five four three two &&\n+\tcheck_revlist \"--no-merges\" one eight seven six five four three two\n+'\n+\n+test_expect_success 'rev-list no octopuses' '\n+\n+\tcheck_revlist \"--max-parents=2\" one normalmerge eight seven six five four three two\n+'\n+\n+test_expect_success 'rev-list no roots' '\n+\n+\tcheck_revlist \"--min-parents=1\" tetrapus tripus normalmerge eight seven six four three two\n+'\n+\n+test_expect_success 'rev-list merges' '\n+\n+\tcheck_revlist \"--min-parents=2\" tetrapus tripus normalmerge &&\n+\tcheck_revlist \"--merges\" tetrapus tripus normalmerge\n+'\n+\n+test_expect_success 'rev-list octopus' '\n+\n+\tcheck_revlist \"--min-parents=3\" tetrapus tripus\n+'\n+\n+test_expect_success 'rev-list ordinary commits' '\n+\n+\tcheck_revlist \"--min-parents=1 --max-parents=1\" eight seven six four three two\n+'\n+\n+test_expect_success 'rev-list --merges --no-merges yields empty set' '\n+\n+\tcheck_revlist \"--min-parents=2 --no-merges\" &&\n+\tcheck_revlist \"--merges --no-merges\" &&\n+\tcheck_revlist \"--no-merges --merges\"\n+'\n+\n+test_expect_success 'rev-list override and infinities' '\n+\n+\tcheck_revlist \"--min-parents=2 --max-parents=1 --max-parents=3\" tripus normalmerge &&\n+\tcheck_revlist \"--min-parents=1 --min-parents=2 --max-parents=7\" tetrapus tripus normalmerge &&\n+\tcheck_revlist \"--min-parents=2 --max-parents=8\" tetrapus tripus normalmerge &&\n+\tcheck_revlist \"--min-parents=2 --max-parents=-1\" tetrapus tripus normalmerge\n+'\n+\n test_done\n-- \n1.7.4.1.511.g72e46\n"},{"id":"163893","messageId":"20110321105431.GB16334@sigill.intra.peff.net","threadId":"26762","inReplyTo":"4D871401.6020900@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-21T10:54:31Z","receivedAt":"2011-03-21T10:54:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 21, 2011 at 10:01:53AM +0100, Michael J Gruber wrote:\n\n> >> +\tShow only commits which have at least resp. at most that many\n> >> +\tcommits, where `--max-parents=8` denotes infinity (i.e. no upper\n> >> +\tlimit). In fact, 7 (or any negative number) does, but 8 is\n> >> +\tinfinity sideways 8-)\n> > \n> > I didn't quite parse this \"resp.\" in the middle.\n> \n> Well, there are two options (--min-parents, --max-parents) which we\n> describe in one paragraph. Sooo...\n\nI figured out what you were trying to say. I just had never seen the\nabbreviation \"resp.\" before. I guessed it meant \"respectively\", but the\nsyntax is all wrong.\n\nDigging around via google, I was able to find that it is a mathematical\nterm with a specific syntax, but one I had never seen before. Maybe I am\njust clueless and sheltered, but after 30-odd years of reading English\n(12 of which involved reading academic computer science papers!), I\ncan't help but think it is not all that common and may confuse other\nreaders. Add on top that it is usually used in parentheses, which helps\nmake it more obvious what is going on.\n\nI really think \"Show only commits which have at least (or at most,\nrespectively) that many commits\" says the same thing, but is way more\naccessible.\n\n> > That way it is obvious that \"--merges\" cancels a previous --min-parents\n> > on the command line (maybe the text should be \"this is an alias for...\"\n> > to make it clear that doing it is exactly the same).\n> \n> Yes, that is helpful. I have doubts about \"alias\" for. Without wanting\n> to sound elitist or something, I have the impression that we start\n> catering for users who understand \"equivalent\" more reliably than \"alias\".\n\nI just wanted to make sure people didn't think \"equivalent\" meant \"has a\nsimilar effect to\" as opposed to \"is exactly as if you did\". But reading\nit again, I think \"equivalent\" is fine, and I see you picked it up in\nthe latest series.\n\n-Peff\n"},{"id":"163894","messageId":"20110321105628.GC16334@sigill.intra.peff.net","threadId":"26762","inReplyTo":"cover.1300702130.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 0/3] rev-list and friends: --min-parents, --max-parents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-21T10:56:28Z","receivedAt":"2011-03-21T10:56:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 21, 2011 at 11:14:04AM +0100, Michael J Gruber wrote:\n\n> In v2, based on the input from Jeff, Junio and Jonathan, I switched to\n> ordinary signed integers for --min-parent and --max-parent. Negative\n> parameters to the latter mean infinity. (Thus, no more range/macro magic.)\n\nFWIW, I like this version much better. Besides my \"resp.\" nit, I have no\ncomplaints.\n\n-Peff\n"},{"id":"163900","messageId":"4D873F56.1060803@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110321105431.GB16334@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-21T12:06:46Z","receivedAt":"2011-03-21T12:06:46Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 21.03.2011 11:54:\n> On Mon, Mar 21, 2011 at 10:01:53AM +0100, Michael J Gruber wrote:\n> \n>>>> +\tShow only commits which have at least resp. at most that many\n>>>> +\tcommits, where `--max-parents=8` denotes infinity (i.e. no upper\n>>>> +\tlimit). In fact, 7 (or any negative number) does, but 8 is\n>>>> +\tinfinity sideways 8-)\n>>>\n>>> I didn't quite parse this \"resp.\" in the middle.\n>>\n>> Well, there are two options (--min-parents, --max-parents) which we\n>> describe in one paragraph. Sooo...\n> \n> I figured out what you were trying to say. I just had never seen the\n> abbreviation \"resp.\" before. I guessed it meant \"respectively\", but the\n> syntax is all wrong.\n> \n> Digging around via google, I was able to find that it is a mathematical\n> term with a specific syntax, but one I had never seen before. Maybe I am\n> just clueless and sheltered, but after 30-odd years of reading English\n> (12 of which involved reading academic computer science papers!), I\n> can't help but think it is not all that common and may confuse other\n> readers. Add on top that it is usually used in parentheses, which helps\n> make it more obvious what is going on.\n\nIn my community it is very common, which may partly be due to the fact\nthat there is a strong proportion of non-native speakers. It took it for\ngranted that it's a standard expression.\n\n> \n> I really think \"Show only commits which have at least (or at most,\n> respectively) that many commits\" says the same thing, but is way more\n> accessible.\n\nSounds good, I'm happy with that. Resend or squash on apply?\n\n> \n>>> That way it is obvious that \"--merges\" cancels a previous --min-parents\n>>> on the command line (maybe the text should be \"this is an alias for...\"\n>>> to make it clear that doing it is exactly the same).\n>>\n>> Yes, that is helpful. I have doubts about \"alias\" for. Without wanting\n>> to sound elitist or something, I have the impression that we start\n>> catering for users who understand \"equivalent\" more reliably than \"alias\".\n> \n> I just wanted to make sure people didn't think \"equivalent\" meant \"has a\n> similar effect to\" as opposed to \"is exactly as if you did\". But reading\n> it again, I think \"equivalent\" is fine, and I see you picked it up in\n> the latest series.\n\nI may be wrong about what is common in this case, too. For me, \"alias\"\nis foremost a technical term, and I would guess that many non-native\nspeaker know \"alias\" either in the technical sense or not at all, but\nnot so much in the common English sense. But either way is fine.\n\nMichael\n"},{"id":"163908","messageId":"4D875B05.50401@drmicha.warpmail.net","threadId":"26762","inReplyTo":"41d12c53966faa9c9cb5acc7646e9098dcd911ea.1300702130.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 2/3] revision.c: introduce --min-parents and --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-21T14:04:53Z","receivedAt":"2011-03-21T14:04:53Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Michael J Gruber venit, vidit, dixit 21.03.2011 11:14:\n> --merges and --no-merges are named confusingly and cannot be overridden\n> by each other, but they are there to stay for plumbers' sake.\n> \n> Introduce --min-parents and --max-parents which limit the revisions to\n> those commits which have at least resp. at most that many commits, where\n> negative arguments for --max-parents= denote infinity (i.e. no upper\n> limit).\n> \n> In particular:\n> \n> --max-parents=1: no merges\n> --min-parents=2: merges only\n> --max-parents=0: only roots\n> --min-parents=3: only octopusses\n> \n> --min-parents=n --max-parents=m with n>m gives you what you ask for\n> (nothing) just like --merges --no-merges does, but at least for an\n> obvious reason.\n> \n> Implementation note:\n> \n> We compute the number of parents only when we limit by that, so there\n> is no performance impact when there are no limiters.\n> \n> Signed-off-by: Michael J Gruber \n> ---\n> builtin/log.c | 2 +-\n> builtin/rev-list.c | 2 ++\n> builtin/rev-parse.c | 2 ++\n> revision.c | 24 +++++++++++++++++-------\n> revision.h | 4 ++--\n> 5 files changed, 24 insertions(+), 10 deletions(-)\n...\n> @@ -2029,10 +2034,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi\n> \t\treturn commit_ignore;\n> \tif (revs->min_age != -1 && (commit->date > revs->min_age))\n> \t\treturn commit_ignore;\n> -\tif (revs->no_merges && commit->parents && commit->parents->next)\n> -\t\treturn commit_ignore;\n> -\tif (revs->merges_only && !(commit->parents && commit->parents->next))\n> -\t\treturn commit_ignore;\n> +\tif (revs->min_parents || (revs->max_parents >= 0)) {\n> +\t\tint n = 0;\n> +\t\tstruct commit_list *p;\n> +\t\tfor (p = commit->parents; p; p = p->next)\n> +\t\t\tn++;\n> +\t\tif ((n < revs->min_parents) ||\n> +\t\t ((revs->max_parents >= 0) && (n > revs->max_parents)))\n> +\t\t\treturn commit_ignore;\n> +\t}\n> \tif (!commit_match(commit, revs))\n> \t\treturn commit_ignore;\n> \tif (revs->prune && revs->dense) {\n\nBTW: Version v2-eps had this conditional:\n\n(n < revs->min_parents) || (n > (unsigned int) revs->max_parents)\n\nI actually compared runs of \"git rev-list origin/next >/dev/null\"\nwithout options and with \"--merges\" or \"--no-merges\" for git without\nthis patch and with it (v2) and do not see any measurable difference\n(between without or with the patch; the runs with limiting options are\nfaster, so I redid it with --count, again no difference with or without\npatch).\n\nSo, the clearer version above should be preferred over the unsigned\ntrick, and special casing seems not worth it.\n\nMichael\n"},{"id":"163911","messageId":"7vbp14ttgu.fsf@alter.siamese.dyndns.org","threadId":"26762","inReplyTo":"4D873F56.1060803@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-21T14:54:09Z","receivedAt":"2011-03-21T14:54:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Jeff King venit, vidit, dixit 21.03.2011 11:54:\n>> On Mon, Mar 21, 2011 at 10:01:53AM +0100, Michael J Gruber wrote:\n>> \n> In my community it is very common, which may partly be due to the fact\n> that there is a strong proportion of non-native speakers. It took it for\n> granted that it's a standard expression.\n\nPerhaps in math circles or something?\n\n>> I really think \"Show only commits which have at least (or at most,\n>> respectively) that many commits\" says the same thing, but is way more\n>> accessible.\n>\n> Sounds good, I'm happy with that. Resend or squash on apply?\n\nThe \"... A resp. B\" composition did made me go \"huh?\", even though I\nmanaged to guess what you meant; I agree Jeff's rewrite is much more\napproacheable.\n\nThe code part of the patch needs a bit of touch-up anyway, so let's do a\nv2.\n\nIt is not like you will have a million copies of \"struct rev_info\", while\nyou will be reading from and comparing with the field for each commit\nduring the traversal, so I'd rather see these max/min stored in the usual\n\"int\", not squashed into bitfields. Also initialize max to the magic\ntoken that means \"unlimited\" in init_revisions() without swapping the\nmeaning of comparison. One initialization assignment you can omit there\nis not worth the resulting confusion.\n\n>>>> That way it is obvious that \"--merges\" cancels a previous --min-parents\n>>>> on the command line (maybe the text should be \"this is an alias for...\"\n>>>> to make it clear that doing it is exactly the same).\n>>>\n>>> Yes, that is helpful. I have doubts about \"alias\" for. Without wanting\n>>> to sound elitist or something, I have the impression that we start\n>>> catering for users who understand \"equivalent\" more reliably than \"alias\".\n>> \n>> I just wanted to make sure people didn't think \"equivalent\" meant \"has a\n>> similar effect to\" as opposed to \"is exactly as if you did\". But reading\n>> it again, I think \"equivalent\" is fine, and I see you picked it up in\n>> the latest series.\n>\n> I may be wrong about what is common in this case, too. For me, \"alias\"\n> is foremost a technical term, and I would guess that many non-native\n> speaker know \"alias\" either in the technical sense or not at all, but\n> not so much in the common English sense. But either way is fine.\n\nWe would want to make sure the reader understands that saying --no-merges\nis _exactly the same_ (your words above) as saying --max-parents=1, so why\nnot spell that out, i.e. \"This is exactly the same as `--max-parents=1`\"?\n\nThanks.\n"},{"id":"163912","messageId":"4D876714.8040604@drmicha.warpmail.net","threadId":"26762","inReplyTo":"7vbp14ttgu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-21T14:56:20Z","receivedAt":"2011-03-21T14:56:20Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 21.03.2011 15:54:\n> Michael J Gruber writes:\n> \n>> Jeff King venit, vidit, dixit 21.03.2011 11:54:\n>>> On Mon, Mar 21, 2011 at 10:01:53AM +0100, Michael J Gruber wrote:\n>>>\n>> In my community it is very common, which may partly be due to the fact\n>> that there is a strong proportion of non-native speakers. It took it for\n>> granted that it's a standard expression.\n> \n> Perhaps in math circles or something?\n\nYeah, those (us) weirdos ;)\n\n> \n>>> I really think \"Show only commits which have at least (or at most,\n>>> respectively) that many commits\" says the same thing, but is way more\n>>> accessible.\n>>\n>> Sounds good, I'm happy with that. Resend or squash on apply?\n> \n> The \"... A resp. B\" composition did made me go \"huh?\", even though I\n> managed to guess what you meant; I agree Jeff's rewrite is much more\n> approacheable.\n> \n> The code part of the patch needs a bit of touch-up anyway, so let's do a\n> v2.\n> \n> It is not like you will have a million copies of \"struct rev_info\", while\n> you will be reading from and comparing with the field for each commit\n> during the traversal, so I'd rather see these max/min stored in the usual\n> \"int\", not squashed into bitfields. Also initialize max to the magic\n> token that means \"unlimited\" in init_revisions() without swapping the\n> meaning of comparison. One initialization assignment you can omit there\n> is not worth the resulting confusion.\n> \n>>>>> That way it is obvious that \"--merges\" cancels a previous --min-parents\n>>>>> on the command line (maybe the text should be \"this is an alias for...\"\n>>>>> to make it clear that doing it is exactly the same).\n>>>>\n>>>> Yes, that is helpful. I have doubts about \"alias\" for. Without wanting\n>>>> to sound elitist or something, I have the impression that we start\n>>>> catering for users who understand \"equivalent\" more reliably than \"alias\".\n>>>\n>>> I just wanted to make sure people didn't think \"equivalent\" meant \"has a\n>>> similar effect to\" as opposed to \"is exactly as if you did\". But reading\n>>> it again, I think \"equivalent\" is fine, and I see you picked it up in\n>>> the latest series.\n>>\n>> I may be wrong about what is common in this case, too. For me, \"alias\"\n>> is foremost a technical term, and I would guess that many non-native\n>> speaker know \"alias\" either in the technical sense or not at all, but\n>> not so much in the common English sense. But either way is fine.\n> \n> We would want to make sure the reader understands that saying --no-merges\n> is _exactly the same_ (your words above) as saying --max-parents=1, so why\n> not spell that out, i.e. \"This is exactly the same as `--max-parents=1`\"?\n> \n> Thanks.\n\nI guess you haven't seen v2 yet, where most of these changes are\nincorporated.\n\nFor the doc, \"exactly the same\" is clean and simple and understandable\nfor both Jeff and myself, I guess ;)\n\nMichael\n"},{"id":"163925","messageId":"7vei60s9ob.fsf@alter.siamese.dyndns.org","threadId":"26762","inReplyTo":"4D876714.8040604@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-21T16:47:00Z","receivedAt":"2011-03-21T16:47:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> I guess you haven't seen v2 yet, where most of these changes are\n> incorporated.\n\nYeah, I think that was before you wrote \"resend or squash\", so my\nautomatic reaction was to discard everything you sent on the topic before\nthe timestamp of that email. Your v2 does seem to be in a very good shape.\n\n> For the doc, \"exactly the same\" is clean and simple and understandable\n> for both Jeff and myself, I guess ;)\n\nOk, will tweak v2 and apply.\n\nThanks.\n"},{"id":"163931","messageId":"7v8vw8qsdq.fsf@alter.siamese.dyndns.org","threadId":"26762","inReplyTo":"41d12c53966faa9c9cb5acc7646e9098dcd911ea.1300702130.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 2/3] revision.c: introduce --min-parents and --max-parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-21T17:45:53Z","receivedAt":"2011-03-21T17:45:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Implementation note:\n>\n> We compute the number of parents only when we limit by that, so there\n> is no performance impact when there are no limiters.\n\nOnly about 16% of the commits in git.git (and 7% in the kernel) are two\nparent merges and the rest are mostly single strand of pearls, so even the\ncost of counting all the parents in a loop that does not exit early when\nit reaches max-parents limit should be negligible, and checking against\nmax-parents in each iteration would probably costs even more.\n\nSo I think this is a good to go.\n\nThanks.\n"},{"id":"163932","messageId":"20110321174917.GA8202@elie","threadId":"26762","inReplyTo":"4D8711DD.2040407@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-21T17:49:17Z","receivedAt":"2011-03-21T17:49:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael J Gruber wrote:\n\n> Come on, with a cover-letter saying doc and tests are in 3/3, how much\n> effort is it to read that before 1/3 if you care?\n\nNonzero. Now multiply that by the number of people who are going to\nlook at the history over the years.\n\n> The tests are bogus before the code and the doc pointless before it.\n>\n> Squashing 1 and 3 is okay, of course. For my own digestion, smaller\n> bites are better.\n\nThe reason I mentioned the possibility of docs and tests before\nimplementation is that that can be a good way to get feedback on the\ndesign of something when the implementation is not even ready yet.\nWhich doesn't apply hear, of course.\n\nAnyway, I don't care too much about this, but I wanted to make the\nconvention clear (if I have understood it correctly, at least).\n\n>> Avoiding the for loop means errors from 'echo' before the last\n>> iteration are not ignored; a more verbose way to write the same\n>\n> Do we really need to safe-guard echo and prints?\n\nNo; sorry about that. I get worried about for loops because they\nhave a tendency to start using git commands later, but I shouldn't\nhave mentioned it.\n\n> Thanks for both of the above, that makes things much better. Although I\n> have to treat the case with empty rev-list specially now, or use the\n> verbose version.\n\nSorry I missed it; good catch. I don't recommend\n\n\tprintf ${1+'%s\\n' \"$@\"}\n\n> Dodeka, really? I leave that to you.\n> I might add a tetrapus, though.\n\nI meant \"greater than eight\", since it seemed like a test that in\nsome alternate universe could fail. I'll write a test on top.\n\n(Dodecapus comes from [1].)\nhttp://thread.gmane.org/gmane.comp.version-control.git/15486 )I mentioned dodecapus\n\n> wins against? Against itself, i.e. it overrides previous occurences. But\n> I'll separate these.\n\nThanks. The details weren't important.\n\n>> \ttest_expect_failure '--max-parents=gobbledegood errors out' '\n>> \t\t...\n>> \t'\n>\n> I don't really want to parse for the string \"infinity\" nor go for strtol\n> instead of atoi. Why shouldn't something \"unparseable\" be 0?\n\nThree reasons:\n\n 1) why shouldn't it be -1 or 27? When in doubt, it's best to let\n the operator know so she can specify what is wanted unambiguously.\n 2) to catch typos on the commandline and bugs in scripts\n 3) less importantly, erroring out makes it less likely someone is\n going to rely on it (since we didn't point out their typo) and\n thus makes future extensions easier\n\nThanks again. Now to look at v2. :)\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/15486\n"},{"id":"163933","messageId":"20110321175817.GB8202@elie","threadId":"26762","inReplyTo":"41d12c53966faa9c9cb5acc7646e9098dcd911ea.1300702130.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 2/3] revision.c: introduce --min-parents and --max-parents","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-21T17:58:17Z","receivedAt":"2011-03-21T17:58:17Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael J Gruber wrote:\n\n> Signed-off-by: Michael J Gruber \n> ---\n> builtin/log.c | 2 +-\n> builtin/rev-list.c | 2 ++\n> builtin/rev-parse.c | 2 ++\n> revision.c | 24 +++++++++++++++++-------\n> revision.h | 4 ++--\n> 5 files changed, 24 insertions(+), 10 deletions(-)\n\nFor what it's worth, the patch looks obviously good to me, too.\n"},{"id":"163954","messageId":"20110321184514.GA1850@elie","threadId":"26762","inReplyTo":"8bad49d4e4897be623b7af3096498a5803dbbd89.1300702130.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-21T18:45:14Z","receivedAt":"2011-03-21T18:45:14Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael J Gruber wrote:\n\n> Based on mg/rev-list-one-side-only (in next) to save Junio a build conflict\n> resolution.\n\nNot a serious problem, but I wish it hadn't been. What particular\nfunctionality from that branch does this use?\n\nAh, now that I check it seems that that is to change a use of\nno_merges in the implementation of --cherry to use the new API? Makes\nsense (and good catch). With that hunk skipped, the patches apply to\nmaster.\n\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -72,11 +72,24 @@ endif::git-rev-list[]\n> \n> --merges::\n> \n> -\tPrint only merge commits.\n> +\tPrint only merge commits. This is equivalent to `--min-parents=2`.\n> \n> --no-merges::\n> \n> -\tDo not print commits with more than one parent.\n> +\tDo not print commits with more than one parent. This is\n> +\tequivalent to `--max-parents=1`.\n> +\n> +--min-parents::\n> +--max-parents::\n> +\n> +\tShow only commits which have at least resp. at most that many\n\nENOPARSE. I guess parentheses around \"resp. at most\" would work as\na minimal fix, but it might be better to say:\n\n --min-parents=::\n\tShow only commits which have at least parents.\n\n --max-parents=::\n\tShow only commits which have at least parents.\n\nand perhaps to put\n\n git log --max-parents=0::\n\tLists all root commits.\n\n git log --min-parents=3::\n\tLists all octopus merges.\n\nunder EXAMPLES.\n\n> +\tcommits, where negative parameters for `--max-parents=` denote\n> +\tinfinity (i.e. no upper\tlimit).\n\nSeems hackish. Maybe --no-max-parents could denote infinity?\n\n> ++\n> +In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is\n> +`--merges` (only), `--max-parents=0` gives all root commits and\n> +`--min-parents=3` all octopuses.\n> +\n> \n> --first-parent::\n\nIt seems there is an extra newline here.\n\n> --- a/t/t6009-rev-list-parent.sh\n> +++ b/t/t6009-rev-list-parent.sh\n> @@ -1,9 +1,17 @@\n> #!/bin/sh\n> \n> -test_description='properly cull all ancestors'\n> +test_description='ancestor culling and limiting by parent number'\n> \n> . ./test-lib.sh\n> \n> +check_revlist () {\n> +\trev_list_args=\"$1\" &&\n> +\tshift &&\n> +\tgit rev-parse \"$@\" >expect &&\n> +\tgit rev-list $rev_list_args --all >actual &&\n> +\ttest_cmp expect actual\n> +}\n> + \n\n\"git am\" warns about trailing whitespace on the line after the closing\nbrace (nothing that --whitespace=fix can't fix, though).\n\nThanks for factoring this out btw. It makes the tests themselves\nvery easy to read.\n\n> +test_expect_success 'rev-list override and infinities' '\n> +\n> +\tcheck_revlist \"--min-parents=2 --max-parents=1 --max-parents=3\" tripus normalmerge &&\n> +\tcheck_revlist \"--min-parents=1 --min-parents=2 --max-parents=7\" tetrapus tripus normalmerge &&\n> +\tcheck_revlist \"--min-parents=2 --max-parents=8\" tetrapus tripus normalmerge &&\n> +\tcheck_revlist \"--min-parents=2 --max-parents=-1\" tetrapus tripus normalmerge\n> +'\n\n7 and 8 don't mean infinity any more, do they? What is this test\nchecking?\n"},{"id":"164016","messageId":"4D8851FA.3040100@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110321174917.GA8202@elie","subject":"Re: [PATCH 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-22T07:38:34Z","receivedAt":"2011-03-22T07:38:34Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jonathan Nieder venit, vidit, dixit 21.03.2011 18:49:\n> Michael J Gruber wrote:\n> \n>> Come on, with a cover-letter saying doc and tests are in 3/3, how much\n>> effort is it to read that before 1/3 if you care?\n> \n> Nonzero. Now multiply that by the number of people who are going to\n> look at the history over the years.\n> \n>> The tests are bogus before the code and the doc pointless before it.\n>>\n>> Squashing 1 and 3 is okay, of course. For my own digestion, smaller\n>> bites are better.\n> \n> The reason I mentioned the possibility of docs and tests before\n> implementation is that that can be a good way to get feedback on the\n> design of something when the implementation is not even ready yet.\n> Which doesn't apply hear, of course.\n> \n> Anyway, I don't care too much about this, but I wanted to make the\n> convention clear (if I have understood it correctly, at least).\n\nMaybe we should clarify this. No, I don't mean by taking it outside ;)\n\nIf there's a different convention I'll try and follow that, of course.\nI'm very used to seeing \"1/2 implementation\" and \"2/2 doc and tests\",\nthe other way round you would introduce failing tests (for new\nfeatures). So I took that as a convention. You could always squash\nthese, of course (just like my 2 and 3 in v2).\n\nI actually have to keep myself from feeling annoyed when I see\ndocumentation patches without any implementation, because a patch\ncreates the impression that something has been done, but without any\nattempt at implementation it's hard to tell whether it's doable at all.\nThis does not apply to pure RFD/design questions, of course.\n\nCheers,\nMichael\n"},{"id":"164019","messageId":"4D8855D4.6080804@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110321184514.GA1850@elie","subject":"Re: [PATCHv2 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-22T07:55:00Z","receivedAt":"2011-03-22T07:55:00Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jonathan Nieder venit, vidit, dixit 21.03.2011 19:45:\n> Michael J Gruber wrote:\n> \n>> Based on mg/rev-list-one-side-only (in next) to save Junio a build conflict\n>> resolution.\n> \n> Not a serious problem, but I wish it hadn't been. What particular\n\nBut why? Basing it on something earlier would have served no purpose\n(that I know of) at all.\n\nI should have mentioned the dependency in v1.\n\n> functionality from that branch does this use?\n> \n> Ah, now that I check it seems that that is to change a use of\n> no_merges in the implementation of --cherry to use the new API? Makes\n> sense (and good catch). With that hunk skipped, the patches apply to\n> master.\n\nYes, revs->no_merges and revs->merges are gone, so a series based on\nmaster would produce a compile failure when applied to next.\n\n> \n>> --- a/Documentation/rev-list-options.txt\n>> +++ b/Documentation/rev-list-options.txt\n>> @@ -72,11 +72,24 @@ endif::git-rev-list[]\n>> \n>> --merges::\n>> \n>> -\tPrint only merge commits.\n>> +\tPrint only merge commits. This is equivalent to `--min-parents=2`.\n>> \n>> --no-merges::\n>> \n>> -\tDo not print commits with more than one parent.\n>> +\tDo not print commits with more than one parent. This is\n>> +\tequivalent to `--max-parents=1`.\n>> +\n>> +--min-parents::\n>> +--max-parents::\n>> +\n>> +\tShow only commits which have at least resp. at most that many\n> \n> ENOPARSE. I guess parentheses around \"resp. at most\" would work as\n> a minimal fix, but it might be better to say:\n> \n> --min-parents=::\n> \tShow only commits which have at least parents.\n> \n> --max-parents=::\n> \tShow only commits which have at least parents.\n> \n> and perhaps to put\n> \n> git log --max-parents=0::\n> \tLists all root commits.\n> \n> git log --min-parents=3::\n> \tLists all octopus merges.\n> \n> under EXAMPLES.\n> \n\nWell, we discussed this under the v1 thread (after v2 was sent). Junio,\nshould I do a v3 with that or this?\n\n>> +\tcommits, where negative parameters for `--max-parents=` denote\n>> +\tinfinity (i.e. no upper\tlimit).\n> \n> Seems hackish. Maybe --no-max-parents could denote infinity?\n\nFor me, \"-1\" is a quite natural way to reset a count type parameter, and\nyou don't even have to think \"unsigned\" or \"mod max_int\" for that.\n\nThere is no problem parsing for \"--max-parents=infinity\" and/or\n\"--no-max-parents\" or even (better?) \"--max-parents=\" without number,\nit's only a matter of bike shedding decisions.\n\n>> ++\n>> +In particular, `--max-parents=1` is `--no-merges`, `--min-parents=2` is\n>> +`--merges` (only), `--max-parents=0` gives all root commits and\n>> +`--min-parents=3` all octopuses.\n>> +\n>> \n>> --first-parent::\n> \n> It seems there is an extra newline here.\n> \n>> --- a/t/t6009-rev-list-parent.sh\n>> +++ b/t/t6009-rev-list-parent.sh\n>> @@ -1,9 +1,17 @@\n>> #!/bin/sh\n>> \n>> -test_description='properly cull all ancestors'\n>> +test_description='ancestor culling and limiting by parent number'\n>> \n>> . ./test-lib.sh\n>> \n>> +check_revlist () {\n>> +\trev_list_args=\"$1\" &&\n>> +\tshift &&\n>> +\tgit rev-parse \"$@\" >expect &&\n>> +\tgit rev-list $rev_list_args --all >actual &&\n>> +\ttest_cmp expect actual\n>> +}\n>> + \n> \n> \"git am\" warns about trailing whitespace on the line after the closing\n> brace (nothing that --whitespace=fix can't fix, though).\n\nHmmm, are there whitespace issues which am warns about and diff does\nnot, or have I missed a warning?\n\n> \n> Thanks for factoring this out btw. It makes the tests themselves\n> very easy to read.\n> \n>> +test_expect_success 'rev-list override and infinities' '\n>> +\n>> +\tcheck_revlist \"--min-parents=2 --max-parents=1 --max-parents=3\" tripus normalmerge &&\n>> +\tcheck_revlist \"--min-parents=1 --min-parents=2 --max-parents=7\" tetrapus tripus normalmerge &&\n>> +\tcheck_revlist \"--min-parents=2 --max-parents=8\" tetrapus tripus normalmerge &&\n>> +\tcheck_revlist \"--min-parents=2 --max-parents=-1\" tetrapus tripus normalmerge\n>> +'\n> \n> 7 and 8 don't mean infinity any more, do they? What is this test\n> checking?\n\nThe test checks \"override and infinities\", where the plural indicates\nthe fact that it tests different ways of spelling (practical) infinity\nsuch as the very suggestive \"8\" which nobody cares about but me.\n\nMichael\n"},{"id":"164104","messageId":"20110323004730.GA10014@elie","threadId":"26762","inReplyTo":"4D8855D4.6080804@drmicha.warpmail.net","subject":"Re: [PATCHv2 3/3] rev-list --min-parents,--max-parents: doc and test and completion","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-23T00:47:30Z","receivedAt":"2011-03-23T00:47:30Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael J Gruber wrote:\n> Jonathan Nieder venit, vidit, dixit 21.03.2011 19:45:\n>> Michael J Gruber wrote:\n\n>>> Based on mg/rev-list-one-side-only (in next) to save Junio a build conflict\n>>> resolution.\n>>\n>> Not a serious problem, but I wish it hadn't been. What particular\n>\n> But why? Basing it on something earlier would have served no purpose\n> (that I know of) at all.\n>\n> I should have mentioned the dependency in v1.\n\n*nod* Sorry, I was just confused before --- I hadn't understood why\nyou were doing it. Giving a heads up like you did about interaction\nbetween topics is indeed very useful.\n\nTo answer your question about why: basing it on master would mean\nthat Junio could merge it to master without merging\nmg/rev-list-one-side-only first. Similarly, I had wanted to test\nagainst master because master is what I use day-to-day so applying\nit there would avoid confusing unrelated changes.\n\n>> Seems hackish. Maybe --no-max-parents could denote infinity?\n>\n> For me, \"-1\" is a quite natural way to reset a count type parameter\n\nNatural to me in code, but not on the command line. I can write a\nseparate patch for --no-max-parents if you'd like.\n\n> There is no problem parsing for \"--max-parents=infinity\" and/or\n> \"--no-max-parents\" or even (better?) \"--max-parents=\" without number,\n> it's only a matter of bike shedding decisions.\n\nWell, if it doesn't matter what color it is, I guess I shouldn't have\nmentioned it then?\n\n> Hmmm, are there whitespace issues which am warns about and diff does\n> not, or have I missed a warning?\n\nYep, diff doesn't warn about anything without --check.\n\n>>> +test_expect_success 'rev-list override and infinities' '\n>>> +\n>>> +\tcheck_revlist \"--min-parents=2 --max-parents=1 --max-parents=3\" tripus normalmerge &&\n>>> +\tcheck_revlist \"--min-parents=1 --min-parents=2 --max-parents=7\" tetrapus tripus normalmerge &&\n>>> +\tcheck_revlist \"--min-parents=2 --max-parents=8\" tetrapus tripus normalmerge &&\n>>> +\tcheck_revlist \"--min-parents=2 --max-parents=-1\" tetrapus tripus normalmerge\n>>> +'\n>> \n>> 7 and 8 don't mean infinity any more, do they? What is this test\n>> checking?\n>\n> The test checks \"override and infinities\", where the plural indicates\n> the fact that it tests different ways of spelling (practical) infinity\n> such as the very suggestive \"8\" which nobody cares about but me.\n\nAh, so that's what you meant. git.git has very few octopus merges,\nso even \"4\" is a practical infinity there (well, technically there's\nan early 6-parent commit).\n\nRegards,\nJonathan\n"},{"id":"164119","messageId":"cover.1300872923.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110321105628.GC16334@sigill.intra.peff.net","subject":"[PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-23T09:38:47Z","receivedAt":"2011-03-23T09:38:47Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Compared to what is currently in pu (which is v2+eps), v3 has:\n\n1/3 -> 1/5 unchanged\n\n2/3 -> 2/5 unchanged\n\n3/5 is !squash for 2/5 and introduces --no-min-parents and --no-max-parents\nas natural ways to reset the limits\n\n3/3 -> 4/5 with a fix to the notation in documentation (spell out =)\nand an additional dodecapus test\n\n5/5 is !fixup for 4/5 and adds test, doc and completion for --no-min-parents\nand --no-max-parents\n\nJunio, please let me/us know whether sending an amended series in this way\n(which I've seen before) is actually convenient for you or not. !squash\ncommits require a message edit, for example. OTOH, I don't know any (other?)\ngood inter diff solution.\n\nThanks for everyone's input which went into this (Junio, Jeff, Jonathan).\n\nMichael J Gruber (5):\n t6009: use test_commit() from test-lib.sh\n revision.c: introduce --min-parents and --max-parents\n squash! revision.c: introduce --min-parents and --max-parents\n rev-list --min-parents,--max-parents: doc, test and completion\n fixup! rev-list --min-parents,--max-parents: doc, test and completion\n\n Documentation/git-rev-list.txt | 4 +\n Documentation/rev-list-options.txt | 19 +++++-\n builtin/log.c | 2 +-\n builtin/rev-list.c | 4 +\n builtin/rev-parse.c | 4 +\n contrib/completion/git-completion.bash | 2 +\n revision.c | 28 ++++++--\n revision.h | 4 +-\n t/t6009-rev-list-parent.sh | 118 +++++++++++++++++++++++++++++---\n 9 files changed, 163 insertions(+), 22 deletions(-)\n\n-- \n1.7.4.1.511.g72e46\n"},{"id":"164118","messageId":"1f39cb4a4fc02a50fa869bef412ea0f2a527e480.1300872923.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110321105628.GC16334@sigill.intra.peff.net","subject":"[PATCHv3 1/5] t6009: use test_commit() from test-lib.sh","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-23T09:38:48Z","receivedAt":"2011-03-23T09:38:48Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Signed-off-by: Michael J Gruber \n---\n t/t6009-rev-list-parent.sh | 15 ++++-----------\n 1 files changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\nindex 52f7b27..0f0e457 100755\n--- a/t/t6009-rev-list-parent.sh\n+++ b/t/t6009-rev-list-parent.sh\n@@ -4,25 +4,18 @@ test_description='properly cull all ancestors'\n \n . ./test-lib.sh\n \n-commit () {\n-\ttest_tick &&\n-\techo $1 >file &&\n-\tgit commit -a -m $1 &&\n-\tgit tag $1\n-}\n-\n test_expect_success setup '\n \n \ttouch file &&\n \tgit add file &&\n \n-\tcommit one &&\n+\ttest_commit one &&\n \n \ttest_tick=$(($test_tick - 2400)) &&\n \n-\tcommit two &&\n-\tcommit three &&\n-\tcommit four &&\n+\ttest_commit two &&\n+\ttest_commit three &&\n+\ttest_commit four &&\n \n \tgit log --pretty=oneline --abbrev-commit\n '\n-- \n1.7.4.1.511.g72e46\n"},{"id":"164121","messageId":"41d12c53966faa9c9cb5acc7646e9098dcd911ea.1300872923.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110321105628.GC16334@sigill.intra.peff.net","subject":"[PATCHv3 2/5] revision.c: introduce --min-parents and --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-23T09:38:49Z","receivedAt":"2011-03-23T09:38:49Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"--merges and --no-merges are named confusingly and cannot be overridden\nby each other, but they are there to stay for plumbers' sake.\n\nIntroduce --min-parents and --max-parents which limit the revisions to\nthose commits which have at least resp. at most that many commits, where\nnegative arguments for --max-parents= denote infinity (i.e. no upper\nlimit).\n\nIn particular:\n\n--max-parents=1: no merges\n--min-parents=2: merges only\n--max-parents=0: only roots\n--min-parents=3: only octopusses\n\n--min-parents=n --max-parents=m with n>m gives you what you ask for\n(nothing) just like --merges --no-merges does, but at least for an\nobvious reason.\n\nImplementation note:\n\nWe compute the number of parents only when we limit by that, so there\nis no performance impact when there are no limiters.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/log.c | 2 +-\n builtin/rev-list.c | 2 ++\n builtin/rev-parse.c | 2 ++\n revision.c | 24 +++++++++++++++++-------\n revision.h | 4 ++--\n 5 files changed, 24 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 796e9e5..4a0f78d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1061,7 +1061,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \trev.commit_format = CMIT_FMT_EMAIL;\n \trev.verbose_header = 1;\n \trev.diff = 1;\n-\trev.no_merges = 1;\n+\trev.max_parents = 1;\n \tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n \trev.subject_prefix = fmt_patch_subject_prefix;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex f458cb7..7b276e0 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -16,6 +16,8 @@ static const char rev_list_usage[] =\n \" --min-age=\\n\"\n \" --sparse\\n\"\n \" --no-merges\\n\"\n+\" --min-parents=\\n\"\n+\" --max-parents=\\n\"\n \" --remove-empty\\n\"\n \" --all\\n\"\n \" --branches\\n\"\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex a5a1c86..9940546 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -48,6 +48,8 @@ static int is_rev_argument(const char *arg)\n \t\t\"--max-count=\",\n \t\t\"--min-age=\",\n \t\t\"--no-merges\",\n+\t\t\"--min-parents=\",\n+\t\t\"--max-parents=\",\n \t\t\"--objects\",\n \t\t\"--objects-edge\",\n \t\t\"--parents\",\ndiff --git a/revision.c b/revision.c\nindex e96c281..8d48870 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -941,6 +941,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n \trevs->min_age = -1;\n \trevs->skip_count = -1;\n \trevs->max_count = -1;\n+\trevs->max_parents = -1;\n \n \trevs->commit_format = CMIT_FMT_DEFAULT;\n \n@@ -1277,9 +1278,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--remove-empty\")) {\n \t\trevs->remove_empty_trees = 1;\n \t} else if (!strcmp(arg, \"--merges\")) {\n-\t\trevs->merges_only = 1;\n+\t\trevs->min_parents = 2;\n \t} else if (!strcmp(arg, \"--no-merges\")) {\n-\t\trevs->no_merges = 1;\n+\t\trevs->max_parents = 1;\n+\t} else if (!prefixcmp(arg, \"--min-parents=\")) {\n+\t\trevs->min_parents = atoi(arg+14);\n+\t} else if (!prefixcmp(arg, \"--max-parents=\")) {\n+\t\trevs->max_parents = atoi(arg+14);\n \t} else if (!strcmp(arg, \"--boundary\")) {\n \t\trevs->boundary = 1;\n \t} else if (!strcmp(arg, \"--left-right\")) {\n@@ -1298,7 +1303,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\t\tdie(\"--cherry is incompatible with --left-only\");\n \t\trevs->cherry_mark = 1;\n \t\trevs->right_only = 1;\n-\t\trevs->no_merges = 1;\n+\t\trevs->max_parents = 1;\n \t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--count\")) {\n \t\trevs->count = 1;\n@@ -2029,10 +2034,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi\n \t\treturn commit_ignore;\n \tif (revs->min_age != -1 && (commit->date > revs->min_age))\n \t\treturn commit_ignore;\n-\tif (revs->no_merges && commit->parents && commit->parents->next)\n-\t\treturn commit_ignore;\n-\tif (revs->merges_only && !(commit->parents && commit->parents->next))\n-\t\treturn commit_ignore;\n+\tif (revs->min_parents || (revs->max_parents >= 0)) {\n+\t\tint n = 0;\n+\t\tstruct commit_list *p;\n+\t\tfor (p = commit->parents; p; p = p->next)\n+\t\t\tn++;\n+\t\tif ((n < revs->min_parents) ||\n+\t\t ((revs->max_parents >= 0) && (n > revs->max_parents)))\n+\t\t\treturn commit_ignore;\n+\t}\n \tif (!commit_match(commit, revs))\n \t\treturn commit_ignore;\n \tif (revs->prune && revs->dense) {\ndiff --git a/revision.h b/revision.h\nindex ae94860..9fd8f30 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -41,8 +41,6 @@ struct rev_info {\n \t/* Traversal flags */\n \tunsigned int\tdense:1,\n \t\t\tprune:1,\n-\t\t\tno_merges:1,\n-\t\t\tmerges_only:1,\n \t\t\tno_walk:1,\n \t\t\tshow_all:1,\n \t\t\tremove_empty_trees:1,\n@@ -126,6 +124,8 @@ struct rev_info {\n \tint max_count;\n \tunsigned long max_age;\n \tunsigned long min_age;\n+\tint min_parents;\n+\tint max_parents;\n \n \t/* diff info for patches and for paths limiting */\n \tstruct diff_options diffopt;\n-- \n1.7.4.1.511.g72e46\n"},{"id":"164120","messageId":"0b4b1841cb30b6840782f7d7d38361cefdf66fb5.1300872923.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110321105628.GC16334@sigill.intra.peff.net","subject":"[PATCHv3 3/5] squash! revision.c: introduce --min-parents and --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-23T09:38:50Z","receivedAt":"2011-03-23T09:38:50Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Also, introduce --no-min-parents and --no-max-parents to do the obvious\nthing for convenience.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/rev-list.c | 2 ++\n builtin/rev-parse.c | 2 ++\n revision.c | 4 ++++\n 3 files changed, 8 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 7b276e0..9bfb942 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -17,7 +17,9 @@ static const char rev_list_usage[] =\n \" --sparse\\n\"\n \" --no-merges\\n\"\n \" --min-parents=\\n\"\n+\" --no-min-parents\\n\"\n \" --max-parents=\\n\"\n+\" --no-max-parents\\n\"\n \" --remove-empty\\n\"\n \" --all\\n\"\n \" --branches\\n\"\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 9940546..adb1cae 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -49,7 +49,9 @@ static int is_rev_argument(const char *arg)\n \t\t\"--min-age=\",\n \t\t\"--no-merges\",\n \t\t\"--min-parents=\",\n+\t\t\"--no-min-parents\",\n \t\t\"--max-parents=\",\n+\t\t\"--no-max-parents\",\n \t\t\"--objects\",\n \t\t\"--objects-edge\",\n \t\t\"--parents\",\ndiff --git a/revision.c b/revision.c\nindex 8d48870..0f38364 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1283,8 +1283,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->max_parents = 1;\n \t} else if (!prefixcmp(arg, \"--min-parents=\")) {\n \t\trevs->min_parents = atoi(arg+14);\n+\t} else if (!prefixcmp(arg, \"--no-min-parents\")) {\n+\t\trevs->min_parents = 0;\n \t} else if (!prefixcmp(arg, \"--max-parents=\")) {\n \t\trevs->max_parents = atoi(arg+14);\n+\t} else if (!prefixcmp(arg, \"--no-max-parents\")) {\n+\t\trevs->max_parents = -1;\n \t} else if (!strcmp(arg, \"--boundary\")) {\n \t\trevs->boundary = 1;\n \t} else if (!strcmp(arg, \"--left-right\")) {\n-- \n1.7.4.1.511.g72e46\n"},{"id":"164122","messageId":"df079eb89fa305bf8d33019ea7089df5950e4506.1300872923.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110321105628.GC16334@sigill.intra.peff.net","subject":"[PATCHv3 4/5] rev-list --min-parents,--max-parents: doc, test and completion","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-23T09:38:51Z","receivedAt":"2011-03-23T09:38:51Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"This also adds test for \"--merges\" and \"--no-merges\" which we did not\nhave so far.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-rev-list.txt | 2 +\n Documentation/rev-list-options.txt | 16 ++++-\n contrib/completion/git-completion.bash | 1 +\n t/t6009-rev-list-parent.sh | 105 +++++++++++++++++++++++++++++++-\n 4 files changed, 121 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt\nindex b08dfbc..c5ea96f 100644\n--- a/Documentation/git-rev-list.txt\n+++ b/Documentation/git-rev-list.txt\n@@ -16,6 +16,8 @@ SYNOPSIS\n \t [ \\--sparse ]\n \t [ \\--merges ]\n \t [ \\--no-merges ]\n+\t [ \\--min-parents= ]\n+\t [ \\--max-parents= ]\n \t [ \\--first-parent ]\n \t [ \\--remove-empty ]\n \t [ \\--full-history ]\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 5c6850f..0bbf7da 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -72,11 +72,23 @@ endif::git-rev-list[]\n \n --merges::\n \n-\tPrint only merge commits.\n+\tPrint only merge commits. This is exactly the same as `--min-parents=2`.\n \n --no-merges::\n \n-\tDo not print commits with more than one parent.\n+\tDo not print commits with more than one parent. This is\n+\texactly the same as `--max-parents=1`.\n+\n+--min-parents=::\n+--max-parents=::\n+\n+\tShow only commits which have at least (or at most) that many\n+\tcommits, where negative parameters for `--max-parents=` denote\n+\tinfinity (i.e. no upper\tlimit).\n++\n+In particular, `--max-parents=1` is the same as `--no-merges`,\n+`--min-parents=2` is the same as `--merges`. `--max-parents=0`\n+gives all root commits and `--min-parents=3` all octopus merges.\n \n --first-parent::\n \tFollow only the first parent commit upon seeing a merge\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 3b1cc83..4da087e 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1577,6 +1577,7 @@ __git_log_common_options=\"\n \t--max-count=\n \t--max-age= --since= --after=\n \t--min-age= --until= --before=\n+\t--min-parents= --max-parents=\n \"\n # Options that go well for log and gitk (not shortlog)\n __git_log_gitk_options=\"\ndiff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\nindex 0f0e457..5309378 100755\n--- a/t/t6009-rev-list-parent.sh\n+++ b/t/t6009-rev-list-parent.sh\n@@ -1,9 +1,17 @@\n #!/bin/sh\n \n-test_description='properly cull all ancestors'\n+test_description='ancestor culling and limiting by parent number'\n \n . ./test-lib.sh\n \n+check_revlist () {\n+\trev_list_args=\"$1\" &&\n+\tshift &&\n+\tgit rev-parse \"$@\" >expect &&\n+\tgit rev-list $rev_list_args --all >actual &&\n+\ttest_cmp expect actual\n+}\n+\n test_expect_success setup '\n \n \ttouch file &&\n@@ -28,4 +36,99 @@ test_expect_success 'one is ancestor of others and should not be shown' '\n \n '\n \n+test_expect_success 'setup roots, merges and octopuses' '\n+\n+\tgit checkout --orphan newroot &&\n+\ttest_commit five &&\n+\tgit checkout -b sidebranch two &&\n+\ttest_commit six &&\n+\tgit checkout -b anotherbranch three &&\n+\ttest_commit seven &&\n+\tgit checkout -b yetanotherbranch four &&\n+\ttest_commit eight &&\n+\tgit checkout master &&\n+\ttest_merge normalmerge newroot &&\n+\ttest_tick &&\n+\tgit merge -m tripus sidebranch anotherbranch &&\n+\tgit tag tripus &&\n+\tgit checkout -b tetrabranch normalmerge &&\n+\ttest_tick &&\n+\tgit merge -m tetrapus sidebranch anotherbranch yetanotherbranch &&\n+\tgit tag tetrapus &&\n+\tgit checkout master\n+'\n+\n+test_expect_success 'rev-list roots' '\n+\n+\tcheck_revlist \"--max-parents=0\" one five\n+'\n+\n+test_expect_success 'rev-list no merges' '\n+\n+\tcheck_revlist \"--max-parents=1\" one eight seven six five four three two &&\n+\tcheck_revlist \"--no-merges\" one eight seven six five four three two\n+'\n+\n+test_expect_success 'rev-list no octopuses' '\n+\n+\tcheck_revlist \"--max-parents=2\" one normalmerge eight seven six five four three two\n+'\n+\n+test_expect_success 'rev-list no roots' '\n+\n+\tcheck_revlist \"--min-parents=1\" tetrapus tripus normalmerge eight seven six four three two\n+'\n+\n+test_expect_success 'rev-list merges' '\n+\n+\tcheck_revlist \"--min-parents=2\" tetrapus tripus normalmerge &&\n+\tcheck_revlist \"--merges\" tetrapus tripus normalmerge\n+'\n+\n+test_expect_success 'rev-list octopus' '\n+\n+\tcheck_revlist \"--min-parents=3\" tetrapus tripus\n+'\n+\n+test_expect_success 'rev-list ordinary commits' '\n+\n+\tcheck_revlist \"--min-parents=1 --max-parents=1\" eight seven six four three two\n+'\n+\n+test_expect_success 'rev-list --merges --no-merges yields empty set' '\n+\n+\tcheck_revlist \"--min-parents=2 --no-merges\" &&\n+\tcheck_revlist \"--merges --no-merges\" &&\n+\tcheck_revlist \"--no-merges --merges\"\n+'\n+\n+test_expect_success 'rev-list override and infinities' '\n+\n+\tcheck_revlist \"--min-parents=2 --max-parents=1 --max-parents=3\" tripus normalmerge &&\n+\tcheck_revlist \"--min-parents=1 --min-parents=2 --max-parents=7\" tetrapus tripus normalmerge &&\n+\tcheck_revlist \"--min-parents=2 --max-parents=8\" tetrapus tripus normalmerge &&\n+\tcheck_revlist \"--min-parents=2 --max-parents=-1\" tetrapus tripus normalmerge\n+'\n+\n+test_expect_success 'set up dodecapus' '\n+\n+\tfor i in 1 2 3 4 5 6 7 8 9 10 11\n+\tdo\n+\t\tgit checkout -b root$i five || return\n+\t\ttest_commit $i || return\n+\tdone &&\n+\tgit checkout master &&\n+\ttest_tick &&\n+\tgit merge -m dodecapus root{1,2,3,4,5,6,7,8,9,10,11} &&\n+\tgit tag dodecapus\n+'\n+\n+test_expect_success 'test with dodecapus' '\n+\n+\tcheck_revlist \"--min-parents=4\" dodecapus tetrapus &&\n+\tcheck_revlist \"--min-parents=8\" dodecapus &&\n+\tcheck_revlist \"--min-parents=12\" dodecapus &&\n+\tcheck_revlist \"--min-parents=13\" &&\n+\tcheck_revlist \"--min-parents=4 --max-parents=11\" tetrapus\n+'\n test_done\n-- \n1.7.4.1.511.g72e46\n"},{"id":"164123","messageId":"37bdb92b4ceaa99e45afd446b1c738d00edec602.1300872923.git.git@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110321105628.GC16334@sigill.intra.peff.net","subject":"[PATCHv3 5/5] fixup! rev-list --min-parents,--max-parents: doc, test and completion","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-23T09:38:52Z","receivedAt":"2011-03-23T09:38:52Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Signed-off-by: Michael J Gruber \n---\n Documentation/git-rev-list.txt | 2 ++\n Documentation/rev-list-options.txt | 13 ++++++++-----\n contrib/completion/git-completion.bash | 1 +\n t/t6009-rev-list-parent.sh | 4 +++-\n 4 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt\nindex c5ea96f..415f4f0 100644\n--- a/Documentation/git-rev-list.txt\n+++ b/Documentation/git-rev-list.txt\n@@ -17,7 +17,9 @@ SYNOPSIS\n \t [ \\--merges ]\n \t [ \\--no-merges ]\n \t [ \\--min-parents= ]\n+\t [ \\--no-min-parents ]\n \t [ \\--max-parents= ]\n+\t [ \\--no-max-parents ]\n \t [ \\--first-parent ]\n \t [ \\--remove-empty ]\n \t [ \\--full-history ]\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 0bbf7da..ea5c6c4 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -81,14 +81,17 @@ endif::git-rev-list[]\n \n --min-parents=::\n --max-parents=::\n+--no-min-parents::\n+--no-max-parents::\n \n \tShow only commits which have at least (or at most) that many\n-\tcommits, where negative parameters for `--max-parents=` denote\n-\tinfinity (i.e. no upper\tlimit).\n+\tcommits. In particular, `--max-parents=1` is the same as `--no-merges`,\n+\t`--min-parents=2` is the same as `--merges`. `--max-parents=0`\n+\tgives all root commits and `--min-parents=3` all octopus merges.\n +\n-In particular, `--max-parents=1` is the same as `--no-merges`,\n-`--min-parents=2` is the same as `--merges`. `--max-parents=0`\n-gives all root commits and `--min-parents=3` all octopus merges.\n+`--no-min-parents` and `--no-max-parents` reset these limits (to no limit)\n+again. Equivalent forms are `--min-parents=0` (any commit has 0 or more\n+parents) and `--max-parents=-1` (negative numbers denote no upper limit).\n \n --first-parent::\n \tFollow only the first parent commit upon seeing a merge\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 4da087e..d5215e8 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1578,6 +1578,7 @@ __git_log_common_options=\"\n \t--max-age= --since= --after=\n \t--min-age= --until= --before=\n \t--min-parents= --max-parents=\n+\t--no-min-parents --no-max-parents\n \"\n # Options that go well for log and gitk (not shortlog)\n __git_log_gitk_options=\"\ndiff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\nindex 5309378..fc89d6d 100755\n--- a/t/t6009-rev-list-parent.sh\n+++ b/t/t6009-rev-list-parent.sh\n@@ -107,7 +107,9 @@ test_expect_success 'rev-list override and infinities' '\n \tcheck_revlist \"--min-parents=2 --max-parents=1 --max-parents=3\" tripus normalmerge &&\n \tcheck_revlist \"--min-parents=1 --min-parents=2 --max-parents=7\" tetrapus tripus normalmerge &&\n \tcheck_revlist \"--min-parents=2 --max-parents=8\" tetrapus tripus normalmerge &&\n-\tcheck_revlist \"--min-parents=2 --max-parents=-1\" tetrapus tripus normalmerge\n+\tcheck_revlist \"--min-parents=2 --max-parents=-1\" tetrapus tripus normalmerge &&\n+\tcheck_revlist \"--min-parents=2 --no-max-parents\" tetrapus tripus normalmerge &&\n+\tcheck_revlist \"--max-parents=0 --min-parents=1 --no-min-parents\" one five\n '\n \n test_expect_success 'set up dodecapus' '\n-- \n1.7.4.1.511.g72e46\n"},{"id":"164150","messageId":"20110323144835.GA30337@sigill.intra.peff.net","threadId":"26762","inReplyTo":"cover.1300872923.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-23T14:48:35Z","receivedAt":"2011-03-23T14:48:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 23, 2011 at 10:38:47AM +0100, Michael J Gruber wrote:\n\n> Compared to what is currently in pu (which is v2+eps), v3 has:\n\nThis one looks good to me (I like the --no-{min,max}-parents).\n\n> 1/3 -> 1/5 unchanged\n> \n> 2/3 -> 2/5 unchanged\n> \n> 3/5 is !squash for 2/5 and introduces --no-min-parents and --no-max-parents\n> as natural ways to reset the limits\n> \n> 3/3 -> 4/5 with a fix to the notation in documentation (spell out =)\n> and an additional dodecapus test\n> \n> 5/5 is !fixup for 4/5 and adds test, doc and completion for --no-min-parents\n> and --no-max-parents\n> \n> Junio, please let me/us know whether sending an amended series in this way\n> (which I've seen before) is actually convenient for you or not. !squash\n> commits require a message edit, for example. OTOH, I don't know any (other?)\n> good inter diff solution.\n\nAs a reviewer, I find it is usually most convenient to just have the\nsubmitter do the squash, and then write below the \"---\" (or in a cover\nletter like this) a brief explanation of the differences.\n\nYeah, I end up reading the whole patch again, including bits I already\nread, but that is probably a good thing. A good patch is hopefully not\ntoo long, and it is easier (to me, anyway) to review it as a whole and\nnot as an interdiff.\n\nTechnically speaking, what you posted gives more information, and I\ncould always apply and squash myself to get the final form. But I'm lazy\nand I tend to review straight from the mail reader or from $EDITOR while\nreplying. So to me it's just an extra step.\n\nI have occasionally seen people with long patches reply to the patch\nthemselves and say \"btw, it is hard to see what is changed here in v2,\nso here is a much more readable interdiff from v1\". And that can be\nuseful at times, but it's nice to have the patch author using their\njudgement about when it will be useful or not.\n\nJust my two cents.\n\n> Thanks for everyone's input which went into this (Junio, Jeff, Jonathan).\n\nThank you for taking the lead on writing it. :)\n\n-Peff\n"},{"id":"164168","messageId":"7v4o6tg3qo.fsf@alter.siamese.dyndns.org","threadId":"26762","inReplyTo":"20110323144835.GA30337@sigill.intra.peff.net","subject":"Re: [PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-23T17:12:47Z","receivedAt":"2011-03-23T17:12:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Junio, please let me/us know whether sending an amended series in this way\n>> (which I've seen before) is actually convenient for you or not. !squash\n>> commits require a message edit, for example. OTOH, I don't know any (other?)\n>> good inter diff solution.\n>\n> As a reviewer, I find it is usually most convenient to just have the\n> submitter do the squash, and then write below the \"---\" (or in a cover\n> letter like this) a brief explanation of the differences.\n>\n> Yeah, I end up reading the whole patch again, including bits I already\n> read, but that is probably a good thing. A good patch is hopefully not\n> too long, and it is easier (to me, anyway) to review it as a whole and\n> not as an interdiff.\n\nYes. And that would also make it easier for people who didn't read the\nearlier round to double check.\n"},{"id":"164172","messageId":"7voc51emoq.fsf@alter.siamese.dyndns.org","threadId":"26762","inReplyTo":"cover.1300872923.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-23T18:06:29Z","receivedAt":"2011-03-23T18:06:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Compared to what is currently in pu (which is v2+eps), v3 has:\n>\n> 1/3 -> 1/5 unchanged\n>\n> 2/3 -> 2/5 unchanged\n>\n> 3/5 is !squash for 2/5 and introduces --no-min-parents and --no-max-parents\n> as natural ways to reset the limits\n>\n> 3/3 -> 4/5 with a fix to the notation in documentation (spell out =)\n> and an additional dodecapus test\n>\n> 5/5 is !fixup for 4/5 and adds test, doc and completion for --no-min-parents\n> and --no-max-parents\n\nLooked good; will replace what is in 'pu' and advance the topic to 'next'.\n"},{"id":"164217","messageId":"20110324082108.GA30196@elie","threadId":"26762","inReplyTo":"cover.1300872923.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-24T08:21:24Z","receivedAt":"2011-03-24T08:21:24Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael J Gruber wrote:\n\n> Compared to what is currently in pu (which is v2+eps), v3 has:\n\nThanks. I reviewed this by looking at what Junio queued in pu; it\nlooks very good. With the following patch, it passes tests.\n\nThe use of \"return\" was surprising. It seems this style has been\nintended to work ever since v0.99.5~24^2~4 (Trapping exit in tests,\nusing return for errors, 2005-08-10).\n\nIt interacts poorly with test_when_finished but since these tests do\nnot use that function, they should be safe. test_when_finished could\nuse some fixes to avoid future surprises but that's another story.\n\n-- 8< --\nSubject: tests: avoid nonportable {foo,bar} glob\n\nUnlike bash and ksh, dash and busybox ash do not support brace\nexpansion (as in 'echo {hello,world}'). So when dash is sh,\nt6009.13 (set up dodecapus) ends up pass a string beginning with\n\"root{1,2,\" to \"git merge\" verbatim and the test fails.\n\nFix it by introducing a variable to hold the list of parents for\nthe dodecapus and populating it in a more low-tech way.\n\nWhile at it, simplify a little by combining this setup code with the\ntest it sets up for.\n\nSigned-off-by: Jonathan Nieder \n---\n t/t6009-rev-list-parent.sh | 16 ++++++++--------\n 1 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\nindex fc89d6d..3050740 100755\n--- a/t/t6009-rev-list-parent.sh\n+++ b/t/t6009-rev-list-parent.sh\n@@ -112,20 +112,20 @@ test_expect_success 'rev-list override and infinities' '\n \tcheck_revlist \"--max-parents=0 --min-parents=1 --no-min-parents\" one five\n '\n \n-test_expect_success 'set up dodecapus' '\n+test_expect_success 'dodecapus' '\n \n+\troots= &&\n \tfor i in 1 2 3 4 5 6 7 8 9 10 11\n \tdo\n-\t\tgit checkout -b root$i five || return\n-\t\ttest_commit $i || return\n+\t\tgit checkout -b root$i five &&\n+\t\ttest_commit $i &&\n+\t\troots=\"$roots root$i\" ||\n+\t\treturn\n \tdone &&\n \tgit checkout master &&\n \ttest_tick &&\n-\tgit merge -m dodecapus root{1,2,3,4,5,6,7,8,9,10,11} &&\n-\tgit tag dodecapus\n-'\n-\n-test_expect_success 'test with dodecapus' '\n+\tgit merge -m dodecapus $roots &&\n+\tgit tag dodecapus &&\n \n \tcheck_revlist \"--min-parents=4\" dodecapus tetrapus &&\n \tcheck_revlist \"--min-parents=8\" dodecapus &&\n-- \n1.7.4.1\n"},{"id":"164218","messageId":"4D8B0712.6010300@drmicha.warpmail.net","threadId":"26762","inReplyTo":"20110324082108.GA30196@elie","subject":"Re: [PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-24T08:55:46Z","receivedAt":"2011-03-24T08:55:46Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jonathan Nieder venit, vidit, dixit 24.03.2011 09:21:\n> Michael J Gruber wrote:\n> \n>> Compared to what is currently in pu (which is v2+eps), v3 has:\n> \n> Thanks. I reviewed this by looking at what Junio queued in pu; it\n\nOK, no more !fixup and !squash next time ;)\n\n> looks very good. With the following patch, it passes tests.\n\nUhm, it does so even without (for me) for bash and ksh, as you point out\nbelow. What's the simplest way to run the tests so that they catch\nnon-POSIXisms?\n\n> The use of \"return\" was surprising. It seems this style has been\n> intended to work ever since v0.99.5~24^2~4 (Trapping exit in tests,\n> using return for errors, 2005-08-10).\n\nI learned that from other tests and considered it safe therefore.\n\n> It interacts poorly with test_when_finished but since these tests do\n> not use that function, they should be safe. test_when_finished could\n> use some fixes to avoid future surprises but that's another story.\n> \n> -- 8< --\n> Subject: tests: avoid nonportable {foo,bar} glob\n> \n> Unlike bash and ksh, dash and busybox ash do not support brace\n> expansion (as in 'echo {hello,world}'). So when dash is sh,\n> t6009.13 (set up dodecapus) ends up pass a string beginning with\n> \"root{1,2,\" to \"git merge\" verbatim and the test fails.\n\nThanks, I was afraid of a problem like that but then forgot about it.\n\n> Fix it by introducing a variable to hold the list of parents for\n> the dodecapus and populating it in a more low-tech way.\n> \n> While at it, simplify a little by combining this setup code with the\n> test it sets up for.\n\nI thought it's considered bad style to mix setup and actual test?\nPersonally, I don't care either way. I just didn't want to have these 12\nextra commits in the initial setup (forcing me to adjust all tests).\n\nMichael\n"},{"id":"164219","messageId":"20110324094231.GC32092@elie","threadId":"26762","inReplyTo":"4D8B0712.6010300@drmicha.warpmail.net","subject":"Re: [PATCHv3 0/5]rev-list and friends: --min-parents, --max-parents","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-24T09:42:31Z","receivedAt":"2011-03-24T09:42:31Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael J Gruber wrote:\n\n> What's the simplest way to run the tests so that they catch\n> non-POSIXisms?\n\nIn theory: use posh from .\n\nIn practice, testing with dash seems to work well for catching these.\ndash tends to be less buggy but more forgiving because some people\n(like me :)) use it day-to-day.\n\n> I thought it's considered bad style to mix setup and actual test?\n> Personally, I don't care either way. I just didn't want to have these 12\n> extra commits in the initial setup (forcing me to adjust all tests).\n\nOther test frameworks (inspired by JUnit?) like to separate errors\nfrom set-up, tear-down, and the meat of the tests, but the git's test\nsuite doesn't do that. Its goal is to test git rather than to track\nbugs in the harness.\n\nStill, many scripts include a test that sets up some state to be\nshared by later tests. This way, if git is severely broken (e.g., it\nsegfaults), then even the setup will fail; and meanwhile, it means it\nis still possible to safely skip other individual tests with\nGIT_SKIP_TESTS.\n\nAnyway, I shouldn't have included such a tiny nit with the brace\nexpansion fix. Sorry about that. Here's a more minimal patch.\n\n-- 8< --\nSubject: tests: avoid nonportable {foo,bar} glob\n\nUnlike bash and ksh, dash and busybox ash do not support brace\nexpansion (as in 'echo {hello,world}'). So when dash is sh,\nt6009.13 (set up dodecapus) ends up passing a string beginning with\n\"root{1,2,\" to \"git merge\" verbatim and the test fails.\n\nFix it by introducing a variable to hold the list of parents for\nthe dodecapus and populating it in a more low-tech way.\n\nSigned-off-by: Jonathan Nieder \n---\n t/t6009-rev-list-parent.sh | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\nindex fc89d6d..b30834d 100755\n--- a/t/t6009-rev-list-parent.sh\n+++ b/t/t6009-rev-list-parent.sh\n@@ -114,14 +114,16 @@ test_expect_success 'rev-list override and infinities' '\n \n test_expect_success 'set up dodecapus' '\n \n+\troots= &&\n \tfor i in 1 2 3 4 5 6 7 8 9 10 11\n \tdo\n \t\tgit checkout -b root$i five || return\n \t\ttest_commit $i || return\n+\t\troots=\"$roots root$i\" || return\n \tdone &&\n \tgit checkout master &&\n \ttest_tick &&\n-\tgit merge -m dodecapus root{1,2,3,4,5,6,7,8,9,10,11} &&\n+\tgit merge -m dodecapus $roots &&\n \tgit tag dodecapus\n '\n \n-- \n1.7.4.1\n"},{"id":"173108","messageId":"20110808011341.GA19551@elie.gateway.2wire.net","threadId":"26762","inReplyTo":"20110324082108.GA30196@elie","subject":"[PATCH/RFC 0/2] test_when_finished and returning early","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-08T01:13:41Z","receivedAt":"2011-08-08T01:13:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> The use of \"return\" was surprising. It seems this style has been\n> intended to work ever since v0.99.5~24^2~4 (Trapping exit in tests,\n> using return for errors, 2005-08-10).\n>\n> It interacts poorly with test_when_finished but since these tests do\n> not use that function, they should be safe. test_when_finished could\n> use some fixes to avoid future surprises but that's another story.\n\nThe above was about some code that looks like this:\n\n|\tfor i in 1 2 3 4 5 6 7 8 9 10 11\n| \tdo\n|\t\tgit checkout -b root$i five || return\n\nThe fixes alluded to might go something like this. Thoughts?\n\nJonathan Nieder (2):\n test: simplify return value of test_run_\n test: cope better with use of return for errors\n\n t/test-lib.sh | 18 +++++++++++-------\n 1 files changed, 11 insertions(+), 7 deletions(-)\n"},{"id":"173109","messageId":"20110808011534.GB19551@elie.gateway.2wire.net","threadId":"26762","inReplyTo":"20110808011341.GA19551@elie.gateway.2wire.net","subject":"[PATCH 1/2] test: simplify return value of test_run_","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-08T01:15:34Z","receivedAt":"2011-08-08T01:15:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"As v0.99.5~24^2~4 (Trapping exit in tests, using return for errors,\n2005-08-10) explains, callers to test_run_ (such as test_expect_code)\nused to check the result from eval and the return value separately so\ntests that fail early could be distinguished from tests that completed\nnormally with successful (nonzero) status. Eventually tests that\nsucceed with nonzero status were phased out (see v1.7.4-rc0~65^2~19,\n2010-10-03 and especially v1.5.5-rc0~271, 2008-02-01) but the weird\ntwo-return-value calling convention lives on.\n\nLet's get rid of it. The new rule: test_run_ succeeds (returns 0)\nif and only if the test succeeded.\n\nSigned-off-by: Jonathan Nieder \n---\n t/test-lib.sh | 8 +++-----\n 1 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex df25f179..b16a9b98 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -457,7 +457,7 @@ test_run_ () {\n \tif test \"$verbose\" = \"t\" && test -n \"$HARNESS_ACTIVE\"; then\n \t\techo \"\"\n \tfi\n-\treturn 0\n+\treturn \"$eval_ret\"\n }\n \n test_skip () {\n@@ -502,8 +502,7 @@ test_expect_failure () {\n \tif ! test_skip \"$@\"\n \tthen\n \t\tsay >&3 \"checking known breakage: $2\"\n-\t\ttest_run_ \"$2\" expecting_failure\n-\t\tif [ \"$?\" = 0 -a \"$eval_ret\" = 0 ]\n+\t\tif test_run_ \"$2\" expecting_failure\n \t\tthen\n \t\t\ttest_known_broken_ok_ \"$1\"\n \t\telse\n@@ -521,8 +520,7 @@ test_expect_success () {\n \tif ! test_skip \"$@\"\n \tthen\n \t\tsay >&3 \"expecting success: $2\"\n-\t\ttest_run_ \"$2\"\n-\t\tif [ \"$?\" = 0 -a \"$eval_ret\" = 0 ]\n+\t\tif test_run_ \"$2\"\n \t\tthen\n \t\t\ttest_ok_ \"$1\"\n \t\telse\n-- \n1.7.6\n"},{"id":"173110","messageId":"20110808011709.GC19551@elie.gateway.2wire.net","threadId":"26762","inReplyTo":"20110808011341.GA19551@elie.gateway.2wire.net","subject":"[PATCH 2/2] test: cope better with use of return for errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-08T01:17:09Z","receivedAt":"2011-08-08T01:17:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"In olden times, tests would quietly exit the script when they failed\nat an inconvenient moment, which was a little disconcerting.\nTherefore v0.99.5~24^2~4 (Trapping exit in tests, using return for\nerrors, 2005-08-10) switched to an idiom of using \"return\" instead,\nwrapping evaluation of test code in a function to make that safe:\n\n\ttest_run_ () {\n\t\teval >&3 2>&4 \"$1\"\n\t\teval_ret=\"$?\"\n\t\treturn 0\n\t}\n\nYears later, the implementation of test_when_finished (v1.7.1.1~95,\n2010-05-02) and v1.7.2-rc2~1^2~13 (test-lib: output a newline before\n\"ok\" under a TAP harness, 2010-06-24) took advantage of test_run_ as a\nplace to put code shared by all test assertion functions, without\npaying attention to the function's former purpose:\n\n\ttest_run_ () {\n\t\t...\n\t\teval >&3 2>&4 \"$1\"\n\t\teval_ret=$?\n\n\t\tif should run cleanup\n\t\tthen\n\t\t\teval >&3 2>&4 \"$test_cleanup\"\n\t\tfi\n\t\tif TAP format requires a newline here\n\t\tthen\n\t\t\techo\n\t\tfi\n\t\treturn 0\n\t}\n\nThat means cleanup commands and the newline to put TAP output at\ncolumn 0 are skipped when tests use \"return\" to fail early. Fix it by\nintroducing a test_eval_ function to catch the \"return\", with a\ncomment explaining the new function's purpose for the next person who\nmight touch this code.\n\nSigned-off-by: Jonathan Nieder \n---\n t/test-lib.sh | 10 ++++++++--\n 1 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex b16a9b98..57c3d532 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -444,15 +444,21 @@ test_debug () {\n \ttest \"$debug\" = \"\" || eval \"$1\"\n }\n \n+test_eval_ () {\n+\t# This is a separate function because some tests use\n+\t# \"return\" to end a test_expect_success block early.\n+\teval >&3 2>&4 \"$*\"\n+}\n+\n test_run_ () {\n \ttest_cleanup=:\n \texpecting_failure=$2\n-\teval >&3 2>&4 \"$1\"\n+\ttest_eval_ \"$1\"\n \teval_ret=$?\n \n \tif test -z \"$immediate\" || test $eval_ret = 0 || test -n \"$expecting_failure\"\n \tthen\n-\t\teval >&3 2>&4 \"$test_cleanup\"\n+\t\ttest_eval_ \"$test_cleanup\"\n \tfi\n \tif test \"$verbose\" = \"t\" && test -n \"$HARNESS_ACTIVE\"; then\n \t\techo \"\"\n-- \n1.7.6\n"},{"id":"173111","messageId":"20110808012637.GA2352@sigill.intra.peff.net","threadId":"26762","inReplyTo":"20110808011341.GA19551@elie.gateway.2wire.net","subject":"Re: [PATCH/RFC 0/2] test_when_finished and returning early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-08T01:26:37Z","receivedAt":"2011-08-08T01:26:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 08, 2011 at 03:13:41AM +0200, Jonathan Nieder wrote:\n\n> Jonathan Nieder wrote:\n> \n> > The use of \"return\" was surprising. It seems this style has been\n> > intended to work ever since v0.99.5~24^2~4 (Trapping exit in tests,\n> > using return for errors, 2005-08-10).\n> >\n> > It interacts poorly with test_when_finished but since these tests do\n> > not use that function, they should be safe. test_when_finished could\n> > use some fixes to avoid future surprises but that's another story.\n> \n> The above was about some code that looks like this:\n> \n> |\tfor i in 1 2 3 4 5 6 7 8 9 10 11\n> | \tdo\n> |\t\tgit checkout -b root$i five || return\n> \n> The fixes alluded to might go something like this. Thoughts?\n> \n> Jonathan Nieder (2):\n> test: simplify return value of test_run_\n> test: cope better with use of return for errors\n\nBoth look sane to me. Thanks for a nicely written set of commit messages\nexplaining the rather subtle issue.\n\n-Peff\n"},{"id":"173202","messageId":"4E40F3EA.8020406@viscovery.net","threadId":"26762","inReplyTo":"20110808011709.GC19551@elie.gateway.2wire.net","subject":"Re: [PATCH 2/2] test: cope better with use of return for errors","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-08-09T08:46:34Z","receivedAt":"2011-08-09T08:46:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 8/8/2011 3:17, schrieb Jonathan Nieder:\n> +test_eval_ () {\n> +\t# This is a separate function because some tests use\n> +\t# \"return\" to end a test_expect_success block early.\n> +\teval >&3 2>&4 \"$*\"\n> +}\n> +\n> test_run_ () {\n> \ttest_cleanup=:\n> \texpecting_failure=$2\n> -\teval >&3 2>&4 \"$1\"\n> +\ttest_eval_ \"$1\"\n> \teval_ret=$?\n> \n> \tif test -z \"$immediate\" || test $eval_ret = 0 || test -n \"$expecting_failure\"\n> \tthen\n> -\t\teval >&3 2>&4 \"$test_cleanup\"\n> +\t\ttest_eval_ \"$test_cleanup\"\n> \tfi\n> \tif test \"$verbose\" = \"t\" && test -n \"$HARNESS_ACTIVE\"; then\n> \t\techo \"\"\n\nThis invalidates at least t3900.29, which accesses an unexpanded $3\nfrom the test script. The patch below fixes this case.\n\nI tried to detect other cases by poisoning test_run_ like this:\n\n- test_eval_ \"$1\"\n+ test_eval_ :\\; :\\; :\\; :\\; :\\; \"$1\"\n\nin the hopes that \":;\" is an error at the place that uses $1, $2, etc.\nt3900.2[89] are the only tests that are uncovered in this way.\n\nI noticed this because I have a patched test-lib.sh that calls\ntest_eval_ in a similarly modified manner.\n\n--- >8 ---\nFrom: Johannes Sixt \nSubject: [PATCH] t3900: do not reference numbered arguments from the test\n script\n\nThe call to test_expect_success is nested inside a function, whose\narguments the test code wants to access. But it is not specified that any\nunexpanded $1, $2, $3, etc in the test code will access the surrounding\nfunction's arguments. Rather, they will access the arguments of the\nfunction that happens to eval the test code.\n\nPlay safe by placing the argument in a named variable.\n\nSigned-off-by: Johannes Sixt \n---\n t/t3900-i18n-commit.sh | 5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh\nindex c06a5ee..3265fac 100755\n--- a/t/t3900-i18n-commit.sh\n+++ b/t/t3900-i18n-commit.sh\n@@ -136,6 +136,7 @@ done\n test_commit_autosquash_flags () {\n \tH=$1\n \tflag=$2\n+\tmopt=$3\n \ttest_expect_success \"commit --$flag with $H encoding\" '\n \t\tgit config i18n.commitencoding $H &&\n \t\tgit checkout -b $H-$flag C0 &&\n@@ -147,7 +148,7 @@ test_commit_autosquash_flags () {\n \t\tgit commit -a -m \"intermediate commit\" &&\n \t\ttest_tick &&\n \t\techo $H $flag >>F &&\n-\t\tgit commit -a --$flag HEAD~1 $3 &&\n+\t\tgit commit -a --$flag HEAD~1 $mopt &&\n \t\tE=$(git cat-file commit '$H-$flag' |\n \t\t\tsed -ne \"s/^encoding //p\") &&\n \t\ttest \"z$E\" = \"z$H\" &&\n@@ -160,6 +161,6 @@ test_commit_autosquash_flags () {\n \n test_commit_autosquash_flags eucJP fixup\n \n-test_commit_autosquash_flags ISO-2022-JP squash '-m \"squash message\"'\n+test_commit_autosquash_flags ISO-2022-JP squash '-m squash_message'\n \n test_done\n-- \n1.7.6.1618.gc932c\n"},{"id":"173209","messageId":"20110809153638.GA15687@sigill.intra.peff.net","threadId":"26762","inReplyTo":"4E40F3EA.8020406@viscovery.net","subject":"Re: [PATCH 2/2] test: cope better with use of return for errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-09T15:36:39Z","receivedAt":"2011-08-09T15:36:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 09, 2011 at 10:46:34AM +0200, Johannes Sixt wrote:\n\n> Am 8/8/2011 3:17, schrieb Jonathan Nieder:\n> > +test_eval_ () {\n> > +\t# This is a separate function because some tests use\n> > +\t# \"return\" to end a test_expect_success block early.\n> > +\teval >&3 2>&4 \"$*\"\n> > +}\n> > +\n> > test_run_ () {\n> > \ttest_cleanup=:\n> > \texpecting_failure=$2\n> > -\teval >&3 2>&4 \"$1\"\n> > +\ttest_eval_ \"$1\"\n> > \teval_ret=$?\n> > \n> > \tif test -z \"$immediate\" || test $eval_ret = 0 || test -n \"$expecting_failure\"\n> > \tthen\n> > -\t\teval >&3 2>&4 \"$test_cleanup\"\n> > +\t\ttest_eval_ \"$test_cleanup\"\n> > \tfi\n> > \tif test \"$verbose\" = \"t\" && test -n \"$HARNESS_ACTIVE\"; then\n> > \t\techo \"\"\n> \n> This invalidates at least t3900.29, which accesses an unexpanded $3\n> from the test script. The patch below fixes this case.\n\nHmm. Isn't t3900 already broken, even without this patch? It does\nsomething like this:\n\n foo() {\n test_expect_success 'bar' 'echo $3'\n }\n\nThat can't possibly access the third positional parameter of foo, as we\nare already inside the test_expect_success function, no matter what\nfunctions we call after that.\n\nIf there is any regression here, it would be that we used to get the\npositional parameters of test_run_, and now we get them from test_eval_.\nSo accessing \"$2\" used to get us $expecting_failure, but now gets\nnothing. I doubt it matters, and tests would be insane to rely on\nsomething internal like that. If it did, the right fix would be:\n\n - test_eval_ \"$1\"\n + test_eval_ \"$@\"\n\nin test_run_.\n\nSo I don't see a problem in Jonathan's patches. But clearly t3900 is\npoorly written and should be fixed.\n\nBut:\n\n> diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh\n> index c06a5ee..3265fac 100755\n> --- a/t/t3900-i18n-commit.sh\n> +++ b/t/t3900-i18n-commit.sh\n> @@ -136,6 +136,7 @@ done\n> test_commit_autosquash_flags () {\n> \tH=$1\n> \tflag=$2\n> +\tmopt=$3\n> \ttest_expect_success \"commit --$flag with $H encoding\" '\n> \t\tgit config i18n.commitencoding $H &&\n> \t\tgit checkout -b $H-$flag C0 &&\n> @@ -147,7 +148,7 @@ test_commit_autosquash_flags () {\n> \t\tgit commit -a -m \"intermediate commit\" &&\n> \t\ttest_tick &&\n> \t\techo $H $flag >>F &&\n> -\t\tgit commit -a --$flag HEAD~1 $3 &&\n> +\t\tgit commit -a --$flag HEAD~1 $mopt &&\n> \t\tE=$(git cat-file commit '$H-$flag' |\n> \t\t\tsed -ne \"s/^encoding //p\") &&\n> \t\ttest \"z$E\" = \"z$H\" &&\n> @@ -160,6 +161,6 @@ test_commit_autosquash_flags () {\n> \n> test_commit_autosquash_flags eucJP fixup\n> \n> -test_commit_autosquash_flags ISO-2022-JP squash '-m \"squash message\"'\n> +test_commit_autosquash_flags ISO-2022-JP squash '-m squash_message'\n\nCan't we just drop $3 here? I don't see how it's actually doing\nanything. It makes us call:\n\n git commit --squash HEAD~1 -m squash_message\n\ninstead of just:\n\n git commit --squash HEAD~1\n\nbut then we never actually check to see that the included message\nbecomes part of the squashed commit, anyway.\n\nAlso, a side note. These tests put their shell snippet inside single\nquotes. And then access the variables sometimes directly (assuming they\nare left unchanged inside the test functions), and sometimes using\nsingle quotes. Which actually places them outside the snippet's single\nquotes, and expands them in place. Which happens to be OK because they\ndon't contain any spaces, but would otherwise pass extra arguments to\ntest_expect_success.\n\nSo it's not a bug, but it does look unintentional and poor style.\n\nAnyway. Your patch is fine and sufficient to solve the problem. Those\nwere all just thoughtst I had while trying to figure out what the test\nwas actually trying to do.\n\n-Peff\n"},{"id":"173260","messageId":"4E437F4C.4020305@viscovery.net","threadId":"26762","inReplyTo":"20110809153638.GA15687@sigill.intra.peff.net","subject":"[PATCH v2] t3900: do not reference numbered arguments from the test script","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-08-11T07:05:48Z","receivedAt":"2011-08-11T07:05:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt \n\nThe call to test_expect_success is nested inside a function, whose\narguments the test code wants to access. But it is not specified that any\nunexpanded $1, $2, $3, etc in the test code will access the surrounding\nfunction's arguments. Rather, they will access the arguments of the\nfunction that happens to eval the test code.\n\nIn this case, the reference is intended to supply '-m message' to a call of\n'git commit --squash'. Remove it because -m is optional and the test case\ndoes not check for it. There are tests in t7500 that check combinations of\n--squash and -m.\n\nSigned-off-by: Johannes Sixt \n---\nAm 8/9/2011 17:36, schrieb Jeff King:\n> Hmm. Isn't t3900 already broken, even without this patch? It does\n> something like this:\n> \n> foo() {\n> test_expect_success 'bar' 'echo $3'\n> }\n\nYes, it's broken and this patch just removes the reference.\n\n t/t3900-i18n-commit.sh | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh\nindex c06a5ee..1f62c15 100755\n--- a/t/t3900-i18n-commit.sh\n+++ b/t/t3900-i18n-commit.sh\n@@ -147,7 +147,7 @@ test_commit_autosquash_flags () {\n \t\tgit commit -a -m \"intermediate commit\" &&\n \t\ttest_tick &&\n \t\techo $H $flag >>F &&\n-\t\tgit commit -a --$flag HEAD~1 $3 &&\n+\t\tgit commit -a --$flag HEAD~1 &&\n \t\tE=$(git cat-file commit '$H-$flag' |\n \t\t\tsed -ne \"s/^encoding //p\") &&\n \t\ttest \"z$E\" = \"z$H\" &&\n@@ -160,6 +160,6 @@ test_commit_autosquash_flags () {\n \n test_commit_autosquash_flags eucJP fixup\n \n-test_commit_autosquash_flags ISO-2022-JP squash '-m \"squash message\"'\n+test_commit_autosquash_flags ISO-2022-JP squash\n \n test_done\n-- \n1.7.6.1618.gc932c\n"},{"id":"173261","messageId":"20110811071155.GA28213@elie.gateway.2wire.net","threadId":"26762","inReplyTo":"4E437F4C.4020305@viscovery.net","subject":"Re: [PATCH v2] t3900: do not reference numbered arguments from the test script","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-11T07:11:55Z","receivedAt":"2011-08-11T07:11:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Sixt wrote:\n\n> Remove it because -m is optional and the test case\n> does not check for it. There are tests in t7500 that check combinations of\n> --squash and -m.\n\nThat's a comfort. Looks obviously good to me, fwiw.\n"},{"id":"173317","messageId":"7v1uwrr540.fsf@alter.siamese.dyndns.org","threadId":"26762","inReplyTo":"20110811071155.GA28213@elie.gateway.2wire.net","subject":"Re: [PATCH v2] t3900: do not reference numbered arguments from the test script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-11T21:49:19Z","receivedAt":"2011-08-11T21:49:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Johannes Sixt wrote:\n>\n>> Remove it because -m is optional and the test case\n>> does not check for it. There are tests in t7500 that check combinations of\n>> --squash and -m.\n>\n> That's a comfort. Looks obviously good to me, fwiw.\n\nYeah, looks sane. Thanks everybody.\n"}]}