{"thread":{"id":"14612","subject":"[PATCH 1/2] builtin-add.c: restructure the code for maintainability","startedAt":"2008-07-23T07:01:41Z","lastAt":"2008-07-23T07:01:42Z","messageCount":2,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"84441","messageId":"1216796502-11227-1-git-send-email-gitster@pobox.com","threadId":"14612","inReplyTo":null,"subject":"[PATCH 1/2] builtin-add.c: restructure the code for maintainability","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-23T07:01:41Z","receivedAt":"2008-07-23T07:01:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A private function add_files_to_cache() in builtin-add.c was borrowed by\ncheckout and commit re-implementors without getting properly refactored to\nmore library-ish place.  This does the refactoring.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * This is just code movement.\n\n builtin-add.c |   57 -----------------------------------------------------\n cache.h       |    1 +\n read-cache.c  |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 62 insertions(+), 57 deletions(-)\n\ndiff --git a/builtin-add.c b/builtin-add.c\nindex fc3f96e..0de516a 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -8,10 +8,6 @@\n #include \"dir.h\"\n #include \"exec_cmd.h\"\n #include \"cache-tree.h\"\n-#include \"diff.h\"\n-#include \"diffcore.h\"\n-#include \"commit.h\"\n-#include \"revision.h\"\n #include \"run-command.h\"\n #include \"parse-options.h\"\n \n@@ -79,59 +75,6 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec,\n \t\tprune_directory(dir, pathspec, baselen);\n }\n \n-struct update_callback_data\n-{\n-\tint flags;\n-\tint add_errors;\n-};\n-\n-static void update_callback(struct diff_queue_struct *q,\n-\t\t\t    struct diff_options *opt, void *cbdata)\n-{\n-\tint i;\n-\tstruct update_callback_data *data = cbdata;\n-\n-\tfor (i = 0; i < q->nr; i++) {\n-\t\tstruct diff_filepair *p = q->queue[i];\n-\t\tconst char *path = p->one->path;\n-\t\tswitch (p->status) {\n-\t\tdefault:\n-\t\t\tdie(\"unexpected diff status %c\", p->status);\n-\t\tcase DIFF_STATUS_UNMERGED:\n-\t\tcase DIFF_STATUS_MODIFIED:\n-\t\tcase DIFF_STATUS_TYPE_CHANGED:\n-\t\t\tif (add_file_to_cache(path, data->flags)) {\n-\t\t\t\tif (!(data->flags & ADD_CACHE_IGNORE_ERRORS))\n-\t\t\t\t\tdie(\"updating files failed\");\n-\t\t\t\tdata->add_errors++;\n-\t\t\t}\n-\t\t\tbreak;\n-\t\tcase DIFF_STATUS_DELETED:\n-\t\t\tif (!(data->flags & ADD_CACHE_PRETEND))\n-\t\t\t\tremove_file_from_cache(path);\n-\t\t\tif (data->flags & (ADD_CACHE_PRETEND|ADD_CACHE_VERBOSE))\n-\t\t\t\tprintf(\"remove '%s'\\n\", path);\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-}\n-\n-int add_files_to_cache(const char *prefix, const char **pathspec, int flags)\n-{\n-\tstruct update_callback_data data;\n-\tstruct rev_info rev;\n-\tinit_revisions(&rev, prefix);\n-\tsetup_revisions(0, NULL, &rev, NULL);\n-\trev.prune_data = pathspec;\n-\trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n-\trev.diffopt.format_callback = update_callback;\n-\tdata.flags = flags;\n-\tdata.add_errors = 0;\n-\trev.diffopt.format_callback_data = &data;\n-\trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n-\treturn !!data.add_errors;\n-}\n-\n static void refresh(int verbose, const char **pathspec)\n {\n \tchar *seen;\ndiff --git a/cache.h b/cache.h\nindex 38985aa..6f374ad 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -375,6 +375,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);\n #define ADD_CACHE_VERBOSE 1\n #define ADD_CACHE_PRETEND 2\n #define ADD_CACHE_IGNORE_ERRORS\t4\n+#define ADD_CACHE_IGNORE_REMOVAL 8\n extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);\n extern int add_file_to_index(struct index_state *, const char *path, int flags);\n extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh);\ndiff --git a/read-cache.c b/read-cache.c\nindex a50a851..6833af6 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -8,6 +8,11 @@\n #include \"cache-tree.h\"\n #include \"refs.h\"\n #include \"dir.h\"\n+#include \"tree.h\"\n+#include \"commit.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n+#include \"revision.h\"\n \n /* Index extensions.\n  *\n@@ -1444,3 +1449,59 @@ int read_index_unmerged(struct index_state *istate)\n \tistate->cache_nr = dst - istate->cache;\n \treturn !!last;\n }\n+\n+struct update_callback_data\n+{\n+\tint flags;\n+\tint add_errors;\n+};\n+\n+static void update_callback(struct diff_queue_struct *q,\n+\t\t\t    struct diff_options *opt, void *cbdata)\n+{\n+\tint i;\n+\tstruct update_callback_data *data = cbdata;\n+\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tconst char *path = p->one->path;\n+\t\tswitch (p->status) {\n+\t\tdefault:\n+\t\t\tdie(\"unexpected diff status %c\", p->status);\n+\t\tcase DIFF_STATUS_UNMERGED:\n+\t\tcase DIFF_STATUS_MODIFIED:\n+\t\tcase DIFF_STATUS_TYPE_CHANGED:\n+\t\t\tif (add_file_to_index(&the_index, path, data->flags)) {\n+\t\t\t\tif (!(data->flags & ADD_CACHE_IGNORE_ERRORS))\n+\t\t\t\t\tdie(\"updating files failed\");\n+\t\t\t\tdata->add_errors++;\n+\t\t\t}\n+\t\t\tbreak;\n+\t\tcase DIFF_STATUS_DELETED:\n+\t\t\tif (data->flags & ADD_CACHE_IGNORE_REMOVAL)\n+\t\t\t\tbreak;\n+\t\t\tif (!(data->flags & ADD_CACHE_PRETEND))\n+\t\t\t\tremove_file_from_index(&the_index, path);\n+\t\t\tif (data->flags & (ADD_CACHE_PRETEND|ADD_CACHE_VERBOSE))\n+\t\t\t\tprintf(\"remove '%s'\\n\", path);\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+}\n+\n+int add_files_to_cache(const char *prefix, const char **pathspec, int flags)\n+{\n+\tstruct update_callback_data data;\n+\tstruct rev_info rev;\n+\tinit_revisions(&rev, prefix);\n+\tsetup_revisions(0, NULL, &rev, NULL);\n+\trev.prune_data = pathspec;\n+\trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n+\trev.diffopt.format_callback = update_callback;\n+\tdata.flags = flags;\n+\tdata.add_errors = 0;\n+\trev.diffopt.format_callback_data = &data;\n+\trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n+\treturn !!data.add_errors;\n+}\n+\n-- \n1.6.0.rc0.15.g0dda1\n"},{"id":"84442","messageId":"1216796502-11227-2-git-send-email-gitster@pobox.com","threadId":"14612","inReplyTo":"1216796502-11227-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/2] builtin-add.c: optimize -A option and \"git add $paths...\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-23T07:01:42Z","receivedAt":"2008-07-23T07:01:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The earlier \"git add -A\" change was done in a quite inefficient\nway (i.e. it is as unefficient as \"git add -u && git add .\" modulo\none fork/exec and read/write index).\n\nFor that matter, the original \"git add $paths...\" could be improved.\n\nWhen the user asks \"git add .\", we do not have to examine all paths\nwe encounter and perform the excluded() and dir_add_name()\nprocessing, both of which are slower code and use slower data structure\nby git standards, especially when the index is already reasonably well\npopulated.\n\nInstead, we implement \"git add $pathspec...\" as:\n\n - read the index;\n\n - read_directory() to process untracked, unignored files the current\n   way, that is, recursively doing readdir(), filtering them by pathspec\n   and excluded(), queueing them via dir_add_name() and finally do\n   add_files(); and\n\n - iterate over the index, filtering them by pathspec, and update only\n   the modified/type changed paths but not deleted ones.\n\nAnd \"git add -A\" becomes exactly the same as above, modulo:\n\n - missing $pathspec means \".\" instead of being an error; and\n\n - \"iterate over the index\" part handles deleted ones as well,\n   i.e. exactly what the current update_callback() in builtin-add.c does.\n\nIn either case, because fill_directory() does not use read_directory() to\nread everything in, we need to add an extra logic to iterate over the\nindex to catch mistyped pathspec.\n\nIn a fully populated kernel tree (without any local change), best of ten\nruns:\n\n(with this patch)\n$ /usr/bin/time ~/git-test/bin/git add .\n0.16user 0.17system 0:00.34elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+3231minor)pagefaults 0swaps\n\n(without this patch)\n$ /usr/bin/time ~/git-master/bin/git add .\n0.21user 0.21system 0:00.42elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+3647minor)pagefaults 0swaps\n\nUnder the same condition, but after \"rm .git/index\" to force re-indexing\neverything (the patch should not make any difference):\n\n(with this patch)\n$ /usr/bin/time ~/git-test/bin/git add .\n3.22user 2.10system 1:35.10elapsed 5%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (30037major+63003minor)pagefaults 0swaps\n\n(without this patch)\n$ /usr/bin/time ~/git-master/bin/git add .\n3.31user 2.28system 1:44.77elapsed 5%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (30472major+62563minor)pagefaults 0swaps\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-add.c |   43 +++++++++++++++++++++++++++++++------------\n 1 files changed, 31 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin-add.c b/builtin-add.c\nindex 0de516a..1834e2d 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -18,6 +18,27 @@ static const char * const builtin_add_usage[] = {\n static int patch_interactive = 0, add_interactive = 0;\n static int take_worktree_changes;\n \n+static void fill_pathspec_matches(const char **pathspec, char *seen, int specs)\n+{\n+\tint num_unmatched = 0, i;\n+\n+\t/*\n+\t * Since we are walking the index as if we are warlking the directory,\n+\t * we have to mark the matched pathspec as seen; otherwise we will\n+\t * mistakenly think that the user gave a pathspec that did not match\n+\t * anything.\n+\t */\n+\tfor (i = 0; i < specs; i++)\n+\t\tif (!seen[i])\n+\t\t\tnum_unmatched++;\n+\tif (!num_unmatched)\n+\t\treturn;\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tstruct cache_entry *ce = active_cache[i];\n+\t\tmatch_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen);\n+\t}\n+}\n+\n static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)\n {\n \tchar *seen;\n@@ -37,6 +58,7 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p\n \t\t\t*dst++ = entry;\n \t}\n \tdir->nr = dst - dir->entries;\n+\tfill_pathspec_matches(pathspec, seen, specs);\n \n \tfor (i = 0; i < specs; i++) {\n \t\tif (!seen[i] && !file_exists(pathspec[i]))\n@@ -201,7 +223,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tif (addremove && take_worktree_changes)\n \t\tdie(\"-A and -u are mutually incompatible\");\n-\tif (addremove && !argc) {\n+\tif ((addremove || take_worktree_changes) && !argc) {\n \t\tstatic const char *here[2] = { \".\", NULL };\n \t\targc = 1;\n \t\targv = here;\n@@ -214,7 +236,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tflags = ((verbose ? ADD_CACHE_VERBOSE : 0) |\n \t\t (show_only ? ADD_CACHE_PRETEND : 0) |\n-\t\t (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0));\n+\t\t (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |\n+\t\t (!(addremove || take_worktree_changes)\n+\t\t  ? ADD_CACHE_IGNORE_REMOVAL : 0));\n \n \tif (require_pathspec && argc == 0) {\n \t\tfprintf(stderr, \"Nothing specified, nothing added.\\n\");\n@@ -223,24 +247,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t}\n \tpathspec = get_pathspec(prefix, argv);\n \n-\t/*\n-\t * If we are adding new files, we need to scan the working\n-\t * tree to find the ones that match pathspecs; this needs\n-\t * to be done before we read the index.\n-\t */\n-\tif (add_new_files)\n-\t\tfill_directory(&dir, pathspec, ignored_too);\n-\n \tif (read_cache() < 0)\n \t\tdie(\"index file corrupt\");\n \n+\tif (add_new_files)\n+\t\t/* This picks up the paths that are not tracked */\n+\t\tfill_directory(&dir, pathspec, ignored_too);\n+\n \tif (refresh_only) {\n \t\trefresh(verbose, pathspec);\n \t\tgoto finish;\n \t}\n \n-\tif (take_worktree_changes || addremove)\n-\t\texit_status |= add_files_to_cache(prefix, pathspec, flags);\n+\texit_status |= add_files_to_cache(prefix, pathspec, flags);\n \n \tif (add_new_files)\n \t\texit_status |= add_files(&dir, flags);\n-- \n1.6.0.rc0.15.g0dda1\n"}]}