{"thread":{"id":"4724","subject":"[PATCH] Enable tree (directory) history display","startedAt":"2006-07-01T01:00:12Z","lastAt":"2006-07-01T22:45:10Z","messageCount":9,"participants":["Luben Tuikov","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"22954","messageId":"20060701010012.1559.qmail@web31809.mail.mud.yahoo.com","threadId":"4724","inReplyTo":null,"subject":"[PATCH] Enable tree (directory) history display","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2006-07-01T01:00:12Z","receivedAt":"2006-07-01T01:00:12Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"This patch allows history display of whole trees/directories a la\n\"git-rev-list HEAD -- \". I find this useful especially\nwhen a project lives in its own subdirectory, as opposed to being all\nof the GIT repository (i.e. when a sub-project is merged into a\nsuper-project).\n\nSigned-off-by: Luben Tuikov \n---\n gitweb/gitweb.cgi | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.cgi b/gitweb/gitweb.cgi\nindex 2705a93..d808900 100755\n--- a/gitweb/gitweb.cgi\n+++ b/gitweb/gitweb.cgi\n@@ -1676,6 +1676,7 @@ #\t\t\t \" | \" . $cgi->a({-href => \"$my\n \t\t\t \"\\n\" .\n \t\t\t \"\" .\n \t\t\t $cgi->a({-href => \"$my_uri?\" .\nesc_param(\"p=$project;a=tree;h=$t_hash$base_key;f=$base$t_name\")}, \"tree\") .\n+\t\t\t \" | \" . $cgi->a({-href => \"$my_uri?\" .\nesc_param(\"p=$project;a=history;h=$hash_base;f=$base$t_name\")}, \"history\") .\n \t\t\t \"\\n\";\n \t\t}\n \t\tprint \"\\n\";\n-- \n1.4.1.rc2.g4ce4\n"},{"id":"22960","messageId":"7vy7vef52m.fsf@assigned-by-dhcp.cox.net","threadId":"4724","inReplyTo":"20060701010012.1559.qmail@web31809.mail.mud.yahoo.com","subject":"Re: [PATCH] Enable tree (directory) history display","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-07-01T02:10:25Z","receivedAt":"2006-07-01T02:10:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luben Tuikov writes:\n\n> This patch allows history display of whole trees/directories a la\n> \"git-rev-list HEAD -- \". I find this useful especially\n> when a project lives in its own subdirectory, as opposed to being all\n> of the GIT repository (i.e. when a sub-project is merged into a\n> super-project).\n\nBoth patches from you were seriously damaged. Check your MUA\nbefore sending further patches, please.\n\nThey were trivial for me to apply by hand, so no need to resend\nthem. I like the effect of this one I am replying to very much.\n"},{"id":"22961","messageId":"20060701024309.63001.qmail@web31805.mail.mud.yahoo.com","threadId":"4724","inReplyTo":"7vy7vef52m.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Enable tree (directory) history display","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2006-07-01T02:43:09Z","receivedAt":"2006-07-01T02:43:09Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- Junio C Hamano wrote:\n> Luben Tuikov writes:\n> \n> > This patch allows history display of whole trees/directories a la\n> > \"git-rev-list HEAD -- \". I find this useful especially\n> > when a project lives in its own subdirectory, as opposed to being all\n> > of the GIT repository (i.e. when a sub-project is merged into a\n> > super-project).\n> \n> Both patches from you were seriously damaged. Check your MUA\n> before sending further patches, please.\n\nYes, I'll take a look.\n\n> \n> They were trivial for me to apply by hand, so no need to resend\n> them. I like the effect of this one I am replying to very much.\n\nJunio, Linus,\n\nI took a comparative look with and without \"--full-history\",\nand FWIW, enabling full history just clobbers the output with a lot\nof unnecessary information. I.e. it shows merges which do not have\ndirect consequence or change to the files in the path spec specified\nafter the \"--\".\n\nI.e. no new, relevant information is being shown when \"--full-history\"\nis enabled. In fact the default git-rev-list case, simplify_history=1,\nstill shows a merge here and there which doesn't have any direct\nchanges into what is being sought, but the difference is\nabout 48% less clobber.\n\nCan you consider the default case to be simplify_history=1,\nwhich is currently the default behaviour of git-rev-list.\n\nIf not, is it possible to make this a configurable flag\nin gitweb, so that people can decide how much non-related\nhistory to show? I.e. I'd opt for the default case,\nno full-history.\n\nFWIW, I think that the original intention (had there been a choice)\nwould've been to show only most relevant history, i.e. changes\ndirectly related to paths and files after the \"--\".\n\nThanks,\n Luben\n"},{"id":"22964","messageId":"7v3bdmf2p6.fsf@assigned-by-dhcp.cox.net","threadId":"4724","inReplyTo":"20060701024309.63001.qmail@web31805.mail.mud.yahoo.com","subject":"Re: [PATCH] Enable tree (directory) history display","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-07-01T03:01:41Z","receivedAt":"2006-07-01T03:01:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luben Tuikov writes:\n\n> I took a comparative look with and without \"--full-history\",\n> and FWIW, enabling full history just clobbers the output with a lot\n> of unnecessary information. I.e. it shows merges which do not have\n> direct consequence or change to the files in the path spec specified\n> after the \"--\".\n>...\n> FWIW, I think that the original intention (had there been a choice)\n> would've been to show only most relevant history, i.e. changes\n> directly related to paths and files after the \"--\".\n\nSounds sane. Maybe we should clearly explain this behaviour\nchange in the commit log and claim it as an improvement. We\nmight even want to do --no-merges if we go that route.\n"},{"id":"22966","messageId":"20060701031004.29384.qmail@web31801.mail.mud.yahoo.com","threadId":"4724","inReplyTo":"7v3bdmf2p6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Enable tree (directory) history display","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2006-07-01T03:10:04Z","receivedAt":"2006-07-01T03:10:04Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- Junio C Hamano wrote:\n> Luben Tuikov writes:\n> \n> > I took a comparative look with and without \"--full-history\",\n> > and FWIW, enabling full history just clobbers the output with a lot\n> > of unnecessary information. I.e. it shows merges which do not have\n> > direct consequence or change to the files in the path spec specified\n> > after the \"--\".\n> >...\n> > FWIW, I think that the original intention (had there been a choice)\n> > would've been to show only most relevant history, i.e. changes\n> > directly related to paths and files after the \"--\".\n> \n> Sounds sane. Maybe we should clearly explain this behaviour\n> change in the commit log and claim it as an improvement. We\n> might even want to do --no-merges if we go that route.\n\nI agree. Indeed, this would definitely be an improvement.\n\n Luben\n"},{"id":"22967","messageId":"Pine.LNX.4.64.0606301954140.12404@g5.osdl.org","threadId":"4724","inReplyTo":"20060701024309.63001.qmail@web31805.mail.mud.yahoo.com","subject":"Re: [PATCH] Enable tree (directory) history display","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-07-01T03:21:59Z","receivedAt":"2006-07-01T03:21:59Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 30 Jun 2006, Luben Tuikov wrote:\n> \n> I took a comparative look with and without \"--full-history\",\n> and FWIW, enabling full history just clobbers the output with a lot\n> of unnecessary information. I.e. it shows merges which do not have\n> direct consequence or change to the files in the path spec specified\n> after the \"--\".\n\nOh, you're right - \"--full-history\" right now is literally geared solely \ntowards the graphical kind, which needs all the merges (regardless of \nwhether they change a file or not) to make sense of the history.\n\n> I.e. no new, relevant information is being shown when \"--full-history\"\n> is enabled. In fact the default git-rev-list case, simplify_history=1,\n> still shows a merge here and there which doesn't have any direct\n> changes into what is being sought, but the difference is\n> about 48% less clobber.\n\nWell, with history simplification, we still show merges that are required \nto make the history _complete_, ie say that you had\n\n\t a\n\t |\n\t b\n\t / \\\n\tc d\n\t| |\n\nand neither \"a\" nor \"b\" actually changed the file, but both \"c\" and \"d\" \ndid: in this case we have to leave \"b\" around just because otherwise there \nwould be no way to show the _relationship_, even if \"b\" itself doesn't \nactually change the tree in any way what-so-ever.\n\n> Can you consider the default case to be simplify_history=1,\n> which is currently the default behaviour of git-rev-list.\n\nActually, for your case, you don't want _any_ merges, unless those merges \nliterally changed the tree from all of the parents.\n\nI think it would make sense to make that further simplification if the \n\"--parents\" flag wasn't present. \n\nHmm. Maybe something like this..\n\nBTW! Junio, I think this patch actually fixes a real bug.\n\nWithout this patch, the \"--parents --full-history\" combination (which \nyou'd get if you do something like\n\n\tgitk --full-history Makefile\n\nor similar) will actually _drop_ merges where all children are identical. \nThat's wrong in the --full-history case, because it measn that the graph \nends up missing lots of entries.\n\nIn the process, this also should make\n\n\tgit-rev-list --full-history Makefile\n\ngive just the _true_ list of all commits that changed Makefile (and \nproperly ignore merges that were identical in one parent), because now \nwe're not asking for \"--parent\", so we don't need the unnecessary merge \ncommits to keep the history together.\n\nLuben, does this fix the problem for you?\n\n\t\tLinus\n\n----\ndiff --git a/revision.c b/revision.c\nindex b963f2a..1cf6276 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -280,7 +280,7 @@ int rev_same_tree_as_empty(struct rev_in\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+\tint tree_changed = 0, tree_same = 0;\n \n \tif (!commit->tree)\n \t\treturn;\n@@ -298,6 +298,7 @@ static void try_to_simplify_commit(struc\n \t\tparse_commit(p);\n \t\tswitch (rev_compare_tree(revs, p->tree, commit->tree)) {\n \t\tcase REV_TREE_SAME:\n+\t\t\ttree_same = 1;\n \t\t\tif (!revs->simplify_history || (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@@ -334,7 +335,7 @@ static void try_to_simplify_commit(struc\n \t\t}\n \t\tdie(\"bad tree compare for commit %s\", sha1_to_hex(commit->object.sha1));\n \t}\n-\tif (tree_changed)\n+\tif (tree_changed && !tree_same)\n \t\tcommit->object.flags |= TREECHANGE;\n }\n \n@@ -896,6 +897,8 @@ static int rewrite_one(struct rev_info *\n \t\tstruct commit *p = *pp;\n \t\tif (!revs->limited)\n \t\t\tadd_parents_to_list(revs, p, &revs->commits);\n+\t\tif (p->parents && p->parents->next)\n+\t\t\treturn 0;\n \t\tif (p->object.flags & (TREECHANGE | UNINTERESTING))\n \t\t\treturn 0;\n \t\tif (!p->parents)\n@@ -988,8 +991,15 @@ struct commit *get_revision(struct rev_i\n \t\t commit->parents && commit->parents->next)\n \t\t\tcontinue;\n \t\tif (revs->prune_fn && revs->dense) {\n-\t\t\tif (!(commit->object.flags & TREECHANGE))\n-\t\t\t\tcontinue;\n+\t\t\t/* Commit without changes? */\n+\t\t\tif (!(commit->object.flags & TREECHANGE)) {\n+\t\t\t\t/* drop merges unless we want parenthood */\n+\t\t\t\tif (!revs->parents)\n+\t\t\t\t\tcontinue;\n+\t\t\t\t/* non-merge - always ignore it */\n+\t\t\t\tif (commit->parents && !commit->parents->next)\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (revs->parents)\n \t\t\t\trewrite_parents(revs, commit);\n \t\t}\n"},{"id":"22968","messageId":"Pine.LNX.4.64.0606302029310.12404@g5.osdl.org","threadId":"4724","inReplyTo":"Pine.LNX.4.64.0606301954140.12404@g5.osdl.org","subject":"Re: [PATCH] Enable tree (directory) history display","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-07-01T03:45:43Z","receivedAt":"2006-07-01T03:45:43Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 30 Jun 2006, Linus Torvalds wrote:\n> \n> BTW! Junio, I think this patch actually fixes a real bug.\n> \n> Without this patch, the \"--parents --full-history\" combination (which \n> you'd get if you do something like\n> \n> \tgitk --full-history Makefile\n> \n> or similar) will actually _drop_ merges where all children are identical. \n> That's wrong in the --full-history case, because it measn that the graph \n> ends up missing lots of entries.\n\nHere's some real numbers.\n\nBefore this patch\n\n\tgit-rev-list --full-history HEAD -- Makefile | wc -l\n\tgit-rev-list --parents --full-history HEAD -- Makefile | wc -l\n\nboth returned 971 commits on my current kernel tree, while\n\n\tgit-rev-list --parents HEAD -- Makefile | wc -l\n\nreturned 145 commits, and\n\n\tgit-rev-list --parents --no-merges HEAD -- Makefile | wc -l\n\nreturns 136.\n\nThat count of 145 is the number of commits that actually _change_ Makefile \nsome way - and some of them really are merges, because they have a content \nmerge, and the merge result is thus different from any of the children. So \nthat's a real number. So is 136, in some sense - it just says that we \ndon't care about commits, even if those commits _do_ end up changing the \nfile.\n\nBut the important part to realize is that the \"971\" number is always \nwrong. It's never a really valid number. It contains a lot of extra \nmerges, but it does _not_ contain enough of them to connect all the dots, \nand it's thus never correct. Either you should drop merges that don't \nchange things (in which case you cannot have full connectivity, and \n\"--parents\" doesn't make sense), or you should keep them all (or at least \nenough to get full connectivity).\n\nNow, AFTER this patch\n\n\tgit-rev-list --full-history HEAD -- Makefile | wc -l\n\nreturns 145 commits (the same 145 commits that really change Makefile, but \nwe've now properly decided that because we don't have \"--parents\" and \ndon't need to keep connectivity we drop _all_ of the merges that have a \nchild that is identical), while\n\n\tgit-rev-list --parents --full-history HEAD -- Makefile | wc -l\n\nreturns 2323 commits, and now really has _all_ the merges (because it \nneeds to include every single merge in the tree - otherwise the \nconnectivity doesn't make sense).\n\nNow, that 2323 is a bit unnecessary - we end up having merges to merges \nthat don't actually have any changes at all in between, and it might be \nnice to simplify the merge history to create a minimal tree that still has \nall potential changes in it, but that's a much harder problem.\n\nAnyway, the patch definitely makes a difference, and I think it's correct. \nThe effects might be a bit easier to visualize on a smaller tree than the \nkernel ;)\n\nWe could still potentially improve on the \"--parents --full-history\" case, \nbut --full-history currently means always walking all possible chains, and \nthat will be shown in the output (ie we will have all possible paths in \nthe result if \"--parents\" is used, even if those paths end up being \ntotally uninteresting)\n\n \"Hey - you asked for full history, you got it. Don't blame me if you got \n a lot of totally uninteresting crud\"\n\n\t\t\t\tLinus\n"},{"id":"22989","messageId":"20060701193838.71618.qmail@web31802.mail.mud.yahoo.com","threadId":"4724","inReplyTo":"Pine.LNX.4.64.0606301954140.12404@g5.osdl.org","subject":"Re: [PATCH] Enable tree (directory) history display","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2006-07-01T19:38:38Z","receivedAt":"2006-07-01T19:38:38Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- Linus Torvalds wrote:\n> Well, with history simplification, we still show merges that are required \n> to make the history _complete_, ie say that you had\n> \n> \t a\n> \t |\n> \t b\n> \t / \\\n> \tc d\n> \t| |\n> \n> and neither \"a\" nor \"b\" actually changed the file, but both \"c\" and \"d\" \n> did: in this case we have to leave \"b\" around just because otherwise there \n> would be no way to show the _relationship_, even if \"b\" itself doesn't \n> actually change the tree in any way what-so-ever.\n\nI agree. If \"c\" and/or \"d\" changed the file but neither \"a\" nor \"b\" did,\nthen by (merge/diff/etc) \"inheritance\" we do need to leave \"b\" around.\n\n(This is similar to git-blame/git-annotate which should show \"b\", so that\nwe can track down the change to \"c\" and/or \"d\".)\n\n> > Can you consider the default case to be simplify_history=1,\n> > which is currently the default behaviour of git-rev-list.\n> \n> Actually, for your case, you don't want _any_ merges, unless those merges \n> literally changed the tree from all of the parents.\n\nYes, that's true. s/all/one or more:\nDon't want to show a merge, unless one or more of the parents,\nchanged the file. If no parent changed the tree, then do not\nshow the commit.\n\n> I think it would make sense to make that further simplification if the \n> \"--parents\" flag wasn't present. \n> \n> Hmm. Maybe something like this..\n> \n> BTW! Junio, I think this patch actually fixes a real bug.\n> \n> Without this patch, the \"--parents --full-history\" combination (which \n> you'd get if you do something like\n> \n> \tgitk --full-history Makefile\n> \n> or similar) will actually _drop_ merges where all children are identical. \n> That's wrong in the --full-history case, because it measn that the graph \n> ends up missing lots of entries.\n> \n> In the process, this also should make\n> \n> \tgit-rev-list --full-history Makefile\n> \n> give just the _true_ list of all commits that changed Makefile (and \n> properly ignore merges that were identical in one parent), because now \n> we're not asking for \"--parent\", so we don't need the unnecessary merge \n> commits to keep the history together.\n> \n> Luben, does this fix the problem for you?\n\nGiven Junio's analysis, and briefly looking at the logic, it does seem\ncorrect. Let me apply it and see what I get, but I think it is a good thing.\n\nThanks for the patch!\n\n Luben\n"},{"id":"22998","messageId":"20060701224510.38530.qmail@web31806.mail.mud.yahoo.com","threadId":"4724","inReplyTo":"Pine.LNX.4.64.0606302029310.12404@g5.osdl.org","subject":"Re: [PATCH] Enable tree (directory) history display","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2006-07-01T22:45:10Z","receivedAt":"2006-07-01T22:45:10Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- Linus Torvalds wrote:\n> That count of 145 is the number of commits that actually _change_ Makefile \n> some way - and some of them really are merges, because they have a content \n> merge, and the merge result is thus different from any of the children. So \n> that's a real number. So is 136, in some sense - it just says that we \n> don't care about commits, even if those commits _do_ end up changing the \n> file.\n\nIndeed, I got the same conclusion using different branches.\n(Sorry, I thought your email was from Junio in my last email, it was\nyour analysis after all.)\n\n> But the important part to realize is that the \"971\" number is always \n> wrong. It's never a really valid number. It contains a lot of extra \n> merges, but it does _not_ contain enough of them to connect all the dots, \n> and it's thus never correct. Either you should drop merges that don't \n> change things (in which case you cannot have full connectivity, and \n> \"--parents\" doesn't make sense), or you should keep them all (or at least \n> enough to get full connectivity).\n\nYes, that makes sense.\n\nOk, I get similar numbers as you do. After the patch, compared to\nthe simple case of git-rev-list HEAD -- , --parents gives\nme only one more commit which is the very first hand made kernel\ncommit ;-) ; --parents --full-history gives me way too many unrelated\ncommits about 10x as many; --full-history gives me a _complete_ list.\n\nThe simple case fails to report a few commits which are\ndue to merges, which do indeed change files in the path spec.\n(--full-history successfully reported those though).\n\nSo your patch plus \"--full-history\" is what I think we want.\n\n Luben\n"}]}