* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.