{"thread":{"id":"43231","subject":"Re: [RFC] Possible optimization for gitweb","startedAt":"2006-12-19T20:54:22Z","lastAt":"2006-12-20T14:59:48Z","messageCount":8,"participants":["Robert Fitzsimons","Junio C Hamano","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"294940","messageId":"20061219205422.GA17864@localhost","threadId":"43231","inReplyTo":null,"subject":"[RFC] Possible optimization for gitweb","fromName":"Robert Fitzsimons","fromEmail":"robfitz@273k.net","sentAt":"2006-12-19T20:54:22Z","receivedAt":"2006-12-19T20:54:22Z","isPatch":false,"sender":{"key":"robfitz@273k.net","avatar":null},"body":"While looking at the gitweb source yesterday, I noticed a number of\nsimilar expensive workflows used by a number of actions (summary,\nshortlog, log, rss, atom, and history).\n\nThe current workflows are:\n\tget ~100 sha1's using rev-list\n\tforeach sha1\n\t\tget/parse 1 commit using rev-list\n\t\toutput commit\n\nThe new workflows I'm proposing would be:\n\tget/parse ~100 commit's using rev-list\n\tforeach commit\n\t\toutput commit\n\nThe following simplified commands gives an idea of the git only overhead\nbetween these two workflows.\n\ntime \\\nfor r in `git-rev-list --max-count=100 HEAD --` ; \\\ndo git-rev-list --header --parents --max-count=1 $r -- ; \\\ndone > /dev/null\n\nreal    0m0.490s\nuser    0m0.224s\nsys     0m0.228s\n\ntime \\\ngit-rev-list --header --parents --max-count=100 HEAD -- > /dev/null\n\nreal    0m0.058s\nuser    0m0.008s\nsys     0m0.004s\n\nThere would seems to be a benefit from making the proposed change to\nthese workflows, when run on my machine against a clone of Linus's tree.\n\nOne issue with this change is that, gitweb is page orientated.  Page 0\nshows the first 100 items from a given hash, page 1 uses the same given\nhash but show 100 to 199 items, etc.  Using 'git-rev-list --header\n--parents' and then throwing away most of the result is very wasteful.\n\nSo I'm suggesting we add a new option to git-rev-list which will only\nstart show results once its has iterated past a given number of items.\nUsing a caret or tilde doesn't seem to return the same result.\n\nI've attached a discussion patch which adds a new option --start-count\nto git-rev-list and changed the summary and showlog actions of gitweb to\nuse this new option.\n\nI'm sure there are many improvements to this patch, comments?\n\nRobert\n\n-----\n\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 4059894..a1e0ccc 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1260,6 +1260,30 @@ sub parse_tag {\n \treturn %tag\n }\n \n+sub parse_commits {\n+\tmy $commit_id = shift;\n+\tmy $start_count = shift;\n+\tmy $max_count = shift;\n+\n+\tmy @cos;\n+\tmy @commit_lines;\n+\n+\tlocal $/ = \"\\0\";\n+\topen my $fd, \"-|\", git_cmd(), \"rev-list\",\n+\t\t\"--header\", \"--parents\", \"--start-count=$start_count\", \"--max-count=$max_count\",\n+\t\t$commit_id, \"--\"\n+\t\tor return;\n+\twhile (my $commit = <$fd>) {\n+\t\t@commit_lines = split '\\n', $commit;\n+\t\tpop @commit_lines;\n+\t\tmy %co = parse_commit(undef, \\@commit_lines);\n+\t\tpush @cos, \\%co;\n+\t}\n+\tclose $fd or return;\n+\n+\treturn @cos;\n+}\n+\n sub parse_commit {\n \tmy $commit_id = shift;\n \tmy $commit_text = shift;\n@@ -2633,29 +2657,29 @@ sub git_project_list_body {\n \n sub git_shortlog_body {\n \t# uses global variable $project\n-\tmy ($revlist, $from, $to, $refs, $extra) = @_;\n+\tmy ($commitlist, $from, $to, $refs, $extra) = @_;\n \n \t$from = 0 unless defined $from;\n-\t$to = $#{$revlist} if (!defined $to || $#{$revlist} < $to);\n+\t$to = $#{$commitlist} if (!defined $to || $#{$commitlist} < $to);\n \n \tprint \"<table class=\\\"shortlog\\\" cellspacing=\\\"0\\\">\\n\";\n \tmy $alternate = 1;\n \tfor (my $i = $from; $i <= $to; $i++) {\n-\t\tmy $commit = $revlist->[$i];\n+\t\tmy $co = $commitlist->[$i];\n+\t\tmy $commit = $co->{'id'};\n \t\t#my $ref = defined $refs ? format_ref_marker($refs, $commit) : '';\n \t\tmy $ref = format_ref_marker($refs, $commit);\n-\t\tmy %co = parse_commit($commit);\n \t\tif ($alternate) {\n \t\t\tprint \"<tr class=\\\"dark\\\">\\n\";\n \t\t} else {\n \t\t\tprint \"<tr class=\\\"light\\\">\\n\";\n \t\t}\n \t\t$alternate ^= 1;\n-\t\t# git_summary() used print \"<td><i>$co{'age_string'}</i></td>\\n\" .\n-\t\tprint \"<td title=\\\"$co{'age_string_age'}\\\"><i>$co{'age_string_date'}</i></td>\\n\" .\n-\t\t      \"<td><i>\" . esc_html(chop_str($co{'author_name'}, 10)) . \"</i></td>\\n\" .\n+\t\t# git_summary() used print \"<td><i>$co->{'age_string'}</i></td>\\n\" .\n+\t\tprint \"<td title=\\\"$co->{'age_string_age'}\\\"><i>$co->{'age_string_date'}</i></td>\\n\" .\n+\t\t      \"<td><i>\" . esc_html(chop_str($co->{'author_name'}, 10)) . \"</i></td>\\n\" .\n \t\t      \"<td>\";\n-\t\tprint format_subject_html($co{'title'}, $co{'title_short'},\n+\t\tprint format_subject_html($co->{'title'}, $co->{'title_short'},\n \t\t                          href(action=>\"commit\", hash=>$commit), $ref);\n \t\tprint \"</td>\\n\" .\n \t\t      \"<td class=\\\"link\\\">\" .\n@@ -2952,13 +2976,9 @@ sub git_summary {\n \t\t}\n \t}\n \n-\topen my $fd, \"-|\", git_cmd(), \"rev-list\", \"--max-count=17\",\n-\t\tgit_get_head_hash($project), \"--\"\n-\t\tor die_error(undef, \"Open git-rev-list failed\");\n-\tmy @revlist = map { chomp; $_ } <$fd>;\n-\tclose $fd;\n+\tmy @commitlist = parse_commits($head, 0, 17);\n \tgit_print_header_div('shortlog');\n-\tgit_shortlog_body(\\@revlist, 0, 15, $refs,\n+\tgit_shortlog_body(\\@commitlist, 0, 15, $refs,\n \t                  $cgi->a({-href => href(action=>\"shortlog\")}, \"...\"));\n \n \tif (@taglist) {\n@@ -4313,15 +4333,12 @@ sub git_shortlog {\n \t}\n \tmy $refs = git_get_references();\n \n-\tmy $limit = sprintf(\"--max-count=%i\", (100 * ($page+1)));\n-\topen my $fd, \"-|\", git_cmd(), \"rev-list\", $limit, $hash, \"--\"\n-\t\tor die_error(undef, \"Open git-rev-list failed\");\n-\tmy @revlist = map { chomp; $_ } <$fd>;\n-\tclose $fd;\n+\tmy $max_count = (100 * ($page+1));\n+\tmy @commitlist = parse_commits($hash, (100 * $page), $max_count);\n \n-\tmy $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#revlist);\n+\tmy $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $max_count);\n \tmy $next_link = '';\n-\tif ($#revlist >= (100 * ($page+1)-1)) {\n+\tif ($max_count >= 100) {\n \t\t$next_link =\n \t\t\t$cgi->a({-href => href(action=>\"shortlog\", hash=>$hash, page=>$page+1),\n \t\t\t         -title => \"Alt-n\"}, \"next\");\n@@ -4332,7 +4349,7 @@ sub git_shortlog {\n \tgit_print_page_nav('shortlog','', $hash,$hash,$hash, $paging_nav);\n \tgit_print_header_div('summary', $project);\n \n-\tgit_shortlog_body(\\@revlist, ($page * 100), $#revlist, $refs, $next_link);\n+\tgit_shortlog_body(\\@commitlist, 0, $#commitlist, $refs, $next_link);\n \n \tgit_footer_html();\n }\ndiff --git a/list-objects.c b/list-objects.c\nindex f1fa21c..d96c8bf 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -108,8 +108,12 @@ void traverse_commit_list(struct rev_info *revs,\n \tstruct object_array objects = { 0, 0, NULL };\n \n \twhile ((commit = get_revision(revs)) != NULL) {\n-\t\tprocess_tree(revs, commit->tree, &objects, NULL, \"\");\n-\t\tshow_commit(commit);\n+\t\tif (revs->start_count <= 0) {\n+\t\t\tprocess_tree(revs, commit->tree, &objects, NULL, \"\");\n+\t\t\tshow_commit(commit);\n+\t\t} else {\n+\t\t\trevs->start_count--;\n+\t\t}\n \t}\n \tfor (i = 0; i < revs->pending.nr; i++) {\n \t\tstruct object_array_entry *pending = revs->pending.objects + i;\ndiff --git a/revision.c b/revision.c\nindex 993bb66..3e3d929 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -524,6 +524,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n \trevs->prefix = prefix;\n \trevs->max_age = -1;\n \trevs->min_age = -1;\n+\trevs->start_count = -1;\n \trevs->max_count = -1;\n \n \trevs->prune_fn = NULL;\n@@ -756,6 +757,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n \t\tconst char *arg = argv[i];\n \t\tif (*arg == '-') {\n \t\t\tint opts;\n+\t\t\tif (!strncmp(arg, \"--start-count=\", 14)) {\n+\t\t\t\trevs->start_count = atoi(arg + 14);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!strncmp(arg, \"--max-count=\", 12)) {\n \t\t\t\trevs->max_count = atoi(arg + 12);\n \t\t\t\tcontinue;\ndiff --git a/revision.h b/revision.h\nindex 3adab95..c2dce8c 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -75,6 +75,7 @@ struct rev_info {\n \tstruct grep_opt\t*grep_filter;\n \n \t/* special limits */\n+\tint start_count;\n \tint max_count;\n \tunsigned long max_age;\n"},{"id":"296088","messageId":"em9mcs$moo$1@sea.gmane.org","threadId":"43231","inReplyTo":"20061219205422.GA17864@localhost","subject":"Re: [RFC] Possible optimization for gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-12-19T21:45:32Z","receivedAt":"2006-12-19T21:45:32Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"[Please send replies Cc: git mailing list]\n\nRobert Fitzsimons wrote:\n\n> While looking at the gitweb source yesterday, I noticed a number of\n> similar expensive workflows used by a number of actions (summary,\n> shortlog, log, rss, atom, and history).\n> \n> The current workflows are:\n>       get ~100 sha1's using rev-list\n>       foreach sha1\n>               get/parse 1 commit using rev-list\n>               output commit\n> \n> The new workflows I'm proposing would be:\n>       get/parse ~100 commit's using rev-list\n>       foreach commit\n>               output commit\n\nI have tried this approach too. Take a look at\n\n  http://repo.or.cz/w/git/jnareb-git.git?a=log;h=Attic/gitweb/parse_rev_list\n\nor at discussion started with\n  Message-Id: <200609061504.40725.jnareb@gmail.com>\n  http://mid.gmane.org/200609061504.40725.jnareb@gmail.com\n\n> The following simplified commands gives an idea of the git only overhead\n> between these two workflows.\n> \n> time \\\n> for r in `git-rev-list --max-count=100 HEAD --` ; \\\n> do git-rev-list --header --parents --max-count=1 $r -- ; \\\n> done > /dev/null\n> \n> real    0m0.490s\n> user    0m0.224s\n> sys     0m0.228s\n> \n> time \\\n> git-rev-list --header --parents --max-count=100 HEAD -- > /dev/null\n> \n> real    0m0.058s\n> user    0m0.008s\n> sys     0m0.004s\n> \n> There would seems to be a benefit from making the proposed change to\n> these workflows, when run on my machine against a clone of Linus's tree.\n\nThe problem is that it works only for \"log\" and \"shortlog\" views, but\nit doesn't work for \"history\" view. Now both share the same infrastructure.\nThe problem is that when there is path limiter (be it file or directory)\nthe history is simplified, and parents are _rewritten_ according to\nsimplified history. And this happen depending on strange combination\nof --header, --parents and --full-history. Should be somewhere in archives.\n\nAnd we don't want to use parents from commit object, because there might\nbe grafts, or it might be shallow clone.\n\nOn the other hand, we don't really need parents for log, shortlog and\nhistory...\n\n> One issue with this change is that, gitweb is page orientated.  Page 0\n> shows the first 100 items from a given hash, page 1 uses the same given\n> hash but show 100 to 199 items, etc.  Using 'git-rev-list --header\n> --parents' and then throwing away most of the result is very wasteful.\n> \n> So I'm suggesting we add a new option to git-rev-list which will only\n> start show results once its has iterated past a given number of items.\n> Using a caret or tilde doesn't seem to return the same result.\n> \n> I've attached a discussion patch which adds a new option --start-count\n> to git-rev-list and changed the summary and showlog actions of gitweb to\n> use this new option.\n\nVery nice idea.\n \n> I'm sure there are many improvements to this patch, comments?\n\nPerhaps this patch should be split in two? (Usually either second mail is\nreply to first mail, or both are replies to introductory letter, usually\nwith table of contents and diffstat of series).\n\n[...]\n\nDocumentation (of --start-count / --skip option), please?\n\n\nP.S. Thanks for the patches.\n\nP.P.S. Do you have any comments to latest \"[RFC] gitweb wishlist and TODO\nlist\" series?\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n\n"},{"id":"298414","messageId":"7v1wmvpmef.fsf@assigned-by-dhcp.cox.net","threadId":"43231","inReplyTo":"20061219205422.GA17864@localhost","subject":"Re: [RFC] Possible optimization for gitweb","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-19T22:10:32Z","receivedAt":"2006-12-19T22:10:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Fitzsimons <robfitz@273k.net> writes:\n\n> The new workflows I'm proposing would be:\n> \tget/parse ~100 commit's using rev-list\n> \tforeach commit\n> \t\toutput commit\n\nAbsolutely.\n\nAnd Ok on rev-list part, but perhaps --skip would be more\nappropriate name.\n"},{"id":"296684","messageId":"em9oi5$72t$1@sea.gmane.org","threadId":"43231","inReplyTo":"7v1wmvpmef.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] Possible optimization for gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-12-19T22:22:25Z","receivedAt":"2006-12-19T22:22:25Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"[Please Cc: git@vger.kernel.org]\n\nJunio C Hamano wrote:\n\n> Robert Fitzsimons <robfitz@273k.net> writes:\n> \n>> The new workflows I'm proposing would be:\n>>      get/parse ~100 commit's using rev-list\n>>      foreach commit\n>>              output commit\n> \n> Absolutely.\n> \n> And Ok on rev-list part, but perhaps --skip would be more\n> appropriate name.\n\nThe only problem that you can't use --parents with \"history\" view, because\ntogether with --full-history it shows also all merges (--full-history\nwithout --parents doesn't show merges which does not affect given file or\ndirectory; the sequence in which --parents and --full-history are taken is\na bit strange to me). So you have to keep current parse_commit (or extend\nit), and if I remember correctly you do that. \n\nI'm also for --skip (not --start-count), although... --start-count with\n--max-count seems more natural; one place it can be confusing is that we\ncount skipped commits or not? I.e. we use --start-count=10 --max-count=20\nto get second 10 of commits, or --skip=10 --max-count=10 to get second 10\nof commits?\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n\n"},{"id":"296155","messageId":"20061220002906.GB17864@localhost","threadId":"43231","inReplyTo":"em9oi5$72t$1@sea.gmane.org","subject":"[PATCH] rev-list: Add a new option --skip.","fromName":"Robert Fitzsimons","fromEmail":"robfitz@273k.net","sentAt":"2006-12-20T00:29:06Z","receivedAt":"2006-12-20T00:29:06Z","isPatch":true,"sender":{"key":"robfitz@273k.net","avatar":null},"body":"Added a new option --skip=<N> which is used to allow the caller to\nspecify how many commit items to skip before displaying the commit\noutput.  This option is most useful for programs which want to display\nfixed length pages of commit items, i.e. gitweb.\n\nSigned-off-by: Robert Fitzsimons <robfitz@273k.net>\n---\n\n> I'm also for --skip (not --start-count), although... --start-count with\n> --max-count seems more natural; one place it can be confusing is that we\n> count skipped commits or not? I.e. we use --start-count=10 --max-count=20\n> to get second 10 of commits, or --skip=10 --max-count=10 to get second 10\n> of commits?\n\nHere's the patch with the renamed option.  It also does not count\nskipped commits, so --skip=10 --max-count=10 will display 10 items.\n\nRobert\n\n\n Documentation/git-rev-list.txt |    5 +++++\n builtin-rev-list.c             |   17 +++++++++++++++++\n list-objects.c                 |    8 ++++++--\n revision.c                     |    1 +\n revision.h                     |    1 +\n 5 files changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt\nindex ec43c0b..7c86abc 100644\n--- a/Documentation/git-rev-list.txt\n+++ b/Documentation/git-rev-list.txt\n@@ -12,6 +12,7 @@ SYNOPSIS\n 'git-rev-list' [ \\--max-count=number ]\n \t     [ \\--max-age=timestamp ]\n \t     [ \\--min-age=timestamp ]\n+\t     [ \\--skip=number ]\n \t     [ \\--sparse ]\n \t     [ \\--no-merges ]\n \t     [ \\--remove-empty ]\n@@ -151,6 +152,10 @@ limiting may be applied.\n \n \tLimit the commits output to specified time range.\n \n+--skip='number'::\n+\n+\tSkip 'number' commits before starting to display commit output.\n+\n --author='pattern', --committer='pattern'::\n \n \tLimit the commits output to ones with author/committer\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex fb7fc92..432f901 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -20,6 +20,7 @@ static const char rev_list_usage[] =\n \"    --max-count=nr\\n\"\n \"    --max-age=epoch\\n\"\n \"    --min-age=epoch\\n\"\n+\"    --skip=nr\\n\"\n \"    --sparse\\n\"\n \"    --no-merges\\n\"\n \"    --remove-empty\\n\"\n@@ -219,6 +220,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tstruct commit_list *list;\n \tint i;\n \tint read_from_stdin = 0;\n+\tint skip = -1;\n \n \tinit_revisions(&revs, prefix);\n \trevs.abbrev = 0;\n@@ -246,6 +248,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tread_revisions_from_stdin(&revs);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strncmp(arg, \"--skip=\", 7)) {\n+\t\t\tskip = atoi(arg + 7);\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(rev_list_usage);\n \n \t}\n@@ -261,6 +267,17 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t/* Only --header was specified */\n \t\trevs.commit_format = CMIT_FMT_RAW;\n \n+\t/*\n+\t * If a skip value is specified set start_count appropriately,\n+\t * and if max_count is set it should be adjusted to account\n+\t * for the skip value.\n+\t */\n+\tif (skip > 0) {\n+\t\trevs.start_count = skip;\n+\t\tif (revs.max_count > 0)\n+\t\t\trevs.max_count += skip;\n+\t}\n+\n \tlist = revs.commits;\n \n \tif ((!list &&\ndiff --git a/list-objects.c b/list-objects.c\nindex f1fa21c..d96c8bf 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -108,8 +108,12 @@ void traverse_commit_list(struct rev_info *revs,\n \tstruct object_array objects = { 0, 0, NULL };\n \n \twhile ((commit = get_revision(revs)) != NULL) {\n-\t\tprocess_tree(revs, commit->tree, &objects, NULL, \"\");\n-\t\tshow_commit(commit);\n+\t\tif (revs->start_count <= 0) {\n+\t\t\tprocess_tree(revs, commit->tree, &objects, NULL, \"\");\n+\t\t\tshow_commit(commit);\n+\t\t} else {\n+\t\t\trevs->start_count--;\n+\t\t}\n \t}\n \tfor (i = 0; i < revs->pending.nr; i++) {\n \t\tstruct object_array_entry *pending = revs->pending.objects + i;\ndiff --git a/revision.c b/revision.c\nindex 993bb66..70f4861 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -524,6 +524,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n \trevs->prefix = prefix;\n \trevs->max_age = -1;\n \trevs->min_age = -1;\n+\trevs->start_count = -1;\n \trevs->max_count = -1;\n \n \trevs->prune_fn = NULL;\ndiff --git a/revision.h b/revision.h\nindex 3adab95..c2dce8c 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -75,6 +75,7 @@ struct rev_info {\n \tstruct grep_opt\t*grep_filter;\n \n \t/* special limits */\n+\tint start_count;\n \tint max_count;\n \tunsigned long max_age;\n \tunsigned long min_age;\n-- \n1.4.4.2.g80fef-dirty\n"},{"id":"294090","messageId":"20061220005204.GC17864@localhost","threadId":"43231","inReplyTo":"em9mcs$moo$1@sea.gmane.org","subject":"Re: [RFC] Possible optimization for gitweb","fromName":"Robert Fitzsimons","fromEmail":"robfitz@273k.net","sentAt":"2006-12-20T00:52:04Z","receivedAt":"2006-12-20T00:52:04Z","isPatch":false,"sender":{"key":"robfitz@273k.net","avatar":null},"body":"> P.P.S. Do you have any comments to latest \"[RFC] gitweb wishlist and TODO\n> list\" series?\n\nI'll have another read through them tomorrow and see what I come up\nwith.  I've already done a bit of reading up on mod_perl.\n\nRobert\n"},{"id":"295910","messageId":"7vbqlznzjm.fsf@assigned-by-dhcp.cox.net","threadId":"43231","inReplyTo":"20061220002906.GB17864@localhost","subject":"Re: [PATCH] rev-list: Add a new option --skip.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-20T01:09:33Z","receivedAt":"2006-12-20T01:09:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Fitzsimons <robfitz@273k.net> writes:\n\n> diff --git a/builtin-rev-list.c b/builtin-rev-list.c\n> index fb7fc92..432f901 100644\n> --- a/builtin-rev-list.c\n> +++ b/builtin-rev-list.c\n> @@ -246,6 +248,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n>  \t\t\tread_revisions_from_stdin(&revs);\n>  \t\t\tcontinue;\n>  \t\t}\n> +\t\tif (!strncmp(arg, \"--skip=\", 7)) {\n> +\t\t\tskip = atoi(arg + 7);\n> +\t\t\tcontinue;\n> +\t\t}\n>  \t\tusage(rev_list_usage);\n>  \n>  \t}\n\nHmph....\n\nI am having a hard time convincing myself that this is a feature\nthat is a narrow special case for rev-list and does not belong\nto the generic revision traversal machinery.\n\nThat is, would people expect that 'log' family allow the to say:\n\n\t$ git log --skip=10 -4 master\n\nDeclaring this as a special case for rev-list is certainly safer\n(no risk to harm the revision machinery which is quite central\npart of git), but if you define and initialize the new field\nnext to max_count, it makes me feel that it should somehow be\nhandled at the same layer.\n\nIn other words,...\n\n---\n\n revision.c |   46 ++++++++++++++++++++++++++++++++--------------\n revision.h |    1 +\n 2 files changed, 33 insertions(+), 14 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 993bb66..aa63d10 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -524,6 +524,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n \trevs->prefix = prefix;\n \trevs->max_age = -1;\n \trevs->min_age = -1;\n+\trevs->skip_count = -1;\n \trevs->max_count = -1;\n \n \trevs->prune_fn = NULL;\n@@ -760,6 +761,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n \t\t\t\trevs->max_count = atoi(arg + 12);\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strncmp(arg, \"--skip=\", 7)) {\n+\t\t\t\trevs->skip_count = atoi(arg + 7);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\t/* accept -<digit>, like traditional \"head\" */\n \t\t\tif ((*arg == '-') && isdigit(arg[1])) {\n \t\t\t\trevs->max_count = atoi(arg + 1);\n@@ -1123,23 +1128,11 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t\t\t   commit->buffer, strlen(commit->buffer));\n }\n \n-struct commit *get_revision(struct rev_info *revs)\n+static struct commit *get_revision_1(struct rev_info *revs)\n {\n-\tstruct commit_list *list = revs->commits;\n-\n-\tif (!list)\n+\tif (!revs->commits)\n \t\treturn NULL;\n \n-\t/* Check the max_count ... */\n-\tswitch (revs->max_count) {\n-\tcase -1:\n-\t\tbreak;\n-\tcase 0:\n-\t\treturn NULL;\n-\tdefault:\n-\t\trevs->max_count--;\n-\t}\n-\n \tdo {\n \t\tstruct commit_list *entry = revs->commits;\n \t\tstruct commit *commit = entry->item;\n@@ -1206,3 +1199,28 @@ struct commit *get_revision(struct rev_info *revs)\n \t} while (revs->commits);\n \treturn NULL;\n }\n+\n+struct commit *get_revision(struct rev_info *revs)\n+{\n+\tstruct commit *c = NULL;\n+\n+\tif (0 < revs->skip_count) {\n+\t\twhile ((c = get_revision_1(revs)) != NULL) {\n+\t\t\tif (revs->skip_count-- <= 0)\n+\t\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\t/* Check the max_count ... */\n+\tswitch (revs->max_count) {\n+\tcase -1:\n+\t\tbreak;\n+\tcase 0:\n+\t\treturn NULL;\n+\tdefault:\n+\t\trevs->max_count--;\n+\t}\n+\tif (c)\n+\t\treturn c;\n+\treturn get_revision_1(revs);\n+}\ndiff --git a/revision.h b/revision.h\nindex 3adab95..81f522c 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -75,6 +75,7 @@ struct rev_info {\n \tstruct grep_opt\t*grep_filter;\n \n \t/* special limits */\n+\tint skip_count;\n \tint max_count;\n \tunsigned long max_age;\n \tunsigned long min_age;\n"},{"id":"295690","messageId":"20061220145948.GD17864@localhost","threadId":"43231","inReplyTo":"7vbqlznzjm.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] rev-list: Document --skip and add test cases.","fromName":"Robert Fitzsimons","fromEmail":"robfitz@273k.net","sentAt":"2006-12-20T14:59:48Z","receivedAt":"2006-12-20T14:59:48Z","isPatch":true,"sender":{"key":"robfitz@273k.net","avatar":null},"body":"Signed-off-by: Robert Fitzsimons <robfitz@273k.net>\n---\n\n> I am having a hard time convincing myself that this is a feature\n> that is a narrow special case for rev-list and does not belong\n> to the generic revision traversal machinery.\n\nYour implementation is much better then mine.  Here's some documentation\nand a set of test cases.\n\nRobert\n\n\n Documentation/git-rev-list.txt |    5 ++++\n t/t6005-rev-list-count.sh      |   51 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 56 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt\nindex ec43c0b..9e0dcf8 100644\n--- a/Documentation/git-rev-list.txt\n+++ b/Documentation/git-rev-list.txt\n@@ -10,6 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git-rev-list' [ \\--max-count=number ]\n+\t     [ \\--skip=number ]\n \t     [ \\--max-age=timestamp ]\n \t     [ \\--min-age=timestamp ]\n \t     [ \\--sparse ]\n@@ -139,6 +140,10 @@ limiting may be applied.\n \n \tLimit the number of commits output.\n \n+--skip='number'::\n+\n+\tSkip 'number' commits before starting to show the commit output.\n+\n --since='date', --after='date'::\n \n \tShow commits more recent than a specific date.\ndiff --git a/t/t6005-rev-list-count.sh b/t/t6005-rev-list-count.sh\nnew file mode 100755\nindex 0000000..334fccf\n--- /dev/null\n+++ b/t/t6005-rev-list-count.sh\n@@ -0,0 +1,51 @@\n+#!/bin/sh\n+\n+test_description='git-rev-list --max-count and --skip test'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+    for n in 1 2 3 4 5 ; do \\\n+        echo $n > a ; \\\n+        git add a ; \\\n+        git commit -m \"$n\" ; \\\n+    done\n+'\n+\n+test_expect_success 'no options' '\n+    test $(git-rev-list HEAD | wc -l) = 5\n+'\n+\n+test_expect_success '--max-count' '\n+    test $(git-rev-list HEAD --max-count=0 | wc -l) = 0 &&\n+    test $(git-rev-list HEAD --max-count=3 | wc -l) = 3 &&\n+    test $(git-rev-list HEAD --max-count=5 | wc -l) = 5 &&\n+    test $(git-rev-list HEAD --max-count=10 | wc -l) = 5\n+'\n+\n+test_expect_success '--max-count all forms' '\n+    test $(git-rev-list HEAD --max-count=1 | wc -l) = 1 &&\n+    test $(git-rev-list HEAD -1 | wc -l) = 1 &&\n+    test $(git-rev-list HEAD -n1 | wc -l) = 1 &&\n+    test $(git-rev-list HEAD -n 1 | wc -l) = 1\n+'\n+\n+test_expect_success '--skip' '\n+    test $(git-rev-list HEAD --skip=0 | wc -l) = 5 &&\n+    test $(git-rev-list HEAD --skip=3 | wc -l) = 2 &&\n+    test $(git-rev-list HEAD --skip=5 | wc -l) = 0 &&\n+    test $(git-rev-list HEAD --skip=10 | wc -l) = 0\n+'\n+\n+test_expect_success '--skip --max-count' '\n+    test $(git-rev-list HEAD --skip=0 --max-count=0 | wc -l) = 0 &&\n+    test $(git-rev-list HEAD --skip=0 --max-count=10 | wc -l) = 5 &&\n+    test $(git-rev-list HEAD --skip=3 --max-count=0 | wc -l) = 0 &&\n+    test $(git-rev-list HEAD --skip=3 --max-count=1 | wc -l) = 1 &&\n+    test $(git-rev-list HEAD --skip=3 --max-count=2 | wc -l) = 2 &&\n+    test $(git-rev-list HEAD --skip=3 --max-count=10 | wc -l) = 2 &&\n+    test $(git-rev-list HEAD --skip=5 --max-count=10 | wc -l) = 0 &&\n+    test $(git-rev-list HEAD --skip=10 --max-count=10 | wc -l) = 0\n+'\n+\n+test_done\n-- \n1.4.4.2.g80fef-dirty\n"}]}