{"thread":{"id":"33789","subject":"[RFC/PATCH 0/2] merge-base: add --merge-child option","startedAt":"2013-05-11T12:23:42Z","lastAt":"2013-05-20T06:43:43Z","messageCount":22,"participants":["John Keeping","Junio C Hamano","Kevin Bracey","Michael J Gruber","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"216991","messageId":"cover.1368274689.git.john@keeping.me.uk","threadId":"33789","inReplyTo":null,"subject":"[RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-11T12:23:42Z","receivedAt":"2013-05-11T12:23:42Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This is helpful when examining branches with disjoint roots, for example\nbecause one is periodically merged into a subtree of the other.\n\nWith the --merge-child option, \"git merge-base\" will print a\nfirst-parent ancestor of the first revision given, where the commit\nprinted is either a merge-base of the supplied revisions or a merge for\nwhich one of its parents (not the first) is a merge-base.\n\nFor example, given the history:\n\n        A---C---G\n             \\\n        B-----D---F\n         \\\n          E\n\nwe have:\n\n        $ git merge-base F E\n        B\n\n        $ git merge-base --merge-child F E\n        D\n\n\t$ git merge-base F G\n\tC\n\n\t$ git merge-base --merge-child F G\n\tC\n\n        $ git log --left-right F...E\n        < F\n        < D\n        < C\n        < A\n        > E\n\n        $ git log --left-right F...E --not $(git merge-base --merge-child F E)\n        < F\n        > E\n\nThe git-log case is useful because it allows us to limit the range of\ncommits that we are examining for patch-identical changes when using\n--cherry.  For example with git-gui in git.git I know that anything\nbefore the last merge of git-gui is not interesting:\n\n        $ time git log --cherry master...git-gui/master >/dev/null\n        real    0m32.731s\n        user    0m31.956s\n        sys     0m0.664s\n\n        $ time git log --cherry master...git-gui/master --not \\\n                $(git merge-base --merge-child master git-gui/master) \\\n                >/dev/null\n        real    0m2.296s\n        user    0m2.193s\n        sys     0m0.092s\n\n\nThe first commit is a small prerequisite to extract a useful function\nfrom builtin/tag.c to commit.c.  The second is the main change (the\ncommit message is identical to the text before this paragraph).\n\nI'm not convinced that '--merge-child' is the right name for this but I\nthink the functionality itself is useful.\n\nJohn Keeping (2):\n  commit: add commit_list_contains function\n  merge-base: add --merge-child option\n\n Documentation/git-merge-base.txt |  6 ++++\n builtin/merge-base.c             | 61 ++++++++++++++++++++++++++++++++++++++--\n builtin/tag.c                    | 10 +------\n commit.c                         |  8 ++++++\n commit.h                         |  1 +\n t/t6010-merge-base.sh            | 25 ++++++++++++++--\n 6 files changed, 98 insertions(+), 13 deletions(-)\n\n-- \n1.8.3.rc1.289.gcb3647f\n"},{"id":"216992","messageId":"f3e086bee6b59fead4f9866b3f616a916b932706.1368274689.git.john@keeping.me.uk","threadId":"33789","inReplyTo":"cover.1368274689.git.john@keeping.me.uk","subject":"[RFC/PATCH 1/2] commit: add commit_list_contains function","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-11T12:23:43Z","receivedAt":"2013-05-11T12:23:43Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This is the same as the in_commit_list function already in builtin/tag.c\nso we also replace that by the new commit_list_contains function.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n builtin/tag.c | 10 +---------\n commit.c      |  8 ++++++++\n commit.h      |  1 +\n 3 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex af3af3f..1c24772 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -65,14 +65,6 @@ static const unsigned char *match_points_at(const char *refname,\n \treturn NULL;\n }\n \n-static int in_commit_list(const struct commit_list *want, struct commit *c)\n-{\n-\tfor (; want; want = want->next)\n-\t\tif (!hashcmp(want->item->object.sha1, c->object.sha1))\n-\t\t\treturn 1;\n-\treturn 0;\n-}\n-\n static int contains_recurse(struct commit *candidate,\n \t\t\t    const struct commit_list *want)\n {\n@@ -85,7 +77,7 @@ static int contains_recurse(struct commit *candidate,\n \tif (candidate->object.flags & UNINTERESTING)\n \t\treturn 0;\n \t/* or are we it? */\n-\tif (in_commit_list(want, candidate))\n+\tif (commit_list_contains(want, candidate))\n \t\treturn 1;\n \n \tif (parse_commit(candidate) < 0)\ndiff --git a/commit.c b/commit.c\nindex 888e02a..a8263c3 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -420,6 +420,14 @@ void commit_list_sort_by_date(struct commit_list **list)\n \t\t\t\tcommit_list_compare_by_date);\n }\n \n+int commit_list_contains(const struct commit_list *list, const struct commit *item)\n+{\n+\tfor (; list; list = list->next)\n+\t\tif (!hashcmp(list->item->object.sha1, item->object.sha1))\n+\t\t\treturn 1;\n+\treturn 0;\n+}\n+\n struct commit *pop_most_recent_commit(struct commit_list **list,\n \t\t\t\t      unsigned int mark)\n {\ndiff --git a/commit.h b/commit.h\nindex 67bd509..d686ea8 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -60,6 +60,7 @@ unsigned commit_list_count(const struct commit_list *l);\n struct commit_list *commit_list_insert_by_date(struct commit *item,\n \t\t\t\t    struct commit_list **list);\n void commit_list_sort_by_date(struct commit_list **list);\n+int commit_list_contains(const struct commit_list *list, const struct commit *item);\n \n void free_commit_list(struct commit_list *list);\n \n-- \n1.8.3.rc1.289.gcb3647f\n"},{"id":"216993","messageId":"b0684a64dc07ef8969fdefdedbff62586e8ea967.1368274689.git.john@keeping.me.uk","threadId":"33789","inReplyTo":"cover.1368274689.git.john@keeping.me.uk","subject":"[RFC/PATCH 2/2] merge-base: add --merge-child option","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-11T12:23:44Z","receivedAt":"2013-05-11T12:23:44Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This is helpful when examining branches with disjoint roots, for example\nbecause one is periodically merged into a subtree of the other.\n\nWith the --merge-child option, \"git merge-base\" will print a\nfirst-parent ancestor of the first revision given, where the commit\nprinted is either a merge-base of the supplied revisions or a merge for\nwhich one of its parents (not the first) is a merge-base.\n\nFor example, given the history:\n\n\tA---C---G\n\t     \\\n\tB-----D---F\n\t \\\n\t  E\n\nwe have:\n\n\t$ git merge-base F E\n\tB\n\n\t$ git merge-base --merge-child F E\n\tD\n\n\t$ git merge-base F G\n\tC\n\n\t$ git merge-base --merge-child F G\n\tC\n\n\t$ git log --left-right F...E\n\t< F\n\t< D\n\t< C\n\t< A\n\t> E\n\n\t$ git log --left-right F...E --not $(git merge-base --merge-child F E)\n\t< F\n\t> E\n\nThe git-log case is useful because it allows us to limit the range of\ncommits that we are examining for patch-identical changes when using\n--cherry.  For example with git-gui in git.git I know that anything\nbefore the last merge of git-gui is not interesting:\n\n\t$ time git log --cherry master...git-gui/master >/dev/null\n\treal    0m32.731s\n\tuser    0m31.956s\n\tsys     0m0.664s\n\n\t$ time git log --cherry master...git-gui/master --not \\\n\t\t$(git merge-base --merge-child master git-gui/master) \\\n\t\t>/dev/null\n\treal    0m2.296s\n\tuser    0m2.193s\n\tsys     0m0.092s\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n Documentation/git-merge-base.txt |  6 ++++\n builtin/merge-base.c             | 61 ++++++++++++++++++++++++++++++++++++++--\n t/t6010-merge-base.sh            | 25 ++++++++++++++--\n 3 files changed, 88 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-merge-base.txt b/Documentation/git-merge-base.txt\nindex 87842e3..a1404e1 100644\n--- a/Documentation/git-merge-base.txt\n+++ b/Documentation/git-merge-base.txt\n@@ -10,6 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git merge-base' [-a|--all] <commit> <commit>...\n+'git merge-base' [-a|--all] --merge-child <commit>...\n 'git merge-base' [-a|--all] --octopus <commit>...\n 'git merge-base' --is-ancestor <commit> <commit>\n 'git merge-base' --independent <commit>...\n@@ -39,6 +40,11 @@ As a consequence, the 'merge base' is not necessarily contained in each of the\n commit arguments if more than two commits are specified. This is different\n from linkgit:git-show-branch[1] when used with the `--merge-base` option.\n \n+--merge-child::\n+\tFind the first-parent ancestor of the first commit that is either\n+\treachable from all of the supplied commits or which has a parent that\n+\tis.\n+\n --octopus::\n \tCompute the best common ancestors of all supplied commits,\n \tin preparation for an n-way merge.  This mimics the behavior\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex 1bc7991..0bf9f6d 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -1,7 +1,9 @@\n #include \"builtin.h\"\n #include \"cache.h\"\n #include \"commit.h\"\n+#include \"diff.h\"\n #include \"parse-options.h\"\n+#include \"revision.h\"\n \n static int show_merge_base(struct commit **rev, int rev_nr, int show_all)\n {\n@@ -24,12 +26,61 @@ static int show_merge_base(struct commit **rev, int rev_nr, int show_all)\n \n static const char * const merge_base_usage[] = {\n \tN_(\"git merge-base [-a|--all] <commit> <commit>...\"),\n+\tN_(\"git merge-base [-a|--all] --merge-child <commit>...\"),\n \tN_(\"git merge-base [-a|--all] --octopus <commit>...\"),\n \tN_(\"git merge-base --independent <commit>...\"),\n \tN_(\"git merge-base --is-ancestor <commit> <commit>\"),\n \tNULL\n };\n \n+static int handle_merge_child(struct commit **rev, int rev_nr, const char *prefix, int show_all)\n+{\n+\tstruct commit_list *merge_bases;\n+\tstruct rev_info revs;\n+\tstruct commit *commit;\n+\n+\tmerge_bases = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1, 0);\n+\n+\tif (!merge_bases)\n+\t\treturn 1;\n+\n+\tinit_revisions(&revs, prefix);\n+\trevs.first_parent_only = 1;\n+\n+\tclear_commit_marks(rev[0], SEEN | UNINTERESTING | SHOWN | ADDED);\n+\tadd_pending_object(&revs, &rev[0]->object, \"rev0\");\n+\tif (prepare_revision_walk(&revs))\n+\t\tdie(_(\"revision walk setup failed\"));\n+\n+\twhile ((commit = get_revision(&revs)) != NULL) {\n+\t\t/*\n+\t\t * If a merge base is a first-parent ancestor of rev[0] then\n+\t\t * we print the commit itself instead of a merge which is its\n+\t\t * child.\n+\t\t */\n+\t\tif (commit_list_contains(merge_bases, commit)) {\n+\t\t\tprintf(\"%s\\n\", sha1_to_hex(commit->object.sha1));\n+\t\t\tif (!show_all)\n+\t\t\t\treturn 0;\n+\t\t}\n+\n+\t\tstruct commit_list *parent;\n+\t\tfor (parent = commit->parents; parent; parent = parent->next) {\n+\t\t\t/* Skip the first parent */\n+\t\t\tif (parent == commit->parents)\n+\t\t\t\tcontinue;\n+\n+\t\t\tif (commit_list_contains(merge_bases, parent->item)) {\n+\t\t\t\tprintf(\"%s\\n\", sha1_to_hex(commit->object.sha1));\n+\t\t\t\tif (!show_all)\n+\t\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\treturn 0;\n+}\n+\n static struct commit *get_commit_reference(const char *arg)\n {\n \tunsigned char revkey[20];\n@@ -93,9 +144,12 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)\n \tint octopus = 0;\n \tint reduce = 0;\n \tint is_ancestor = 0;\n+\tint merge_child = 0;\n \n \tstruct option options[] = {\n \t\tOPT_BOOLEAN('a', \"all\", &show_all, N_(\"output all common ancestors\")),\n+\t\tOPT_BOOLEAN(0, \"merge-child\", &merge_child,\n+\t\t\t    N_(\"find a merge with a parent reachable from others\")),\n \t\tOPT_BOOLEAN(0, \"octopus\", &octopus, N_(\"find ancestors for a single n-way merge\")),\n \t\tOPT_BOOLEAN(0, \"independent\", &reduce, N_(\"list revs not reachable from others\")),\n \t\tOPT_BOOLEAN(0, \"is-ancestor\", &is_ancestor,\n@@ -107,11 +161,11 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, merge_base_usage, 0);\n \tif (!octopus && !reduce && argc < 2)\n \t\tusage_with_options(merge_base_usage, options);\n-\tif (is_ancestor && (show_all | octopus | reduce))\n+\tif (is_ancestor && (show_all | octopus | reduce | merge_child))\n \t\tdie(\"--is-ancestor cannot be used with other options\");\n \tif (is_ancestor)\n \t\treturn handle_is_ancestor(argc, argv);\n-\tif (reduce && (show_all || octopus))\n+\tif (reduce && (show_all || octopus || merge_child))\n \t\tdie(\"--independent cannot be used with other options\");\n \n \tif (octopus || reduce)\n@@ -120,5 +174,8 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)\n \trev = xmalloc(argc * sizeof(*rev));\n \twhile (argc-- > 0)\n \t\trev[rev_nr++] = get_commit_reference(*argv++);\n+\n+\tif (merge_child)\n+\t\treturn handle_merge_child(rev, rev_nr, prefix, show_all);\n \treturn show_merge_base(rev, rev_nr, show_all);\n }\ndiff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh\nindex f80bba8..454577e 100755\n--- a/t/t6010-merge-base.sh\n+++ b/t/t6010-merge-base.sh\n@@ -42,12 +42,16 @@ test_expect_success 'setup' '\n \tT=$(git mktree </dev/null)\n '\n \n-test_expect_success 'set up G and H' '\n+test_expect_success 'set up G, H and L' '\n \t# E---D---C---B---A\n \t# \\\"-_         \\   \\\n \t#  \\  `---------G   \\\n \t#   \\                \\\n \t#    F----------------H\n+\t#                      \\\n+\t# I---------------------J---K\n+\t#  \\\n+\t#   L\n \tE=$(doit 5 E) &&\n \tD=$(doit 4 D $E) &&\n \tF=$(doit 6 F $E) &&\n@@ -55,7 +59,11 @@ test_expect_success 'set up G and H' '\n \tB=$(doit 2 B $C) &&\n \tA=$(doit 1 A $B) &&\n \tG=$(doit 7 G $B $E) &&\n-\tH=$(doit 8 H $A $F)\n+\tH=$(doit 8 H $A $F) &&\n+\tI=$(doit 9 I) &&\n+\tJ=$(doit 10 J $H $I) &&\n+\tK=$(doit 11 K $J) &&\n+\tL=$(doit 12 L $I)\n '\n \n test_expect_success 'merge-base G H' '\n@@ -64,6 +72,9 @@ test_expect_success 'merge-base G H' '\n \tMB=$(git merge-base G H) &&\n \tgit name-rev \"$MB\" >actual.single &&\n \n+\tMB=$(git merge-base --merge-child G H) &&\n+\tgit name-rev \"$MB\" >actual.merge_child &&\n+\n \tMB=$(git merge-base --all G H) &&\n \tgit name-rev \"$MB\" >actual.all &&\n \n@@ -71,6 +82,7 @@ test_expect_success 'merge-base G H' '\n \tgit name-rev \"$MB\" >actual.sb &&\n \n \ttest_cmp expected actual.single &&\n+\ttest_cmp expected actual.merge_child &&\n \ttest_cmp expected actual.all &&\n \ttest_cmp expected actual.sb\n '\n@@ -95,6 +107,15 @@ test_expect_success 'merge-base/show-branch --independent' '\n \ttest_cmp expected2 actual2.sb\n '\n \n+test_expect_success 'merge-base --merge-child K L' '\n+\tgit name-rev \"$J\" >expected &&\n+\n+\tMB=$(git merge-base --merge-child K L) &&\n+\tgit name-rev \"$MB\" >actual &&\n+\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'unsynchronized clocks' '\n \t# This test is to demonstrate that relying on timestamps in a distributed\n \t# SCM to provide a _consistent_ partial ordering of commits leads to\n-- \n1.8.3.rc1.289.gcb3647f\n"},{"id":"217029","messageId":"7vmws1xv0b.fsf@alter.siamese.dyndns.org","threadId":"33789","inReplyTo":"cover.1368274689.git.john@keeping.me.uk","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-11T17:54:12Z","receivedAt":"2013-05-11T17:54:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> This is helpful when examining branches with disjoint roots, for example\n> because one is periodically merged into a subtree of the other.\n>\n> With the --merge-child option, \"git merge-base\" will print a\n> first-parent ancestor of the first revision given, where the commit\n> printed is either a merge-base of the supplied revisions or a merge for\n> which one of its parents (not the first) is a merge-base.\n\nThe above two doe snot connect at least to me.  The second paragraph\nseems to describe how this mysterious mode decides its output to a\nsufficient detail, but what the output _means_, and it is unclear\nhow it relates to gitk/git-gui style merges.\n\n> For example, given the history:\n>\n>         A---C---G\n>              \\\n>         B-----D---F\n>          \\\n>           E\n>\n> we have:\n> ...\n>         $ git log --left-right F...E --not $(git merge-base --merge-child F E)\n>         < F\n>         > E\n>\n> The git-log case is useful because it allows us to limit the range of\n> commits that we are examining for patch-identical changes when using\n> --cherry.\n\nHmph, is this reinventing ancestry-path in a different way?  At the\nlow level machinery, you are finding D to show only F and E, and\nyour goal seems to be to ignore the side ancestry A--C--G, but it is\nnot clear if you prefer \"E D F\"(which would be what F...E would give\nin a history limited to ancestry-path, ignoring C) over \"E F\".\n\n> For example with git-gui in git.git I know that anything\n> before the last merge of git-gui is not interesting:\n\nCan this be extended to find the second last such merge?  Or is the\nlast one always special?\n\nStill skeptical, but I'll let people discuss it during the feature\nfreeze ;-).\n"},{"id":"217032","messageId":"20130511184855.GE2299@serenity.lan","threadId":"33789","inReplyTo":"7vmws1xv0b.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-11T18:48:55Z","receivedAt":"2013-05-11T18:48:55Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sat, May 11, 2013 at 10:54:12AM -0700, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > This is helpful when examining branches with disjoint roots, for example\n> > because one is periodically merged into a subtree of the other.\n> >\n> > With the --merge-child option, \"git merge-base\" will print a\n> > first-parent ancestor of the first revision given, where the commit\n> > printed is either a merge-base of the supplied revisions or a merge for\n> > which one of its parents (not the first) is a merge-base.\n> \n> The above two doe snot connect at least to me.  The second paragraph\n> seems to describe how this mysterious mode decides its output to a\n> sufficient detail, but what the output _means_, and it is unclear\n> how it relates to gitk/git-gui style merges.\n> \n> > For example, given the history:\n> >\n> >         A---C---G\n> >              \\\n> >         B-----D---F\n> >          \\\n> >           E\n> >\n> > we have:\n> > ...\n> >         $ git log --left-right F...E --not $(git merge-base --merge-child F E)\n> >         < F\n> >         > E\n> >\n> > The git-log case is useful because it allows us to limit the range of\n> > commits that we are examining for patch-identical changes when using\n> > --cherry.\n> \n> Hmph, is this reinventing ancestry-path in a different way?  At the\n> low level machinery, you are finding D to show only F and E, and\n> your goal seems to be to ignore the side ancestry A--C--G, but it is\n> not clear if you prefer \"E D F\"(which would be what F...E would give\n> in a history limited to ancestry-path, ignoring C) over \"E F\".\n\nI hadn't considered ancestry-path, but I don't think it does what I\nwant.  What I want if for LEFT to be B--D--F and RIGHT to be B--E,\nignoring A--C--G because I know that none of those are patch identical\nto anything in B--E.\n\nSo what I want is more descendant-path than ancestry path in that I\ndon't want anything that isn't a descendant of the merge base of the\nsupplied arguments.\n\n> > For example with git-gui in git.git I know that anything\n> > before the last merge of git-gui is not interesting:\n> \n> Can this be extended to find the second last such merge?  Or is the\n> last one always special?\n\nIn this implementation it only finds the last one because that's where\nthe merge base is.\n\n> Still skeptical, but I'll let people discuss it during the feature\n> freeze ;-).\n\nI'm not convinced this is easy to explain myself, which may make it a\nbad idea.  Perhaps a --descendant-path argument to git-log is a better\nway to help with this case.\n"},{"id":"217080","messageId":"518FB8DE.7070004@bracey.fi","threadId":"33789","inReplyTo":"cover.1368274689.git.john@keeping.me.uk","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-05-12T15:44:30Z","receivedAt":"2013-05-12T15:44:30Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 11/05/2013 15:23, John Keeping wrote:\n> This is helpful when examining branches with disjoint roots, for example\n> because one is periodically merged into a subtree of the other.\n>\n>\n>\n>          $ git log --left-right F...E --not $(git merge-base --merge-child F E)\n>          < F\n>          > E\n>\n\nWouldn't \"--left-right --ancestry-path F...E\" do the job? I can't \nimmediately see how that differs.\n\nUnfortunately, that syntax doesn't currently work - I just stumbled \nacross this problem, and my \"history traversal refinements\" series in pu \nfixes it, somewhat incidentally to the main subject in there.\n\nHowever, without that patch, the alternative way of expressing it:\n\n\"--left-right --ancestry path F E --not $(git merge-base --all F E)\"\n\nshould still work. Unless --left-right is magic for \"...\"? If it is, \nthen my patch is more significant than I thought.\n\nMy series will also be better at optimising away D too, through a \ncombination of my and Junio's efforts. Try it out.\n\nOn this subject, is there any way to exclude a path from a log query? Is \nthere a \"not\" operator for paths? Might be another way of doing this - \ndisjoint histories probably have disjoint paths...\n\nKevin\n"},{"id":"217081","messageId":"20130512162823.GK2299@serenity.lan","threadId":"33789","inReplyTo":"518FB8DE.7070004@bracey.fi","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-12T16:28:24Z","receivedAt":"2013-05-12T16:28:24Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, May 12, 2013 at 06:44:30PM +0300, Kevin Bracey wrote:\n> On 11/05/2013 15:23, John Keeping wrote:\n> > This is helpful when examining branches with disjoint roots, for example\n> > because one is periodically merged into a subtree of the other.\n> >\n> >\n> >\n> >          $ git log --left-right F...E --not $(git merge-base --merge-child F E)\n> >          < F\n> >          > E\n> >\n> \n> Wouldn't \"--left-right --ancestry-path F...E\" do the job? I can't \n> immediately see how that differs.\n> \n> Unfortunately, that syntax doesn't currently work - I just stumbled \n> across this problem, and my \"history traversal refinements\" series in pu \n> fixes it, somewhat incidentally to the main subject in there.\n\nYou're right (and I was wrong in my reply to Junio's parallel message)\nancestry path does seem to be what I want:\n\n    $ git rev-list --left-right --count master...git-gui/master\n    31959   5\n\n    $ git rev-list --ancestry-path --left-right --count \\\n            master...git-gui/master\n    2056    5\n\n    $ git rev-list --ancestry-path --left-right --count \\\n            master...git-gui/master \\\n            --not $(git merge-base --all master git-gui/master)\n    2056    5\n\nHowever, this doesn't seem to make a difference to the time taken when I\nadd in --cherry-mark (which is why I was partially correct in the\nparallel thread - it doesn't have the effect on cherry-mark that I want\nit to):\n\n    $ time git rev-list --ancestry-path --left-right --count --cherry-mark \\\n            origin/master...git-gui/master \n    2056    5       0\n\n    real    0m32.266s\n    user    0m31.522s\n    sys     0m0.749s\n\n    $ time git rev-list  --left-right --count --cherry-mark \\\n            origin/master...git-gui/master\n    31959   5       0\n\n    real    0m32.140s\n    user    0m31.337s\n    sys     0m0.807s\n\nThis seems to be caused by the code in revision.c::limit_list() which\ndoes the cherry detection then limits to left/right and only then\napplies the ancestry path.  I haven't looked further than that, but is\nthere any reason not to apply the ancestry path restriction before\nlooking for patch-identical commits?\n\n> However, without that patch, the alternative way of expressing it:\n> \n> \"--left-right --ancestry path F E --not $(git merge-base --all F E)\"\n> \n> should still work. Unless --left-right is magic for \"...\"? If it is, \n> then my patch is more significant than I thought.\n\nYes, --left-right only applies to symmetric differences (\"...\").  But I\nget the results above both on master and on pu.\n\n> My series will also be better at optimising away D too, through a \n> combination of my and Junio's efforts. Try it out.\n> \n> On this subject, is there any way to exclude a path from a log query? Is \n> there a \"not\" operator for paths? Might be another way of doing this - \n> disjoint histories probably have disjoint paths...\n\nThat relates to another idea I had about optimizing the detection of\npatch-identical commits.  If the smaller side of a symmetric difference\nis quite small (as it is likely to be if it's a topic branch), would it\nbe a good idea to calculate the set of paths touched by commits on that\nside and then skip calculating the patch ID for any commits that touch\npaths outside that set.  The tree comparison is a lot cheaper than doing\na diff on all of the files.\n"},{"id":"217082","messageId":"20130512163317.GL2299@serenity.lan","threadId":"33789","inReplyTo":"20130512162823.GK2299@serenity.lan","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-12T16:33:18Z","receivedAt":"2013-05-12T16:33:18Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, May 12, 2013 at 05:28:24PM +0100, John Keeping wrote:\n> On Sun, May 12, 2013 at 06:44:30PM +0300, Kevin Bracey wrote:\n> > On 11/05/2013 15:23, John Keeping wrote:\n> > > This is helpful when examining branches with disjoint roots, for example\n> > > because one is periodically merged into a subtree of the other.\n> > >\n> > >\n> > >\n> > >          $ git log --left-right F...E --not $(git merge-base --merge-child F E)\n> > >          < F\n> > >          > E\n> > >\n> > \n> > Wouldn't \"--left-right --ancestry-path F...E\" do the job? I can't \n> > immediately see how that differs.\n> > \n> > Unfortunately, that syntax doesn't currently work - I just stumbled \n> > across this problem, and my \"history traversal refinements\" series in pu \n> > fixes it, somewhat incidentally to the main subject in there.\n> \n> You're right (and I was wrong in my reply to Junio's parallel message)\n> ancestry path does seem to be what I want:\n> \n>     $ git rev-list --left-right --count master...git-gui/master\n>     31959   5\n> \n>     $ git rev-list --ancestry-path --left-right --count \\\n>             master...git-gui/master\n>     2056    5\n> \n>     $ git rev-list --ancestry-path --left-right --count \\\n>             master...git-gui/master \\\n>             --not $(git merge-base --all master git-gui/master)\n>     2056    5\n> \n> However, this doesn't seem to make a difference to the time taken when I\n> add in --cherry-mark (which is why I was partially correct in the\n> parallel thread - it doesn't have the effect on cherry-mark that I want\n> it to):\n> \n>     $ time git rev-list --ancestry-path --left-right --count --cherry-mark \\\n>             origin/master...git-gui/master \n>     2056    5       0\n> \n>     real    0m32.266s\n>     user    0m31.522s\n>     sys     0m0.749s\n> \n>     $ time git rev-list  --left-right --count --cherry-mark \\\n>             origin/master...git-gui/master\n>     31959   5       0\n> \n>     real    0m32.140s\n>     user    0m31.337s\n>     sys     0m0.807s\n> \n> This seems to be caused by the code in revision.c::limit_list() which\n> does the cherry detection then limits to left/right and only then\n> applies the ancestry path.  I haven't looked further than that, but is\n> there any reason not to apply the ancestry path restriction before\n> looking for patch-identical commits?\n> \n> > However, without that patch, the alternative way of expressing it:\n> > \n> > \"--left-right --ancestry path F E --not $(git merge-base --all F E)\"\n> > \n> > should still work. Unless --left-right is magic for \"...\"? If it is, \n> > then my patch is more significant than I thought.\n> \n> Yes, --left-right only applies to symmetric differences (\"...\").  But I\n> get the results above both on master and on pu.\n\nNo I didn't.  I forgot to update my $PATH when I built on master - those\nresults are from pu.  master says:\n\n    fatal: --ancestry-path given but there are no bottom commits\n\n> > My series will also be better at optimising away D too, through a \n> > combination of my and Junio's efforts. Try it out.\n> > \n> > On this subject, is there any way to exclude a path from a log query? Is \n> > there a \"not\" operator for paths? Might be another way of doing this - \n> > disjoint histories probably have disjoint paths...\n> \n> That relates to another idea I had about optimizing the detection of\n> patch-identical commits.  If the smaller side of a symmetric difference\n> is quite small (as it is likely to be if it's a topic branch), would it\n> be a good idea to calculate the set of paths touched by commits on that\n> side and then skip calculating the patch ID for any commits that touch\n> paths outside that set.  The tree comparison is a lot cheaper than doing\n> a diff on all of the files.\n"},{"id":"217084","messageId":"20130512165829.GM2299@serenity.lan","threadId":"33789","inReplyTo":"20130512162823.GK2299@serenity.lan","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-12T16:58:29Z","receivedAt":"2013-05-12T16:58:29Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, May 12, 2013 at 05:28:24PM +0100, John Keeping wrote:\n> However, this doesn't seem to make a difference to the time taken when I\n> add in --cherry-mark (which is why I was partially correct in the\n> parallel thread - it doesn't have the effect on cherry-mark that I want\n> it to):\n> \n>     $ time git rev-list --ancestry-path --left-right --count --cherry-mark \\\n>             origin/master...git-gui/master \n>     2056    5       0\n> \n>     real    0m32.266s\n>     user    0m31.522s\n>     sys     0m0.749s\n> \n>     $ time git rev-list  --left-right --count --cherry-mark \\\n>             origin/master...git-gui/master\n>     31959   5       0\n> \n>     real    0m32.140s\n>     user    0m31.337s\n>     sys     0m0.807s\n> \n> This seems to be caused by the code in revision.c::limit_list() which\n> does the cherry detection then limits to left/right and only then\n> applies the ancestry path.  I haven't looked further than that, but is\n> there any reason not to apply the ancestry path restriction before\n> looking for patch-identical commits?\n\nWith the patch below, the --ancestry-path version drops to under 2\nseconds.\n\nI'm not sure if this is a good idea though.  It helps me say \"I know\nnothing that isn't on the ancestry path can be patch-identical, so don't\nbother checking if it is\" but it regresses users who want the full\ncherry-pick check while only limiting the output.\n\nPerhaps we need --cherry-no-uninteresting to apply the first 3 hunks of\nthe patch at runtime :-S\n\n-- >8 --\ndiff --git a/revision.c b/revision.c\nindex de3b058..d721d83 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -837,7 +837,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)\n \tfor (p = list; p; p = p->next) {\n \t\tstruct commit *commit = p->item;\n \t\tunsigned flags = commit->object.flags;\n-\t\tif (flags & BOUNDARY)\n+\t\tif (flags & (BOUNDARY | UNINTERESTING))\n \t\t\t;\n \t\telse if (flags & SYMMETRIC_LEFT)\n \t\t\tleft_count++;\n@@ -858,7 +858,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)\n \t\tstruct commit *commit = p->item;\n \t\tunsigned flags = commit->object.flags;\n \n-\t\tif (flags & BOUNDARY)\n+\t\tif (flags & (BOUNDARY | UNINTERESTING))\n \t\t\tcontinue;\n \t\t/*\n \t\t * If we have fewer left, left_first is set and we omit\n@@ -879,7 +879,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)\n \t\tstruct patch_id *id;\n \t\tunsigned flags = commit->object.flags;\n \n-\t\tif (flags & BOUNDARY)\n+\t\tif (flags & (BOUNDARY | UNINTERESTING))\n \t\t\tcontinue;\n \t\t/*\n \t\t * If we have fewer left, left_first is set and we omit\n@@ -1103,17 +1103,18 @@ static int limit_list(struct rev_info *revs)\n \t\tshow(revs, newlist);\n \t\tshow_early_output = NULL;\n \t}\n-\tif (revs->cherry_pick || revs->cherry_mark)\n-\t\tcherry_pick_list(newlist, revs);\n-\n-\tif (revs->left_only || revs->right_only)\n-\t\tlimit_left_right(newlist, revs);\n \n \tif (bottom) {\n \t\tlimit_to_ancestry(bottom, newlist);\n \t\tfree_commit_list(bottom);\n \t}\n \n+\tif (revs->cherry_pick || revs->cherry_mark)\n+\t\tcherry_pick_list(newlist, revs);\n+\n+\tif (revs->left_only || revs->right_only)\n+\t\tlimit_left_right(newlist, revs);\n+\n \t/*\n \t * Check if any commits have become TREESAME by some of their parents\n \t * becoming UNINTERESTING.\n"},{"id":"217086","messageId":"518FCDDE.9040707@bracey.fi","threadId":"33789","inReplyTo":"20130512163317.GL2299@serenity.lan","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-05-12T17:14:06Z","receivedAt":"2013-05-12T17:14:06Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 12/05/2013 19:33, John Keeping wrote:\n> On Sun, May 12, 2013 at 05:28:24PM +0100, John Keeping wrote:\n>> You're right (and I was wrong in my reply to Junio's parallel message)\n>> ancestry path does seem to be what I want:\n>>\n>>      $ git rev-list --ancestry-path --left-right --count \\\n>>              master...git-gui/master\n>>      2056    5\n>>\n>> However, this doesn't seem to make a difference to the time taken when I\n>> add in --cherry-mark (which is why I was partially correct in the\n>> parallel thread - it doesn't have the effect on cherry-mark that I want\n>> it to):\n>>\n>> This seems to be caused by the code in revision.c::limit_list() which\n>> does the cherry detection then limits to left/right and only then\n>> applies the ancestry path.  I haven't looked further than that, but is\n>> there any reason not to apply the ancestry path restriction before\n>> looking for patch-identical commits?\n\nThat certainly sounds like it's doing it the wrong way round. At first \nsight, it seems obviously suboptimal.\n\n> No I didn't.  I forgot to update my $PATH when I built on master - those\n> results are from pu.  master says:\n>\n>      fatal: --ancestry-path given but there are no bottom commits\n\nWell, then it looks like we have a user for that particular syntax. \nSeemed a bit esoteric to me :)  Although I realised after sending my \nmail you could also use\n\n    git log --ancestry-path --left-right E...F --not $(git merge-base \n--all E F)\n\nwhich looks like we're having to repeat ourselves because it's not \npaying attention...\n\nI hit it because one of my optimisations relies on knowing the bottom \ncommits, and I made absolutely sure I was using the exactly same \ndefinition of \"bottom\" as --ancestry-path. And then I found that my \noptimisations didn't work properly with \"...\"\n\nI suggest we pull my patch out from the more complex optimisation series \nso it can proceed to next faster. It shouldn't have to wait for all my \nnew fancy stuff - it's fixing something that appears to be a clear bug.\n\nAlthough Junio did have a comment about the implementation - I'll \nrevisit it tomorrow and send a revised version separately, if everyone \nthinks that's sensible.\n\n>\n>>> On this subject, is there any way to exclude a path from a log query? Is\n>>> there a \"not\" operator for paths? Might be another way of doing this -\n>>> disjoint histories probably have disjoint paths...\n>> That relates to another idea I had about optimizing the detection of\n>> patch-identical commits.  If the smaller side of a symmetric difference\n>> is quite small (as it is likely to be if it's a topic branch), would it\n>> be a good idea to calculate the set of paths touched by commits on that\n>> side and then skip calculating the patch ID for any commits that touch\n>> paths outside that set.  The tree comparison is a lot cheaper than doing\n>> a diff on all of the files.\n>>\nSounds cute. Go for it :)\n\nKevin\n"},{"id":"217090","messageId":"518FD182.5020606@bracey.fi","threadId":"33789","inReplyTo":"20130512165829.GM2299@serenity.lan","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-05-12T17:29:38Z","receivedAt":"2013-05-12T17:29:38Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 12/05/2013 19:58, John Keeping wrote:\n>\n> With the patch below, the --ancestry-path version drops to under 2\n> seconds.\n>\n> I'm not sure if this is a good idea though.  It helps me say \"I know\n> nothing that isn't on the ancestry path can be patch-identical, so don't\n> bother checking if it is\" but it regresses users who want the full\n> cherry-pick check while only limiting the output.\n\nHmm. Should an excluded commit be a valid comparator? Is it \nsensible/correct to show a left commit as \"=\" to a right commit that has \nbeen excluded by the revision specifiers? Doesn't sound right to me.\n\nI'm not convinced that there's a valid use-case that you're regressing \nhere. If --ancestry-path is being misused (the user's assertion that \nnon-ancestry doesn't matter is wrong) the \"error\" of not noting culled \npatch-identical commits is nothing compared to the fact we're already \ntotally omitting the non-identical ones.\n\nKevin\n"},{"id":"217097","messageId":"7vwqr3u9c5.fsf@alter.siamese.dyndns.org","threadId":"33789","inReplyTo":"518FCDDE.9040707@bracey.fi","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-12T22:22:50Z","receivedAt":"2013-05-12T22:22:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Bracey <kevin@bracey.fi> writes:\n\n> Although I realised after\n> sending my mail you could also use\n>\n>    git log --ancestry-path --left-right E...F --not $(git merge-base\n> --all E F)\n>\n> which looks like we're having to repeat ourselves because it's not\n> paying attention...\n\nYou are half wrong; \"--left-right\" is about \"do we show the </>/=\nmarker in the output?\", so it is true that it does not make sense\nwithout \"...\", but the reverse is not true: A...B does not and\nshould not imply --left-right.\n"},{"id":"217118","messageId":"7vppwvqxow.fsf@alter.siamese.dyndns.org","threadId":"33789","inReplyTo":"518FD182.5020606@bracey.fi","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-13T05:02:39Z","receivedAt":"2013-05-13T05:02:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Bracey <kevin@bracey.fi> writes:\n\n> On 12/05/2013 19:58, John Keeping wrote:\n>>\n>> With the patch below, the --ancestry-path version drops to under 2\n>> seconds.\n>>\n>> I'm not sure if this is a good idea though.  It helps me say \"I know\n>> nothing that isn't on the ancestry path can be patch-identical, so don't\n>> bother checking if it is\" but it regresses users who want the full\n>> cherry-pick check while only limiting the output.\n>\n> Hmm. Should an excluded commit be a valid comparator? Is it\n> sensible/correct to show a left commit as \"=\" to a right commit that\n> has been excluded by the revision specifiers? Doesn't sound right to\n> me.\n\nNeither to me.\n"},{"id":"217132","messageId":"20130513075230.GO2299@serenity.lan","threadId":"33789","inReplyTo":"7vppwvqxow.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-13T07:52:31Z","receivedAt":"2013-05-13T07:52:31Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, May 12, 2013 at 10:02:39PM -0700, Junio C Hamano wrote:\n> Kevin Bracey <kevin@bracey.fi> writes:\n> \n> > On 12/05/2013 19:58, John Keeping wrote:\n> >>\n> >> With the patch below, the --ancestry-path version drops to under 2\n> >> seconds.\n> >>\n> >> I'm not sure if this is a good idea though.  It helps me say \"I know\n> >> nothing that isn't on the ancestry path can be patch-identical, so don't\n> >> bother checking if it is\" but it regresses users who want the full\n> >> cherry-pick check while only limiting the output.\n> >\n> > Hmm. Should an excluded commit be a valid comparator? Is it\n> > sensible/correct to show a left commit as \"=\" to a right commit that\n> > has been excluded by the revision specifiers? Doesn't sound right to\n> > me.\n> \n> Neither to me.\n\nOK.  I'll add some tests and send a proper patch once 1.8.3 is out of\nthe way.\n"},{"id":"217181","messageId":"5190F806.6040207@bracey.fi","threadId":"33789","inReplyTo":"7vwqr3u9c5.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-05-13T14:26:14Z","receivedAt":"2013-05-13T14:26:14Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 13/05/2013 01:22, Junio C Hamano wrote:\n> Kevin Bracey <kevin@bracey.fi> writes:\n>\n>>     git log --ancestry-path --left-right E...F --not $(git merge-base\n>> --all E F)\n>>\n>> which looks like we're having to repeat ourselves because it's not\n>> paying attention...\n> You are half wrong; \"--left-right\" is about \"do we show the </>/=\n> marker in the output?\", so it is true that it does not make sense\n> without \"...\", but the reverse is not true: A...B does not and\n> should not imply --left-right.\n>\nThe repetition I meant is that by the definition of ancestry-path, the \nabove would seem to be equivalent to\n\n   git log --ancestry-path --left-right E F --not $(git merge-base --all E F) $(git merge-base --all E F)\n\nAnyway, revised separated-out version of the patch follows.\n\nKevin\n"},{"id":"217185","messageId":"5190FC97.6020800@drmicha.warpmail.net","threadId":"33789","inReplyTo":"5190F806.6040207@bracey.fi","subject":"Re: [RFC/PATCH 0/2] merge-base: add --merge-child option","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-05-13T14:45:43Z","receivedAt":"2013-05-13T14:45:43Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Kevin Bracey venit, vidit, dixit 13.05.2013 16:26:\n> On 13/05/2013 01:22, Junio C Hamano wrote:\n>> Kevin Bracey <kevin@bracey.fi> writes:\n>>\n>>>     git log --ancestry-path --left-right E...F --not $(git merge-base\n>>> --all E F)\n>>>\n>>> which looks like we're having to repeat ourselves because it's not\n>>> paying attention...\n>> You are half wrong; \"--left-right\" is about \"do we show the </>/=\n>> marker in the output?\", so it is true that it does not make sense\n>> without \"...\", but the reverse is not true: A...B does not and\n>> should not imply --left-right.\n>>\n> The repetition I meant is that by the definition of ancestry-path, the \n> above would seem to be equivalent to\n> \n>    git log --ancestry-path --left-right E F --not $(git merge-base --all E F) $(git merge-base --all E F)\n> \n> Anyway, revised separated-out version of the patch follows.\n> \n> Kevin\n\nIt is certainly true that \"git log --cherry\" needs much less information\nthan what the merge base machinery provides. I've been experimenting\nwith that in order to get the speedup which is necessary for replacing\nthe \"git cherry\" code with calls into the revision walker using \"--cherry\".\n\nBut I can't wrap my head around the feature proposed here, sorry.\n\nMichael\n"},{"id":"217194","messageId":"1368457247-19822-1-git-send-email-kevin@bracey.fi","threadId":"33789","inReplyTo":"5190F806.6040207@bracey.fi","subject":"[PATCH 0/2] Make --ancestry-path A...B work","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-05-13T15:00:45Z","receivedAt":"2013-05-13T15:00:45Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"This patch is a revised form of the one in my \"history traversal refinements\"\nseries. Pulled out to allow it to proceed faster, given that John Keeping has\nfound a use for the command.\n\nI suggest this is placed onto pu ahead of the existing series, dropping the\nequivalent final commit there. And then hopefully this can proceed to next\nfaster.\n\n(Dropping that commit will drop the only --ancestry-path A...B test in t6111,\nmeaning no immediate dependencies. But the next version of that series will be\nsent with t6111 testing and expecting a pass due to this fix being in.)\n\nKevin Bracey (2):\n  t6019: demonstrate --ancestry-path A...B breakage\n  revision.c: treat A...B merge bases as if manually specified\n\n revision.c                        | 17 +++++++++++++++++\n revision.h                        |  1 +\n t/t6019-rev-list-ancestry-path.sh | 21 ++++++++++++++++++++-\n 3 files changed, 38 insertions(+), 1 deletion(-)\n\n-- \n1.8.3.rc0.28.g4b02ef5\n"},{"id":"217198","messageId":"1368457247-19822-2-git-send-email-kevin@bracey.fi","threadId":"33789","inReplyTo":"1368457247-19822-1-git-send-email-kevin@bracey.fi","subject":"[PATCH 1/2] t6019: demonstrate --ancestry-path A...B breakage","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-05-13T15:00:46Z","receivedAt":"2013-05-13T15:00:46Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Signed-off-by: Kevin Bracey <kevin@bracey.fi>\n---\n t/t6019-rev-list-ancestry-path.sh | 21 ++++++++++++++++++++-\n 1 file changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\nindex 39b4cb0..5287f6a 100755\n--- a/t/t6019-rev-list-ancestry-path.sh\n+++ b/t/t6019-rev-list-ancestry-path.sh\n@@ -13,6 +13,9 @@ test_description='--ancestry-path'\n #\n #  D..M -- M.t                 == M\n #  --ancestry-path D..M -- M.t == M\n+#\n+#  F...I                 == F G H I\n+#  --ancestry-path F...I == F H I\n \n . ./test-lib.sh\n \n@@ -63,13 +66,29 @@ test_expect_success 'rev-list D..M -- M.t' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'rev-list --ancestry-patch D..M -- M.t' '\n+test_expect_success 'rev-list --ancestry-path D..M -- M.t' '\n \techo M >expect &&\n \tgit rev-list --ancestry-path --format=%s D..M -- M.t |\n \tsed -e \"/^commit /d\" >actual &&\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rev-list F...I' '\n+\tfor c in F G H I; do echo $c; done >expect &&\n+\tgit rev-list --format=%s F...I |\n+\tsed -e \"/^commit /d\" |\n+\tsort >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'rev-list --ancestry-path F...I' '\n+\tfor c in F H I; do echo $c; done >expect &&\n+\tgit rev-list --ancestry-path --format=%s F...I |\n+\tsed -e \"/^commit /d\" |\n+\tsort >actual &&\n+\ttest_cmp expect actual\n+'\n+\n #   b---bc\n #  / \\ /\n # a   X\n-- \n1.8.3.rc0.28.g4b02ef5\n"},{"id":"217195","messageId":"1368457247-19822-3-git-send-email-kevin@bracey.fi","threadId":"33789","inReplyTo":"1368457247-19822-1-git-send-email-kevin@bracey.fi","subject":"[PATCH 2/2] revision.c: treat A...B merge bases as if manually specified","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-05-13T15:00:47Z","receivedAt":"2013-05-13T15:00:47Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"The documentation assures users that \"A...B\" is defined as \"A B --not\n$(git merge-base --all A B)\". This wasn't in fact quite true, because\nthe calculated merge bases were not sent to add_rev_cmdline().\n\nThe main effect of this was that although\n\n  git rev-list --ancestry-path A B --not $(git merge-base --all A B)\n\nworked, the simpler form\n\n  git rev-list --ancestry-path A...B\n\nfailed with a \"no bottom commits\" error.\n\nOther potential users of bottom commits could also be affected by this\nproblem, if they examine revs->cmdline_info; I came across the issue in\nmy proposed history traversal refinements series.\n\nSo ensure that the calculated merge bases are sent to add_rev_cmdline(),\nflagged with new 'whence' enum value REV_CMD_MERGE_BASE.\n\nSigned-off-by: Kevin Bracey <kevin@bracey.fi>\n---\n revision.c                        | 17 +++++++++++++++++\n revision.h                        |  1 +\n t/t6019-rev-list-ancestry-path.sh |  2 +-\n 3 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex a67b615..7f7a8ab 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -915,6 +915,19 @@ static void add_rev_cmdline(struct rev_info *revs,\n \tinfo->nr++;\n }\n \n+static void add_rev_cmdline_list(struct rev_info *revs,\n+\t\t\t\t struct commit_list *commit_list,\n+\t\t\t\t int whence,\n+\t\t\t\t unsigned flags)\n+{\n+\twhile (commit_list) {\n+\t\tstruct object *object = &commit_list->item->object;\n+\t\tadd_rev_cmdline(revs, object, sha1_to_hex(object->sha1),\n+\t\t\t\twhence, flags);\n+\t\tcommit_list = commit_list->next;\n+\t}\n+}\n+\n struct all_refs_cb {\n \tint all_flags;\n \tint warned_bad_reflog;\n@@ -1092,6 +1105,7 @@ static void prepare_show_merge(struct rev_info *revs)\n \tadd_pending_object(revs, &head->object, \"HEAD\");\n \tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n \tbases = get_merge_bases(head, other, 1);\n+\tadd_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING);\n \tadd_pending_commit_list(revs, bases, UNINTERESTING);\n \tfree_commit_list(bases);\n \thead->object.flags |= SYMMETRIC_LEFT;\n@@ -1179,6 +1193,9 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \n \t\t\tif (symmetric) {\n \t\t\t\texclude = get_merge_bases(a, b, 1);\n+\t\t\t\tadd_rev_cmdline_list(revs, exclude,\n+\t\t\t\t\t\t     REV_CMD_MERGE_BASE,\n+\t\t\t\t\t\t     flags_exclude);\n \t\t\t\tadd_pending_commit_list(revs, exclude,\n \t\t\t\t\t\t\tflags_exclude);\n \t\t\t\tfree_commit_list(exclude);\ndiff --git a/revision.h b/revision.h\nindex 01bd2b7..878a555 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -35,6 +35,7 @@ struct rev_cmdline_info {\n \t\t\tREV_CMD_PARENTS_ONLY,\n \t\t\tREV_CMD_LEFT,\n \t\t\tREV_CMD_RIGHT,\n+\t\t\tREV_CMD_MERGE_BASE,\n \t\t\tREV_CMD_REV\n \t\t} whence;\n \t\tunsigned flags;\ndiff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\nindex 5287f6a..dd5b0e5 100755\n--- a/t/t6019-rev-list-ancestry-path.sh\n+++ b/t/t6019-rev-list-ancestry-path.sh\n@@ -81,7 +81,7 @@ test_expect_success 'rev-list F...I' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'rev-list --ancestry-path F...I' '\n+test_expect_success 'rev-list --ancestry-path F...I' '\n \tfor c in F H I; do echo $c; done >expect &&\n \tgit rev-list --ancestry-path --format=%s F...I |\n \tsed -e \"/^commit /d\" |\n-- \n1.8.3.rc0.28.g4b02ef5\n"},{"id":"217204","messageId":"7v1u9aoogy.fsf@alter.siamese.dyndns.org","threadId":"33789","inReplyTo":"1368457247-19822-3-git-send-email-kevin@bracey.fi","subject":"Re: [PATCH 2/2] revision.c: treat A...B merge bases as if manually specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-13T16:04:45Z","receivedAt":"2013-05-13T16:04:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Looks good.  Thanks.\n"},{"id":"217906","messageId":"20130519124059.GJ27005@serenity.lan","threadId":"33789","inReplyTo":"5190FC97.6020800@drmicha.warpmail.net","subject":"log --cherry and merges (was [RFC/PATCH 0/2] merge-base: add --merge-child option)","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-19T12:40:59Z","receivedAt":"2013-05-19T12:40:59Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, May 13, 2013 at 04:45:43PM +0200, Michael J Gruber wrote:\n> Kevin Bracey venit, vidit, dixit 13.05.2013 16:26:\n> > On 13/05/2013 01:22, Junio C Hamano wrote:\n> >> Kevin Bracey <kevin@bracey.fi> writes:\n> >>\n> >>>     git log --ancestry-path --left-right E...F --not $(git merge-base\n> >>> --all E F)\n> >>>\n> >>> which looks like we're having to repeat ourselves because it's not\n> >>> paying attention...\n> >> You are half wrong; \"--left-right\" is about \"do we show the </>/=\n> >> marker in the output?\", so it is true that it does not make sense\n> >> without \"...\", but the reverse is not true: A...B does not and\n> >> should not imply --left-right.\n> >>\n> > The repetition I meant is that by the definition of ancestry-path, the \n> > above would seem to be equivalent to\n> > \n> >    git log --ancestry-path --left-right E F --not $(git merge-base --all E F) $(git merge-base --all E F)\n> > \n> > Anyway, revised separated-out version of the patch follows.\n> > \n> > Kevin\n> \n> It is certainly true that \"git log --cherry\" needs much less information\n> than what the merge base machinery provides. I've been experimenting\n> with that in order to get the speedup which is necessary for replacing\n> the \"git cherry\" code with calls into the revision walker using \"--cherry\".\n\nI think the revision machinery is the same speed as the \"git cherry\"\ncode, it's just that \"git cherry\" ignores merges and the cherry code in\nrevision.c doesn't.\n\nSince the patch ID of a merge is just being calculated to its first\nparent, I don't think it's meaningful to consider merges for \"log\n--cherry\" but I can't quite convince myself that there's no corner case\nwhere it is.\n\nThe following patch makes the revision cherry machinery ignore merges\nunconditionally.  With it applied, there's not noticeable difference in\nspeed between \"git cherry\" and \"git log --cherry\".\n\n-- >8 --\ndiff --git a/revision.c b/revision.c\nindex a67b615..19d0683 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -640,6 +640,11 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)\n \n \t\tif (flags & BOUNDARY)\n \t\t\tcontinue;\n+\n+\t\t/* Patch ID is meaningless for merges. */\n+\t\tif (commit->parents && commit->parents->next)\n+\t\t\tcontinue;\n+\n \t\t/*\n \t\t * If we have fewer left, left_first is set and we omit\n \t\t * commits on the right branch in this loop.  If we have\n@@ -661,6 +666,11 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)\n \n \t\tif (flags & BOUNDARY)\n \t\t\tcontinue;\n+\n+\t\t/* Patch ID is meaningless for merges. */\n+\t\tif (commit->parents && commit->parents->next)\n+\t\t\tcontinue;\n+\n \t\t/*\n \t\t * If we have fewer left, left_first is set and we omit\n \t\t * commits on the left branch in this loop.\n-- \n1.8.3.rc3.372.g721bad8\n"},{"id":"217962","messageId":"20130520064343.GC2919@elie.Belkin","threadId":"33789","inReplyTo":"20130519124059.GJ27005@serenity.lan","subject":"Re: log --cherry and merges (was [RFC/PATCH 0/2] merge-base: add --merge-child option)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-05-20T06:43:43Z","receivedAt":"2013-05-20T06:43:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"John Keeping wrote:\n\n> The following patch makes the revision cherry machinery ignore merges\n> unconditionally.  With it applied, there's not noticeable difference in\n> speed between \"git cherry\" and \"git log --cherry\".\n>\n> -- >8 --\n> diff --git a/revision.c b/revision.c\n> index a67b615..19d0683 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -640,6 +640,11 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)\n>  \n>  \t\tif (flags & BOUNDARY)\n>  \t\t\tcontinue;\n> +\n> +\t\t/* Patch ID is meaningless for merges. */\n> +\t\tif (commit->parents && commit->parents->next)\n> +\t\t\tcontinue;\n> +\n\nI guess merges should be skipped in the left-vs-right tally earlier,\ntoo?\n\n\t\tif (flags & BOUNDARY)\n\t\t\t;\n\t\telse if (commit->parents && commit->parents->next)\n\t\t\t;\n\t\telse if (flags & SYMMETRIC_LEFT)\n\t\t\tleft_count++;\n\t\telse\n\t\t\tright_count++;\n\nWith that tweak (or without it --- a sloppy count is fine), this\npatch makes sense to me.  I guess some tests would be useful to\ndemonstrate that --cherry doesn't notice duplicate first-parent\ndiffs in merges.\n\nThanks,\nJonathan\n"}]}