* [PATCH] git.c: reorder builtin command list @ 2011-02-14 13:26 Nguyễn Thái Ngọc Duy 2011-02-14 19:20 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2011-02-14 13:26 UTC (permalink / raw) To: git; +Cc: Nguyễn Thái Ngọc Duy The majority of commands is in alphabet order except some. Reorder them so it's easier to locate a command by (my trained) eye. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- I originally wanted to put "stage" in alphabet order. It was near "add" probably because it's an alias of "add". But the rest was in alphabet order.. Then I wondered if there were any other out-of-order commands... A worthless OCD patch in a sense. git.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-) diff --git a/git.c b/git.c index 23610aa..17ce68c 100644 --- a/git.c +++ b/git.c @@ -313,7 +313,6 @@ static void handle_internal_command(int argc, const char **argv) const char *cmd = argv[0]; static struct cmd_struct commands[] = { { "add", cmd_add, RUN_SETUP | NEED_WORK_TREE }, - { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE }, { "annotate", cmd_annotate, RUN_SETUP }, { "apply", cmd_apply, RUN_SETUP_GENTLY }, { "archive", cmd_archive }, @@ -325,12 +324,12 @@ static void handle_internal_command(int argc, const char **argv) { "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE }, { "checkout-index", cmd_checkout_index, RUN_SETUP | NEED_WORK_TREE}, - { "check-ref-format", cmd_check_ref_format }, { "check-attr", cmd_check_attr, RUN_SETUP }, + { "check-ref-format", cmd_check_ref_format }, { "cherry", cmd_cherry, RUN_SETUP }, { "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE }, - { "clone", cmd_clone }, { "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE }, + { "clone", cmd_clone }, { "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE }, { "commit-tree", cmd_commit_tree, RUN_SETUP }, { "config", cmd_config, RUN_SETUP_GENTLY }, @@ -358,8 +357,8 @@ static void handle_internal_command(int argc, const char **argv) { "init-db", cmd_init_db }, { "log", cmd_log, RUN_SETUP }, { "ls-files", cmd_ls_files, RUN_SETUP }, - { "ls-tree", cmd_ls_tree, RUN_SETUP }, { "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY }, + { "ls-tree", cmd_ls_tree, RUN_SETUP }, { "mailinfo", cmd_mailinfo }, { "mailsplit", cmd_mailsplit }, { "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE }, @@ -379,6 +378,7 @@ static void handle_internal_command(int argc, const char **argv) { "notes", cmd_notes, RUN_SETUP }, { "pack-objects", cmd_pack_objects, RUN_SETUP }, { "pack-redundant", cmd_pack_redundant, RUN_SETUP }, + { "pack-refs", cmd_pack_refs, RUN_SETUP }, { "patch-id", cmd_patch_id }, { "peek-remote", cmd_ls_remote, RUN_SETUP_GENTLY }, { "pickaxe", cmd_blame, RUN_SETUP }, @@ -401,8 +401,10 @@ static void handle_internal_command(int argc, const char **argv) { "rm", cmd_rm, RUN_SETUP }, { "send-pack", cmd_send_pack, RUN_SETUP }, { "shortlog", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER }, - { "show-branch", cmd_show_branch, RUN_SETUP }, { "show", cmd_show, RUN_SETUP }, + { "show-branch", cmd_show_branch, RUN_SETUP }, + { "show-ref", cmd_show_ref, RUN_SETUP }, + { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE }, { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE }, { "stripspace", cmd_stripspace }, { "symbolic-ref", cmd_symbolic_ref, RUN_SETUP }, @@ -415,13 +417,11 @@ static void handle_internal_command(int argc, const char **argv) { "update-server-info", cmd_update_server_info, RUN_SETUP }, { "upload-archive", cmd_upload_archive }, { "var", cmd_var, RUN_SETUP_GENTLY }, + { "verify-pack", cmd_verify_pack }, { "verify-tag", cmd_verify_tag, RUN_SETUP }, { "version", cmd_version }, { "whatchanged", cmd_whatchanged, RUN_SETUP }, { "write-tree", cmd_write_tree, RUN_SETUP }, - { "verify-pack", cmd_verify_pack }, - { "show-ref", cmd_show_ref, RUN_SETUP }, - { "pack-refs", cmd_pack_refs, RUN_SETUP }, }; int i; static const char ext[] = STRIP_EXTENSION; -- 1.7.4.74.g639db ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] git.c: reorder builtin command list 2011-02-14 13:26 [PATCH] git.c: reorder builtin command list Nguyễn Thái Ngọc Duy @ 2011-02-14 19:20 ` Junio C Hamano 2011-02-15 3:09 ` [PATCH 1/2] " Nguyễn Thái Ngọc Duy 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2011-02-14 19:20 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > The majority of commands is in alphabet order except some. Reorder > them so it's easier to locate a command by (my trained) eye. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > I originally wanted to put "stage" in alphabet order. It was near > "add" probably because it's an alias of "add". But the rest was in > alphabet order.. Then I wondered if there were any other out-of-order > commands... > > A worthless OCD patch in a sense. I did this myself the other day, as I think it simply is a good project hygiene. If this were 1/2 of a series followed by 2/2 that runs binary search in the table, that would make it make more sense ;-) Will apply. I don't think there is anything in-flight to conflict with it at this moment. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] git.c: reorder builtin command list 2011-02-14 19:20 ` Junio C Hamano @ 2011-02-15 3:09 ` Nguyễn Thái Ngọc Duy 2011-02-15 3:09 ` [PATCH 2/2] git.c: binary-search builtin commands Nguyễn Thái Ngọc Duy 0 siblings, 1 reply; 6+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2011-02-15 3:09 UTC (permalink / raw) To: git, Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy The majority of commands is in alphabet order except some. Reorder them so it's easier to locate a command by eye and able to binary search. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- check-ref-format and check-attr are moved up compared to previous patch. I checked the order with qsort() so it should be correct. git.c | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-) diff --git a/git.c b/git.c index 23610aa..57701e3 100644 --- a/git.c +++ b/git.c @@ -313,7 +313,6 @@ static void handle_internal_command(int argc, const char **argv) const char *cmd = argv[0]; static struct cmd_struct commands[] = { { "add", cmd_add, RUN_SETUP | NEED_WORK_TREE }, - { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE }, { "annotate", cmd_annotate, RUN_SETUP }, { "apply", cmd_apply, RUN_SETUP_GENTLY }, { "archive", cmd_archive }, @@ -322,15 +321,15 @@ static void handle_internal_command(int argc, const char **argv) { "branch", cmd_branch, RUN_SETUP }, { "bundle", cmd_bundle, RUN_SETUP_GENTLY }, { "cat-file", cmd_cat_file, RUN_SETUP }, + { "check-attr", cmd_check_attr, RUN_SETUP }, + { "check-ref-format", cmd_check_ref_format }, { "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE }, { "checkout-index", cmd_checkout_index, RUN_SETUP | NEED_WORK_TREE}, - { "check-ref-format", cmd_check_ref_format }, - { "check-attr", cmd_check_attr, RUN_SETUP }, { "cherry", cmd_cherry, RUN_SETUP }, { "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE }, - { "clone", cmd_clone }, { "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE }, + { "clone", cmd_clone }, { "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE }, { "commit-tree", cmd_commit_tree, RUN_SETUP }, { "config", cmd_config, RUN_SETUP_GENTLY }, @@ -358,8 +357,8 @@ static void handle_internal_command(int argc, const char **argv) { "init-db", cmd_init_db }, { "log", cmd_log, RUN_SETUP }, { "ls-files", cmd_ls_files, RUN_SETUP }, - { "ls-tree", cmd_ls_tree, RUN_SETUP }, { "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY }, + { "ls-tree", cmd_ls_tree, RUN_SETUP }, { "mailinfo", cmd_mailinfo }, { "mailsplit", cmd_mailsplit }, { "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE }, @@ -379,6 +378,7 @@ static void handle_internal_command(int argc, const char **argv) { "notes", cmd_notes, RUN_SETUP }, { "pack-objects", cmd_pack_objects, RUN_SETUP }, { "pack-redundant", cmd_pack_redundant, RUN_SETUP }, + { "pack-refs", cmd_pack_refs, RUN_SETUP }, { "patch-id", cmd_patch_id }, { "peek-remote", cmd_ls_remote, RUN_SETUP_GENTLY }, { "pickaxe", cmd_blame, RUN_SETUP }, @@ -401,8 +401,10 @@ static void handle_internal_command(int argc, const char **argv) { "rm", cmd_rm, RUN_SETUP }, { "send-pack", cmd_send_pack, RUN_SETUP }, { "shortlog", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER }, - { "show-branch", cmd_show_branch, RUN_SETUP }, { "show", cmd_show, RUN_SETUP }, + { "show-branch", cmd_show_branch, RUN_SETUP }, + { "show-ref", cmd_show_ref, RUN_SETUP }, + { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE }, { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE }, { "stripspace", cmd_stripspace }, { "symbolic-ref", cmd_symbolic_ref, RUN_SETUP }, @@ -415,13 +417,11 @@ static void handle_internal_command(int argc, const char **argv) { "update-server-info", cmd_update_server_info, RUN_SETUP }, { "upload-archive", cmd_upload_archive }, { "var", cmd_var, RUN_SETUP_GENTLY }, + { "verify-pack", cmd_verify_pack }, { "verify-tag", cmd_verify_tag, RUN_SETUP }, { "version", cmd_version }, { "whatchanged", cmd_whatchanged, RUN_SETUP }, { "write-tree", cmd_write_tree, RUN_SETUP }, - { "verify-pack", cmd_verify_pack }, - { "show-ref", cmd_show_ref, RUN_SETUP }, - { "pack-refs", cmd_pack_refs, RUN_SETUP }, }; int i; static const char ext[] = STRIP_EXTENSION; -- 1.7.4.74.g639db ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] git.c: binary-search builtin commands 2011-02-15 3:09 ` [PATCH 1/2] " Nguyễn Thái Ngọc Duy @ 2011-02-15 3:09 ` Nguyễn Thái Ngọc Duy 2011-02-15 20:12 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2011-02-15 3:09 UTC (permalink / raw) To: git, Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy An obvious implication of this patch: commands array must be in correct order. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- 2011/2/15 Junio C Hamano <gitster@pobox.com>: > I did this myself the other day, as I think it simply is a good project > hygiene. If this were 1/2 of a series followed by 2/2 that runs binary > search in the table, that would make it make more sense ;-) I did think the array was binary-searched and nearly claimed "git-stage won't work because it's in wrong order". This patch won't give any performance gain, but it would force people to keep the array in order :-) git.c | 15 ++++++++++----- 1 files changed, 10 insertions(+), 5 deletions(-) diff --git a/git.c b/git.c index 57701e3..c36117a 100644 --- a/git.c +++ b/git.c @@ -308,6 +308,13 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) return 0; } +static int cmd_cmp(const void *a, const void *b) +{ + const char *key = a; + const struct cmd_struct *cmd = b; + return strcmp(key, cmd->cmd); +} + static void handle_internal_command(int argc, const char **argv) { const char *cmd = argv[0]; @@ -423,6 +430,7 @@ static void handle_internal_command(int argc, const char **argv) { "whatchanged", cmd_whatchanged, RUN_SETUP }, { "write-tree", cmd_write_tree, RUN_SETUP }, }; + struct cmd_struct *p; int i; static const char ext[] = STRIP_EXTENSION; @@ -441,12 +449,9 @@ static void handle_internal_command(int argc, const char **argv) argv[0] = cmd = "help"; } - for (i = 0; i < ARRAY_SIZE(commands); i++) { - struct cmd_struct *p = commands+i; - if (strcmp(p->cmd, cmd)) - continue; + p = bsearch(cmd, commands, ARRAY_SIZE(commands), sizeof(*commands), cmd_cmp); + if (p) exit(run_builtin(p, argc, argv)); - } } static void execv_dashed_external(const char **argv) -- 1.7.4.74.g639db ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] git.c: binary-search builtin commands 2011-02-15 3:09 ` [PATCH 2/2] git.c: binary-search builtin commands Nguyễn Thái Ngọc Duy @ 2011-02-15 20:12 ` Junio C Hamano 2011-02-16 3:46 ` Nguyen Thai Ngoc Duy 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2011-02-15 20:12 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > An obvious implication of this patch: commands array must be in correct > order. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > 2011/2/15 Junio C Hamano <gitster@pobox.com>: > > I did this myself the other day, as I think it simply is a good project > > hygiene. If this were 1/2 of a series followed by 2/2 that runs binary > > search in the table, that would make it make more sense ;-) > > I did think the array was binary-searched and nearly claimed "git-stage > won't work because it's in wrong order". Heh, that "binary search" was a tongue-in-cheek comment. I am sorry that you took it too seriously. > This patch won't give any performance gain, but it would force > people to keep the array in order :-) That is exactly why I discarded what I did the other day. Without an active mechanism to force the orderedness, such a change simply introduces a downside of letting a mistake go unnoticed, without any real upside (as you measured and saw no performance gain). A better project hygine is a good thing to aim for, and I would imagine that you could add "--verify-builtin-command-table" as an unadvertised option to "git" wrapper, and make t/t0000-basic.sh call it to minimize the downside risk. But without such an active measure to prevent mistakes, we would be relying on somebody getting caught on a ticking bomb and reporting it, which is not a good tradeoff between risk and reward. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] git.c: binary-search builtin commands 2011-02-15 20:12 ` Junio C Hamano @ 2011-02-16 3:46 ` Nguyen Thai Ngoc Duy 0 siblings, 0 replies; 6+ messages in thread From: Nguyen Thai Ngoc Duy @ 2011-02-16 3:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 2011/2/16 Junio C Hamano <gitster@pobox.com>: > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> >> --- >> 2011/2/15 Junio C Hamano <gitster@pobox.com>: >> > I did this myself the other day, as I think it simply is a good project >> > hygiene. If this were 1/2 of a series followed by 2/2 that runs binary >> > search in the table, that would make it make more sense ;-) >> >> I did think the array was binary-searched and nearly claimed "git-stage >> won't work because it's in wrong order". > > Heh, that "binary search" was a tongue-in-cheek comment. I am sorry that > you took it too seriously. > >> This patch won't give any performance gain, but it would force >> people to keep the array in order :-) > > That is exactly why I discarded what I did the other day. Without an > active mechanism to force the orderedness, such a change simply introduces > a downside of letting a mistake go unnoticed, without any real upside (as > you measured and saw no performance gain). > > A better project hygine is a good thing to aim for, and I would imagine > that you could add "--verify-builtin-command-table" as an unadvertised > option to "git" wrapper, and make t/t0000-basic.sh call it to minimize the > downside risk. But without such an active measure to prevent mistakes, we > would be relying on somebody getting caught on a ticking bomb and > reporting it, which is not a good tradeoff between risk and reward. Ah, OK. Just drop this patch. I don't think doing binary search gains us much anyway. -- Duy ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-02-16 3:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-02-14 13:26 [PATCH] git.c: reorder builtin command list Nguyễn Thái Ngọc Duy 2011-02-14 19:20 ` Junio C Hamano 2011-02-15 3:09 ` [PATCH 1/2] " Nguyễn Thái Ngọc Duy 2011-02-15 3:09 ` [PATCH 2/2] git.c: binary-search builtin commands Nguyễn Thái Ngọc Duy 2011-02-15 20:12 ` Junio C Hamano 2011-02-16 3:46 ` Nguyen Thai Ngoc Duy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).