{"thread":{"id":"9977","subject":"History over-simplification","startedAt":"2007-09-23T04:46:28Z","lastAt":"2007-09-26T15:55:29Z","messageCount":3,"participants":["Shawn O. Pearce","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"53820","messageId":"20070923044628.GA3099@spearce.org","threadId":"9977","inReplyTo":null,"subject":"History over-simplification","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-23T04:46:28Z","receivedAt":"2007-09-23T04:46:28Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"This is an interesting problem for me.  We had a bad merge in the\nrepository that produced this particular gitk graph:\n\n  http://www.spearce.org/2007/07/ugly-gitk.gif\n\nThe project maintainer tried to locate the commits that altered\na given file with `gitk -- path` and found only one commit\n(the introduction of the path) but expected to at least find\na modification from a side branch.  What he was looking for\nspecifically was the merge (or revert) that threw away that\nside branch's change as the change wasn't supposed to have been\nreverted/lost.\n\nHe knew what the commit from the side branch was (the author of\nit still had a branch pointing at the commit).  But try finding\nthe proper child commit in gitk without a path limiter in that\ngraph I link to above.  If you don't remember what a mess it is,\nclick through briefly to refresh your memory.  The path limiter is\nreally required here to boil the graph down to something a human\ncan reason with.\n\nAdding in --full-history *almost* gave us the data he needed.\nIt did find the modification on the side branch but it couldn't\npoint to the child commit which reverted the path.  It turns\nout the bad change was an octopus merge commit done incorrectly.\nYes, really.  Don't bother telling me how bad octopus merges are.\nThis problem happens in a two parent merge as well.\n\nBelow is a test which shows the problem.  Without the associated\npatch to revision.c we cannot find the ours merge that trashed the\nside branch's change.  In this toy repository its no big deal to\nfind the ours merge and figure out what happened.  In the production\nrepository we were looking at above its impossible without support\nfrom Git.\n\nI don't really like the patch to revision.c because it winds up\nshowing trivial merges too.  What I really want is to have the\n\"--full-history\" option include a merge if either of the following\nis true:\n\n a) The resulting path does not match _any_ of the parents.  We\n    already handle this case correctly in revision.c and correctly\n    show the \"evil\" merge.\n\n b) The resulting path matches one of the parents but not one of\n    the others.  In such a case the merge should still be output if\n    a 3-way read-tree would not have chosen this result by default.\n\nThe problem is computing b) from within the revision walker.\nYou can't compute the merge bases while inside the inner loop of the\nrevision walker itself, its already busy and the flags are already\nin-use on all objects.  Worse commit parents are being rewritten\nin ways that may break merge base computation.  Oh, and lets not\neven talk about how expensive that merge base computation is if we\nwere to perform it on every merge.  :-\\\n\nThoughts?\n\n\ndiff --git a/revision.c b/revision.c\nindex 33d092c..aca62a6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -365,7 +365,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n \t\t}\n \t\tdie(\"bad tree compare for commit %s\", sha1_to_hex(commit->object.sha1));\n \t}\n-\tif (tree_changed && !tree_same)\n+\tif (tree_changed && (!tree_same || !revs->simplify_history))\n \t\tcommit->object.flags |= TREECHANGE;\n }\n \ndiff --git a/t/t6008-rev-list-toosimple.sh b/t/t6008-rev-list-toosimple.sh\nnew file mode 100755\nindex 0000000..696b057\n--- /dev/null\n+++ b/t/t6008-rev-list-toosimple.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='test git-rev-list history over simplification'\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\techo BASE >content &&\n+\tgit add content &&\n+\tgit commit -m BASE &&\n+\tbase=$(git rev-parse --verify HEAD) &&\n+\n+\ttest_tick &&\n+\tgit checkout -b side &&\n+\techo SIDE >content &&\n+\tgit commit -m SIDE content &&\n+\tside=$(git rev-parse --verify HEAD) &&\n+\n+\ttest_tick &&\n+\tgit checkout master &&\n+\techo OTHER >other &&\n+\tgit add other &&\n+\tgit commit -m OTHER &&\n+\n+\ttest_tick &&\n+\tgit merge -s ours side &&\n+\tkeep=$(git rev-parse --verify HEAD) &&\n+\ttest OTHER = $(git cat-file blob HEAD:other) &&\n+\ttest BASE = $(git cat-file blob HEAD:content)\n+'\n+\n+cat >expect <<EOF\n+$keep\n+$base\n+$side\n+EOF\n+test_expect_success 'revert merge in history' '\n+\tgit rev-list --full-history HEAD -- content >actual &&\n+\tgit diff expect actual\n+'\n+\n+test_done\n\n-- \nShawn.\n"},{"id":"54054","messageId":"7v4phi4b9t.fsf@gitster.siamese.dyndns.org","threadId":"9977","inReplyTo":"20070923044628.GA3099@spearce.org","subject":"Re: History over-simplification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-25T22:42:54Z","receivedAt":"2007-09-25T22:42:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> I don't really like the patch to revision.c because it winds up\n> showing trivial merges too.  What I really want is to have the\n> \"--full-history\" option include a merge if either of the following\n> is true:\n>\n>  a) The resulting path does not match _any_ of the parents.  We\n>     already handle this case correctly in revision.c and correctly\n>     show the \"evil\" merge.\n>\n>  b) The resulting path matches one of the parents but not one of\n>     the others.  In such a case the merge should still be output if\n>     a 3-way read-tree would not have chosen this result by default.\n\nI am not sure (b) is useful in general.  Merging two branches\nthat fix the same issue but in different ways (think: 'maint'\nand 'master' have different infrastructure and a fix initially\nmade on 'master' was backported to 'maint', and then later\n'maint' needed to be merged to 'master' to carry forward other\nfixes) is a norm, and in such cases taking the version from the\nmerged-to branch is almost always what happens.\n\nAlso it sounds to me by \"if read-tree would not have chosen this\nresult by default\" you mean this feature would not just need to\nrun merge-base but also recursive merge-base synthesis, and also\nrecreate the structural merge (aka \"rename detection\") there as\nwell.  Even if (b) is useful, it sounds like a very expensive\noption, and the current merge-recursive code is structured in\nsuch a way to be easily reused for this purpose.\n"},{"id":"54105","messageId":"20070926155529.GM3099@spearce.org","threadId":"9977","inReplyTo":"7v4phi4b9t.fsf@gitster.siamese.dyndns.org","subject":"Re: History over-simplification","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-26T15:55:29Z","receivedAt":"2007-09-26T15:55:29Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> \n> > I don't really like the patch to revision.c because it winds up\n> > showing trivial merges too.  What I really want is to have the\n> > \"--full-history\" option include a merge if either of the following\n> > is true:\n> >\n> >  a) The resulting path does not match _any_ of the parents.  We\n> >     already handle this case correctly in revision.c and correctly\n> >     show the \"evil\" merge.\n> >\n> >  b) The resulting path matches one of the parents but not one of\n> >     the others.  In such a case the merge should still be output if\n> >     a 3-way read-tree would not have chosen this result by default.\n> \n> I am not sure (b) is useful in general.  Merging two branches\n> that fix the same issue but in different ways (think: 'maint'\n> and 'master' have different infrastructure and a fix initially\n> made on 'master' was backported to 'maint', and then later\n> 'maint' needed to be merged to 'master' to carry forward other\n> fixes) is a norm, and in such cases taking the version from the\n> merged-to branch is almost always what happens.\n\n(b) is useful, even in the case you just described.  Try doing the\nabove backport today.  Run `git log maint -- foo.c` and find the\nbackported commit B.  Now try to see if that commit is in master in\nfoo.c with `git log master -- foo.c`.  B won't appear as the merge\nof maint into master chose master's revision and maint's history\nis pruned away.\n\nNow if the backported change was logically the same but was rewritten\nconsiderably it might take you a while to figure out why a merge\ndiscarded the change B.  The merge commit might actually describe\nwhy in its message, but you can't find that particular merge commit\ntoday, even with --full-history.  Not without my one-line patch.\n\nUnfortunately my one-line patch causes all of the merges that the\npath is involved in to be output.  That can be quite a lot of them.\n \n> Also it sounds to me by \"if read-tree would not have chosen this\n> result by default\" you mean this feature would not just need to\n> run merge-base but also recursive merge-base synthesis, and also\n> recreate the structural merge (aka \"rename detection\") there as\n> well.  Even if (b) is useful, it sounds like a very expensive\n> option, and the current merge-recursive code is structured in\n> such a way to be easily reused for this purpose.\n\nI think you meant here that it is *not* structured in a way to be\neasily reused for this purpose.  That code relies heavily on the\nindex to allow it to create that synthetic merge base.  Making it\ncallable from within the revision walker would not be a small change.\n\nBut as you say, running merge-recursive here would be a very\nexpensive option.  Which is why I was saying the 3-way read-tree\nresult as that is cheaper to compute.  However the read-tree result\ncan be different from the merge-recursive result (think renames)\nand yet both are still \"trivial\" resolutions that the user was\nnever involved in.\n\nOf course other kinds of trivial merges (where changes from the\ntwo branches are in the same file but far enough part it can be\neasily merged as a 3-way file merge) appear to the path limiter\nas though they were an evil merge, and such merges are output.\n\nSo maybe having --full-history output all merges that affected\nthat path really is the right choice.\n\n-- \nShawn.\n"}]}