{"thread":{"id":"34822","subject":"[PATCH 0/2] fast-export: simplification","startedAt":"2013-09-01T07:05:46Z","lastAt":"2013-09-03T20:17:48Z","messageCount":5,"participants":["Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"226465","messageId":"1378019148-1565-1-git-send-email-felipe.contreras@gmail.com","threadId":"34822","inReplyTo":null,"subject":"[PATCH 0/2] fast-export: simplification","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-01T07:05:46Z","receivedAt":"2013-09-01T07:05:46Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nNo functional changes, but get_tags_and_duplicates() is quite complex as it is,\nand can be simplified by spliting code into a separate function.\n\nFelipe Contreras (2):\n  fast-export: make extra_refs global\n  fast-export: refactor get_tags_and_duplicates()\n\n builtin/fast-export.c | 87 ++++++++++++++++++++++++++++-----------------------\n 1 file changed, 47 insertions(+), 40 deletions(-)\n\n-- \n1.8.4-337-g7358a66-dirty\n"},{"id":"226466","messageId":"1378019148-1565-2-git-send-email-felipe.contreras@gmail.com","threadId":"34822","inReplyTo":"1378019148-1565-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/2] fast-export: make extra_refs global","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-01T07:05:47Z","receivedAt":"2013-09-01T07:05:47Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"There's no need to pass it around everywhere. This would make easier\nfurther refactoring that makes use of this variable.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n builtin/fast-export.c | 21 ++++++++++-----------\n 1 file changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 8e19058..957392c 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -30,6 +30,7 @@ static int fake_missing_tagger;\n static int use_done_feature;\n static int no_data;\n static int full_tree;\n+static struct string_list extra_refs = STRING_LIST_INIT_NODUP;\n \n static int parse_opt_signed_tag_mode(const struct option *opt,\n \t\t\t\t     const char *arg, int unset)\n@@ -484,8 +485,7 @@ static void handle_tag(const char *name, struct tag *tag)\n \t       (int)message_size, (int)message_size, message ? message : \"\");\n }\n \n-static void get_tags_and_duplicates(struct rev_cmdline_info *info,\n-\t\t\t\t    struct string_list *extra_refs)\n+static void get_tags_and_duplicates(struct rev_cmdline_info *info)\n {\n \tstruct tag *tag;\n \tint i;\n@@ -512,7 +512,7 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info,\n \t\t\t/* handle nested tags */\n \t\t\twhile (tag && tag->object.type == OBJ_TAG) {\n \t\t\t\tparse_object(tag->object.sha1);\n-\t\t\t\tstring_list_append(extra_refs, full_name)->util = tag;\n+\t\t\t\tstring_list_append(&extra_refs, full_name)->util = tag;\n \t\t\t\ttag = (struct tag *)tag->tagged;\n \t\t\t}\n \t\t\tif (!tag)\n@@ -542,20 +542,20 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info,\n \t\t * sure it gets properly updated eventually.\n \t\t */\n \t\tif (commit->util || commit->object.flags & SHOWN)\n-\t\t\tstring_list_append(extra_refs, full_name)->util = commit;\n+\t\t\tstring_list_append(&extra_refs, full_name)->util = commit;\n \t\tif (!commit->util)\n \t\t\tcommit->util = full_name;\n \t}\n }\n \n-static void handle_tags_and_duplicates(struct string_list *extra_refs)\n+static void handle_tags_and_duplicates(void)\n {\n \tstruct commit *commit;\n \tint i;\n \n-\tfor (i = extra_refs->nr - 1; i >= 0; i--) {\n-\t\tconst char *name = extra_refs->items[i].string;\n-\t\tstruct object *object = extra_refs->items[i].util;\n+\tfor (i = extra_refs.nr - 1; i >= 0; i--) {\n+\t\tconst char *name = extra_refs.items[i].string;\n+\t\tstruct object *object = extra_refs.items[i].util;\n \t\tswitch (object->type) {\n \t\tcase OBJ_TAG:\n \t\t\thandle_tag(name, (struct tag *)object);\n@@ -657,7 +657,6 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n \tstruct object_array commits = OBJECT_ARRAY_INIT;\n-\tstruct string_list extra_refs = STRING_LIST_INIT_NODUP;\n \tstruct commit *commit;\n \tchar *export_filename = NULL, *import_filename = NULL;\n \tuint32_t lastimportid;\n@@ -709,7 +708,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \tif (import_filename && revs.prune_data.nr)\n \t\tfull_tree = 1;\n \n-\tget_tags_and_duplicates(&revs.cmdline, &extra_refs);\n+\tget_tags_and_duplicates(&revs.cmdline);\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n@@ -725,7 +724,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\thandle_tags_and_duplicates(&extra_refs);\n+\thandle_tags_and_duplicates();\n \n \tif (export_filename && lastimportid != last_idnum)\n \t\texport_marks(export_filename);\n-- \n1.8.4-337-g7358a66-dirty\n"},{"id":"226467","messageId":"1378019148-1565-3-git-send-email-felipe.contreras@gmail.com","threadId":"34822","inReplyTo":"1378019148-1565-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/2] fast-export: refactor get_tags_and_duplicates()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-01T07:05:48Z","receivedAt":"2013-09-01T07:05:48Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Split into a separate helper function get_commit() so that the part that\nfinds the relevant commit, and the part that does something with it\n(handle tag object, etc.) are in different places.\n\nNo functional changes.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n builtin/fast-export.c | 68 ++++++++++++++++++++++++++++-----------------------\n 1 file changed, 38 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 957392c..03e1090 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -485,9 +485,32 @@ static void handle_tag(const char *name, struct tag *tag)\n \t       (int)message_size, (int)message_size, message ? message : \"\");\n }\n \n+static struct commit *get_commit(struct rev_cmdline_entry *e, char *full_name)\n+{\n+\tswitch (e->item->type) {\n+\tcase OBJ_COMMIT:\n+\t\treturn (struct commit *)e->item;\n+\tcase OBJ_TAG: {\n+\t\tstruct tag *tag = (struct tag *)e->item;\n+\n+\t\t/* handle nested tags */\n+\t\twhile (tag && tag->object.type == OBJ_TAG) {\n+\t\t\tparse_object(tag->object.sha1);\n+\t\t\tstring_list_append(&extra_refs, full_name)->util = tag;\n+\t\t\ttag = (struct tag *)tag->tagged;\n+\t\t}\n+\t\tif (!tag)\n+\t\t\tdie(\"Tag %s points nowhere?\", e->name);\n+\t\treturn (struct commit *)tag;\n+\t\tbreak;\n+\t}\n+\tdefault:\n+\t\treturn NULL;\n+\t}\n+}\n+\n static void get_tags_and_duplicates(struct rev_cmdline_info *info)\n {\n-\tstruct tag *tag;\n \tint i;\n \n \tfor (i = 0; i < info->nr; i++) {\n@@ -502,41 +525,26 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info)\n \t\tif (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)\n \t\t\tcontinue;\n \n-\t\tswitch (e->item->type) {\n-\t\tcase OBJ_COMMIT:\n-\t\t\tcommit = (struct commit *)e->item;\n-\t\t\tbreak;\n-\t\tcase OBJ_TAG:\n-\t\t\ttag = (struct tag *)e->item;\n-\n-\t\t\t/* handle nested tags */\n-\t\t\twhile (tag && tag->object.type == OBJ_TAG) {\n-\t\t\t\tparse_object(tag->object.sha1);\n-\t\t\t\tstring_list_append(&extra_refs, full_name)->util = tag;\n-\t\t\t\ttag = (struct tag *)tag->tagged;\n-\t\t\t}\n-\t\t\tif (!tag)\n-\t\t\t\tdie (\"Tag %s points nowhere?\", e->name);\n-\t\t\tswitch(tag->object.type) {\n-\t\t\tcase OBJ_COMMIT:\n-\t\t\t\tcommit = (struct commit *)tag;\n-\t\t\t\tbreak;\n-\t\t\tcase OBJ_BLOB:\n-\t\t\t\texport_blob(tag->object.sha1);\n-\t\t\t\tcontinue;\n-\t\t\tdefault: /* OBJ_TAG (nested tags) is already handled */\n-\t\t\t\twarning(\"Tag points to object of unexpected type %s, skipping.\",\n-\t\t\t\t\ttypename(tag->object.type));\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tbreak;\n-\t\tdefault:\n+\t\tcommit = get_commit(e, full_name);\n+\t\tif (!commit) {\n \t\t\twarning(\"%s: Unexpected object of type %s, skipping.\",\n \t\t\t\te->name,\n \t\t\t\ttypename(e->item->type));\n \t\t\tcontinue;\n \t\t}\n \n+\t\tswitch(commit->object.type) {\n+\t\tcase OBJ_COMMIT:\n+\t\t\tbreak;\n+\t\tcase OBJ_BLOB:\n+\t\t\texport_blob(commit->object.sha1);\n+\t\t\tcontinue;\n+\t\tdefault: /* OBJ_TAG (nested tags) is already handled */\n+\t\t\twarning(\"Tag points to object of unexpected type %s, skipping.\",\n+\t\t\t\ttypename(commit->object.type));\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\t/*\n \t\t * This ref will not be updated through a commit, lets make\n \t\t * sure it gets properly updated eventually.\n-- \n1.8.4-337-g7358a66-dirty\n"},{"id":"226661","messageId":"xmqqsixlr8p2.fsf@gitster.dls.corp.google.com","threadId":"34822","inReplyTo":"1378019148-1565-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 2/2] fast-export: refactor get_tags_and_duplicates()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-03T19:44:57Z","receivedAt":"2013-09-03T19:44:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Split into a separate helper function get_commit() so that the part that\n> finds the relevant commit, and the part that does something with it\n> (handle tag object, etc.) are in different places.\n>\n> No functional changes.\n\nActually, the old code used to use commit unchecked if e->item->type\nsaid it is OBJ_COMMIT but e->item was somehow NULL.  The new code\nchecks this case and skips with a warning(), which I think is an\nimprovement, if not a bugfix (it only makes it easier to diagnose a\nbug in the code that populates rev_cmdline_entry).\n\nThanks; will queue.\n\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n>  builtin/fast-export.c | 68 ++++++++++++++++++++++++++++-----------------------\n>  1 file changed, 38 insertions(+), 30 deletions(-)\n>\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 957392c..03e1090 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -485,9 +485,32 @@ static void handle_tag(const char *name, struct tag *tag)\n>  \t       (int)message_size, (int)message_size, message ? message : \"\");\n>  }\n>  \n> +static struct commit *get_commit(struct rev_cmdline_entry *e, char *full_name)\n> +{\n> +\tswitch (e->item->type) {\n> +\tcase OBJ_COMMIT:\n> +\t\treturn (struct commit *)e->item;\n> +\tcase OBJ_TAG: {\n> +\t\tstruct tag *tag = (struct tag *)e->item;\n> +\n> +\t\t/* handle nested tags */\n> +\t\twhile (tag && tag->object.type == OBJ_TAG) {\n> +\t\t\tparse_object(tag->object.sha1);\n> +\t\t\tstring_list_append(&extra_refs, full_name)->util = tag;\n> +\t\t\ttag = (struct tag *)tag->tagged;\n> +\t\t}\n> +\t\tif (!tag)\n> +\t\t\tdie(\"Tag %s points nowhere?\", e->name);\n> +\t\treturn (struct commit *)tag;\n> +\t\tbreak;\n> +\t}\n> +\tdefault:\n> +\t\treturn NULL;\n> +\t}\n> +}\n> +\n>  static void get_tags_and_duplicates(struct rev_cmdline_info *info)\n>  {\n> -\tstruct tag *tag;\n>  \tint i;\n>  \n>  \tfor (i = 0; i < info->nr; i++) {\n> @@ -502,41 +525,26 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info)\n>  \t\tif (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)\n>  \t\t\tcontinue;\n>  \n> -\t\tswitch (e->item->type) {\n> -\t\tcase OBJ_COMMIT:\n> -\t\t\tcommit = (struct commit *)e->item;\n> -\t\t\tbreak;\n> -\t\tcase OBJ_TAG:\n> -\t\t\ttag = (struct tag *)e->item;\n> -\n> -\t\t\t/* handle nested tags */\n> -\t\t\twhile (tag && tag->object.type == OBJ_TAG) {\n> -\t\t\t\tparse_object(tag->object.sha1);\n> -\t\t\t\tstring_list_append(&extra_refs, full_name)->util = tag;\n> -\t\t\t\ttag = (struct tag *)tag->tagged;\n> -\t\t\t}\n> -\t\t\tif (!tag)\n> -\t\t\t\tdie (\"Tag %s points nowhere?\", e->name);\n> -\t\t\tswitch(tag->object.type) {\n> -\t\t\tcase OBJ_COMMIT:\n> -\t\t\t\tcommit = (struct commit *)tag;\n> -\t\t\t\tbreak;\n> -\t\t\tcase OBJ_BLOB:\n> -\t\t\t\texport_blob(tag->object.sha1);\n> -\t\t\t\tcontinue;\n> -\t\t\tdefault: /* OBJ_TAG (nested tags) is already handled */\n> -\t\t\t\twarning(\"Tag points to object of unexpected type %s, skipping.\",\n> -\t\t\t\t\ttypename(tag->object.type));\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> -\t\t\tbreak;\n> -\t\tdefault:\n> +\t\tcommit = get_commit(e, full_name);\n> +\t\tif (!commit) {\n>  \t\t\twarning(\"%s: Unexpected object of type %s, skipping.\",\n>  \t\t\t\te->name,\n>  \t\t\t\ttypename(e->item->type));\n>  \t\t\tcontinue;\n>  \t\t}\n>  \n> +\t\tswitch(commit->object.type) {\n> +\t\tcase OBJ_COMMIT:\n> +\t\t\tbreak;\n> +\t\tcase OBJ_BLOB:\n> +\t\t\texport_blob(commit->object.sha1);\n> +\t\t\tcontinue;\n> +\t\tdefault: /* OBJ_TAG (nested tags) is already handled */\n> +\t\t\twarning(\"Tag points to object of unexpected type %s, skipping.\",\n> +\t\t\t\ttypename(commit->object.type));\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n>  \t\t/*\n>  \t\t * This ref will not be updated through a commit, lets make\n>  \t\t * sure it gets properly updated eventually.\n"},{"id":"226670","messageId":"xmqq38plr76b.fsf@gitster.dls.corp.google.com","threadId":"34822","inReplyTo":"xmqqsixlr8p2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] fast-export: refactor get_tags_and_duplicates()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-03T20:17:48Z","receivedAt":"2013-09-03T20:17:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> Split into a separate helper function get_commit() so that the part that\n>> finds the relevant commit, and the part that does something with it\n>> (handle tag object, etc.) are in different places.\n>>\n>> No functional changes.\n>\n> Actually, the old code used to use commit unchecked if e->item->type\n> said it is OBJ_COMMIT but e->item was somehow NULL.  The new code\n> checks ...\n\nNah, I am an idiot.  We will segfault where we check e->item->type\nbefore reaching that far down the code either way, so this is really\na no-op but makes it easier to fix (if we wanted to---I do not think\nit matters that much).\n"}]}