Re: [PATCH 3/7] ls-files: remove cache juggling + sorting
From: Elijah Newren <hidden>
Date: 2021-03-06 21:37:50
On Sat, Mar 6, 2021 at 11:35 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
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().
Removing ce_stage(ce) == 1 and the use of read_one_entry() as the read_tree_fn_t, keeping read_one_entry_quick() as read_tree_fn_t in all cases. This basically means you are assuming there are no entries with ce_stage(ce) == 1 to start with; what if the user calls `ls-files --with-tree` when in the middle of a merge conflict? Will those entries be duplicated and/or overwritten?
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.
Could you spell this out more? I don't understand. The initial index
might be something like (using the syntax of filename, stage):
file 0
random 0
some 0
and after read_tree_recursive() reading into stage 1, wouldn't it be
something like
file 0
random 0
some 0
file 1
some 1
strange 1
when the ordering should be
file 0
file 1
random 0
some 0
some 1
strange 1
?
I'm probably missing something here.
quoted hunk ↗ jump to hunk
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted> --- builtin/ls-files.c | 76 +++++++--------------------------------------- 1 file changed, 11 insertions(+), 65 deletions(-)diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 74d572a3e4a..f5239437809 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c@@ -12,7 +12,6 @@ #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"@@ -421,12 +420,15 @@ 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, - 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 index_state *istate = context; + const char *base = basebuf->buf; + const int baselen = basebuf->len; int len; struct cache_entry *ce;@@ -442,64 +444,7 @@ static int read_one_entry_opt(struct index_state *istate, memcpy(ce->name, base, baselen); memcpy(ce->name + baselen, 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 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->buf, base->len, pathname, - mode, stage, - ADD_CACHE_JUST_APPEND); -} - - -static int read_tree(struct repository *r, struct tree *tree, - struct pathspec *match, struct index_state *istate) -{ - read_tree_fn_t fn = NULL; - int i, err; - - - /* - * 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(r, tree, "", 0, 1, match, fn, istate); - if (fn == read_one_entry || err) - return err; - - /* - * Sort the cache entry -- we need to nuke the cache tree, though. - */ - 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); } /*@@ -540,7 +485,8 @@ 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)) die("unable to read tree entries %s", tree_name); for (i = 0; i < istate->cache_nr; i++) { --2.31.0.rc0.126.g04f22c5b82