[PATCH v2 0/6] Move the read_tree() function to its only user

["Ævar Arnfjörð Bjarmason" <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