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

From: Elijah Newren <newren@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2 0/6] Move the read_tree() function to its only user
Date: Mon, 8 Mar 2021 11:18:51 -0800	[thread overview]
Message-ID: <CABPp-BEfEQZzymkTGVTJ7cHjaPfXNeek+ikm5C0YYD2YYjpsFw@mail.gmail.com> (raw)
In-Reply-To: <20210308022138.28166-1-avarab@gmail.com>

On Sun, Mar 7, 2021 at 6:22 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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@mail.gmail.com/

This addresses my concerns with v1; so:

Reviewed-by: Elijah Newren <newren@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
>