{"thread":{"id":"26494","subject":"[PATCH] git.c: reorder builtin command list","startedAt":"2011-02-14T13:26:24Z","lastAt":"2011-02-16T03:46:30Z","messageCount":6,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"161044","messageId":"1297689984-3151-1-git-send-email-pclouds@gmail.com","threadId":"26494","inReplyTo":null,"subject":"[PATCH] git.c: reorder builtin command list","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-14T13:26:24Z","receivedAt":"2011-02-14T13:26:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The majority of commands is in alphabet order except some. Reorder\nthem so it's easier to locate a command by (my trained) eye.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n I originally wanted to put \"stage\" in alphabet order. It was near\n \"add\" probably because it's an alias of \"add\". But the rest was in\n alphabet order.. Then I wondered if there were any other out-of-order\n commands...\n\n A worthless OCD patch in a sense.\n\n git.c | 16 ++++++++--------\n 1 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 23610aa..17ce68c 100644\n--- a/git.c\n+++ b/git.c\n@@ -313,7 +313,6 @@ static void handle_internal_command(int argc, const char **argv)\n \tconst char *cmd = argv[0];\n \tstatic struct cmd_struct commands[] = {\n \t\t{ \"add\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n-\t\t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"annotate\", cmd_annotate, RUN_SETUP },\n \t\t{ \"apply\", cmd_apply, RUN_SETUP_GENTLY },\n \t\t{ \"archive\", cmd_archive },\n@@ -325,12 +324,12 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"checkout\", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"checkout-index\", cmd_checkout_index,\n \t\t\tRUN_SETUP | NEED_WORK_TREE},\n-\t\t{ \"check-ref-format\", cmd_check_ref_format },\n \t\t{ \"check-attr\", cmd_check_attr, RUN_SETUP },\n+\t\t{ \"check-ref-format\", cmd_check_ref_format },\n \t\t{ \"cherry\", cmd_cherry, RUN_SETUP },\n \t\t{ \"cherry-pick\", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },\n-\t\t{ \"clone\", cmd_clone },\n \t\t{ \"clean\", cmd_clean, RUN_SETUP | NEED_WORK_TREE },\n+\t\t{ \"clone\", cmd_clone },\n \t\t{ \"commit\", cmd_commit, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"commit-tree\", cmd_commit_tree, RUN_SETUP },\n \t\t{ \"config\", cmd_config, RUN_SETUP_GENTLY },\n@@ -358,8 +357,8 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"init-db\", cmd_init_db },\n \t\t{ \"log\", cmd_log, RUN_SETUP },\n \t\t{ \"ls-files\", cmd_ls_files, RUN_SETUP },\n-\t\t{ \"ls-tree\", cmd_ls_tree, RUN_SETUP },\n \t\t{ \"ls-remote\", cmd_ls_remote, RUN_SETUP_GENTLY },\n+\t\t{ \"ls-tree\", cmd_ls_tree, RUN_SETUP },\n \t\t{ \"mailinfo\", cmd_mailinfo },\n \t\t{ \"mailsplit\", cmd_mailsplit },\n \t\t{ \"merge\", cmd_merge, RUN_SETUP | NEED_WORK_TREE },\n@@ -379,6 +378,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"notes\", cmd_notes, RUN_SETUP },\n \t\t{ \"pack-objects\", cmd_pack_objects, RUN_SETUP },\n \t\t{ \"pack-redundant\", cmd_pack_redundant, RUN_SETUP },\n+\t\t{ \"pack-refs\", cmd_pack_refs, RUN_SETUP },\n \t\t{ \"patch-id\", cmd_patch_id },\n \t\t{ \"peek-remote\", cmd_ls_remote, RUN_SETUP_GENTLY },\n \t\t{ \"pickaxe\", cmd_blame, RUN_SETUP },\n@@ -401,8 +401,10 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"rm\", cmd_rm, RUN_SETUP },\n \t\t{ \"send-pack\", cmd_send_pack, RUN_SETUP },\n \t\t{ \"shortlog\", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },\n-\t\t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\n \t\t{ \"show\", cmd_show, RUN_SETUP },\n+\t\t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\n+\t\t{ \"show-ref\", cmd_show_ref, RUN_SETUP },\n+\t\t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"stripspace\", cmd_stripspace },\n \t\t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n@@ -415,13 +417,11 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"update-server-info\", cmd_update_server_info, RUN_SETUP },\n \t\t{ \"upload-archive\", cmd_upload_archive },\n \t\t{ \"var\", cmd_var, RUN_SETUP_GENTLY },\n+\t\t{ \"verify-pack\", cmd_verify_pack },\n \t\t{ \"verify-tag\", cmd_verify_tag, RUN_SETUP },\n \t\t{ \"version\", cmd_version },\n \t\t{ \"whatchanged\", cmd_whatchanged, RUN_SETUP },\n \t\t{ \"write-tree\", cmd_write_tree, RUN_SETUP },\n-\t\t{ \"verify-pack\", cmd_verify_pack },\n-\t\t{ \"show-ref\", cmd_show_ref, RUN_SETUP },\n-\t\t{ \"pack-refs\", cmd_pack_refs, RUN_SETUP },\n \t};\n \tint i;\n \tstatic const char ext[] = STRIP_EXTENSION;\n-- \n1.7.4.74.g639db\n"},{"id":"161086","messageId":"7vvd0ms9ra.fsf@alter.siamese.dyndns.org","threadId":"26494","inReplyTo":"1297689984-3151-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] git.c: reorder builtin command list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-14T19:20:57Z","receivedAt":"2011-02-14T19:20:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> The majority of commands is in alphabet order except some. Reorder\n> them so it's easier to locate a command by (my trained) eye.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> I originally wanted to put \"stage\" in alphabet order. It was near\n> \"add\" probably because it's an alias of \"add\". But the rest was in\n> alphabet order.. Then I wondered if there were any other out-of-order\n> commands...\n>\n> A worthless OCD patch in a sense.\n\nI did this myself the other day, as I think it simply is a good project\nhygiene. If this were 1/2 of a series followed by 2/2 that runs binary\nsearch in the table, that would make it make more sense ;-)\n\nWill apply. I don't think there is anything in-flight to conflict with\nit at this moment.\n"},{"id":"161142","messageId":"1297739344-28961-1-git-send-email-pclouds@gmail.com","threadId":"26494","inReplyTo":"7vvd0ms9ra.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/2] git.c: reorder builtin command list","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-15T03:09:03Z","receivedAt":"2011-02-15T03:09:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The majority of commands is in alphabet order except some. Reorder\nthem so it's easier to locate a command by eye and able to binary\nsearch.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n check-ref-format and check-attr are moved up compared to previous\n patch. I checked the order with qsort() so it should be correct.\n\n git.c | 18 +++++++++---------\n 1 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 23610aa..57701e3 100644\n--- a/git.c\n+++ b/git.c\n@@ -313,7 +313,6 @@ static void handle_internal_command(int argc, const char **argv)\n \tconst char *cmd = argv[0];\n \tstatic struct cmd_struct commands[] = {\n \t\t{ \"add\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n-\t\t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"annotate\", cmd_annotate, RUN_SETUP },\n \t\t{ \"apply\", cmd_apply, RUN_SETUP_GENTLY },\n \t\t{ \"archive\", cmd_archive },\n@@ -322,15 +321,15 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"branch\", cmd_branch, RUN_SETUP },\n \t\t{ \"bundle\", cmd_bundle, RUN_SETUP_GENTLY },\n \t\t{ \"cat-file\", cmd_cat_file, RUN_SETUP },\n+\t\t{ \"check-attr\", cmd_check_attr, RUN_SETUP },\n+\t\t{ \"check-ref-format\", cmd_check_ref_format },\n \t\t{ \"checkout\", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"checkout-index\", cmd_checkout_index,\n \t\t\tRUN_SETUP | NEED_WORK_TREE},\n-\t\t{ \"check-ref-format\", cmd_check_ref_format },\n-\t\t{ \"check-attr\", cmd_check_attr, RUN_SETUP },\n \t\t{ \"cherry\", cmd_cherry, RUN_SETUP },\n \t\t{ \"cherry-pick\", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },\n-\t\t{ \"clone\", cmd_clone },\n \t\t{ \"clean\", cmd_clean, RUN_SETUP | NEED_WORK_TREE },\n+\t\t{ \"clone\", cmd_clone },\n \t\t{ \"commit\", cmd_commit, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"commit-tree\", cmd_commit_tree, RUN_SETUP },\n \t\t{ \"config\", cmd_config, RUN_SETUP_GENTLY },\n@@ -358,8 +357,8 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"init-db\", cmd_init_db },\n \t\t{ \"log\", cmd_log, RUN_SETUP },\n \t\t{ \"ls-files\", cmd_ls_files, RUN_SETUP },\n-\t\t{ \"ls-tree\", cmd_ls_tree, RUN_SETUP },\n \t\t{ \"ls-remote\", cmd_ls_remote, RUN_SETUP_GENTLY },\n+\t\t{ \"ls-tree\", cmd_ls_tree, RUN_SETUP },\n \t\t{ \"mailinfo\", cmd_mailinfo },\n \t\t{ \"mailsplit\", cmd_mailsplit },\n \t\t{ \"merge\", cmd_merge, RUN_SETUP | NEED_WORK_TREE },\n@@ -379,6 +378,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"notes\", cmd_notes, RUN_SETUP },\n \t\t{ \"pack-objects\", cmd_pack_objects, RUN_SETUP },\n \t\t{ \"pack-redundant\", cmd_pack_redundant, RUN_SETUP },\n+\t\t{ \"pack-refs\", cmd_pack_refs, RUN_SETUP },\n \t\t{ \"patch-id\", cmd_patch_id },\n \t\t{ \"peek-remote\", cmd_ls_remote, RUN_SETUP_GENTLY },\n \t\t{ \"pickaxe\", cmd_blame, RUN_SETUP },\n@@ -401,8 +401,10 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"rm\", cmd_rm, RUN_SETUP },\n \t\t{ \"send-pack\", cmd_send_pack, RUN_SETUP },\n \t\t{ \"shortlog\", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },\n-\t\t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\n \t\t{ \"show\", cmd_show, RUN_SETUP },\n+\t\t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\n+\t\t{ \"show-ref\", cmd_show_ref, RUN_SETUP },\n+\t\t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"stripspace\", cmd_stripspace },\n \t\t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n@@ -415,13 +417,11 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"update-server-info\", cmd_update_server_info, RUN_SETUP },\n \t\t{ \"upload-archive\", cmd_upload_archive },\n \t\t{ \"var\", cmd_var, RUN_SETUP_GENTLY },\n+\t\t{ \"verify-pack\", cmd_verify_pack },\n \t\t{ \"verify-tag\", cmd_verify_tag, RUN_SETUP },\n \t\t{ \"version\", cmd_version },\n \t\t{ \"whatchanged\", cmd_whatchanged, RUN_SETUP },\n \t\t{ \"write-tree\", cmd_write_tree, RUN_SETUP },\n-\t\t{ \"verify-pack\", cmd_verify_pack },\n-\t\t{ \"show-ref\", cmd_show_ref, RUN_SETUP },\n-\t\t{ \"pack-refs\", cmd_pack_refs, RUN_SETUP },\n \t};\n \tint i;\n \tstatic const char ext[] = STRIP_EXTENSION;\n-- \n1.7.4.74.g639db\n"},{"id":"161143","messageId":"1297739344-28961-2-git-send-email-pclouds@gmail.com","threadId":"26494","inReplyTo":"1297739344-28961-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] git.c: binary-search builtin commands","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-15T03:09:04Z","receivedAt":"2011-02-15T03:09:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"An obvious implication of this patch: commands array must be in correct\norder.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n 2011/2/15 Junio C Hamano :\n > I did this myself the other day, as I think it simply is a good project\n > hygiene. If this were 1/2 of a series followed by 2/2 that runs binary\n > search in the table, that would make it make more sense ;-)\n \n I did think the array was binary-searched and nearly claimed \"git-stage\n won't work because it's in wrong order\".\n\n This patch won't give any performance gain, but it would force\n people to keep the array in order :-)\n\n git.c | 15 ++++++++++-----\n 1 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 57701e3..c36117a 100644\n--- a/git.c\n+++ b/git.c\n@@ -308,6 +308,13 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \treturn 0;\n }\n \n+static int cmd_cmp(const void *a, const void *b)\n+{\n+\tconst char *key = a;\n+\tconst struct cmd_struct *cmd = b;\n+\treturn strcmp(key, cmd->cmd);\n+}\n+\n static void handle_internal_command(int argc, const char **argv)\n {\n \tconst char *cmd = argv[0];\n@@ -423,6 +430,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"whatchanged\", cmd_whatchanged, RUN_SETUP },\n \t\t{ \"write-tree\", cmd_write_tree, RUN_SETUP },\n \t};\n+\tstruct cmd_struct *p;\n \tint i;\n \tstatic const char ext[] = STRIP_EXTENSION;\n \n@@ -441,12 +449,9 @@ static void handle_internal_command(int argc, const char **argv)\n \t\targv[0] = cmd = \"help\";\n \t}\n \n-\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n-\t\tstruct cmd_struct *p = commands+i;\n-\t\tif (strcmp(p->cmd, cmd))\n-\t\t\tcontinue;\n+\tp = bsearch(cmd, commands, ARRAY_SIZE(commands), sizeof(*commands), cmd_cmp);\n+\tif (p)\n \t\texit(run_builtin(p, argc, argv));\n-\t}\n }\n \n static void execv_dashed_external(const char **argv)\n-- \n1.7.4.74.g639db\n"},{"id":"161210","messageId":"7v4o85njjz.fsf@alter.siamese.dyndns.org","threadId":"26494","inReplyTo":"1297739344-28961-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] git.c: binary-search builtin commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-15T20:12:48Z","receivedAt":"2011-02-15T20:12:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> An obvious implication of this patch: commands array must be in correct\n> order.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> 2011/2/15 Junio C Hamano :\n> > I did this myself the other day, as I think it simply is a good project\n> > hygiene. If this were 1/2 of a series followed by 2/2 that runs binary\n> > search in the table, that would make it make more sense ;-)\n> \n> I did think the array was binary-searched and nearly claimed \"git-stage\n> won't work because it's in wrong order\".\n\nHeh, that \"binary search\" was a tongue-in-cheek comment. I am sorry that\nyou took it too seriously.\n\n> This patch won't give any performance gain, but it would force\n> people to keep the array in order :-)\n\nThat is exactly why I discarded what I did the other day. Without an\nactive mechanism to force the orderedness, such a change simply introduces\na downside of letting a mistake go unnoticed, without any real upside (as\nyou measured and saw no performance gain).\n\nA better project hygine is a good thing to aim for, and I would imagine\nthat you could add \"--verify-builtin-command-table\" as an unadvertised\noption to \"git\" wrapper, and make t/t0000-basic.sh call it to minimize the\ndownside risk. But without such an active measure to prevent mistakes, we\nwould be relying on somebody getting caught on a ticking bomb and\nreporting it, which is not a good tradeoff between risk and reward.\n"},{"id":"161259","messageId":"AANLkTi=xnM+ejrDMCNBr21BtnqwD5TXXoLZbE7uPMhR0@mail.gmail.com","threadId":"26494","inReplyTo":"7v4o85njjz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] git.c: binary-search builtin commands","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-16T03:46:30Z","receivedAt":"2011-02-16T03:46:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/2/16 Junio C Hamano :\n> Nguyễn Thái Ngọc Duy   writes:\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>>   2011/2/15 Junio C Hamano :\n>>   > I did this myself the other day, as I think it simply is a good project\n>>   > hygiene.  If this were 1/2 of a series followed by 2/2 that runs binary\n>>   > search in the table, that would make it make more sense ;-)\n>>\n>>   I did think the array was binary-searched and nearly claimed \"git-stage\n>>   won't work because it's in wrong order\".\n>\n> Heh, that \"binary search\" was a tongue-in-cheek comment.  I am sorry that\n> you took it too seriously.\n>\n>>   This patch won't give any performance gain, but it would force\n>>   people to keep the array in order :-)\n>\n> That is exactly why I discarded what I did the other day.  Without an\n> active mechanism to force the orderedness, such a change simply introduces\n> a downside of letting a mistake go unnoticed, without any real upside (as\n> you measured and saw no performance gain).\n>\n> A better project hygine is a good thing to aim for, and I would imagine\n> that you could add \"--verify-builtin-command-table\" as an unadvertised\n> option to \"git\" wrapper, and make t/t0000-basic.sh call it to minimize the\n> downside risk.  But without such an active measure to prevent mistakes, we\n> would be relying on somebody getting caught on a ticking bomb and\n> reporting it, which is not a good tradeoff between risk and reward.\n\nAh, OK. Just drop this patch. I don't think doing binary search gains\nus much anyway.\n-- \nDuy\n"}]}