[PATCH v2 0/6] Move the read_tree() function to its only user
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 8, 2021, 02:21 UTC
- Message-ID
- <20210308022138.28166-1-avarab@gmail.com>
- In-Reply-To
- <20210306193458.20633-1-avarab@gmail.com>
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@mail.gmail.com/
Æ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@gmail.com>
## 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@gmail.com>
## 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@gmail.com>
@@ 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