{"thread":{"id":"15139","subject":"rev-list --reverse --full-history --parents with path limiter disconnects history","startedAt":"2008-08-21T16:27:50Z","lastAt":"2008-08-31T05:51:10Z","messageCount":4,"participants":["Johannes Sixt","Thomas Rast","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"88003","messageId":"48AD9786.80707@viscovery.net","threadId":"15139","inReplyTo":null,"subject":"rev-list --reverse --full-history --parents with path limiter disconnects history","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-08-21T16:27:50Z","receivedAt":"2008-08-21T16:27:50Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Attached is a repository (to be imported via git fast-import) that has\nsuch a history:\n\no--o--A--B--o\n \\           \\\n  o--o--C-----M--o--X   <- master\n\nwhere the A,B,C touch path P2 such that P2 is the same after B and C.\nCommit X touches P2 as well.\n\nThis command does not give the expected result: It reports *two* initial\ncommits and one merge; in particular, commit X is reported as initial commit.\n\n$ git rev-list --parents --reverse --full-history master -- P2\n\nThis command gives the expected result, i.e. there is one initial commit,\none merge, and some more regular commits:\n\n$ git rev-list --parents --full-history master -- P2\n\nAnd this works as well, i.e there is a linear history that omits the\nbranch via C:\n\n$ git rev-list --parents --reverse master -- P2\n\nPlease help!\n\nBTW, I've observed this behavior earlier when I debugged David Tweed's\nrepository, after which we removed --full-history from git-filter-branch.\nBut I could not easily find a simple history that exposes the problem. Now\nI have such a history.\n\nBTW2, using --simplify-merges instead of --full-history has the same\nproblem (but I don't think that comes as a surprise).\n\n-- Hannes\n"},{"id":"89149","messageId":"1220037518-11219-1-git-send-email-trast@student.ethz.ch","threadId":"15139","inReplyTo":"48AD9786.80707@viscovery.net","subject":"[PATCH] rev-list: fix --reverse interaction with --parents","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2008-08-29T19:18:38Z","receivedAt":"2008-08-29T19:18:38Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"--reverse did not interact well with --parents, as the included test\ncase shows: in a history like\n\n  A--B.\n   \\   \\\n    `C--M--D\n\nthe command\n\n  git rev-list --reverse --parents --full-history HEAD\n\nerroneously lists D as having no parents at all.  (Without --reverse,\nit correctly lists M.)\n\nThis is caused by the machinery driving --reverse: it first grabs all\ncommits through the normal routines, then runs them through the same\nroutines again, effectively simplifying them twice.\n\nFix this by moving the --reverse one level up, into get_revision().\nThis way we can cleanly grab all commits via the normal calls, then\njust pop them off the list one by one without interfering with\nget_revision_internal().\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nJohannes Sixt wrote:\n> This command does not give the expected result: It reports *two* initial\n> commits and one merge; in particular, commit X is reported as initial commit.\n> \n> $ git rev-list --parents --reverse --full-history master -- P2\n\nThe good news is: the patch really does fix your issue.  It also fixes\nthe test case (which uses the history from the diagram above).  And it\npasses the test suite.\n\nThe bad news: I got as far as understanding that the repeated\nsimplification must be at fault, but gave up on trying to see exactly\n_which step_ breaks the output.  Obviously parent rewriting is the\nprime suspect.\n\nAnd while I'm quite convinced that this is the \"right\" fix in the\nsense that the new implementation captures how --reverse would be\ndescribed by someone not knowing the code, it does change behaviour in\nsome cases.  For example, with --boundary we always output boundaries\nlast, regardless of --reverse.  With this patch, --reverse is\nessentially just |tac, so they move to the top.\n\n- Thomas\n\nPS: Apologies for the subject braino earlier today.  As advised on\nIRC, I have labeled the resulting hole in my foot ***FIXME***.\n\n\n revision.c                          |   38 ++++++++++++++-----------------\n revision.h                          |    1 +\n t/t6013-rev-list-reverse-parents.sh |   42 +++++++++++++++++++++++++++++++++++\n 3 files changed, 60 insertions(+), 21 deletions(-)\n create mode 100755 t/t6013-rev-list-reverse-parents.sh\n\ndiff --git a/revision.c b/revision.c\nindex 36291b6..ae4062c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1646,26 +1646,6 @@ static struct commit *get_revision_internal(struct rev_info *revs)\n \t\treturn c;\n \t}\n \n-\tif (revs->reverse) {\n-\t\tint limit = -1;\n-\n-\t\tif (0 <= revs->max_count) {\n-\t\t\tlimit = revs->max_count;\n-\t\t\tif (0 < revs->skip_count)\n-\t\t\t\tlimit += revs->skip_count;\n-\t\t}\n-\t\tl = NULL;\n-\t\twhile ((c = get_revision_1(revs))) {\n-\t\t\tcommit_list_insert(c, &l);\n-\t\t\tif ((0 < limit) && !--limit)\n-\t\t\t\tbreak;\n-\t\t}\n-\t\trevs->commits = l;\n-\t\trevs->reverse = 0;\n-\t\trevs->max_count = -1;\n-\t\tc = NULL;\n-\t}\n-\n \t/*\n \t * Now pick up what they want to give us\n \t */\n@@ -1738,7 +1718,23 @@ static struct commit *get_revision_internal(struct rev_info *revs)\n \n struct commit *get_revision(struct rev_info *revs)\n {\n-\tstruct commit *c = get_revision_internal(revs);\n+\tstruct commit *c;\n+\tstruct commit_list *reversed;\n+\n+\tif (revs->reverse) {\n+\t\treversed = NULL;\n+\t\twhile ((c = get_revision_internal(revs))) {\n+\t\t\tcommit_list_insert(c, &reversed);\n+\t\t}\n+\t\trevs->commits = reversed;\n+\t\trevs->reverse = 0;\n+\t\trevs->reverse_output_stage = 1;\n+\t}\n+\n+\tif (revs->reverse_output_stage)\n+\t\treturn pop_commit(&revs->commits);\n+\n+\tc = get_revision_internal(revs);\n \tif (c && revs->graph)\n \t\tgraph_update(revs->graph, c);\n \treturn c;\ndiff --git a/revision.h b/revision.h\nindex 91f1944..a636247 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -53,6 +53,7 @@ struct rev_info {\n \t\t\trewrite_parents:1,\n \t\t\tprint_parents:1,\n \t\t\treverse:1,\n+\t\t\treverse_output_stage:1,\n \t\t\tcherry_pick:1,\n \t\t\tfirst_parent_only:1;\n \ndiff --git a/t/t6013-rev-list-reverse-parents.sh b/t/t6013-rev-list-reverse-parents.sh\nnew file mode 100755\nindex 0000000..d294466\n--- /dev/null\n+++ b/t/t6013-rev-list-reverse-parents.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description='--reverse combines with --parents'\n+\n+. ./test-lib.sh\n+\n+\n+commit () {\n+\ttest_tick &&\n+\techo $1 > foo &&\n+\tgit add foo &&\n+\tgit commit -m \"$1\"\n+}\n+\n+test_expect_success 'set up --reverse example' '\n+\tcommit one &&\n+\tgit tag root &&\n+\tcommit two &&\n+\tgit checkout -b side HEAD^ &&\n+\tcommit three &&\n+\tgit checkout master &&\n+\tgit merge -s ours side &&\n+\tcommit five\n+\t'\n+\n+test_expect_success '--reverse --parents --full-history combines correctly' '\n+\tgit rev-list --parents --full-history master -- foo |\n+\t\ttac > expected &&\n+\tgit rev-list --reverse --parents --full-history master -- foo \\\n+\t\t> actual &&\n+\ttest_cmp actual expected\n+\t'\n+\n+test_expect_success '--boundary does too' '\n+\tgit rev-list --boundary --parents --full-history master ^root -- foo |\n+\t\ttac > expected &&\n+\tgit rev-list --boundary --reverse --parents --full-history \\\n+\t\tmaster ^root -- foo > actual &&\n+\ttest_cmp actual expected\n+\t'\n+\n+test_done\n-- \n1.6.0.1.24.g4c7bd\n"},{"id":"89151","messageId":"200808292153.59376.johannes.sixt@telecom.at","threadId":"15139","inReplyTo":"1220037518-11219-1-git-send-email-trast@student.ethz.ch","subject":"Re: [PATCH] rev-list: fix --reverse interaction with --parents","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-08-29T19:53:59Z","receivedAt":"2008-08-29T19:53:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 29. August 2008, Thomas Rast wrote:\n> Johannes Sixt wrote:\n> > This command does not give the expected result: It reports *two* initial\n> > commits and one merge; in particular, commit X is reported as initial\n> > commit.\n> >\n> > $ git rev-list --parents --reverse --full-history master -- P2\n>\n> The good news is: the patch really does fix your issue.  It also fixes\n> the test case (which uses the history from the diagram above).  And it\n> passes the test suite.\n\nThanks for picking up the topic and helping out. It does indeed work with my \nexample.\n\nI let others comment whether this patch is good. I've no clue how the revision \nwalker machinery works.\n\n-- Hannes\n"},{"id":"89286","messageId":"7vtzd1d969.fsf@gitster.siamese.dyndns.org","threadId":"15139","inReplyTo":"1220037518-11219-1-git-send-email-trast@student.ethz.ch","subject":"Re: [PATCH] rev-list: fix --reverse interaction with --parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-31T05:51:10Z","receivedAt":"2008-08-31T05:51:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> The bad news: I got as far as understanding that the repeated\n> simplification must be at fault, but gave up on trying to see exactly\n> _which step_ breaks the output.  Obviously parent rewriting is the\n> prime suspect.\n\nYeah, I noticed that running simplify_commit() (hence rewrite_parents())\nmore than once on the same commit seems to do \"unexpected\" things, during\nmy (hitherto unsuccessful) attempt to to make the --simplify-merges\nmachinery incremental.\n"}]}