{"thread":{"id":"18928","subject":"[RFC/PATCH] rev-list: embed \"struct rev_info revs\" into \"struct rev_list_info\"","startedAt":"2009-04-18T04:43:28Z","lastAt":"2009-04-18T10:56:04Z","messageCount":3,"participants":["Christian Couder","Nanako Shiraishi","Paolo Bonzini"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"111558","messageId":"20090418064328.ad64accf.chriscool@tuxfamily.org","threadId":"18928","inReplyTo":null,"subject":"[RFC/PATCH] rev-list: embed \"struct rev_info revs\" into \"struct rev_list_info\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-04-18T04:43:28Z","receivedAt":"2009-04-18T04:43:28Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"There are downsides to this patch. First it is big. And \"revs\" is\ninitialized once when doing \"memset(&info, 0, sizeof(info))\" and\nonce in \"init_revisions\".\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n bisect.c                 |   14 ++++------\n bisect.h                 |    2 +-\n builtin-bisect--helper.c |    3 ++\n builtin-rev-list.c       |   58 ++++++++++++++++++++++-----------------------\n 4 files changed, 38 insertions(+), 39 deletions(-)\n\nSo I think this patch should not be applied.\n\ndiff --git a/bisect.c b/bisect.c\nindex 58f7e6f..6425501 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -534,23 +534,21 @@ static void bisect_rev_setup(struct rev_info *revs, const char *prefix)\n \n int bisect_next_vars(const char *prefix)\n {\n-\tstruct rev_info revs;\n \tstruct rev_list_info info;\n \tint reaches = 0, all = 0;\n \n \tmemset(&info, 0, sizeof(info));\n-\tinfo.revs = &revs;\n \tinfo.bisect_show_flags = BISECT_SHOW_TRIED | BISECT_SHOW_STRINGED;\n \n-\tbisect_rev_setup(&revs, prefix);\n+\tbisect_rev_setup(&info.revs, prefix);\n \n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&info.revs))\n \t\tdie(\"revision walk setup failed\");\n-\tif (revs.tree_objects)\n-\t\tmark_edges_uninteresting(revs.commits, &revs, NULL);\n+\tif (info.revs.tree_objects)\n+\t\tmark_edges_uninteresting(info.revs.commits, &info.revs, NULL);\n \n-\trevs.commits = find_bisection(revs.commits, &reaches, &all,\n-\t\t\t\t      !!skipped_sha1_nr);\n+\tinfo.revs.commits = find_bisection(info.revs.commits, &reaches, &all,\n+\t\t\t\t\t   !!skipped_sha1_nr);\n \n \treturn show_bisect_vars(&info, reaches, all);\n }\ndiff --git a/bisect.h b/bisect.h\nindex fdba913..afedfaa 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -15,7 +15,7 @@ extern struct commit_list *filter_skipped(struct commit_list *list,\n #define BISECT_SHOW_STRINGED\t(1<<2)\n \n struct rev_list_info {\n-\tstruct rev_info *revs;\n+\tstruct rev_info revs;\n \tint bisect_show_flags;\n \tint show_timestamp;\n \tint hdr_termination;\ndiff --git a/builtin-bisect--helper.c b/builtin-bisect--helper.c\nindex 8fe7787..6d3ccb0 100644\n--- a/builtin-bisect--helper.c\n+++ b/builtin-bisect--helper.c\n@@ -1,6 +1,9 @@\n #include \"builtin.h\"\n #include \"cache.h\"\n #include \"parse-options.h\"\n+#include \"diff.h\"\n+#include \"commit.h\"\n+#include \"revision.h\"\n #include \"bisect.h\"\n \n static const char * const git_bisect_helper_usage[] = {\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 193993c..433c5bc 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -46,7 +46,7 @@ static void finish_commit(struct commit *commit, void *data);\n static void show_commit(struct commit *commit, void *data)\n {\n \tstruct rev_list_info *info = data;\n-\tstruct rev_info *revs = info->revs;\n+\tstruct rev_info *revs = &info->revs;\n \n \tgraph_show_commit(revs->graph);\n \n@@ -238,7 +238,7 @@ int show_bisect_vars(struct rev_list_info *info, int reaches, int all)\n \tint cnt, flags = info->bisect_show_flags;\n \tchar hex[41] = \"\", *format;\n \tstruct commit_list *tried;\n-\tstruct rev_info *revs = info->revs;\n+\tstruct rev_info *revs = &info->revs;\n \n \tif (!revs->commits && !(flags & BISECT_SHOW_TRIED))\n \t\treturn 1;\n@@ -295,7 +295,6 @@ int show_bisect_vars(struct rev_list_info *info, int reaches, int all)\n \n int cmd_rev_list(int argc, const char **argv, const char *prefix)\n {\n-\tstruct rev_info revs;\n \tstruct rev_list_info info;\n \tint i;\n \tint read_from_stdin = 0;\n@@ -304,21 +303,20 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tint bisect_find_all = 0;\n \tint quiet = 0;\n \n-\tgit_config(git_default_config, NULL);\n-\tinit_revisions(&revs, prefix);\n-\trevs.abbrev = 0;\n-\trevs.commit_format = CMIT_FMT_UNSPECIFIED;\n-\targc = setup_revisions(argc, argv, &revs, NULL);\n-\n \tmemset(&info, 0, sizeof(info));\n-\tinfo.revs = &revs;\n \n-\tquiet = DIFF_OPT_TST(&revs.diffopt, QUIET);\n+\tgit_config(git_default_config, NULL);\n+\tinit_revisions(&info.revs, prefix);\n+\tinfo.revs.abbrev = 0;\n+\tinfo.revs.commit_format = CMIT_FMT_UNSPECIFIED;\n+\targc = setup_revisions(argc, argv, &info.revs, NULL);\n+\n+\tquiet = DIFF_OPT_TST(&info.revs.diffopt, QUIET);\n \tfor (i = 1 ; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \n \t\tif (!strcmp(arg, \"--header\")) {\n-\t\t\trevs.verbose_header = 1;\n+\t\t\tinfo.revs.verbose_header = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--timestamp\")) {\n@@ -333,7 +331,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tbisect_list = 1;\n \t\t\tbisect_find_all = 1;\n \t\t\tinfo.bisect_show_flags = BISECT_SHOW_ALL;\n-\t\t\trevs.show_decorations = 1;\n+\t\t\tinfo.revs.show_decorations = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--bisect-vars\")) {\n@@ -344,51 +342,51 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\tif (!strcmp(arg, \"--stdin\")) {\n \t\t\tif (read_from_stdin++)\n \t\t\t\tdie(\"--stdin given twice?\");\n-\t\t\tread_revisions_from_stdin(&revs);\n+\t\t\tread_revisions_from_stdin(&info.revs);\n \t\t\tcontinue;\n \t\t}\n \t\tusage(rev_list_usage);\n \n \t}\n-\tif (revs.commit_format != CMIT_FMT_UNSPECIFIED) {\n+\tif (info.revs.commit_format != CMIT_FMT_UNSPECIFIED) {\n \t\t/* The command line has a --pretty  */\n \t\tinfo.hdr_termination = '\\n';\n-\t\tif (revs.commit_format == CMIT_FMT_ONELINE)\n+\t\tif (info.revs.commit_format == CMIT_FMT_ONELINE)\n \t\t\tinfo.header_prefix = \"\";\n \t\telse\n \t\t\tinfo.header_prefix = \"commit \";\n \t}\n-\telse if (revs.verbose_header)\n+\telse if (info.revs.verbose_header)\n \t\t/* Only --header was specified */\n-\t\trevs.commit_format = CMIT_FMT_RAW;\n+\t\tinfo.revs.commit_format = CMIT_FMT_RAW;\n \n-\tif ((!revs.commits &&\n-\t     (!(revs.tag_objects||revs.tree_objects||revs.blob_objects) &&\n-\t      !revs.pending.nr)) ||\n-\t    revs.diff)\n+\tif ((!info.revs.commits &&\n+\t     (!(info.revs.tag_objects||info.revs.tree_objects||info.revs.blob_objects) &&\n+\t      !info.revs.pending.nr)) ||\n+\t    info.revs.diff)\n \t\tusage(rev_list_usage);\n \n-\tsave_commit_buffer = revs.verbose_header ||\n-\t\trevs.grep_filter.pattern_list;\n+\tsave_commit_buffer = info.revs.verbose_header ||\n+\t\tinfo.revs.grep_filter.pattern_list;\n \tif (bisect_list)\n-\t\trevs.limited = 1;\n+\t\tinfo.revs.limited = 1;\n \n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&info.revs))\n \t\tdie(\"revision walk setup failed\");\n-\tif (revs.tree_objects)\n-\t\tmark_edges_uninteresting(revs.commits, &revs, show_edge);\n+\tif (info.revs.tree_objects)\n+\t\tmark_edges_uninteresting(info.revs.commits, &info.revs, show_edge);\n \n \tif (bisect_list) {\n \t\tint reaches = reaches, all = all;\n \n-\t\trevs.commits = find_bisection(revs.commits, &reaches, &all,\n+\t\tinfo.revs.commits = find_bisection(info.revs.commits, &reaches, &all,\n \t\t\t\t\t      bisect_find_all);\n \n \t\tif (bisect_show_vars)\n \t\t\treturn show_bisect_vars(&info, reaches, all);\n \t}\n \n-\ttraverse_commit_list(&revs,\n+\ttraverse_commit_list(&info.revs,\n \t\t\t     quiet ? finish_commit : show_commit,\n \t\t\t     quiet ? finish_object : show_object,\n \t\t\t     &info);\n-- \n1.6.2.2.574.g3b0a.dirty\n"},{"id":"111569","messageId":"20090418191125.6117@nanako3.lavabit.com","threadId":"18928","inReplyTo":"20090418064328.ad64accf.chriscool@tuxfamily.org","subject":"Re: [RFC/PATCH] rev-list: embed \"struct rev_info revs\" into \"struct rev_list_info\"","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2009-04-18T10:11:25Z","receivedAt":"2009-04-18T10:11:25Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Christian Couder <chriscool@tuxfamily.org>:\n\n> There are downsides to this patch. First it is big. And \"revs\" is\n> initialized once when doing \"memset(&info, 0, sizeof(info))\" and\n> once in \"init_revisions\".\n\nI'm sorry if it is obvious, but is there any upsides?\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n"},{"id":"111571","messageId":"49E9B1C4.50100@gnu.org","threadId":"18928","inReplyTo":"20090418191125.6117@nanako3.lavabit.com","subject":"Re: [RFC/PATCH] rev-list: embed \"struct rev_info revs\" into \"struct rev_list_info\"","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2009-04-18T10:56:04Z","receivedAt":"2009-04-18T10:56:04Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"Nanako Shiraishi wrote:\n> Quoting Christian Couder <chriscool@tuxfamily.org>:\n> \n>> There are downsides to this patch. First it is big. And \"revs\" is\n>> initialized once when doing \"memset(&info, 0, sizeof(info))\" and\n>> once in \"init_revisions\".\n> \n> I'm sorry if it is obvious, but is there any upsides?\n\nAs the patch shows, the lifetime of the revs is the same as that of info\n(i.e., there are never memcpys from something into revs).\n\nPaolo\n"}]}