{"thread":{"id":"3599","subject":"git-rev-list bug?","startedAt":"2006-03-08T16:19:48Z","lastAt":"2006-03-10T22:42:03Z","messageCount":9,"participants":["Catalin Marinas","Junio C Hamano","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"17335","messageId":"b0943d9e0603080819i227c637fo@mail.gmail.com","threadId":"3599","inReplyTo":null,"subject":"git-rev-list bug?","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2006-03-08T16:19:48Z","receivedAt":"2006-03-08T16:19:48Z","isPatch":false,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"Sorry if this was previously discussed. I ran git-rev-list on a linear\ngraph and tried to filter the results by a file name:\n\n git rev-list since.. path/to/file\n\nbut it always shows the child commit of \"since\" even if it didn't\ntouch the file. The same behaviour is for git-log (since it uses\ngit-rev-list) but git-whatchanged seems to be fine.\n\nIs this the intended behaviour? The \"stg patches\" command based on\ngit-rev-list used to work fine a few weeks ago but now it is always\nreporting the bottom patch in the stack as modifying a given file.\n\nThanks.\n\n--\nCatalin\n"},{"id":"17344","messageId":"7vacc0iten.fsf@assigned-by-dhcp.cox.net","threadId":"3599","inReplyTo":"b0943d9e0603080819i227c637fo@mail.gmail.com","subject":"Re: git-rev-list bug?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-08T20:16:32Z","receivedAt":"2006-03-08T20:16:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Catalin Marinas\" writes:\n\n> Sorry if this was previously discussed. I ran git-rev-list on a linear\n> graph and tried to filter the results by a file name:\n>\n> git rev-list since.. path/to/file\n>\n> but it always shows the child commit of \"since\" even if it didn't\n> touch the file. The same behaviour is for git-log (since it uses\n> git-rev-list) but git-whatchanged seems to be fine.\n>\n> Is this the intended behaviour? The \"stg patches\" command based on\n> git-rev-list used to work fine a few weeks ago but now it is always\n> reporting the bottom patch in the stack as modifying a given file.\n\nI can confirm that this is a recent breakage, but since it is\nunfortunately my day-job day the more detailed analysis and fix\nneeds to wait. Sorry.\n"},{"id":"17418","messageId":"7vmzfy1zjb.fsf@assigned-by-dhcp.cox.net","threadId":"3599","inReplyTo":"7vacc0iten.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-rev-list bug?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-10T08:20:40Z","receivedAt":"2006-03-10T08:20:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Catalin Marinas\" writes:\n>\n>> Sorry if this was previously discussed. I ran git-rev-list on a linear\n>> graph and tried to filter the results by a file name:\n>>\n>> git rev-list since.. path/to/file\n>>\n>> but it always shows the child commit of \"since\" even if it didn't\n>> touch the file. The same behaviour is for git-log (since it uses\n>> git-rev-list) but git-whatchanged seems to be fine.\n>>\n>> Is this the intended behaviour? The \"stg patches\" command based on\n>> git-rev-list used to work fine a few weeks ago but now it is always\n>> reporting the bottom patch in the stack as modifying a given file.\n>\n> I can confirm that this is a recent breakage, but since it is\n> unfortunately my day-job day the more detailed analysis and fix\n> needs to wait. Sorry.\n\nTo my surprise, it turns out that this regression was not very\nrecent. Bisecting points at this commite:\n\ndiff-tree 461cf59... (from 6b94f1e...)\nAuthor: Linus Torvalds \nDate: Wed Jan 18 14:47:30 2006 -0800\n\n rev-list: stop when the file disappears\n \n The one thing I've considered doing (I really should) is to add a \"stop\n when you don't find the file\" option to \"git-rev-list\". This patch does\n some of the work towards that: it removes the \"parent\" thing when the\n file disappears, so a \"git annotate\" could do do something like\n \n \tgit-rev-list --remove-empty --parents HEAD -- \"$filename\"\n \n and it would get a good graph that stops when the filename disappears\n (it's not perfect though: it won't remove all the unintersting commits).\n \n It also simplifies the logic of finding tree differences a bit, at the\n cost of making it a tad less efficient.\n \n The old logic was two-phase: it would first simplify _only_ merges tree as\n it traversed the tree, and then simplify the linear parts of the remainder\n independently. That was pretty optimal from an efficiency standpoint\n because it avoids doing any comparisons that we can see are unnecessary,\n but it made it much harder to understand than it really needed to be.\n \n The new logic is a lot more straightforward, and compares the trees as it\n traverses the graph (ie everything is a single phase). That makes it much\n easier to stop graph traversal at any point where a file disappears.\n \n\nI haven't had time to dig into this deeper yet...\n"},{"id":"17423","messageId":"7virqmzlhb.fsf@assigned-by-dhcp.cox.net","threadId":"3599","inReplyTo":"7vmzfy1zjb.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-rev-list bug?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-10T09:40:16Z","receivedAt":"2006-03-10T09:40:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> To my surprise, it turns out that this regression was not very\n> recent. Bisecting points at this commite:\n>...\n> I haven't had time to dig into this deeper yet...\n\n\nI am wondering why try_to_simplify_commit() skips parents marked\nwith UNINTERESTING. I think this is causing a problem Catalin\nfound with rev-list.\n\nIf you have (time flows from top to bottom):\n\n\tcommit#1\t(it does not matter what this does)\n commit#2 change file B only\n commit#3\tchange file A only\n commit#4\tchange file B only\n\n\"git-rev-list commit#1..commit#4 -- A\" shows commit#3 (correct)\nand commit#2 (incorrect).\n\nIt pushes commit#1 (UNINTERESTING) and commit#4 (~UNINTERESTING)\nand starts traversing. try-to-simplify(commit#4) says \"no tree\nchange between #3 and #4\" and it returns without marking\ncommit#4 with TREECHANGE flag. But when looking at commit#2 and\ntrying to simplify it, it says \"Ah, its parent is uninteresting,\nso I would not do compare_tree()\". Iteration over parents of\ncommit#2 leaves the while() loop and at the end of function the\ncommit is marked with TREECHANGE and is shown.\n\nThe attached patch seems to fix it (without losing the logic to\nomit tree comparison with UNINTERESTING parent, which I do not\nquite understand).\n\n\n\n---\ndiff --git a/revision.c b/revision.c\nindex 713f27e..23c9b9d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -282,6 +282,7 @@ static int same_tree_as_empty(struct tre\n static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n {\n \tstruct commit_list **pp, *parent;\n+\tint changed = 0;\n \n \tif (!commit->tree)\n \t\treturn;\n@@ -315,12 +316,14 @@ static void try_to_simplify_commit(struc\n \t\t\t}\n \t\t/* fallthrough */\n \t\tcase TREE_DIFFERENT:\n+\t\t\tchanged = 1;\n \t\t\tpp = &parent->next;\n \t\t\tcontinue;\n \t\t}\n \t\tdie(\"bad tree compare for commit %s\", sha1_to_hex(commit->object.sha1));\n \t}\n-\tcommit->object.flags |= TREECHANGE;\n+\tif (changed)\n+\t\tcommit->object.flags |= TREECHANGE;\n }\n \n static void add_parents_to_list(struct rev_info *revs, struct commit *commit, struct commit_list **list)\n"},{"id":"17424","messageId":"7v4q26zklx.fsf@assigned-by-dhcp.cox.net","threadId":"3599","inReplyTo":"7virqmzlhb.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-rev-list bug?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-10T09:59:06Z","receivedAt":"2006-03-10T09:59:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I am wondering why try_to_simplify_commit() skips parents marked\n> with UNINTERESTING. I think this is causing a problem Catalin\n> found with rev-list.\n> ...\n> The attached patch seems to fix it (without losing the logic to\n> omit tree comparison with UNINTERESTING parent, which I do not\n> quite understand).\n\nActually the previous patch is not right either. If I ask \"what\nchanges path B between commit#1..commit#4\", it would still omit\ncommit#2.\n\nIt should not matter if the parent is uninteresting while\nchecking if a commit touches the specified path. The attached\npatch which replaces the previous botched one does exactly that.\n\nIt however has a side effect -- uninteresting commits were never\nparsed here, but now they get parsed. I am not sure if there\nare correctness implications...\n\n---\ndiff --git a/revision.c b/revision.c\nindex 713f27e..9d0934a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -296,11 +296,6 @@ static void try_to_simplify_commit(struc\n \twhile ((parent = *pp) != NULL) {\n \t\tstruct commit *p = parent->item;\n \n-\t\tif (p->object.flags & UNINTERESTING) {\n-\t\t\tpp = &parent->next;\n-\t\t\tcontinue;\n-\t\t}\n-\n \t\tparse_commit(p);\n \t\tswitch (compare_tree(p->tree, commit->tree)) {\n \t\tcase TREE_SAME:\n"},{"id":"17425","messageId":"7vslpqy4u7.fsf@assigned-by-dhcp.cox.net","threadId":"3599","inReplyTo":"7v4q26zklx.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-rev-list bug?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-10T10:25:04Z","receivedAt":"2006-03-10T10:25:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> It however has a side effect -- uninteresting commits were never\n> parsed here, but now they get parsed. I am not sure if there\n> are correctness implications...\n\nActually there is. If a merge with an uninteresting side branch\nwas the only thing that brought changes to paths we are\ninterested in, we do not want TREE_SAME logic to remove other\nparents (i.e. the branches we are interested in) from the merge\ncommit. \n\nSo we would need a combination of both, something like this?\n\n---\ndiff --git a/revision.c b/revision.c\nindex 713f27e..c8d93ff 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -282,6 +282,7 @@ static int same_tree_as_empty(struct tre\n static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n {\n \tstruct commit_list **pp, *parent;\n+\tint tree_changed = 0;\n \n \tif (!commit->tree)\n \t\treturn;\n@@ -296,14 +297,19 @@ static void try_to_simplify_commit(struc\n \twhile ((parent = *pp) != NULL) {\n \t\tstruct commit *p = parent->item;\n \n-\t\tif (p->object.flags & UNINTERESTING) {\n-\t\t\tpp = &parent->next;\n-\t\t\tcontinue;\n-\t\t}\n-\n \t\tparse_commit(p);\n \t\tswitch (compare_tree(p->tree, commit->tree)) {\n \t\tcase TREE_SAME:\n+\t\t\tif (p->object.flags & UNINTERESTING) {\n+\t\t\t\t/* Even if a merge with an uninteresting\n+\t\t\t\t * side branch brought the entire change\n+\t\t\t\t * we are interested in, we do not want\n+\t\t\t\t * to lose the other branches of this\n+\t\t\t\t * merge, so we just keep going.\n+\t\t\t\t */\n+\t\t\t\tpp = &parent->next;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tparent->next = NULL;\n \t\t\tcommit->parents = parent;\n \t\t\treturn;\n@@ -315,12 +321,14 @@ static void try_to_simplify_commit(struc\n \t\t\t}\n \t\t/* fallthrough */\n \t\tcase TREE_DIFFERENT:\n+\t\t\ttree_changed = 1;\n \t\t\tpp = &parent->next;\n \t\t\tcontinue;\n \t\t}\n \t\tdie(\"bad tree compare for commit %s\", sha1_to_hex(commit->object.sha1));\n \t}\n-\tcommit->object.flags |= TREECHANGE;\n+\tif (tree_changed)\n+\t\tcommit->object.flags |= TREECHANGE;\n }\n \n static void add_parents_to_list(struct rev_info *revs, struct commit *commit, struct commit_list **list)\n"},{"id":"17427","messageId":"tnx64mmpnq4.fsf@arm.com","threadId":"3599","inReplyTo":"7vslpqy4u7.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-rev-list bug?","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@arm.com","sentAt":"2006-03-10T11:01:55Z","receivedAt":"2006-03-10T11:01:55Z","isPatch":false,"sender":{"key":"catalin.marinas@arm.com","avatar":null},"body":"Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> It however has a side effect -- uninteresting commits were never\n>> parsed here, but now they get parsed. I am not sure if there\n>> are correctness implications...\n>\n> Actually there is. If a merge with an uninteresting side branch\n> was the only thing that brought changes to paths we are\n> interested in, we do not want TREE_SAME logic to remove other\n> parents (i.e. the branches we are interested in) from the merge\n> commit. \n>\n> So we would need a combination of both, something like this?\n\nI can confirm that it \"stg patches\" works fine with this patch for\ngit-rev-list.\n\nThanks.\n\n-- \nCatalin\n"},{"id":"17438","messageId":"Pine.LNX.4.64.0603101030160.18022@g5.osdl.org","threadId":"3599","inReplyTo":"7vslpqy4u7.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-rev-list bug?","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-03-10T18:31:31Z","receivedAt":"2006-03-10T18:31:31Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 10 Mar 2006, Junio C Hamano wrote:\n> \n> So we would need a combination of both, something like this?\n\nLooks good to me.\n\nAs does the 'revs.prune_fn' abstraction by Fredrik Kuivinen, for that \nmatter. Ack to both.\n\n\t\tLinus\n"},{"id":"17442","messageId":"7voe0dylac.fsf@assigned-by-dhcp.cox.net","threadId":"3599","inReplyTo":"Pine.LNX.4.64.0603101030160.18022@g5.osdl.org","subject":"Re: git-rev-list bug?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-10T22:42:03Z","receivedAt":"2006-03-10T22:42:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Fri, 10 Mar 2006, Junio C Hamano wrote:\n>> \n>> So we would need a combination of both, something like this?\n>\n> Looks good to me.\n>\n> As does the 'revs.prune_fn' abstraction by Fredrik Kuivinen, for that \n> matter. Ack to both.\n\nThanks. I'm swamped by day-job today so will apply it sometime\ntomorrow.\n"}]}