On Sun, Mar 7, 2021 at 6:22 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > 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/ This addresses my concerns with v1; so: Reviewed-by: Elijah Newren <newren@xxxxxxxxx> > > Æ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 >
