{"thread":{"id":"3763","subject":"[PATCH/RFC 1/2] Move \"--parent\" parsing into generic revision.c library code","startedAt":"2006-03-31T00:52:42Z","lastAt":"2006-04-03T04:22:12Z","messageCount":17,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"18163","messageId":"Pine.LNX.4.64.0603301648530.27203@g5.osdl.org","threadId":"3763","inReplyTo":null,"subject":"[PATCH/RFC 1/2] Move \"--parent\" parsing into generic revision.c library code","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-03-31T00:52:42Z","receivedAt":"2006-03-31T00:52:42Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nMove \"--parent\" parsing into generic revision.c library code\n\nNot only do we do it in both rev-list.c and git.c, the revision walking\ncode will soon want to know whether we should rewrite parenthood\ninformation or not.\n\nSigned-off-by: Linus Torvalds \n---\n\nThis is the trivial part. The next email will contain the real meat of the \nchange, which starts doing path limiting incrementally, and makes doing \nthings like\n\n\tgit log drivers/\n\non the kernel a hell of a lot more pleasant, because it starts outputting \nthe log immediately instead of pausing for about 20 seconds before it has \nparsed all of the history and then outputting it all in one go.\n\nThere's a HUGE difference to the \"feel\" of doing pathname limiting \ngit-rev-list options. Not very well tested, but at least this initial \npreparatory patch is \"obviously safe\".\n\ndiff --git a/git.c b/git.c\nindex 0b40e30..72039c6 100644\n--- a/git.c\n+++ b/git.c\n@@ -283,7 +283,6 @@ static int cmd_log(int argc, const char \n \tchar *buf = xmalloc(LOGSIZE);\n \tstatic enum cmit_fmt commit_format = CMIT_FMT_DEFAULT;\n \tint abbrev = DEFAULT_ABBREV;\n-\tint show_parents = 0;\n \tconst char *commit_prefix = \"commit \";\n \n \targc = setup_revisions(argc, argv, &rev, \"HEAD\");\n@@ -294,9 +293,6 @@ static int cmd_log(int argc, const char \n \t\t\tif (commit_format == CMIT_FMT_ONELINE)\n \t\t\t\tcommit_prefix = \"\";\n \t\t}\n-\t\telse if (!strcmp(arg, \"--parents\")) {\n-\t\t\tshow_parents = 1;\n-\t\t}\n \t\telse if (!strcmp(arg, \"--no-abbrev\")) {\n \t\t\tabbrev = 0;\n \t\t}\n@@ -317,7 +313,7 @@ static int cmd_log(int argc, const char \n \twhile ((commit = get_revision(&rev)) != NULL) {\n \t\tprintf(\"%s%s\", commit_prefix,\n \t\t sha1_to_hex(commit->object.sha1));\n-\t\tif (show_parents) {\n+\t\tif (rev.parents) {\n \t\t\tstruct commit_list *parents = commit->parents;\n \t\t\twhile (parents) {\n \t\t\t\tstruct object *o = &(parents->item->object);\ndiff --git a/rev-list.c b/rev-list.c\nindex f3a989c..19a547a 100644\n--- a/rev-list.c\n+++ b/rev-list.c\n@@ -39,7 +39,6 @@ struct rev_info revs;\n static int bisect_list = 0;\n static int verbose_header = 0;\n static int abbrev = DEFAULT_ABBREV;\n-static int show_parents = 0;\n static int show_timestamp = 0;\n static int hdr_termination = 0;\n static const char *commit_prefix = \"\";\n@@ -54,7 +53,7 @@ static void show_commit(struct commit *c\n \tif (commit->object.flags & BOUNDARY)\n \t\tputchar('-');\n \tfputs(sha1_to_hex(commit->object.sha1), stdout);\n-\tif (show_parents) {\n+\tif (revs.parents) {\n \t\tstruct commit_list *parents = commit->parents;\n \t\twhile (parents) {\n \t\t\tstruct object *o = &(parents->item->object);\n@@ -336,10 +335,6 @@ int main(int argc, const char **argv)\n \t\t\t\tcommit_prefix = \"\";\n \t\t\telse\n \t\t\t\tcommit_prefix = \"commit \";\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--parents\")) {\n-\t\t\tshow_parents = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--timestamp\")) {\ndiff --git a/revision.c b/revision.c\nindex abc8745..0330f9f 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -596,6 +596,10 @@ int setup_revisions(int argc, const char\n \t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strcmp(arg, \"--parents\")) {\n+\t\t\t\trevs->parents = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!strcmp(arg, \"--dense\")) {\n \t\t\t\trevs->dense = 1;\n \t\t\t\tcontinue;\ndiff --git a/revision.h b/revision.h\nindex 61e6bc9..0caeecf 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -34,7 +34,8 @@ struct rev_info {\n \t\t\tedge_hint:1,\n \t\t\tlimited:1,\n \t\t\tunpacked:1,\n-\t\t\tboundary:1;\n+\t\t\tboundary:1,\n+\t\t\tparents:1;\n \n \t/* special limits */\n \tint max_count;\n"},{"id":"18164","messageId":"Pine.LNX.4.64.0603301652531.27203@g5.osdl.org","threadId":"3763","inReplyTo":"Pine.LNX.4.64.0603301648530.27203@g5.osdl.org","subject":"[PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-03-31T01:05:25Z","receivedAt":"2006-03-31T01:05:25Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nThis makes git-rev-list able to do path-limiting without having to parse\nall of history before it starts showing the results.\n\nThis makes it things like \"git log -- pathname\" much more pleasant to use.\n\nThis is actually a pretty small patch, and the biggest part of it is \npurely cleanups (turning the \"goto next\" statements into \"continue\"), but \nit's conceptually a lot bigger than it looks.\n\nWhat it does is that if you do a path-limited revision list, and you do \n_not_ ask for pseudo-parenthood information, it won't do all the \npath-limiting up-front, but instead do it incrementally in \n\"get_revision()\".\n\nThis is an absolutely huge deal for anything like \"git log -- \", \nbut also for some things that we don't do yet - like the \"find where \nthings changed\" logic I've described elsewhere, where we want to find the \nprevious revision that changed a file.\n\nThe reason I put \"RFC\" in the subject line is that while I've validated it \nvarious ways, like doing\n\n\tgit-rev-list HEAD -- drivers/char/ | md5sum \n\nbefore-and-after on the kernel archive, it's \"git-rev-list\" after all. In \nother words, it's that really really subtle and complex central piece of \nsoftware. So while I think this is important and should go in asap, I also \nthink it should get lots of testing and eyeballs looking at the code.\n\nBtw, don't even bother testing this with the git archive. git itself is so \nsmall that parsing the whole revision history for it takes about a second \neven with path limiting. The thing that _really_ shows this off is doing\n\n\tgit log drivers/\n\non the kernel archive, or even better, on the _historic_ kernel archive.\n\nWith this change, the response is instantaneous (although seeking to the \nend of the result will obviously take as long as it ever did). Before this \nchange, the command would think about the result for tens of seconds - or \neven minutes, in the case of the bigger old kernel archive - before \nstarting to output the results.\n\nNOTE NOTE NOTE! Using path limiting with things like \"gitk\", which uses \nthe \"--parents\" flag to actually generate a pseudo-history of the \nresulting commits won't actually see the improvement in interactivity, \nsince that forces git-rev-list to do the whole-history thing after all. \n\nMAYBE we can fix that too at some point, but I won't promise anything.\n\nSigned-off-by: Linus Torvalds \n---\n\n\ndiff --git a/revision.c b/revision.c\nindex 0330f9f..0e3f074 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -702,7 +702,13 @@ int setup_revisions(int argc, const char\n \tif (revs->prune_data) {\n \t\tdiff_tree_setup_paths(revs->prune_data);\n \t\trevs->prune_fn = try_to_simplify_commit;\n-\t\trevs->limited = 1;\n+\n+\t\t/*\n+\t\t * If we fix up parent data, we currently cannot\n+\t\t * do that on-the-fly.\n+\t\t */\n+\t\tif (revs->parents)\n+\t\t\trevs->limited = 1;\n \t}\n \n \treturn left;\n@@ -763,23 +769,32 @@ struct commit *get_revision(struct rev_i\n \n \tdo {\n \t\tstruct commit *commit = revs->commits->item;\n+\n+\t\trevs->commits = revs->commits->next;\n \n+\t\t/*\n+\t\t * If we haven't done the list limiting, we need to look at\n+\t\t * the parents here\n+\t\t */\n+\t\tif (!revs->limited)\n+\t\t\tadd_parents_to_list(revs, commit, &revs->commits);\n \t\tif (commit->object.flags & SHOWN)\n-\t\t\tgoto next;\n+\t\t\tcontinue;\n \t\tif (!(commit->object.flags & BOUNDARY) &&\n \t\t (commit->object.flags & UNINTERESTING))\n-\t\t\tgoto next;\n+\t\t\tcontinue;\n \t\tif (revs->min_age != -1 && (commit->date > revs->min_age))\n-\t\t\tgoto next;\n+\t\t\tcontinue;\n \t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n \t\t\treturn NULL;\n \t\tif (revs->no_merges &&\n \t\t commit->parents && commit->parents->next)\n-\t\t\tgoto 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\tgoto next;\n-\t\t\trewrite_parents(commit);\n+\t\t\t\tcontinue;\n+\t\t\tif (revs->parents)\n+\t\t\t\trewrite_parents(commit);\n \t\t}\n \t\t/* More to go? */\n \t\tif (revs->max_count) {\n@@ -792,13 +807,9 @@ struct commit *get_revision(struct rev_i\n \t\t\t\trevs->commits = it->next;\n \t\t\t\tfree(it);\n \t\t\t}\n-\t\t\telse\n-\t\t\t\tpop_most_recent_commit(&revs->commits, SEEN);\n \t\t}\n \t\tcommit->object.flags |= SHOWN;\n \t\treturn commit;\n-next:\n-\t\tpop_most_recent_commit(&revs->commits, SEEN);\n \t} while (revs->commits);\n \treturn NULL;\n }\n"},{"id":"18167","messageId":"Pine.LNX.4.64.0603302153350.27203@g5.osdl.org","threadId":"3763","inReplyTo":"Pine.LNX.4.64.0603301652531.27203@g5.osdl.org","subject":"Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-03-31T06:05:26Z","receivedAt":"2006-03-31T06:05:26Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 30 Mar 2006, Linus Torvalds wrote:\n> \n> This makes git-rev-list able to do path-limiting without having to parse\n> all of history before it starts showing the results.\n> \n> This makes it things like \"git log -- pathname\" much more pleasant to use.\n\nSadly, it seems to react really badly to Junio's new --boundary logic for \nsome reason that I haven't quite figured out yet.\n\nThat reaction is independent of the actual pathname restriction, and seems \nto be related to how the --boundary logic expected \npop_most_recent_commit() to work. In particular:\n\n\t...\n if (commit->object.flags & BOUNDARY) {\n /* this is already uninteresting,\n * so there is no point popping its\n * parents into the list.\n */\n\nthat code is magic, and seems to depend on something subtle going on with \nthe list, and the incremental thing already popped the parent earlier and \nscrewed up whatever magic that the BOUNDARY code depends on.\n\nJunio? I think you did some funky special case with BOUNDARY commits, and \nI broke it for you, can you look at the patch and see if you can see it? \nI'd really like to have the incremental path-limiter, because it really \nmakes a huge difference in the usability of \"git log pathname\".\n\n\t\tLinus\n"},{"id":"18168","messageId":"7vr74jxhp3.fsf@assigned-by-dhcp.cox.net","threadId":"3763","inReplyTo":"Pine.LNX.4.64.0603301652531.27203@g5.osdl.org","subject":"Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-31T06:16:08Z","receivedAt":"2006-03-31T06:16:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> The reason I put \"RFC\" in the subject line is that while I've validated it \n> various ways, like doing\n>\n> \tgit-rev-list HEAD -- drivers/char/ | md5sum \n>\n> before-and-after on the kernel archive, it's \"git-rev-list\" after all. In \n> other words, it's that really really subtle and complex central piece of \n> software. So while I think this is important and should go in asap, I also \n> think it should get lots of testing and eyeballs looking at the code.\n\nLet me make sure I understand what is going on.\n\nCurrently, limit_list(), which is called when revs->limited is\nset, serves two different purposes:\n\n * traverse the ancestry chain and mark ancestors of\n uninteresting commits also uninteresting;\n\n * \"simplify\" each commit that is still interesting, by\n comparing the tree object of it and its parents.\n\nWe used to call limit_list() when we are limiting the commits by\npaths, because that was what the latter does as a side effect of\nadd_parents_to_list(). You made it streamable by moving the\ncall to get_revision() and not calling limit_list() when you do\nnot have other reasons to call it.\n\nYou currently do not do this streaming optimization when you\nhave to show simplified parents, because the streaming version\ntraverses ancestry chain one step at a time as it goes, and you\ncannot obviously see who the final \"fake\" parent is until you\nsimplify the parents enouogh.\n\nNow, I have some observations, not necessarily limited to this\npatch but also to the code from the \"master\" version.\n\n * get_commit_reference() sets \"limited\" when the user gives ^exclude\n commit. no question about this --- we would need to see the\n reachability in this case.\n\n * when we are going to sort the result we are going to show,\n we set \"limited\" -- we cannot sort without knowing the set we\n are sorting.\n\n * giving commit timestamp limits via --max-age, --min-age\n etc. sets \"limited\". I have doubts about this. We currently\n look at the commit timestamp in limit_list() and mark a\n commit old enough to be \"uninteresting\" -- which makes\n ancestor of such commit also uninteresting. Similarly\n commits that are too young are not pushed into the result.\n\n There is a filter in get_revision() to omit ineligible\n commits by time range already, so I wonder if we can remove\n that code from limit_list() and not set \"limited\" when these\n timestamp ranges are given. This would let us stream even\n more.\n\n Admittably, ancestors of an old commit had better be even\n older, so marking them uninteresting to stop the traversal\n early is a good way to optimize the full-traversal case, but\n not having to call limit_list() would help streaming usage.\n\n Also I have doubts about correctness issue about the max-age\n handling. Is it correct to mark a commit that is old enough\n to be uninteresting, implying that its ancestors are all\n uninteresting? With clock skew among people, it is possible\n to make a merge commit that has parents one of which is \"in\n the future\".\n\n * As to handling of --unpacked, exactly the same comment as\n \"max-age\" applies, but it might be even worse. Marking an\n unpacked one \"uninteresting\" means if a packed commit refers\n to loose commit, the loose one will be also marked\n uninteresting, I think.\n\n\"--objects --unpacked\" is an idiom to repack objects that are\nstill loose, but I suspect it would do interesting thing if the\ncommit is in a pack but its trees and blobs are still loose. Am\nI confused, or have I just spotted a potential (but hard to\ntrigger) bug?\n\n\n--\ndiff --git a/revision.c b/revision.c\nindex 0e3f074..a55c0d1 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -404,10 +404,6 @@ static void limit_list(struct rev_info *\n \t\tlist = list->next;\n \t\tfree(entry);\n \n-\t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n-\t\t\tobj->flags |= UNINTERESTING;\n-\t\tif (revs->unpacked && has_sha1_pack(obj->sha1))\n-\t\t\tobj->flags |= UNINTERESTING;\n \t\tadd_parents_to_list(revs, commit, &list);\n \t\tif (obj->flags & UNINTERESTING) {\n \t\t\tmark_parents_uninteresting(commit);\n@@ -415,8 +411,6 @@ static void limit_list(struct rev_info *\n \t\t\t\tbreak;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (revs->min_age != -1 && (commit->date > revs->min_age))\n-\t\t\tcontinue;\n \t\tp = &commit_list_insert(commit, p)->next;\n \t}\n \tif (revs->boundary) {\n@@ -543,32 +537,26 @@ int setup_revisions(int argc, const char\n \t\t\t}\n \t\t\tif (!strncmp(arg, \"--max-age=\", 10)) {\n \t\t\t\trevs->max_age = atoi(arg + 10);\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strncmp(arg, \"--min-age=\", 10)) {\n \t\t\t\trevs->min_age = atoi(arg + 10);\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strncmp(arg, \"--since=\", 8)) {\n \t\t\t\trevs->max_age = approxidate(arg + 8);\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strncmp(arg, \"--after=\", 8)) {\n \t\t\t\trevs->max_age = approxidate(arg + 8);\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strncmp(arg, \"--before=\", 9)) {\n \t\t\t\trevs->min_age = approxidate(arg + 9);\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strncmp(arg, \"--until=\", 8)) {\n \t\t\t\trevs->min_age = approxidate(arg + 8);\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--all\")) {\n@@ -635,7 +623,6 @@ int setup_revisions(int argc, const char\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--unpacked\")) {\n \t\t\t\trevs->unpacked = 1;\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\t*unrecognized++ = arg;\n@@ -786,7 +773,9 @@ struct commit *get_revision(struct rev_i\n \t\tif (revs->min_age != -1 && (commit->date > revs->min_age))\n \t\t\tcontinue;\n \t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n-\t\t\treturn NULL;\n+\t\t\tcontinue;\n+\t\tif (revs->unpacked && has_sha1_pack(commit->object.sha1))\n+\t\t\tcontinue;\n \t\tif (revs->no_merges &&\n \t\t commit->parents && commit->parents->next)\n \t\t\tcontinue;\n"},{"id":"18170","messageId":"Pine.LNX.4.64.0603302225160.27203@g5.osdl.org","threadId":"3763","inReplyTo":"7vr74jxhp3.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-03-31T06:42:26Z","receivedAt":"2006-03-31T06:42:26Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 30 Mar 2006, Junio C Hamano wrote:\n>\n> Linus Torvalds writes:\n> \n> > The reason I put \"RFC\" in the subject line is that while I've validated it \n> > various ways, like doing\n> >\n> > \tgit-rev-list HEAD -- drivers/char/ | md5sum \n> >\n> > before-and-after on the kernel archive, it's \"git-rev-list\" after all. In \n> > other words, it's that really really subtle and complex central piece of \n> > software. So while I think this is important and should go in asap, I also \n> > think it should get lots of testing and eyeballs looking at the code.\n> \n> Let me make sure I understand what is going on.\n> \n> Currently, limit_list(), which is called when revs->limited is\n> set, serves two different purposes:\n> \n> * traverse the ancestry chain and mark ancestors of\n> uninteresting commits also uninteresting;\n\nRight.\n\nWe _also_ traverse the ancestry chain in the \"walking\" stage later in \nget_revision(), but if we have a \"limit\" case, we'll pre-traverse \neverything early in limit_list.\n\n> * \"simplify\" each commit that is still interesting, by\n> comparing the tree object of it and its parents.\n\nThrough \"add_parent_to_list()\", yes. \n\n> We used to call limit_list() when we are limiting the commits by\n> paths, because that was what the latter does as a side effect of\n> add_parents_to_list(). You made it streamable by moving the\n> call to get_revision() and not calling limit_list() when you do\n> not have other reasons to call it.\n\nYes, and by using \"add_parent_to_list()\" instead of the \nnon-pathname-limit-aware \"pop_most_recent_commit()\".\n\nSo we don't call limit-list any more, because we do the same thing in \nget_revision() that it used to do at limit time.\n\n> You currently do not do this streaming optimization when you\n> have to show simplified parents, because the streaming version\n> traverses ancestry chain one step at a time as it goes, and you\n> cannot obviously see who the final \"fake\" parent is until you\n> simplify the parents enouogh.\n\nYes. For exactly the same reason some other things ause us to do \nlimit_list(): some ops simply need the fully connected end result in order \nto be able to work correctly.\n\n> * get_commit_reference() sets \"limited\" when the user gives ^exclude\n> commit. no question about this --- we would need to see the\n> reachability in this case.\n\nRight.\n\n> * when we are going to sort the result we are going to show,\n> we set \"limited\" -- we cannot sort without knowing the set we\n> are sorting.\n\nRight.\n\n> * giving commit timestamp limits via --max-age, --min-age\n> etc. sets \"limited\". I have doubts about this.\n\nI agree. I looked at it when I did the \"rev-list.c\" vs \"revision.c\" split, \nbut I wanted to do as few changes as possible, and for some reason we've \nalways considered \"time\" to cause us to limit things.\n\nHowever, I think your patch is wrong. Even if you don't limit things when \nwe have a date specifier, you still want to stop doing traversal in \nlimit_list when you hit that date. Why? Because it might be limited for \nsome _other_ reason, eg due to --topo-order or or some other issue.\n\n> * As to handling of --unpacked, exactly the same comment as\n> \"max-age\" applies, but it might be even worse. Marking an\n> unpacked one \"uninteresting\" means if a packed commit refers\n> to loose commit, the loose one will be also marked\n> uninteresting, I think.\n> \n> \"--objects --unpacked\" is an idiom to repack objects that are\n> still loose, but I suspect it would do interesting thing if the\n> commit is in a pack but its trees and blobs are still loose. Am\n> I confused, or have I just spotted a potential (but hard to\n> trigger) bug?\n\nRegardless of where you do the \"unpacked\" test, it will always really end \nup stopping when it hits a packed commit. So you won't ever get away from \nthat.\n\nSo same logic applies. Once you hit a packed commit, you should stop, both \nin limit_list _and_ in get_revision(). Otherwise you'll do too much work \nwhen trying to limit things, for no gain.\n\nThe fact is, \"--unpacked\" means that we traverse the part of the chain \nthat hasn't been packed yet. Anything that is packed automatically cuts \noff parsing, whether there is anything else unpacked below it or not. It's \nnot a bug, it's a feature, and if you want to pack everything, you should \njust do\n\n\tgit repack -a -d\n\nand not use --unpacked.\n\nSame goes for dates, btw. We've always stopped when we reached a certain \ndate, even though there _could_ be commits before it that are from a more \nrecent date (due to clocks being set wrong). Neither --unpacked nor \n--since=xyzzy are meant to be any kind of \"guarantees\" that you get all \ncommits that match some pattern. They are just \"stop early\" rules.\n\n\t\tLinus\n\nSo the following is wrong:\n\n> diff --git a/revision.c b/revision.c\n> index 0e3f074..a55c0d1 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -404,10 +404,6 @@ static void limit_list(struct rev_info *\n> \t\tlist = list->next;\n> \t\tfree(entry);\n> \n> -\t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n> -\t\t\tobj->flags |= UNINTERESTING;\n> -\t\tif (revs->unpacked && has_sha1_pack(obj->sha1))\n> -\t\t\tobj->flags |= UNINTERESTING;\n> \t\tadd_parents_to_list(revs, commit, &list);\n> \t\tif (obj->flags & UNINTERESTING) {\n> \t\t\tmark_parents_uninteresting(commit);\n> @@ -415,8 +411,6 @@ static void limit_list(struct rev_info *\n> \t\t\t\tbreak;\n> \t\t\tcontinue;\n> \t\t}\n> -\t\tif (revs->min_age != -1 && (commit->date > revs->min_age))\n> -\t\t\tcontinue;\n> \t\tp = &commit_list_insert(commit, p)->next;\n> \t}\n> \tif (revs->boundary) {\n\n..but the later part of the patch looks fine (modulo testing, of course):\n\n> @@ -543,32 +537,26 @@ int setup_revisions(int argc, const char\n> \t\t\t}\n> \t\t\tif (!strncmp(arg, \"--max-age=\", 10)) {\n> \t\t\t\trevs->max_age = atoi(arg + 10);\n> -\t\t\t\trevs->limited = 1;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strncmp(arg, \"--min-age=\", 10)) {\n> \t\t\t\trevs->min_age = atoi(arg + 10);\n> -\t\t\t\trevs->limited = 1;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strncmp(arg, \"--since=\", 8)) {\n> \t\t\t\trevs->max_age = approxidate(arg + 8);\n> -\t\t\t\trevs->limited = 1;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strncmp(arg, \"--after=\", 8)) {\n> \t\t\t\trevs->max_age = approxidate(arg + 8);\n> -\t\t\t\trevs->limited = 1;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strncmp(arg, \"--before=\", 9)) {\n> \t\t\t\trevs->min_age = approxidate(arg + 9);\n> -\t\t\t\trevs->limited = 1;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strncmp(arg, \"--until=\", 8)) {\n> \t\t\t\trevs->min_age = approxidate(arg + 8);\n> -\t\t\t\trevs->limited = 1;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strcmp(arg, \"--all\")) {\n> @@ -635,7 +623,6 @@ int setup_revisions(int argc, const char\n> \t\t\t}\n> \t\t\tif (!strcmp(arg, \"--unpacked\")) {\n> \t\t\t\trevs->unpacked = 1;\n> -\t\t\t\trevs->limited = 1;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\t*unrecognized++ = arg;\n> @@ -786,7 +773,9 @@ struct commit *get_revision(struct rev_i\n> \t\tif (revs->min_age != -1 && (commit->date > revs->min_age))\n> \t\t\tcontinue;\n> \t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n> -\t\t\treturn NULL;\n> +\t\t\tcontinue;\n> +\t\tif (revs->unpacked && has_sha1_pack(commit->object.sha1))\n> +\t\t\tcontinue;\n> \t\tif (revs->no_merges &&\n> \t\t commit->parents && commit->parents->next)\n> \t\t\tcontinue;\n> \n"},{"id":"18171","messageId":"7v3bgzxgbg.fsf@assigned-by-dhcp.cox.net","threadId":"3763","inReplyTo":"Pine.LNX.4.64.0603302153350.27203@g5.osdl.org","subject":"Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-31T06:45:55Z","receivedAt":"2006-03-31T06:45:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Sadly, it seems to react really badly to Junio's new --boundary logic for \n> some reason that I haven't quite figured out yet.\n\nThere already was a report that --boundary stuff is not quite\nright, so what you are seeing might be that the new code exposes\nits original breakage even more. I haven't looked into the\nbreakage of the original version yet either, so I cannot really\nsay how your change breaks it.\n\n> That reaction is independent of the actual pathname restriction, and seems \n> to be related to how the --boundary logic expected \n> pop_most_recent_commit() to work. In particular:\n>\n> \t...\n> if (commit->object.flags & BOUNDARY) {\n> /* this is already uninteresting,\n> * so there is no point popping its\n> * parents into the list.\n> */\n>\n> that code is magic, and seems to depend on something subtle going on with \n> the list, and the incremental thing already popped the parent earlier and \n> screwed up whatever magic that the BOUNDARY code depends on.\n\nThis was not so magic, but the magic was actually in the code\nadded to limit_list(). Usually, \"newlist\" consists interesting\ncommits, and what are found interesting initially but marked as\nuninteresting when a different ancestry chain coming from an\nuninteresting head leading to it was later discovered. The\nmagic code looks at still-interesting commits, and re-injects\nits parents that are uninteresting to the list (and I just\nspotted a bug there -- it does not check if what is being \"re-\"\ninjected are already on the list -- should I check for SEEN flag\nthere perhaps?), while marking them as boundary. This was done\nto make sure that all the open-circle (in gitk) commits are on\nthe resulting list.\n\nThe part of the code you quoted is just a short-cut for not\ndoing pop_most_recent_commit() -- we used to have\npop_most_recent_commit() there, which pushed the parents of the\ncommit being processed into the list. Because we are processing\na boundary commit, we know it is uninteresting -- and by\ndefinition, its parents are uninteresting and that is why it\njust advances the list without calling pop_most_recent_commit(),\nbypassing its side effect to push its parents into the list.\n\nSince the new code has already advanced the list immediately\nafter the beginning of do {} block, I think you can remove the\nentire \"if (revs->max_count) {}\" block. As the code currently\nstands, you are skipping what happens to be next to the boundary\ncommit on the result list.\n\n> Junio? I think you did some funky special case with BOUNDARY commits, and \n> I broke it for you, can you look at the patch and see if you can see it? \n> I'd really like to have the incremental path-limiter, because it really \n> makes a huge difference in the usability of \"git log pathname\".\n\nOh, there is no question about making it streamable in more\ncases is a good thing.\n"},{"id":"18172","messageId":"7vr74jw0zj.fsf@assigned-by-dhcp.cox.net","threadId":"3763","inReplyTo":"Pine.LNX.4.64.0603302225160.27203@g5.osdl.org","subject":"Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-31T07:02:24Z","receivedAt":"2006-03-31T07:02:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> So the following is wrong:\n>\n>> diff --git a/revision.c b/revision.c\n>> index 0e3f074..a55c0d1 100644\n>> --- a/revision.c\n>> +++ b/revision.c\n>> @@ -404,10 +404,6 @@ static void limit_list(struct rev_info *\n>> \t\tlist = list->next;\n>> \t\tfree(entry);\n>> \n>> -\t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n>> -\t\t\tobj->flags |= UNINTERESTING;\n>> -\t\tif (revs->unpacked && has_sha1_pack(obj->sha1))\n>> -\t\t\tobj->flags |= UNINTERESTING;\n>> \t\tadd_parents_to_list(revs, commit, &list);\n>> \t\tif (obj->flags & UNINTERESTING) {\n>> \t\t\tmark_parents_uninteresting(commit);\n>...\n> ..but the later part of the patch looks fine (modulo testing, of course):\n>\n>> @@ -786,7 +773,9 @@ struct commit *get_revision(struct rev_i\n>> \t\tif (revs->min_age != -1 && (commit->date > revs->min_age))\n>> \t\t\tcontinue;\n>> \t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n>> -\t\t\treturn NULL;\n>> +\t\t\tcontinue;\n>> +\t\tif (revs->unpacked && has_sha1_pack(commit->object.sha1))\n>> +\t\t\tcontinue;\n\nOK, so let's say I agree with you that --unpacked and --since\nare \"stop early\" rules. I fully agree with that from usability\nand implementation point of view, but I now wonder if the\n\"output filter\" in get_revision() and \"stop early\" in limit_list()\nwould do the same thing. That is, if we are _otherwise_\nlimited, limit_list() would use them for \"stop early\" rule, but\nif we end up not calling limit_list() we would simply filter\nthem and keep going (which is what my patch did for both\ntimestamp and packedness), or we could also stop there.\n\nI am not sure which one is correct, but I suspect whichever we\ndo in get_revision() we would get inconsistent results between a\ncase where we call limit_list() and we do not. That is, the set\nof commits we are going to show when --(topo|date)-order is\ngiven and not given can be different. Is this acceptable?\n\nI would say, from the implementation point of view, it should be\ntolerated, but...\n"},{"id":"18174","messageId":"7v1wwjvwz9.fsf@assigned-by-dhcp.cox.net","threadId":"3763","inReplyTo":"Pine.LNX.4.64.0603301652531.27203@g5.osdl.org","subject":"Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-31T08:28:58Z","receivedAt":"2006-03-31T08:28:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> This is an absolutely huge deal for anything like \"git log -- \", \n> but also for some things that we don't do yet - like the \"find where \n> things changed\" logic I've described elsewhere, where we want to find the \n> previous revision that changed a file.\n>...\n> Btw, don't even bother testing this with the git archive. git itself is so \n> small that parsing the whole revision history for it takes about a second \n> even with path limiting.\n\nBy the way, I forgot to praise you ;-). \n\nEven on a fast machine, the old one was not very useful, but\nthis one is _instantaneous_. Very good job.\n\n$ PAGER=cat GIT_DIR=/pub/scm/linux/kernel/git/torvalds/linux-2.6.git/ \\\n /usr/bin/time git log -1 --pretty=short -- drivers/\ncommit ce362c009250340358a7221f3cdb7954cbf19c01\nMerge: 064c94f... cd7a920...\nAuthor: Linus Torvalds \n\n Merge git://git.kernel.org/pub/scm/linux/kernel/git/kyle/parisc-2.6\n\n15.44user 0.19system 0:25.11elapsed 62%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+18050minor)pagefaults 0swaps\n\n$ PAGER=cat GIT_DIR=/pub/scm/linux/kernel/git/torvalds/linux-2.6.git/ \\\n /usr/bin/time ./git.pu log -1 --pretty=short -- drivers/\ncommit ce362c009250340358a7221f3cdb7954cbf19c01\nMerge: 064c94f... cd7a920...\nAuthor: Linus Torvalds \n\n Merge git://git.kernel.org/pub/scm/linux/kernel/git/kyle/parisc-2.6\n\n0.00user 0.00system 0:00.00elapsed 50%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+388minor)pagefaults 0swaps\n"},{"id":"18180","messageId":"Pine.LNX.4.64.0603311135290.27203@g5.osdl.org","threadId":"3763","inReplyTo":"7v3bgzxgbg.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-03-31T19:39:26Z","receivedAt":"2006-03-31T19:39:26Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 30 Mar 2006, Junio C Hamano wrote:\n> \n> There already was a report that --boundary stuff is not quite\n> right, so what you are seeing might be that the new code exposes\n> its original breakage even more. I haven't looked into the\n> breakage of the original version yet either, so I cannot really\n> say how your change breaks it.\n\n[ Awake and thinking about this again ]\n\nNo, I think the new breakage was just because I was being a stupid ass.\n\nWhen I converted get_revision() to do the parent parsing up at the top \ninstead of at the bottom, I just didn't think correctly about your new \nBOUNDARY code, and my conversion for that was just wrong. Part of the \"do \nthe parents early\" code was also removing the current commit early, so \nsuddenly your BOUNDARY case for doing that thing was just removing some \nother random commit instead, which was obviously wrong.\n\nThe fix is trivial - with the new get_revision() organization, the \nBOUNDARY case special-case actually goes away entirely, and this trivial \npatch (on top of my 2/2 patch) should just fix it.\n\nAt least it passes my tests again now, and looking at the code everything \nseems sane.\n\n\t\tLinus\n---\ndiff --git a/revision.c b/revision.c\nindex 0e3f074..753633e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -796,18 +796,6 @@ struct commit *get_revision(struct rev_i\n \t\t\tif (revs->parents)\n \t\t\t\trewrite_parents(commit);\n \t\t}\n-\t\t/* More to go? */\n-\t\tif (revs->max_count) {\n-\t\t\tif (commit->object.flags & BOUNDARY) {\n-\t\t\t\t/* this is already uninteresting,\n-\t\t\t\t * so there is no point popping its\n-\t\t\t\t * parents into the list.\n-\t\t\t\t */\n-\t\t\t\tstruct commit_list *it = revs->commits;\n-\t\t\t\trevs->commits = it->next;\n-\t\t\t\tfree(it);\n-\t\t\t}\n-\t\t}\n \t\tcommit->object.flags |= SHOWN;\n \t\treturn commit;\n \t} while (revs->commits);\n"},{"id":"18181","messageId":"Pine.LNX.4.64.0603311139540.27203@g5.osdl.org","threadId":"3763","inReplyTo":"7v1wwjvwz9.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-03-31T19:44:51Z","receivedAt":"2006-03-31T19:44:51Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 31 Mar 2006, Junio C Hamano wrote:\n\n> Linus Torvalds writes:\n> \n> > This is an absolutely huge deal for anything like \"git log -- \", \n> > but also for some things that we don't do yet - like the \"find where \n> > things changed\" logic I've described elsewhere, where we want to find the \n> > previous revision that changed a file.\n> >...\n> > Btw, don't even bother testing this with the git archive. git itself is so \n> > small that parsing the whole revision history for it takes about a second \n> > even with path limiting.\n> \n> By the way, I forgot to praise you ;-). \n> \n> Even on a fast machine, the old one was not very useful, but\n> this one is _instantaneous_. Very good job.\n\nIndeed. It's why I'd really like this to be merged before 1.3.0 - it moves \na certain class of problems from \"it works\" to \"it's actually usable\".\n\nNow, the _real_ usage I foresee (which just wasn't practical before) is \nthe interactive annotation thing - this won't help a _full_file_ annotate \n(which usually needs to go back to the very first version of a file \nanyway), but it should make it possible to play with an incremental one \n(the \"graphical git-whatchanged\" kind).\n\nBut even just the \"git log\" difference makes it worth it. \n\n\t\tLinus\n"},{"id":"18182","messageId":"7vfykyuzc2.fsf@assigned-by-dhcp.cox.net","threadId":"3763","inReplyTo":"Pine.LNX.4.64.0603311135290.27203@g5.osdl.org","subject":"Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-31T20:35:41Z","receivedAt":"2006-03-31T20:35:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> The fix is trivial - with the new get_revision() organization, the \n> BOUNDARY case special-case actually goes away entirely, and this trivial \n> patch (on top of my 2/2 patch) should just fix it.\n\nYes, that is exactly the fix I have in \"pu\" -- I suspect you\nreplied before getting to my response last night.\n"},{"id":"18183","messageId":"Pine.LNX.4.64.0603311249550.27203@g5.osdl.org","threadId":"3763","inReplyTo":"7vfykyuzc2.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-03-31T20:50:47Z","receivedAt":"2006-03-31T20:50:47Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 31 Mar 2006, Junio C Hamano wrote:\n> \n> Yes, that is exactly the fix I have in \"pu\" -- I suspect you\n> replied before getting to my response last night.\n\nNo, I just get too much email, and hadn't read your response carefully \nenough, so I just missed the fact that you had already found the problem. \n\n\t\t\tLinus\n"},{"id":"18203","messageId":"Pine.LNX.4.64.0604011628500.3684@g5.osdl.org","threadId":"3763","inReplyTo":"7vr74jw0zj.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-02T00:35:06Z","receivedAt":"2006-04-02T00:35:06Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 30 Mar 2006, Junio C Hamano wrote:\n> \n> OK, so let's say I agree with you that --unpacked and --since\n> are \"stop early\" rules. I fully agree with that from usability\n> and implementation point of view, but I now wonder if the\n> \"output filter\" in get_revision() and \"stop early\" in limit_list()\n> would do the same thing.\n\nThey don't.\n\nWhat ends up not working very well at all is the combination of \n\"--topo-order\" and the output filter in get_revision. It will return NULL \nwhen we see the first commit out of date-order, even if we have other \ncommits coming.\n\nSo we really should do the \"past the date order\" thing in get_revision() \nonly if we have _not_ done it already in limit_list().\n\nSomething like this.\n\nThe easiest way to test this is with just\n\n\tgitk --since=3.days.ago\n\non the kernel tree. Without this patch, it tends to be pretty obviously \nbroken.\n\nSigned-off-by: Linus Torvalds \n\n\t\tLinus\n\n---\ndiff --git a/revision.c b/revision.c\nindex a8a54b6..558ed01 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -783,10 +783,14 @@ struct commit *get_revision(struct rev_i\n \n \t\t/*\n \t\t * If we haven't done the list limiting, we need to look at\n-\t\t * the parents here\n+\t\t * the parents here. We also need to do the date-based limiting\n+\t\t * that we'd otherwise have done in limit_list().\n \t\t */\n-\t\tif (!revs->limited)\n+\t\tif (!revs->limited) {\n+\t\t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n+\t\t\t\tcontinue;\n \t\t\tadd_parents_to_list(revs, commit, &revs->commits);\n+\t\t}\n \t\tif (commit->object.flags & SHOWN)\n \t\t\tcontinue;\n \t\tif (!(commit->object.flags & BOUNDARY) &&\n@@ -794,8 +798,6 @@ struct commit *get_revision(struct rev_i\n \t\t\tcontinue;\n \t\tif (revs->min_age != -1 && (commit->date > revs->min_age))\n \t\t\tcontinue;\n-\t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n-\t\t\treturn NULL;\n \t\tif (revs->no_merges &&\n \t\t commit->parents && commit->parents->next)\n \t\t\tcontinue;\n"},{"id":"18206","messageId":"7vacb4oene.fsf@assigned-by-dhcp.cox.net","threadId":"3763","inReplyTo":"Pine.LNX.4.64.0604011628500.3684@g5.osdl.org","subject":"Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-02T03:11:17Z","receivedAt":"2006-04-02T03:11:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> What ends up not working very well at all is the combination of \n> \"--topo-order\" and the output filter in get_revision. It will return NULL \n> when we see the first commit out of date-order, even if we have other \n> commits coming.\n>\n> So we really should do the \"past the date order\" thing in get_revision() \n> only if we have _not_ done it already in limit_list().\n\nRight now --max-age causes \"limited\" so there should not be a\ndifference, if I am not mistaken. But I've been meaning to\nchange that, so the patch makes sense. Similarly,...\n\n-- >8 --\n[PATCH] revision: --topo-order and --unpacked\n\nNow, using --unpacked without limit_list() does not make much\nsense, but this is parallel to the earlier --max-age fix.\n\nSigned-off-by: Junio C Hamano \n\n---\n\n revision.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\n22c31bf183bff576c7807f9d67abfc11ee8e1fa4\ndiff --git a/revision.c b/revision.c\nindex 558ed01..07cc86f 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -787,7 +787,10 @@ struct commit *get_revision(struct rev_i\n \t\t * that we'd otherwise have done in limit_list().\n \t\t */\n \t\tif (!revs->limited) {\n-\t\t\tif (revs->max_age != -1 && (commit->date < revs->max_age))\n+\t\t\tif ((revs->unpacked &&\n+\t\t\t has_sha1_pack(commit->object.sha1)) ||\n+\t\t\t (revs->max_age != -1 &&\n+\t\t\t (commit->date < revs->max_age)))\n \t\t\t\tcontinue;\n \t\t\tadd_parents_to_list(revs, commit, &revs->commits);\n \t\t}\n-- \n1.3.0.rc1.gf385\n"},{"id":"18207","messageId":"7vmzf4mzsv.fsf@assigned-by-dhcp.cox.net","threadId":"3763","inReplyTo":"Pine.LNX.4.64.0604011628500.3684@g5.osdl.org","subject":"[PATCH] revision: simplify argument parsing.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-02T03:17:20Z","receivedAt":"2006-04-02T03:17:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This just moves code around to consolidate the part that sets\nrevs->limited to one place based on various flags.\n\nSigned-off-by: Junio C Hamano \n\n---\n * Just the preparation for the next step...\n\n revision.c | 20 +++++++-------------\n 1 files changed, 7 insertions(+), 13 deletions(-)\n\n53069686601d156dea3787a100ffc4e35c78040f\ndiff --git a/revision.c b/revision.c\nindex 07cc86f..cdd29c5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -552,32 +552,26 @@ int setup_revisions(int argc, const char\n \t\t\t}\n \t\t\tif (!strncmp(arg, \"--max-age=\", 10)) {\n \t\t\t\trevs->max_age = atoi(arg + 10);\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (!strncmp(arg, \"--min-age=\", 10)) {\n-\t\t\t\trevs->min_age = atoi(arg + 10);\n-\t\t\t\trevs->limited = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n \t\t\tif (!strncmp(arg, \"--since=\", 8)) {\n \t\t\t\trevs->max_age = approxidate(arg + 8);\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strncmp(arg, \"--after=\", 8)) {\n \t\t\t\trevs->max_age = approxidate(arg + 8);\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strncmp(arg, \"--min-age=\", 10)) {\n+\t\t\t\trevs->min_age = atoi(arg + 10);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!strncmp(arg, \"--before=\", 9)) {\n \t\t\t\trevs->min_age = approxidate(arg + 9);\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strncmp(arg, \"--until=\", 8)) {\n \t\t\t\trevs->min_age = approxidate(arg + 8);\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--all\")) {\n@@ -596,13 +590,11 @@ int setup_revisions(int argc, const char\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--topo-order\")) {\n \t\t\t\trevs->topo_order = 1;\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--date-order\")) {\n \t\t\t\trevs->lifo = 0;\n \t\t\t\trevs->topo_order = 1;\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--parents\")) {\n@@ -644,7 +636,6 @@ int setup_revisions(int argc, const char\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--unpacked\")) {\n \t\t\t\trevs->unpacked = 1;\n-\t\t\t\trevs->limited = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\t*unrecognized++ = arg;\n@@ -707,6 +698,9 @@ int setup_revisions(int argc, const char\n \t\tcommit = get_commit_reference(revs, def, sha1, 0);\n \t\tadd_one_commit(commit, revs);\n \t}\n+\n+\tif ((revs->max_age != -1) || revs->topo_order || revs->unpacked)\n+\t\trevs->limited = 1;\n \n \tif (revs->prune_data) {\n \t\tdiff_tree_setup_paths(revs->prune_data);\n-- \n1.3.0.rc1.gf385\n"},{"id":"18208","messageId":"7vhd5cmzsn.fsf@assigned-by-dhcp.cox.net","threadId":"3763","inReplyTo":"Pine.LNX.4.64.0604011628500.3684@g5.osdl.org","subject":"[PATCH] revision: --max-age alone does not need limit_list() anymore.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-02T03:17:28Z","receivedAt":"2006-04-02T03:17:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This makes git log --since=7.days to be streamable.\n\nSigned-off-by: Junio C Hamano \n\n---\n\n revision.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\nbbbc8c3a8d455e1f5d15c3764eba70250b5479e9\ndiff --git a/revision.c b/revision.c\nindex cdd29c5..728b6d1 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -699,7 +699,7 @@ int setup_revisions(int argc, const char\n \t\tadd_one_commit(commit, revs);\n \t}\n \n-\tif ((revs->max_age != -1) || revs->topo_order || revs->unpacked)\n+\tif (revs->topo_order || revs->unpacked)\n \t\trevs->limited = 1;\n \n \tif (revs->prune_data) {\n-- \n1.3.0.rc1.gf385\n"},{"id":"18263","messageId":"7vpsjzguff.fsf@assigned-by-dhcp.cox.net","threadId":"3763","inReplyTo":"443063E2.1040904@lsrfire.ath.cx","subject":"Re: [PATCH] revision: simplify argument parsing.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-03T04:22:12Z","receivedAt":"2006-04-03T04:22:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rene Scharfe writes:\n\n> The comment is misleading because the patch not only moves stuff around,\n> it also removes setting of revs->limited when revs->min_age has been\n> set.\n\nYou are correct that the patch does more than it claims to.\n\nThat is an independent fix mentioned in my earlier message\n<7vr74jxhp3.fsf@assigned-by-dhcp.cox.net>. We did not have to\npay the overhead of having to call limit_list() when only\nmin_age was specified because we did not do traversal filtering\nusing UNINTERESTING logic for min_age, but we called it anyway.\nUnlike --max-age that marks an old one _and_ its parents\nuninteresting, it needs to filter a young one without making its\nparents uninteresting.\n"}]}