[v5,0/8] read_tree() and read_tree_recursive() refactoring

Message

Ævar Arnfjörð Bjarmason March 20, 2021, 10:37 p.m. UTC

A refactoring of v4[1], not quite as as suggested by Junio in[2], but
which I think makes this whole thing less confusing and more
straightforward.

I now start by entirely getting rid of the meaningful passing of the
"stage" variable before removing it and the path variable entirely in
8/8, but before that in 7/8 I've introduced the read_tree_at()
function Derrick Stolee needs for his in-flight series.

1. http://lore.kernel.org/git/[e-mail]
2. https://lore.kernel.org/git/xmqqpmzxy939.fsf@gitster.g/

Ævar Arnfjörð Bjarmason (8):
  show tests: add test for "git show <tree>"
  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()
  archive: stop passing "stage" through read_tree_recursive()
  tree.h API: expose read_tree_1() as read_tree_at()
  tree.h API: simplify read_tree_recursive() signature

 archive.c                     |  30 ++++-----
 builtin/checkout.c            |   8 +--
 builtin/log.c                 |   8 +--
 builtin/ls-files.c            |  75 ++++++++++++++++++++-
 builtin/ls-tree.c             |   6 +-
 cache.h                       |   2 +-
 merge-recursive.c             |   6 +-
 t/t3060-ls-files-with-tree.sh |  41 ++++++++++++
 t/t7007-show.sh               |  39 +++++++++++
 tree.c                        | 119 ++++------------------------------
 tree.h                        |  18 ++---
 11 files changed, 205 insertions(+), 147 deletions(-)

Range-diff:
 8:  8a6bebde234 !  1:  c307eb53331 show tests: add test for "git show <tree>"
    @@ Commit message
     
         Let's add this common mode of operation to the "show" tests
         themselves. It's more obvious, and the tests in
    -    t7701-repack-unpack-unreachable.sh happily parse if we start buggily
    +    t7701-repack-unpack-unreachable.sh happily pass if we start buggily
         emitting trees recursively.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
 1:  b338f2c01a4 =  2:  f37d7705cf0 ls-files tests: add meaningful --with-tree tests
 2:  4578b83944c =  3:  6291d8a1b5e tree.c API: move read_tree() into builtin/ls-files.c
 3:  33656ff63b8 !  4:  466b518e915 ls-files: don't needlessly pass around stage variable
    @@ Commit message
         Now that read_tree() has been moved to ls-files.c we can get rid of
         the stage != 1 case that'll never happen.
     
    +    Let's not use read_tree_recursive() as a pass-through to pass "stage =
    +    1" either. For now we'll pass an unused "stage = 0" for consistency
    +    with other read_tree_recursive() callers, that argument will be
    +    removed in a follow-up commit.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## builtin/ls-files.c ##
    +@@ builtin/ls-files.c: 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)
    ++			      unsigned mode, int opt)
    + {
    + 	int len;
    + 	struct cache_entry *ce;
    +@@ builtin/ls-files.c: static int read_one_entry_opt(struct index_state *istate,
    + 	ce = make_empty_cache_entry(istate, baselen + len);
    + 
    + 	ce->ce_mode = create_ce_mode(mode);
    +-	ce->ce_flags = create_ce_flags(stage);
    ++	ce->ce_flags = create_ce_flags(1);
    + 	ce->ce_namelen = baselen + len;
    + 	memcpy(ce->name, base, baselen);
    + 	memcpy(ce->name + baselen, pathname, len+1);
    +@@ builtin/ls-files.c: static int read_one_entry(const struct object_id *oid, struct strbuf *base,
    + {
    + 	struct index_state *istate = context;
    + 	return read_one_entry_opt(istate, oid, base->buf, base->len, pathname,
    +-				  mode, stage,
    ++				  mode,
    + 				  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
    + {
    + 	struct index_state *istate = context;
    + 	return read_one_entry_opt(istate, oid, base->buf, base->len, pathname,
    +-				  mode, stage,
    ++				  mode,
    + 				  ADD_CACHE_JUST_APPEND);
      }
      
      
    @@ builtin/ls-files.c: static int read_tree(struct repository *r, struct tree *tree
      	if (!fn)
      		fn = read_one_entry_quick;
     -	err = read_tree_recursive(r, tree, "", 0, stage, match, fn, istate);
    -+	err = read_tree_recursive(r, tree, "", 0, 1, match, fn, istate);
    ++	err = read_tree_recursive(r, tree, "", 0, 0, match, fn, istate);
      	if (fn == read_one_entry || err)
      		return err;
      
 4:  1c96d5d3611 !  5:  30c3abfe9b9 ls-files: refactor away read_tree()
    @@ builtin/ls-files.c: static int get_common_prefix_len(const char *common_prefix)
     -			      const char *base, int baselen,
     +			      struct strbuf *base,
      			      const char *pathname,
    - 			      unsigned mode, int stage, int opt)
    + 			      unsigned mode, int opt)
      {
     @@ builtin/ls-files.c: static int read_one_entry_opt(struct index_state *istate,
      		return READ_TREE_RECURSIVE;
    @@ 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);
    - 	ce->ce_flags = create_ce_flags(stage);
    + 	ce->ce_flags = create_ce_flags(1);
     -	ce->ce_namelen = baselen + len;
     -	memcpy(ce->name, base, baselen);
     -	memcpy(ce->name + baselen, pathname, len+1);
    @@ builtin/ls-files.c: static int read_one_entry(const struct object_id *oid, struc
      	struct index_state *istate = context;
     -	return read_one_entry_opt(istate, oid, base->buf, base->len, pathname,
     +	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_quick(const struct object_id *oid, struct strbuf *base
    @@ builtin/ls-files.c: static int read_one_entry_quick(const struct object_id *oid,
      {
      	struct index_state *istate = context;
     -	return read_one_entry_opt(istate, oid, base->buf, base->len, pathname,
    -+	return read_one_entry_opt(istate, oid, base, pathname,
    - 				  mode, stage,
    - 				  ADD_CACHE_JUST_APPEND);
    - }
    - 
    +-				  mode,
    +-				  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_quick(const struct object_id *oid,
     -
     -	if (!fn)
     -		fn = read_one_entry_quick;
    --	err = read_tree_recursive(r, tree, "", 0, 1, match, fn, istate);
    +-	err = read_tree_recursive(r, tree, "", 0, 0, match, fn, istate);
     -	if (fn == read_one_entry || err)
     -		return err;
     -
    @@ builtin/ls-files.c: static int read_one_entry_quick(const struct object_id *oid,
     -	cache_tree_free(&istate->cache_tree);
     -	QSORT(istate->cache, istate->cache_nr, cmp_cache_name_compare);
     -	return 0;
    --}
    --
    ++	return read_one_entry_opt(istate, oid, base, pathname,
    ++				  mode, 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;
 5:  367cb99224b <  -:  ----------- tree.h API: remove support for starting at prefix != ""
 6:  38e36780e22 <  -:  ----------- tree.h API: remove "stage" parameter from read_tree_recursive()
 7:  859902ffd83 <  -:  ----------- tree.h API: rename read_tree_recursive() to read_tree()
 -:  ----------- >  6:  02c42be9249 archive: stop passing "stage" through read_tree_recursive()
 9:  29996dd82bc !  7:  d55e8d4042b tree.h API: expose read_tree_1() as read_tree_at()
    @@ Metadata
      ## Commit message ##
         tree.h API: expose read_tree_1() as read_tree_at()
     
    -    Rename the static read_tree_1() function to read_tree_at(). This will
    -    allow for the old read_tree_recursive() mode of operation where we
    -    start at a given path instead of "".
    +    Rename the static read_tree_1() function to read_tree_at(). This
    +    function works just like read_tree_recursive(), except you provide
    +    your own strbuf.
     
    -    See [1] for the discussion of one such future in-tree user, unlike the
    -    old read_tree_recursive() this function takes a strbuf. Since that's
    -    what read_tree_1() used internally this should allow us to avoid
    -    casting and/or reallocations in the future.
    +    This step doesn't make much sense now, but in follow-up commits I'll
    +    remove the base/baselen/stage arguments to read_tree_recursive(). At
    +    that point an anticipated in-tree user[1] for the old
    +    read_tree_recursive() couldn't provide a path to start the
    +    traversal.
     
    -    1. https://lore.kernel.org/git/xmqqft106sok.fsf@gitster.g/#t
    +    Let's give them a function to do so with an API that makes more sense
    +    for them, by taking a strbuf we should be able to avoid more casting
    +    and/or reallocations in the future.
    +
    +    1. https://lore.kernel.org/git/xmqqft106sok.fsf@gitster.g
     
      ## tree.c ##
     @@
    @@ tree.c
      
     -static int read_tree_1(struct repository *r,
     -		       struct tree *tree, struct strbuf *base,
    --		       const struct pathspec *pathspec,
    +-		       int stage, const struct pathspec *pathspec,
     -		       read_tree_fn_t fn, void *context)
     +int read_tree_at(struct repository *r,
     +		 struct tree *tree, struct strbuf *base,
    ++		 int stage,
     +		 const struct pathspec *pathspec,
     +		 read_tree_fn_t fn, void *context)
      {
    @@ tree.c: static int read_tree_1(struct repository *r,
      		strbuf_add(base, entry.path, len);
      		strbuf_addch(base, '/');
     -		retval = read_tree_1(r, lookup_tree(r, &oid),
    --				     base, pathspec,
    +-				     base, stage, pathspec,
     -				     fn, context);
     +		retval = read_tree_at(r, lookup_tree(r, &oid),
    -+				      base, pathspec,
    ++				      base, stage, pathspec,
     +				      fn, context);
      		strbuf_setlen(base, oldlen);
      		if (retval)
      			return -1;
    -@@ tree.c: int read_tree(struct repository *r,
    - 	struct strbuf sb = STRBUF_INIT;
    +@@ tree.c: int read_tree_recursive(struct repository *r,
      	int ret;
      
    --	ret = read_tree_1(r, tree, &sb, pathspec, fn, context);
    -+	ret = read_tree_at(r, tree, &sb, pathspec, fn, context);
    + 	strbuf_add(&sb, base, baselen);
    +-	ret = read_tree_1(r, tree, &sb, stage, pathspec, fn, context);
    ++	ret = read_tree_at(r, tree, &sb, stage, pathspec, fn, context);
      	strbuf_release(&sb);
      	return ret;
      }
    @@ tree.c: int read_tree(struct repository *r,
      ## tree.h ##
     @@ tree.h: 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, void *);
    + typedef int (*read_tree_fn_t)(const struct object_id *, struct strbuf *, const char *, unsigned int, int, void *);
      
     +int read_tree_at(struct repository *r,
    -+		 struct tree *tree,
    -+		 struct strbuf *at,
    ++		 struct tree *tree, struct strbuf *base,
    ++		 int stage,
     +		 const struct pathspec *pathspec,
     +		 read_tree_fn_t fn, void *context);
     +
    - int read_tree(struct repository *r,
    - 	      struct tree *tree,
    - 	      const struct pathspec *pathspec,
    + int read_tree_recursive(struct repository *r,
    + 			struct tree *tree,
    + 			const char *base, int baselen,
 -:  ----------- >  8:  fa157d8baee tree.h API: simplify read_tree_recursive() signature

Comments

Junio C Hamano March 20, 2021, 11:08 p.m. UTC | #1

Ævar Arnfjörð Bjarmason  <[e-mail]> writes:

> A refactoring of v4[1], not quite as as suggested by Junio in[2], but
> which I think makes this whole thing less confusing and more
> straightforward.

Yeah, I admit that I gave only a cursory look, but the progression
presented in this round makes a lot of sense to me.

Thanks, will replace.