Re: [PATCH 6/7] tree.h API: remove support for starting at prefix != ""

From: Elijah Newren <newren@gmail.com>
Date: Mar 6, 2021, 21:55 UTC
Message-ID: <CABPp-BGj2k=idn8wqpek-3m5NwNbSPCNM6mFXVyFMTDoWu44JA@mail.gmail.com>
In-Reply-To: <20210306193458.20633-7-avarab@gmail.com>

On Sat, Mar 6, 2021 at 11:35 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

>
> Every caller or the read_tree_recursive() function hardcoded a

s/or/of/?

> starting point of "" in the tree. So let's simply remove that
> parameter.

Interesting. It appears that read_tree_recursive() was the last function to call read_tree_recursive() with a starting point other than "", but that was changed in commit ffd31f661d ("Reimplement read_tree_recursive() using tree_entry_interesting()", 2011-03-25).

The last caller of read_tree_recursive() other than itself to call it with base != "", was ebfbdb340a ("Git archive and trailing "/" in prefix", 2009-10-08).

> 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.

This paragraph is very hard to parse.

> 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().

Starting with "But if and when that happens" suggests this shouldn't be an independent paragraph.

> In the meantime there's no reason to keep around what amounts to dead
> code just in case we need it in the future.

Makes sense to me.

Show 137 quoted lines

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  archive.c          | 8 ++++----
>  builtin/checkout.c | 2 +-
>  builtin/log.c      | 4 ++--
>  builtin/ls-files.c | 2 +-
>  builtin/ls-tree.c  | 2 +-
>  merge-recursive.c  | 2 +-
>  tree.c             | 2 --
>  tree.h             | 1 -
>  8 files changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/archive.c b/archive.c
> index 5919d9e5050..9394f170f7f 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -316,8 +316,8 @@ int write_archive_entries(struct archiver_args *args,
>                 git_attr_set_direction(GIT_ATTR_INDEX);
>         }
>
> -       err = read_tree_recursive(args->repo, args->tree, "",
> -                                 0, 0, &args->pathspec,
> +       err = read_tree_recursive(args->repo, args->tree,
> +                                 0, &args->pathspec,
>                                   queue_or_write_archive_entry,
>                                   &context);
>         if (err == READ_TREE_RECURSIVE)
> @@ -405,8 +405,8 @@ static int path_exists(struct archiver_args *args, const char *path)
>         ctx.args = args;
>         parse_pathspec(&ctx.pathspec, 0, 0, "", paths);
>         ctx.pathspec.recursive = 1;
> -       ret = read_tree_recursive(args->repo, args->tree, "",
> -                                 0, 0, &ctx.pathspec,
> +       ret = read_tree_recursive(args->repo, args->tree,
> +                                 0, &ctx.pathspec,
>                                   reject_entry, &ctx);
>         clear_pathspec(&ctx.pathspec);
>         return ret != 0;
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2d6550bc3c8..21b742c0f07 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -155,7 +155,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base,
>
>  static int read_tree_some(struct tree *tree, const struct pathspec *pathspec)
>  {
> -       read_tree_recursive(the_repository, tree, "", 0, 0,
> +       read_tree_recursive(the_repository, tree, 0,
>                             pathspec, update_some, NULL);
>
>         /* update the index with the given tree's info
> diff --git a/builtin/log.c b/builtin/log.c
> index f67b67d80ed..ffa3fb8c286 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -681,8 +681,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>                                         diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
>                                         name,
>                                         diff_get_color_opt(&rev.diffopt, DIFF_RESET));
> -                       read_tree_recursive(the_repository, (struct tree *)o, "",
> -                                           0, 0, &match_all, show_tree_object,
> +                       read_tree_recursive(the_repository, (struct tree *)o,
> +                                           0, &match_all, show_tree_object,
>                                             rev.diffopt.file);
>                         rev.shown_one = 1;
>                         break;
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index c0349a7b206..2c609428eea 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -483,7 +483,7 @@ 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))
>                 die("unable to read tree entries %s", tree_name);
>
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 7cad3f24ebd..7d3fb2e6d0f 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -185,6 +185,6 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>         tree = parse_tree_indirect(&oid);
>         if (!tree)
>                 die("not a tree object");
> -       return !!read_tree_recursive(the_repository, tree, "", 0, 0,
> +       return !!read_tree_recursive(the_repository, tree, 0,
>                                      &pathspec, show_tree, NULL);
>  }
> diff --git a/merge-recursive.c b/merge-recursive.c
> index b052974f191..fa7602ff0f2 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -473,7 +473,7 @@ static void get_files_dirs(struct merge_options *opt, struct tree *tree)
>  {
>         struct pathspec match_all;
>         memset(&match_all, 0, sizeof(match_all));
> -       read_tree_recursive(opt->repo, tree, "", 0, 0,
> +       read_tree_recursive(opt->repo, tree, 0,
>                             &match_all, save_files_dirs, opt);
>  }
>
> diff --git a/tree.c b/tree.c
> index c1bde9314d0..285633892c2 100644
> --- a/tree.c
> +++ b/tree.c
> @@ -82,14 +82,12 @@ static int read_tree_1(struct repository *r,
>
>  int read_tree_recursive(struct repository *r,
>                         struct tree *tree,
> -                       const char *base, int baselen,
>                         int stage, const struct pathspec *pathspec,
>                         read_tree_fn_t fn, void *context)
>  {
>         struct strbuf sb = STRBUF_INIT;
>         int ret;
>
> -       strbuf_add(&sb, base, baselen);
>         ret = read_tree_1(r, tree, &sb, stage, pathspec, fn, context);
>         strbuf_release(&sb);
>         return ret;
> diff --git a/tree.h b/tree.h
> index 34549c86c9f..9a0fd3221e3 100644
> --- a/tree.h
> +++ b/tree.h
> @@ -33,7 +33,6 @@ typedef int (*read_tree_fn_t)(const struct object_id *, struct strbuf *, const c
>
>  int read_tree_recursive(struct repository *r,
>                         struct tree *tree,
> -                       const char *base, int baselen,
>                         int stage, const struct pathspec *pathspec,
>                         read_tree_fn_t fn, void *context);
>
> --
> 2.31.0.rc0.126.g04f22c5b82

Looks good.