{"thread":{"id":"44163","subject":"[RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev","startedAt":"2016-09-26T20:53:21Z","lastAt":"2016-09-27T06:10:45Z","messageCount":5,"participants":["Vegard Nossum","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"302635","messageId":"20160926204959.26007-1-vegard.nossum@oracle.com","threadId":"44163","inReplyTo":null,"subject":"[RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2016-09-26T20:49:59Z","receivedAt":"2016-09-26T20:53:21Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"I often use rev^..rev to get all the commits in the branch that was merged\nin by the merge commit 'rev' (including the merge itself). To save typing\n(or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)\nwe can make rev^- a shorthand for that.\n\nThe existing syntax rev^! seems like it should do the same thing, but it\ndoesn't really do the right thing for merge commits (it doesn't include\nthe commits from side branches).\n\nAs a natural generalisation, we also accept rev^-n where n excludes the\nnth parent of rev. For example, for a two-parent merge, you can use rev^-2\nto get the set of commits which were made to the main branch while the\ntopic branch was prepared.\n\nSigned-off-by: Vegard Nossum <vegard.nossum@oracle.com>\n\n---\n[v2: Use ^- instead of % as suggested by Junio Hamano and use some\n common helper functions for parsing.]\n\n[v3: Use 'struct object_id' instead of 'char[20]' and add some tests as\n suggested by Matthieu Moy; fix missing '-' in Documentation/revisions.txt\n as suggested by Ramsay Jones; misc changelog + documentation fixes as\n suggested by Philip Oakley.]\n\n[v4: Documentation fixes and parsing rework suggested by Junio Hamano\n and add some more tests.]\n---\n Documentation/revisions.txt     | 17 +++++++-\n builtin/rev-parse.c             | 47 +++++++++++++++------\n revision.c                      | 32 +++++++++++++--\n t/t6070-rev-parent-exclusion.sh | 90 +++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 168 insertions(+), 18 deletions(-)\n create mode 100755 t/t6070-rev-parent-exclusion.sh\n\ndiff --git Documentation/revisions.txt Documentation/revisions.txt\nindex 4bed5b1..ba11b9c 100644\n--- Documentation/revisions.txt\n+++ Documentation/revisions.txt\n@@ -283,7 +283,7 @@ empty range that is both reachable and unreachable from HEAD.\n \n Other <rev>{caret} Parent Shorthand Notations\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n-Two other shorthands exist, particularly useful for merge commits,\n+Three other shorthands exist, particularly useful for merge commits,\n for naming a set that is formed by a commit and its parent commits.\n \n The 'r1{caret}@' notation means all parents of 'r1'.\n@@ -291,8 +291,15 @@ The 'r1{caret}@' notation means all parents of 'r1'.\n The 'r1{caret}!' notation includes commit 'r1' but excludes all of its parents.\n By itself, this notation denotes the single commit 'r1'.\n \n+The '<rev>{caret}-{<n>}' notation includes '<rev>' but excludes the <n>th\n+parent (i.e. a shorthand for '<rev>{caret}<n>..<rev>'), with '<n>' = 1 if\n+not given. This is typically useful for merge commits where you\n+can just pass '<commit>{caret}-' to get all the commits in the branch\n+that was merged in merge commit '<commit>' (including '<commit>'\n+itself).\n+\n While '<rev>{caret}<n>' was about specifying a single commit parent, these\n-two notations consider all its parents. For example you can say\n+three notations also consider its parents. For example you can say\n 'HEAD{caret}2{caret}@', however you cannot say 'HEAD{caret}@{caret}2'.\n \n Revision Range Summary\n@@ -326,6 +333,10 @@ Revision Range Summary\n   as giving commit '<rev>' and then all its parents prefixed with\n   '{caret}' to exclude them (and their ancestors).\n \n+'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}-, HEAD{caret}-2'::\n+\tEquivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not\n+\tgiven.\n+\n Here are a handful of examples using the Loeliger illustration above,\n with each step in the notation's expansion and selection carefully\n spelt out:\n@@ -339,6 +350,8 @@ spelt out:\n    C                            I J F C\n    B..C   = ^B C                C\n    B...C  = B ^F C              G H D E B C\n+   B^-    = B^..B\n+\t  = ^B^1 B              E I J F B\n    C^@    = C^1\n \t  = F                   I J F\n    B^@    = B^1 B^2 B^3\ndiff --git builtin/rev-parse.c builtin/rev-parse.c\nindex 76cf05e..2c3da19 100644\n--- builtin/rev-parse.c\n+++ builtin/rev-parse.c\n@@ -298,14 +298,30 @@ static int try_parent_shorthands(const char *arg)\n \tunsigned char sha1[20];\n \tstruct commit *commit;\n \tstruct commit_list *parents;\n-\tint parents_only;\n-\n-\tif ((dotdot = strstr(arg, \"^!\")))\n-\t\tparents_only = 0;\n-\telse if ((dotdot = strstr(arg, \"^@\")))\n-\t\tparents_only = 1;\n-\n-\tif (!dotdot || dotdot[2])\n+\tint parent_number;\n+\tint include_rev = 0;\n+\tint include_parents = 0;\n+\tint exclude_parent = 0;\n+\n+\tif ((dotdot = strstr(arg, \"^!\"))) {\n+\t\tinclude_rev = 1;\n+\t\tif (dotdot[2])\n+\t\t\treturn 0;\n+\t} else if ((dotdot = strstr(arg, \"^@\"))) {\n+\t\tinclude_parents = 1;\n+\t\tif (dotdot[2])\n+\t\t\treturn 0;\n+\t} else if ((dotdot = strstr(arg, \"^-\"))) {\n+\t\tinclude_rev = 1;\n+\t\texclude_parent = 1;\n+\n+\t\tif (dotdot[2]) {\n+\t\t\tchar *end;\n+\t\t\texclude_parent = strtoul(dotdot + 2, &end, 10);\n+\t\t\tif (*end != '\\0' || !exclude_parent)\n+\t\t\t\treturn 0;\n+\t\t}\n+\t} else\n \t\treturn 0;\n \n \t*dotdot = 0;\n@@ -314,14 +330,21 @@ static int try_parent_shorthands(const char *arg)\n \t\treturn 0;\n \t}\n \n-\tif (!parents_only)\n+\tif (include_rev)\n \t\tshow_rev(NORMAL, sha1, arg);\n \tcommit = lookup_commit_reference(sha1);\n-\tfor (parents = commit->parents; parents; parents = parents->next)\n-\t\tshow_rev(parents_only ? NORMAL : REVERSED,\n-\t\t\t\tparents->item->object.oid.hash, arg);\n+\tfor (parent_number = 1, parents = commit->parents;\n+\t     parents; parents = parents->next, parent_number++) {\n+\t\tif (exclude_parent && parent_number != exclude_parent)\n+\t\t\tcontinue;\n+\n+\t\tshow_rev(include_parents ? NORMAL : REVERSED,\n+\t\t\t parents->item->object.oid.hash, arg);\n+\t}\n \n \t*dotdot = '^';\n+\tif (exclude_parent >= parent_number)\n+\t\treturn 0;\n \treturn 1;\n }\n \ndiff --git revision.c revision.c\nindex 969b3d1..9ae95bf 100644\n--- revision.c\n+++ revision.c\n@@ -1289,12 +1289,14 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)\n \t}\n }\n \n-static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)\n+static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n+\t\t\t    int exclude_parent)\n {\n \tunsigned char sha1[20];\n \tstruct object *it;\n \tstruct commit *commit;\n \tstruct commit_list *parents;\n+\tint parent_number;\n \tconst char *arg = arg_;\n \n \tif (*arg == '^') {\n@@ -1316,12 +1318,18 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)\n \tif (it->type != OBJ_COMMIT)\n \t\treturn 0;\n \tcommit = (struct commit *)it;\n-\tfor (parents = commit->parents; parents; parents = parents->next) {\n+\tfor (parent_number = 1, parents = commit->parents;\n+\t     parents; parents = parents->next, parent_number++) {\n+\t\tif (exclude_parent && parent_number != exclude_parent)\n+\t\t\tcontinue;\n+\n \t\tit = &parents->item->object;\n \t\tit->flags |= flags;\n \t\tadd_rev_cmdline(revs, it, arg_, REV_CMD_PARENTS_ONLY, flags);\n \t\tadd_pending_object(revs, it, arg);\n \t}\n+\tif (exclude_parent >= parent_number)\n+\t\treturn 0;\n \treturn 1;\n }\n \n@@ -1519,17 +1527,33 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t}\n \t\t*dotdot = '.';\n \t}\n+\n \tdotdot = strstr(arg, \"^@\");\n \tif (dotdot && !dotdot[2]) {\n \t\t*dotdot = 0;\n-\t\tif (add_parents_only(revs, arg, flags))\n+\t\tif (add_parents_only(revs, arg, flags, 0))\n \t\t\treturn 0;\n \t\t*dotdot = '^';\n \t}\n \tdotdot = strstr(arg, \"^!\");\n \tif (dotdot && !dotdot[2]) {\n \t\t*dotdot = 0;\n-\t\tif (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM)))\n+\t\tif (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), 0))\n+\t\t\t*dotdot = '^';\n+\t}\n+\tdotdot = strstr(arg, \"^-\");\n+\tif (dotdot) {\n+\t\tint exclude_parent = 1;\n+\n+\t\tif (dotdot[2]) {\n+\t\t\tchar *end;\n+\t\t\texclude_parent = strtoul(dotdot + 2, &end, 10);\n+\t\t\tif (*end != '\\0' || !exclude_parent)\n+\t\t\t\treturn -1;\n+\t\t}\n+\n+\t\t*dotdot = 0;\n+\t\tif (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), exclude_parent))\n \t\t\t*dotdot = '^';\n \t}\n \ndiff --git t/t6070-rev-parent-exclusion.sh t/t6070-rev-parent-exclusion.sh\nnew file mode 100755\nindex 0000000..e6e5a8d\n--- /dev/null\n+++ t/t6070-rev-parent-exclusion.sh\n@@ -0,0 +1,90 @@\n+#!/bin/sh\n+\n+test_description='rev-list/rev-parse rev^- parsing'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\ttest_commit three &&\n+\n+\t# Merge in a branch for testing ^-\n+\tgit checkout -b branch &&\n+\tgit checkout HEAD^^ &&\n+\tgit merge -m merge --no-edit --no-ff branch &&\n+\tgit checkout -b merge\n+'\n+\n+# The merged branch has 2 commits + the merge\n+test_expect_success 'rev-list --count merge^- = merge^..merge' '\n+\tgit rev-list --count merge^..merge >expect &&\n+\techo 3 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# All rev-parse tests\n+\n+test_expect_success 'rev-parse merge^- = merge^..merge' '\n+\tgit rev-parse merge^..merge >expect &&\n+\tgit rev-parse merge^- >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-parse merge^-1 = merge^..merge' '\n+\tgit rev-parse merge^1..merge >expect &&\n+\tgit rev-parse merge^-1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-parse merge^-2 = merge^2..merge' '\n+\tgit rev-parse merge^2..merge >expect &&\n+\tgit rev-parse merge^-2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-parse merge^-0' '\n+\ttest_must_fail git rev-parse merge^-0\n+'\n+\n+test_expect_success 'rev-parse merge^-3' '\n+\ttest_must_fail git rev-parse merge^-3\n+'\n+\n+test_expect_success 'rev-parse merge^-^' '\n+\ttest_must_fail git rev-parse merge^-^\n+'\n+\n+# All rev-list tests (should be mostly the same as rev-parse)\n+\n+test_expect_success 'rev-list merge^- = merge^..merge' '\n+\tgit rev-list merge^..merge >expect &&\n+\tgit rev-list merge^- >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-list merge^-1 = merge^1..merge' '\n+\tgit rev-list merge^1..merge >expect &&\n+\tgit rev-list merge^-1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-list merge^-2 = merge^2..merge' '\n+\tgit rev-list merge^2..merge >expect &&\n+\tgit rev-list merge^-2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-list merge^-0' '\n+\ttest_must_fail git rev-list merge^-0\n+'\n+\n+test_expect_success 'rev-list merge^-3' '\n+\ttest_must_fail git rev-list merge^-3\n+'\n+\n+test_expect_success 'rev-list merge^-^' '\n+\ttest_must_fail git rev-list merge^-^\n+'\n+\n+test_done\n-- \n2.10.0.rc0.1.g07c9292\n\n"},{"id":"302639","messageId":"xmqqr386pd7d.fsf@gitster.mtv.corp.google.com","threadId":"44163","inReplyTo":"20160926204959.26007-1-vegard.nossum@oracle.com","subject":"Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T21:23:50Z","receivedAt":"2016-09-26T21:24:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vegard Nossum <vegard.nossum@oracle.com> writes:\n\n> I often use rev^..rev to get all the commits in the branch that was merged\n> in by the merge commit 'rev' (including the merge itself). To save typing\n> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)\n> we can make rev^- a shorthand for that.\n>\n> The existing syntax rev^! seems like it should do the same thing, but it\n> doesn't really do the right thing for merge commits (it doesn't include\n> the commits from side branches).\n>\n> As a natural generalisation, we also accept rev^-n where n excludes the\n> nth parent of rev. For example, for a two-parent merge, you can use rev^-2\n> to get the set of commits which were made to the main branch while the\n> topic branch was prepared.\n\nI am tempted to suggest that this four-line paragraph may be\nsufficient:\n    \n    \"git log rev^..rev\" is commonly used to show all work done on\n    and merged from a side branch.  Introduce a short-hand \"rev^-\"\n    for this, and also allow it to take \"rev^-$n\" to mean \"reachable\n    from rev, excluding what is reachable from n-th parent of rev\".\n\nThis alone is not a strong enough reason to ask you to reroll the\npatch.\n\n> diff --git builtin/rev-parse.c builtin/rev-parse.c\n> index 76cf05e..2c3da19 100644\n> --- builtin/rev-parse.c\n> +++ builtin/rev-parse.c\n> @@ -298,14 +298,30 @@ static int try_parent_shorthands(const char *arg)\n>  \tunsigned char sha1[20];\n>  \tstruct commit *commit;\n>  \tstruct commit_list *parents;\n> -\tint parents_only;\n> -\n> -\tif ((dotdot = strstr(arg, \"^!\")))\n> -\t\tparents_only = 0;\n> -\telse if ((dotdot = strstr(arg, \"^@\")))\n> -\t\tparents_only = 1;\n> -\n> -\tif (!dotdot || dotdot[2])\n> +\tint parent_number;\n> +\tint include_rev = 0;\n> +\tint include_parents = 0;\n> +\tint exclude_parent = 0;\n> +\n> +\tif ((dotdot = strstr(arg, \"^!\"))) {\n> +\t\tinclude_rev = 1;\n> +\t\tif (dotdot[2])\n> +\t\t\treturn 0;\n> +\t} else if ((dotdot = strstr(arg, \"^@\"))) {\n> +\t\tinclude_parents = 1;\n> +\t\tif (dotdot[2])\n> +\t\t\treturn 0;\n> +\t} else if ((dotdot = strstr(arg, \"^-\"))) {\n> +\t\tinclude_rev = 1;\n> +\t\texclude_parent = 1;\n> +\n> +\t\tif (dotdot[2]) {\n> +\t\t\tchar *end;\n> +\t\t\texclude_parent = strtoul(dotdot + 2, &end, 10);\n> +\t\t\tif (*end != '\\0' || !exclude_parent)\n> +\t\t\t\treturn 0;\n> +\t\t}\n> +\t} else\n>  \t\treturn 0;\n\nNice; we can tell where this is going without looking at the rest,\nwhich is a very good sign that the new variables are doing their\nwork of telling the readers what is going on clearly.\n\n> @@ -314,14 +330,21 @@ static int try_parent_shorthands(const char *arg)\n>  \t\treturn 0;\n>  \t}\n>  \n> -\tif (!parents_only)\n> +\tif (include_rev)\n>  \t\tshow_rev(NORMAL, sha1, arg);\n>  \tcommit = lookup_commit_reference(sha1);\n> -\tfor (parents = commit->parents; parents; parents = parents->next)\n> -\t\tshow_rev(parents_only ? NORMAL : REVERSED,\n> -\t\t\t\tparents->item->object.oid.hash, arg);\n> +\tfor (parent_number = 1, parents = commit->parents;\n> +\t     parents; parents = parents->next, parent_number++) {\n\nMicronit.  When splitting \"for (init; fini; cont)\" into multiple\nlines, it is often easier to read to make that into three lines:\n\n\tfor (parent_number = 1, parents = commit->parents;\n\t     parents;\n\t     parents = parents->next, parent_number++) {\n\n> +\t\tif (exclude_parent && parent_number != exclude_parent)\n> +\t\t\tcontinue;\n> +\n> +\t\tshow_rev(include_parents ? NORMAL : REVERSED,\n> +\t\t\t parents->item->object.oid.hash, arg);\n> +\t}\n\nIt is very clear to see what is going on.  Good job.\n\n>  \t*dotdot = '^';\n> +\tif (exclude_parent >= parent_number)\n> +\t\treturn 0;\n\nThis is not quite nice.  You've already called show_rev() number of\ntimes, and it is too late to signal an error here.  I think you\nwould need to count the number of parents much earlier when\nexclude_parent option is in effect and error out before making any\ncall to show_rev().\n\n> diff --git revision.c revision.c\n> index 969b3d1..9ae95bf 100644\n> --- revision.c\n> +++ revision.c\n> @@ -1289,12 +1289,14 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)\n>  \t}\n>  }\n>  \n> -static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)\n> +static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n> +\t\t\t    int exclude_parent)\n>  {\n>  \tunsigned char sha1[20];\n>  \tstruct object *it;\n>  \tstruct commit *commit;\n>  \tstruct commit_list *parents;\n> +\tint parent_number;\n>  \tconst char *arg = arg_;\n>  \n>  \tif (*arg == '^') {\n> @@ -1316,12 +1318,18 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)\n>  \tif (it->type != OBJ_COMMIT)\n>  \t\treturn 0;\n>  \tcommit = (struct commit *)it;\n> -\tfor (parents = commit->parents; parents; parents = parents->next) {\n> +\tfor (parent_number = 1, parents = commit->parents;\n> +\t     parents; parents = parents->next, parent_number++) {\n> +\t\tif (exclude_parent && parent_number != exclude_parent)\n> +\t\t\tcontinue;\n> +\n>  \t\tit = &parents->item->object;\n>  \t\tit->flags |= flags;\n>  \t\tadd_rev_cmdline(revs, it, arg_, REV_CMD_PARENTS_ONLY, flags);\n>  \t\tadd_pending_object(revs, it, arg);\n>  \t}\n> +\tif (exclude_parent >= parent_number)\n> +\t\treturn 0;\n\nLikewise.  It is way too late to say \"Nah, this wasn't a valid rev^-\nnotation after all\" to the caller after calling add_rev_cmdline()\nand add_pending_object() in the above loop.  Just like \"blob^-\"\nsilently returns 0 in the pre-context in this hunk, count the number\nof parents before entering this loop when exclude_parent is in\neffect, and if the number after '-' exceeds the actual number of\nparents, silently return 0, perhaps?\n\n> diff --git t/t6070-rev-parent-exclusion.sh t/t6070-rev-parent-exclusion.sh\n\nWe already seem to have t6101 as the best place to add test for this\nnew feature.  Near the end of that script, ^@ and ^! are tested.\n\nThanks.\n"},{"id":"302642","messageId":"xmqqh992pbq6.fsf@gitster.mtv.corp.google.com","threadId":"44163","inReplyTo":"xmqqr386pd7d.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T21:55:45Z","receivedAt":"2016-09-26T21:56:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Micronit.  When splitting \"for (init; fini; cont)\" into multiple\n> lines, it is often easier to read to make that into three lines:\n>\n> \tfor (parent_number = 1, parents = commit->parents;\n> \t     parents;\n> \t     parents = parents->next, parent_number++) {\n>\n>> +\t\tif (exclude_parent && parent_number != exclude_parent)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tshow_rev(include_parents ? NORMAL : REVERSED,\n>> +\t\t\t parents->item->object.oid.hash, arg);\n>> +\t}\n>\n> It is very clear to see what is going on.  Good job.\n>\n>>  \t*dotdot = '^';\n>> +\tif (exclude_parent >= parent_number)\n>> +\t\treturn 0;\n>\n> This is not quite nice.  You've already called show_rev() number of\n> times, and it is too late to signal an error here.  I think you\n> would need to count the number of parents much earlier when\n> exclude_parent option is in effect and error out before making any\n> call to show_rev().\n> ...\n> Likewise.  It is way too late to say \"Nah, this wasn't a valid rev^-\n> notation after all\" to the caller after calling add_rev_cmdline()\n> and add_pending_object() in the above loop.\n\nTaking these two together, perhaps squashing this in may be\nsufficient.\n\nPlease do not use --no-prefix when sending a patch to this list, by\nthe way.\n\n builtin/rev-parse.c | 18 +++++++++++++++---\n revision.c          | 17 ++++++++++++++++-\n 2 files changed, 31 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 2c3da19..9474c37 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -333,8 +333,22 @@ static int try_parent_shorthands(const char *arg)\n \tif (include_rev)\n \t\tshow_rev(NORMAL, sha1, arg);\n \tcommit = lookup_commit_reference(sha1);\n+\n+\tif (exclude_parent) {\n+\t\t/* do we have enough parents? */\n+\t\tfor (parent_number = 0, parents = commit->parents;\n+\t\t     parents;\n+\t\t     parents = parents->next)\n+\t\t\tparent_number++;\n+\t\tif (parent_number < exclude_parent) {\n+\t\t\t*dotdot = '^';\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n \tfor (parent_number = 1, parents = commit->parents;\n-\t     parents; parents = parents->next, parent_number++) {\n+\t     parents;\n+\t     parents = parents->next, parent_number++) {\n \t\tif (exclude_parent && parent_number != exclude_parent)\n \t\t\tcontinue;\n \n@@ -343,8 +357,6 @@ static int try_parent_shorthands(const char *arg)\n \t}\n \n \t*dotdot = '^';\n-\tif (exclude_parent >= parent_number)\n-\t\treturn 0;\n \treturn 1;\n }\n \ndiff --git a/revision.c b/revision.c\nindex 511e1ed..09da7f4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1318,8 +1318,23 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n \tif (it->type != OBJ_COMMIT)\n \t\treturn 0;\n \tcommit = (struct commit *)it;\n+\n+\tif (exclude_parent) {\n+\t\tstruct commit_list *parents;\n+\t\tint parent_number;\n+\n+\t\t/* do we have enough parents? */\n+\t\tfor (parent_number = 0, parents = commit->parents;\n+\t\t     parents;\n+\t\t     parents = parents->next)\n+\t\t\tparent_number++;\n+\t\tif (parent_number < exclude_parent)\n+\t\t\treturn 0;\n+\t}\n+\n \tfor (parent_number = 1, parents = commit->parents;\n-\t     parents; parents = parents->next, parent_number++) {\n+\t     parents;\n+\t     parents = parents->next, parent_number++) {\n \t\tif (exclude_parent && parent_number != exclude_parent)\n \t\t\tcontinue;\n \n"},{"id":"302644","messageId":"xmqqbmzapaza.fsf@gitster.mtv.corp.google.com","threadId":"44163","inReplyTo":"20160926204959.26007-1-vegard.nossum@oracle.com","subject":"Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T22:11:53Z","receivedAt":"2016-09-26T22:12:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vegard Nossum <vegard.nossum@oracle.com> writes:\n\n> +test_expect_success 'rev-parse merge^-0' '\n> +\ttest_must_fail git rev-parse merge^-0\n> +'\n> +\n> +test_expect_success 'rev-parse merge^-3' '\n> +\ttest_must_fail git rev-parse merge^-3\n> +'\n> +\n> +test_expect_success 'rev-parse merge^-^' '\n> +\ttest_must_fail git rev-parse merge^-^\n> +'\n> +\n> +test_expect_success 'rev-list merge^-0' '\n> +\ttest_must_fail git rev-list merge^-0\n> +'\n> +\n> +test_expect_success 'rev-list merge^-3' '\n> +\ttest_must_fail git rev-list merge^-3\n> +'\n> +\n> +test_expect_success 'rev-list merge^-^' '\n> +\ttest_must_fail git rev-list merge^-^\n> +'\n\nThis seems to be testing failure cases fairly thoroughly, which is a\ngood sign.  One thing not tested is \"merge^-1x\" to ensure that no\nchange mistakenly will break the strtoul() check you have to parse\nthe parent number in the future, but other than that (and possibly\nreusing the set-up of an already existing test), I am fairly happy\nwith the tests in this patch.\n\nThanks.\n\n\n"},{"id":"302681","messageId":"20160927061038.pgb4uyx4sillmq5r@sigill.intra.peff.net","threadId":"44163","inReplyTo":"xmqqh992pbq6.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-27T06:10:38Z","receivedAt":"2016-09-27T06:10:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 26, 2016 at 02:55:45PM -0700, Junio C Hamano wrote:\n\n> Taking these two together, perhaps squashing this in may be\n> sufficient.\n> [...]\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index 2c3da19..9474c37 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -333,8 +333,22 @@ static int try_parent_shorthands(const char *arg)\n>  \tif (include_rev)\n>  \t\tshow_rev(NORMAL, sha1, arg);\n>  \tcommit = lookup_commit_reference(sha1);\n> +\n> +\tif (exclude_parent) {\n> +\t\t/* do we have enough parents? */\n> +\t\tfor (parent_number = 0, parents = commit->parents;\n> +\t\t     parents;\n> +\t\t     parents = parents->next)\n> +\t\t\tparent_number++;\n> +\t\tif (parent_number < exclude_parent) {\n> +\t\t\t*dotdot = '^';\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t}\n\nI think you can use commit_list_count() to make this a bit shorter,\nlike:\n\n  if (exclude_parent &&\n      commit_list_count(commit->parents) < exclude_parent) {\n          *dotdot = '^';\n\t  return 0;\n  }\n\nTechnically you can drop the first half of the &&, but it is probably a\ngood idea to avoid the traversal when exclude_parent is not in use.\n\nAlso technically, you can stop counting when you hit exclude_parent\n(which is only possible with a custom traversal), but it is unlikely\nenough that it is probably not worth caring about.\n\n-Peff\n"}]}