Skip to content

Commit 74ca493

Browse files
gitstergitster
committed
Merge branch 'ss/builtin-cleanup'
"git help $cmd" unnecessarily enumerated potential command names from the filesystem, even when $cmd is known to be a built-in. Ideas for further optimization, primarily by killing the use of is_in_cmdlist(), were suggested in the discussion, but they can come as follow-ups on top of this series. * ss/builtin-cleanup: builtin/help.c: speed up is_git_command() by checking for builtin commands first builtin/help.c: call load_command_list() only when it is needed git.c: consistently use the term "builtin" instead of "internal command"
2 parents 4243b2d + c6127fa commit 74ca493

File tree

4 files changed

+143
-125
lines changed

4 files changed

+143
-125
lines changed

‎Documentation/technical/api-builtin.txt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ Git:
1414

1515
. Add the external declaration for the function to `builtin.h`.
1616

17-
. Add the command to `commands[]` table in `handle_internal_command()`,
18-
defined in `git.c`. The entry should look like:
17+
. Add the command to the `commands[]` table defined in `git.c`.
18+
The entry should look like:
1919

2020
{ "foo", cmd_foo, <options> },
2121
+

‎builtin.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
2727

2828
extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, int sha1_valid, char **buf, unsigned long *buf_size);
2929

30+
extern int is_builtin(const char *s);
31+
3032
extern int cmd_add(int argc, const char **argv, const char *prefix);
3133
extern int cmd_annotate(int argc, const char **argv, const char *prefix);
3234
extern int cmd_apply(int argc, const char **argv, const char *prefix);

‎builtin/help.c‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,10 @@ static struct cmdnames main_cmds, other_cmds;
288288

289289
static int is_git_command(const char *s)
290290
{
291+
if (is_builtin(s))
292+
return 1;
293+
294+
load_command_list("git-", &main_cmds, &other_cmds);
291295
return is_in_cmdlist(&main_cmds, s) ||
292296
is_in_cmdlist(&other_cmds, s);
293297
}
@@ -449,7 +453,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)
449453
int nongit;
450454
const char *alias;
451455
enum help_format parsed_help_format;
452-
load_command_list("git-", &main_cmds, &other_cmds);
453456

454457
argc = parse_options(argc, argv, prefix, builtin_help_options,
455458
builtin_help_usage, 0);
@@ -458,6 +461,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
458461
if (show_all) {
459462
git_config(git_help_config, NULL);
460463
printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
464+
load_command_list("git-", &main_cmds, &other_cmds);
461465
list_commands(colopts, &main_cmds, &other_cmds);
462466
}
463467

‎git.c‎

Lines changed: 134 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -332,124 +332,136 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
332332
return 0;
333333
}
334334

335-
static void handle_internal_command(int argc, const char **argv)
335+
static struct cmd_struct commands[] = {
336+
{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
337+
{ "annotate", cmd_annotate, RUN_SETUP },
338+
{ "apply", cmd_apply, RUN_SETUP_GENTLY },
339+
{ "archive", cmd_archive },
340+
{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },
341+
{ "blame", cmd_blame, RUN_SETUP },
342+
{ "branch", cmd_branch, RUN_SETUP },
343+
{ "bundle", cmd_bundle, RUN_SETUP_GENTLY },
344+
{ "cat-file", cmd_cat_file, RUN_SETUP },
345+
{ "check-attr", cmd_check_attr, RUN_SETUP },
346+
{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
347+
{ "check-mailmap", cmd_check_mailmap, RUN_SETUP },
348+
{ "check-ref-format", cmd_check_ref_format },
349+
{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
350+
{ "checkout-index", cmd_checkout_index,
351+
RUN_SETUP | NEED_WORK_TREE},
352+
{ "cherry", cmd_cherry, RUN_SETUP },
353+
{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
354+
{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
355+
{ "clone", cmd_clone },
356+
{ "column", cmd_column, RUN_SETUP_GENTLY },
357+
{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
358+
{ "commit-tree", cmd_commit_tree, RUN_SETUP },
359+
{ "config", cmd_config, RUN_SETUP_GENTLY },
360+
{ "count-objects", cmd_count_objects, RUN_SETUP },
361+
{ "credential", cmd_credential, RUN_SETUP_GENTLY },
362+
{ "describe", cmd_describe, RUN_SETUP },
363+
{ "diff", cmd_diff },
364+
{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
365+
{ "diff-index", cmd_diff_index, RUN_SETUP },
366+
{ "diff-tree", cmd_diff_tree, RUN_SETUP },
367+
{ "fast-export", cmd_fast_export, RUN_SETUP },
368+
{ "fetch", cmd_fetch, RUN_SETUP },
369+
{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
370+
{ "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
371+
{ "for-each-ref", cmd_for_each_ref, RUN_SETUP },
372+
{ "format-patch", cmd_format_patch, RUN_SETUP },
373+
{ "fsck", cmd_fsck, RUN_SETUP },
374+
{ "fsck-objects", cmd_fsck, RUN_SETUP },
375+
{ "gc", cmd_gc, RUN_SETUP },
376+
{ "get-tar-commit-id", cmd_get_tar_commit_id },
377+
{ "grep", cmd_grep, RUN_SETUP_GENTLY },
378+
{ "hash-object", cmd_hash_object },
379+
{ "help", cmd_help },
380+
{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
381+
{ "init", cmd_init_db },
382+
{ "init-db", cmd_init_db },
383+
{ "log", cmd_log, RUN_SETUP },
384+
{ "ls-files", cmd_ls_files, RUN_SETUP },
385+
{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
386+
{ "ls-tree", cmd_ls_tree, RUN_SETUP },
387+
{ "mailinfo", cmd_mailinfo },
388+
{ "mailsplit", cmd_mailsplit },
389+
{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
390+
{ "merge-base", cmd_merge_base, RUN_SETUP },
391+
{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
392+
{ "merge-index", cmd_merge_index, RUN_SETUP },
393+
{ "merge-ours", cmd_merge_ours, RUN_SETUP },
394+
{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
395+
{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
396+
{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
397+
{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
398+
{ "merge-tree", cmd_merge_tree, RUN_SETUP },
399+
{ "mktag", cmd_mktag, RUN_SETUP },
400+
{ "mktree", cmd_mktree, RUN_SETUP },
401+
{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
402+
{ "name-rev", cmd_name_rev, RUN_SETUP },
403+
{ "notes", cmd_notes, RUN_SETUP },
404+
{ "pack-objects", cmd_pack_objects, RUN_SETUP },
405+
{ "pack-redundant", cmd_pack_redundant, RUN_SETUP },
406+
{ "pack-refs", cmd_pack_refs, RUN_SETUP },
407+
{ "patch-id", cmd_patch_id },
408+
{ "pickaxe", cmd_blame, RUN_SETUP },
409+
{ "prune", cmd_prune, RUN_SETUP },
410+
{ "prune-packed", cmd_prune_packed, RUN_SETUP },
411+
{ "push", cmd_push, RUN_SETUP },
412+
{ "read-tree", cmd_read_tree, RUN_SETUP },
413+
{ "receive-pack", cmd_receive_pack },
414+
{ "reflog", cmd_reflog, RUN_SETUP },
415+
{ "remote", cmd_remote, RUN_SETUP },
416+
{ "remote-ext", cmd_remote_ext },
417+
{ "remote-fd", cmd_remote_fd },
418+
{ "repack", cmd_repack, RUN_SETUP },
419+
{ "replace", cmd_replace, RUN_SETUP },
420+
{ "rerere", cmd_rerere, RUN_SETUP },
421+
{ "reset", cmd_reset, RUN_SETUP },
422+
{ "rev-list", cmd_rev_list, RUN_SETUP },
423+
{ "rev-parse", cmd_rev_parse },
424+
{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
425+
{ "rm", cmd_rm, RUN_SETUP },
426+
{ "send-pack", cmd_send_pack, RUN_SETUP },
427+
{ "shortlog", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },
428+
{ "show", cmd_show, RUN_SETUP },
429+
{ "show-branch", cmd_show_branch, RUN_SETUP },
430+
{ "show-ref", cmd_show_ref, RUN_SETUP },
431+
{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
432+
{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
433+
{ "stripspace", cmd_stripspace },
434+
{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
435+
{ "tag", cmd_tag, RUN_SETUP },
436+
{ "unpack-file", cmd_unpack_file, RUN_SETUP },
437+
{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
438+
{ "update-index", cmd_update_index, RUN_SETUP },
439+
{ "update-ref", cmd_update_ref, RUN_SETUP },
440+
{ "update-server-info", cmd_update_server_info, RUN_SETUP },
441+
{ "upload-archive", cmd_upload_archive },
442+
{ "upload-archive--writer", cmd_upload_archive_writer },
443+
{ "var", cmd_var, RUN_SETUP_GENTLY },
444+
{ "verify-pack", cmd_verify_pack },
445+
{ "verify-tag", cmd_verify_tag, RUN_SETUP },
446+
{ "version", cmd_version },
447+
{ "whatchanged", cmd_whatchanged, RUN_SETUP },
448+
{ "write-tree", cmd_write_tree, RUN_SETUP },
449+
};
450+
451+
int is_builtin(const char *s)
452+
{
453+
int i;
454+
for (i = 0; i < ARRAY_SIZE(commands); i++) {
455+
struct cmd_struct *p = commands+i;
456+
if (!strcmp(s, p->cmd))
457+
return 1;
458+
}
459+
return 0;
460+
}
461+
462+
static void handle_builtin(int argc, const char **argv)
336463
{
337464
const char *cmd = argv[0];
338-
static struct cmd_struct commands[] = {
339-
{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
340-
{ "annotate", cmd_annotate, RUN_SETUP },
341-
{ "apply", cmd_apply, RUN_SETUP_GENTLY },
342-
{ "archive", cmd_archive },
343-
{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },
344-
{ "blame", cmd_blame, RUN_SETUP },
345-
{ "branch", cmd_branch, RUN_SETUP },
346-
{ "bundle", cmd_bundle, RUN_SETUP_GENTLY },
347-
{ "cat-file", cmd_cat_file, RUN_SETUP },
348-
{ "check-attr", cmd_check_attr, RUN_SETUP },
349-
{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
350-
{ "check-mailmap", cmd_check_mailmap, RUN_SETUP },
351-
{ "check-ref-format", cmd_check_ref_format },
352-
{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
353-
{ "checkout-index", cmd_checkout_index,
354-
RUN_SETUP | NEED_WORK_TREE},
355-
{ "cherry", cmd_cherry, RUN_SETUP },
356-
{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
357-
{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
358-
{ "clone", cmd_clone },
359-
{ "column", cmd_column, RUN_SETUP_GENTLY },
360-
{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
361-
{ "commit-tree", cmd_commit_tree, RUN_SETUP },
362-
{ "config", cmd_config, RUN_SETUP_GENTLY },
363-
{ "count-objects", cmd_count_objects, RUN_SETUP },
364-
{ "credential", cmd_credential, RUN_SETUP_GENTLY },
365-
{ "describe", cmd_describe, RUN_SETUP },
366-
{ "diff", cmd_diff },
367-
{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
368-
{ "diff-index", cmd_diff_index, RUN_SETUP },
369-
{ "diff-tree", cmd_diff_tree, RUN_SETUP },
370-
{ "fast-export", cmd_fast_export, RUN_SETUP },
371-
{ "fetch", cmd_fetch, RUN_SETUP },
372-
{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
373-
{ "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
374-
{ "for-each-ref", cmd_for_each_ref, RUN_SETUP },
375-
{ "format-patch", cmd_format_patch, RUN_SETUP },
376-
{ "fsck", cmd_fsck, RUN_SETUP },
377-
{ "fsck-objects", cmd_fsck, RUN_SETUP },
378-
{ "gc", cmd_gc, RUN_SETUP },
379-
{ "get-tar-commit-id", cmd_get_tar_commit_id },
380-
{ "grep", cmd_grep, RUN_SETUP_GENTLY },
381-
{ "hash-object", cmd_hash_object },
382-
{ "help", cmd_help },
383-
{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
384-
{ "init", cmd_init_db },
385-
{ "init-db", cmd_init_db },
386-
{ "log", cmd_log, RUN_SETUP },
387-
{ "ls-files", cmd_ls_files, RUN_SETUP },
388-
{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
389-
{ "ls-tree", cmd_ls_tree, RUN_SETUP },
390-
{ "mailinfo", cmd_mailinfo },
391-
{ "mailsplit", cmd_mailsplit },
392-
{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
393-
{ "merge-base", cmd_merge_base, RUN_SETUP },
394-
{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
395-
{ "merge-index", cmd_merge_index, RUN_SETUP },
396-
{ "merge-ours", cmd_merge_ours, RUN_SETUP },
397-
{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
398-
{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
399-
{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
400-
{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
401-
{ "merge-tree", cmd_merge_tree, RUN_SETUP },
402-
{ "mktag", cmd_mktag, RUN_SETUP },
403-
{ "mktree", cmd_mktree, RUN_SETUP },
404-
{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
405-
{ "name-rev", cmd_name_rev, RUN_SETUP },
406-
{ "notes", cmd_notes, RUN_SETUP },
407-
{ "pack-objects", cmd_pack_objects, RUN_SETUP },
408-
{ "pack-redundant", cmd_pack_redundant, RUN_SETUP },
409-
{ "pack-refs", cmd_pack_refs, RUN_SETUP },
410-
{ "patch-id", cmd_patch_id },
411-
{ "pickaxe", cmd_blame, RUN_SETUP },
412-
{ "prune", cmd_prune, RUN_SETUP },
413-
{ "prune-packed", cmd_prune_packed, RUN_SETUP },
414-
{ "push", cmd_push, RUN_SETUP },
415-
{ "read-tree", cmd_read_tree, RUN_SETUP },
416-
{ "receive-pack", cmd_receive_pack },
417-
{ "reflog", cmd_reflog, RUN_SETUP },
418-
{ "remote", cmd_remote, RUN_SETUP },
419-
{ "remote-ext", cmd_remote_ext },
420-
{ "remote-fd", cmd_remote_fd },
421-
{ "repack", cmd_repack, RUN_SETUP },
422-
{ "replace", cmd_replace, RUN_SETUP },
423-
{ "rerere", cmd_rerere, RUN_SETUP },
424-
{ "reset", cmd_reset, RUN_SETUP },
425-
{ "rev-list", cmd_rev_list, RUN_SETUP },
426-
{ "rev-parse", cmd_rev_parse },
427-
{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
428-
{ "rm", cmd_rm, RUN_SETUP },
429-
{ "send-pack", cmd_send_pack, RUN_SETUP },
430-
{ "shortlog", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },
431-
{ "show", cmd_show, RUN_SETUP },
432-
{ "show-branch", cmd_show_branch, RUN_SETUP },
433-
{ "show-ref", cmd_show_ref, RUN_SETUP },
434-
{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
435-
{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
436-
{ "stripspace", cmd_stripspace },
437-
{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
438-
{ "tag", cmd_tag, RUN_SETUP },
439-
{ "unpack-file", cmd_unpack_file, RUN_SETUP },
440-
{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
441-
{ "update-index", cmd_update_index, RUN_SETUP },
442-
{ "update-ref", cmd_update_ref, RUN_SETUP },
443-
{ "update-server-info", cmd_update_server_info, RUN_SETUP },
444-
{ "upload-archive", cmd_upload_archive },
445-
{ "upload-archive--writer", cmd_upload_archive_writer },
446-
{ "var", cmd_var, RUN_SETUP_GENTLY },
447-
{ "verify-pack", cmd_verify_pack },
448-
{ "verify-tag", cmd_verify_tag, RUN_SETUP },
449-
{ "version", cmd_version },
450-
{ "whatchanged", cmd_whatchanged, RUN_SETUP },
451-
{ "write-tree", cmd_write_tree, RUN_SETUP },
452-
};
453465
int i;
454466
static const char ext[] = STRIP_EXTENSION;
455467

@@ -517,8 +529,8 @@ static int run_argv(int *argcp, const char ***argv)
517529
int done_alias = 0;
518530

519531
while (1) {
520-
/* See if it's an internal command */
521-
handle_internal_command(*argcp, *argv);
532+
/* See if it's a builtin */
533+
handle_builtin(*argcp, *argv);
522534

523535
/* .. then try the external ones */
524536
execv_dashed_external(*argv);
@@ -563,14 +575,14 @@ int main(int argc, char **av)
563575
* - cannot execute it externally (since it would just do
564576
* the same thing over again)
565577
*
566-
* So we just directly call the internal command handler, and
567-
* die if that one cannot handle it.
578+
* So we just directly call the builtin handler, and die if
579+
* that one cannot handle it.
568580
*/
569581
if (starts_with(cmd, "git-")) {
570582
cmd += 4;
571583
argv[0] = cmd;
572-
handle_internal_command(argc, argv);
573-
die("cannot handle %s internally", cmd);
584+
handle_builtin(argc, argv);
585+
die("cannot handle %s as a builtin", cmd);
574586
}
575587

576588
/* Look for flags.. */

0 commit comments

Comments
 (0)