v1 of this series over-removed code supporting the "ls-files --with-tree=*" parameter. In v2 there's no change to its behavior, just refactoring away of read_tree() from the tree.c API and the cleanup of read_tree_recursive(). Thanks to Elijah for spotting that. I've added a test at the start of this series that would have caught that regression in v1 (and more). 1. https://lore.kernel.org/git/CABPp-BF982muRS4GO=zYegvetQyrPMwaEM3uEBvcbPRP=krfmQ@xxxxxxxxxxxxxx/ Ævar Arnfjörð Bjarmason (6): ls-files tests: add meaningful --with-tree tests tree.c API: move read_tree() into builtin/ls-files.c ls-files: don't needlessly pass around stage variable ls-files: refactor away read_tree() tree.h API: remove support for starting at prefix != "" tree.h API: remove "stage" parameter from read_tree_recursive() archive.c | 13 +++-- builtin/checkout.c | 4 +- builtin/log.c | 6 +- builtin/ls-files.c | 76 ++++++++++++++++++++++++- builtin/ls-tree.c | 4 +- merge-recursive.c | 4 +- t/t3060-ls-files-with-tree.sh | 41 ++++++++++++++ tree.c | 101 ++-------------------------------- tree.h | 10 +--- 9 files changed, 139 insertions(+), 120 deletions(-) Range-diff: -: ----------- > 1: 6416da0dee2 ls-files tests: add meaningful --with-tree tests 1: 020534164d3 ! 2: 765001b44cd tree.c API: move read_tree() into builtin/ls-files.c @@ tree.c: int cmp_cache_name_compare(const void *a_, const void *b_) ## tree.h ## @@ tree.h: int read_tree_recursive(struct repository *r, + const char *base, int baselen, int stage, const struct pathspec *pathspec, read_tree_fn_t fn, void *context); - +- -int read_tree(struct repository *r, struct tree *tree, - int stage, struct pathspec *pathspec, - struct index_state *istate); 2: 6aa6ba2fbb5 = 3: a71ffba7d04 ls-files: don't needlessly pass around stage variable 3: 4f27e5d2970 ! 4: e78d1810b89 ls-files: remove cache juggling + sorting @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## Commit message ## - ls-files: remove cache juggling + sorting + ls-files: refactor away read_tree() - Remove the "ce_stage(ce) == 1" and "Sort the cache entry" code from - read_tree(), which allows us to remove the function entirely and move - over to read_tree_recursive(). + Refactor away the read_tree() function into its only user, + overlay_tree_on_index(). - I don't think the "Sort the cached entry" code was needed here, see - af3785dc5a7 (Optimize "diff --cached" performance., 2007-08-09) for - the use-case it was intended for. The only user of this code is - "ls-files --with-tree", which isn't the sort of use-case that needs to - care about "ce_stage(ce) != 0" or sorting tree entries. + First, change read_one_entry_opt() to use the strbuf parameter + read_tree_recursive() passes down in place. This finishes up a partial + refactoring started in 6a0b0b6de99 (tree.c: update read_tree_recursive + callback to pass strbuf as base, 2014-11-30). + + Moving the rest into overlay_tree_on_index() makes this index juggling + we're doing easier to read. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/ls-files.c ## -@@ - #include "dir.h" - #include "builtin.h" - #include "tree.h" --#include "cache-tree.h" - #include "parse-options.h" - #include "resolve-undo.h" - #include "string-list.h" @@ builtin/ls-files.c: static int get_common_prefix_len(const char *common_prefix) - return common_prefix_len; - } --static int read_one_entry_opt(struct index_state *istate, -- const struct object_id *oid, + static int read_one_entry_opt(struct index_state *istate, + const struct object_id *oid, - const char *base, int baselen, -- const char *pathname, -- unsigned mode, int stage, int opt) -+static int read_one_entry_quick(const struct object_id *oid, -+ struct strbuf *basebuf, -+ const char *pathname, -+ unsigned mode, -+ int stage, void *context) ++ struct strbuf *base, + const char *pathname, + unsigned mode, int stage, int opt) { -+ struct index_state *istate = context; -+ const char *base = basebuf->buf; -+ const int baselen = basebuf->len; - int len; - struct cache_entry *ce; - @@ builtin/ls-files.c: static int read_one_entry_opt(struct index_state *istate, - memcpy(ce->name, base, baselen); - memcpy(ce->name + baselen, pathname, len+1); + return READ_TREE_RECURSIVE; + + len = strlen(pathname); +- ce = make_empty_cache_entry(istate, baselen + len); ++ ce = make_empty_cache_entry(istate, base->len + len); + + ce->ce_mode = create_ce_mode(mode); + ce->ce_flags = create_ce_flags(stage); +- ce->ce_namelen = baselen + len; +- memcpy(ce->name, base, baselen); +- memcpy(ce->name + baselen, pathname, len+1); ++ ce->ce_namelen = base->len + len; ++ memcpy(ce->name, base->buf, base->len); ++ memcpy(ce->name + base->len, pathname, len+1); oidcpy(&ce->oid, oid); -- return add_index_entry(istate, ce, opt); --} -- --static int read_one_entry(const struct object_id *oid, struct strbuf *base, -- const char *pathname, unsigned mode, int stage, -- void *context) --{ -- struct index_state *istate = context; + return add_index_entry(istate, ce, opt); + } +@@ builtin/ls-files.c: static int read_one_entry(const struct object_id *oid, struct strbuf *base, + void *context) + { + struct index_state *istate = context; - return read_one_entry_opt(istate, oid, base->buf, base->len, pathname, -- mode, stage, -- ADD_CACHE_OK_TO_ADD|ADD_CACHE_SKIP_DFCHECK); --} -- --/* -- * This is used when the caller knows there is no existing entries at -- * the stage that will conflict with the entry being added. -- */ --static int read_one_entry_quick(const struct object_id *oid, struct strbuf *base, -- const char *pathname, unsigned mode, int stage, -- void *context) --{ -- struct index_state *istate = context; ++ return read_one_entry_opt(istate, oid, base, pathname, + mode, stage, + ADD_CACHE_OK_TO_ADD|ADD_CACHE_SKIP_DFCHECK); + } +@@ builtin/ls-files.c: static int read_one_entry_quick(const struct object_id *oid, struct strbuf *base + void *context) + { + struct index_state *istate = context; - return read_one_entry_opt(istate, oid, base->buf, base->len, pathname, -- mode, stage, -- ADD_CACHE_JUST_APPEND); --} -- ++ return read_one_entry_opt(istate, oid, base, pathname, + mode, stage, + ADD_CACHE_JUST_APPEND); + } + - -static int read_tree(struct repository *r, struct tree *tree, - struct pathspec *match, struct index_state *istate) @@ builtin/ls-files.c: static int read_one_entry_opt(struct index_state *istate, - cache_tree_free(&istate->cache_tree); - QSORT(istate->cache, istate->cache_nr, cmp_cache_name_compare); - return 0; -+ return add_index_entry(istate, ce, ADD_CACHE_JUST_APPEND); - } - +-} +- /* + * Read the tree specified with --with-tree option + * (typically, HEAD) into stage #1 and then +@@ builtin/ls-files.c: void overlay_tree_on_index(struct index_state *istate, + struct pathspec pathspec; + struct cache_entry *last_stage0 = NULL; + int i; ++ read_tree_fn_t fn = NULL; ++ int err; + + if (get_oid(tree_name, &oid)) + die("tree-ish %s not found.", tree_name); @@ builtin/ls-files.c: void overlay_tree_on_index(struct index_state *istate, PATHSPEC_PREFER_CWD, prefix, matchbuf); } else memset(&pathspec, 0, sizeof(pathspec)); - if (read_tree(the_repository, tree, &pathspec, istate)) -+ if (read_tree_recursive(the_repository, tree, "", 0, 1, -+ &pathspec, read_one_entry_quick, istate)) ++ ++ /* ++ * See if we have cache entry at the stage. If so, ++ * do it the original slow way, otherwise, append and then ++ * sort at the end. ++ */ ++ for (i = 0; !fn && i < istate->cache_nr; i++) { ++ const struct cache_entry *ce = istate->cache[i]; ++ if (ce_stage(ce) == 1) ++ fn = read_one_entry; ++ } ++ ++ if (!fn) ++ fn = read_one_entry_quick; ++ err = read_tree_recursive(the_repository, tree, "", 0, 1, &pathspec, fn, istate); ++ if (err) die("unable to read tree entries %s", tree_name); ++ /* ++ * Sort the cache entry -- we need to nuke the cache tree, though. ++ */ ++ if (fn == read_one_entry_quick) { ++ cache_tree_free(&istate->cache_tree); ++ QSORT(istate->cache, istate->cache_nr, cmp_cache_name_compare); ++ } ++ for (i = 0; i < istate->cache_nr; i++) { + struct cache_entry *ce = istate->cache[i]; + switch (ce_stage(ce)) { 4: 33810d3c10c < -: ----------- merge-ort: move cmp_cache_name_compare() from tree.c 5: fb10246b85b < -: ----------- ls-files: refactor read_one_entry_quick() to use a strbuf 6: 0c065615aec ! 5: 05eecdd7519 tree.h API: remove support for starting at prefix != "" @@ Metadata ## Commit message ## tree.h API: remove support for starting at prefix != "" - Every caller or the read_tree_recursive() function hardcoded a + Every caller of the read_tree_recursive() function hardcoded a starting point of "" in the tree. So let's simply remove that parameter. - It might be useful in the future to get this functionality back, - there's no reason we won't have a read_tree_recursive() use-case that - would want to start in a subdirectory. + The last function to call read_tree_recursive() with a non-"" path was + read_tree_recursive() itself, but that was changed in + ffd31f661d5 (Reimplement read_tree_recursive() using + tree_entry_interesting(), 2011-03-25). - But if and when that happens we can just add something like a - read_tree_recursive_subdir() and have both read_tree_recursive() and - that function be a thin wrapper for read_tree_1(). + If in the future we need to support recursively reading trees without + starting at the root we can easily add a read_tree_recursive_subdir(), + and make that function a thin wrapper for read_tree_1(). In the meantime there's no reason to keep around what amounts to dead - code just in case we need it in the future. + code, just in case we need it in the future. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> @@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix) ## builtin/ls-files.c ## @@ builtin/ls-files.c: void overlay_tree_on_index(struct index_state *istate, - PATHSPEC_PREFER_CWD, prefix, matchbuf); - } else - memset(&pathspec, 0, sizeof(pathspec)); -- if (read_tree_recursive(the_repository, tree, "", 0, 1, -+ if (read_tree_recursive(the_repository, tree, 1, - &pathspec, read_one_entry_quick, istate)) + + if (!fn) + fn = read_one_entry_quick; +- err = read_tree_recursive(the_repository, tree, "", 0, 1, &pathspec, fn, istate); ++ err = read_tree_recursive(the_repository, tree, 1, &pathspec, fn, istate); + if (err) die("unable to read tree entries %s", tree_name); @@ tree.h: typedef int (*read_tree_fn_t)(const struct object_id *, struct strbuf *, - const char *base, int baselen, int stage, const struct pathspec *pathspec, read_tree_fn_t fn, void *context); - + #endif /* TREE_H */ 7: 9685c7c5c50 ! 6: fcecc82e1c8 tree.h API: remove "stage" parameter from read_tree_recursive() @@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix) break; ## builtin/ls-files.c ## -@@ builtin/ls-files.c: static int read_one_entry_quick(const struct object_id *oid, - struct strbuf *base, - const char *pathname, - unsigned mode, -- int stage, void *context) -+ void *context) +@@ builtin/ls-files.c: static int read_one_entry_opt(struct index_state *istate, + const struct object_id *oid, + struct strbuf *base, + const char *pathname, +- unsigned mode, int stage, int opt) ++ unsigned mode, int opt) { - struct index_state *istate = context; int len; -@@ builtin/ls-files.c: static int read_one_entry_quick(const struct object_id *oid, + struct cache_entry *ce; +@@ builtin/ls-files.c: static int read_one_entry_opt(struct index_state *istate, ce = make_empty_cache_entry(istate, base->len + len); ce->ce_mode = create_ce_mode(mode); @@ builtin/ls-files.c: static int read_one_entry_quick(const struct object_id *oid, ce->ce_namelen = base->len + len; memcpy(ce->name, base->buf, base->len); memcpy(ce->name + base->len, pathname, len+1); +@@ builtin/ls-files.c: static int read_one_entry_opt(struct index_state *istate, + } + + static int read_one_entry(const struct object_id *oid, struct strbuf *base, +- const char *pathname, unsigned mode, int stage, ++ const char *pathname, unsigned mode, + void *context) + { + struct index_state *istate = context; + return read_one_entry_opt(istate, oid, base, pathname, +- mode, stage, ++ mode, + ADD_CACHE_OK_TO_ADD|ADD_CACHE_SKIP_DFCHECK); + } + +@@ builtin/ls-files.c: static int read_one_entry(const struct object_id *oid, struct strbuf *base, + * the stage that will conflict with the entry being added. + */ + static int read_one_entry_quick(const struct object_id *oid, struct strbuf *base, +- const char *pathname, unsigned mode, int stage, ++ const char *pathname, unsigned mode, + void *context) + { + struct index_state *istate = context; + return read_one_entry_opt(istate, oid, base, pathname, +- mode, stage, ++ mode, + ADD_CACHE_JUST_APPEND); + } + @@ builtin/ls-files.c: void overlay_tree_on_index(struct index_state *istate, - PATHSPEC_PREFER_CWD, prefix, matchbuf); - } else - memset(&pathspec, 0, sizeof(pathspec)); -- if (read_tree_recursive(the_repository, tree, 1, -+ if (read_tree_recursive(the_repository, tree, - &pathspec, read_one_entry_quick, istate)) + + if (!fn) + fn = read_one_entry_quick; +- err = read_tree_recursive(the_repository, tree, 1, &pathspec, fn, istate); ++ err = read_tree_recursive(the_repository, tree, &pathspec, fn, istate); + if (err) die("unable to read tree entries %s", tree_name); @@ tree.c: static int read_tree_1(struct repository *r, } ## tree.h ## -@@ tree.h: void free_tree_buffer(struct tree *tree); - struct tree *parse_tree_indirect(const struct object_id *oid); +@@ tree.h: struct tree *parse_tree_indirect(const struct object_id *oid); + int cmp_cache_name_compare(const void *a_, const void *b_); #define READ_TREE_RECURSIVE 1 -typedef int (*read_tree_fn_t)(const struct object_id *, struct strbuf *, const char *, unsigned int, int, void *); @@ tree.h: void free_tree_buffer(struct tree *tree); - int stage, const struct pathspec *pathspec, + const struct pathspec *pathspec, read_tree_fn_t fn, void *context); - #endif /* TREE_H */ -- 2.31.0.rc0.126.g04f22c5b82
