Re: [PATCH 23/30] tree-walk.h API: add a get_tree_entry_path() function

From: Elijah Newren <newren@gmail.com>
Date: Mar 9, 2021, 18:17 UTC
Message-ID: <CABPp-BF6pNbnVoYi3+iO=RRPskg1w6V=iJbKFiDYJxWM7ZFAwA@mail.gmail.com>
In-Reply-To: <20210308150650.18626-24-avarab@gmail.com>

On Mon, Mar 8, 2021 at 7:07 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 8 quoted lines

>
> Add a get_tree_entry_path() variant in addition to
> get_tree_entry_path_{mode,type,all}(). This is for those callers that
> need neither the mode nor "enum object_type" parameters filled for
> them.
>
> There's callers here which doesn't need the "struct object_id" filled
> either, and provides a throwaway variable for us.

I think I understood this sentence, but perhaps we could reword it? Something like:

There are callers here which don't need the "struct object_id" filled; forcing callers to pass one just requires they create a throwaway variable.

Show 6 quoted lines

>
> See the following commits for the introduction of such code that's
> being modified here:
>
>  - shift_tree(): 68faf68938e (A new merge stragety 'subtree'.,
>     2007-02-15) for the shift_tree()

What a lovely commit.  I especially appreciate this gem:

"Heuristics galore!  That's the git way ;-)."

Show 56 quoted lines

>
>  - tree_has_path(): 96e7ffbdc31 (merge-recursive: check for directory
>    level conflicts, 2018-04-19)
>
>  - init_notes(): fd53c9eb445 (Speed up git notes lookup, 2009-10-09)
>
>  - diagnose_invalid_oid_path(): 009fee4774d (Detailed diagnosis when
>    parsing an object name fails., 2009-12-07)
>
> Those could potentially be refactored too, but I've got to stop at
> some point, and right now I'm focusing downstream code that depends on
> "mode" (or "enum object_type").
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  match-trees.c     |  4 +---
>  merge-recursive.c |  6 ++----
>  notes.c           |  3 +--
>  object-name.c     |  3 +--
>  tree-walk.c       | 11 +++++++++++
>  tree-walk.h       |  3 +++
>  6 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/match-trees.c b/match-trees.c
> index ce3f811ec04..60a17b92d70 100644
> --- a/match-trees.c
> +++ b/match-trees.c
> @@ -288,12 +288,10 @@ void shift_tree(struct repository *r,
>
>         if (add_score < del_score) {
>                 /* We need to pick a subtree of two */
> -               unsigned short mode;
> -
>                 if (!*del_prefix)
>                         return;
>
> -               if (get_tree_entry_mode(r, hash2, del_prefix, shifted, &mode))
> +               if (get_tree_entry_path(r, hash2, del_prefix, shifted))
>                         die("cannot find path %s in tree %s",
>                             del_prefix, oid_to_hex(hash2));
>                 return;
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 0e891360e7e..b26d9d418f9 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1884,11 +1884,9 @@ static int tree_has_path(struct repository *r, struct tree *tree,
>                          const char *path)
>  {
>         struct object_id hashy;
> -       unsigned short mode_o;
> -
> -       return !get_tree_entry_mode(r,
> +       return !get_tree_entry_path(r,
>                                     &tree->object.oid, path,
> -                                   &hashy, &mode_o);
> +                                   &hashy);

Poor Hashimoto has lost half his name and is now stuck with the nickname of Hashy... ;-)

(The mode was just a temporary and the name "mode_o" had no real meaning; it was there just because I mis-remembered Quasimodo's name at the time as Hashimodo and found "hashy mode_o" slightly amusing.)

Show 87 quoted lines

>  }
>
>  /*
> diff --git a/notes.c b/notes.c
> index ef138606146..aa46cb2b09e 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -994,7 +994,6 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
>                 combine_notes_fn combine_notes, int flags)
>  {
>         struct object_id oid, object_oid;
> -       unsigned short mode;
>         struct leaf_node root_tree;
>
>         if (!t)
> @@ -1021,7 +1020,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
>                 return;
>         if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, &object_oid))
>                 die("Cannot use notes ref %s", notes_ref);
> -       if (get_tree_entry_mode(the_repository, &object_oid, "", &oid, &mode))
> +       if (get_tree_entry_path(the_repository, &object_oid, "", &oid))
>                 die("Failed to read notes tree referenced by %s (%s)",
>                     notes_ref, oid_to_hex(&object_oid));
>
> diff --git a/object-name.c b/object-name.c
> index 7e3b2d6d739..9ff5f83c1ff 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -1693,7 +1693,6 @@ static void diagnose_invalid_oid_path(struct repository *r,
>                                       int object_name_len)
>  {
>         struct object_id oid;
> -       unsigned short mode;
>
>         if (!prefix)
>                 prefix = "";
> @@ -1704,7 +1703,7 @@ static void diagnose_invalid_oid_path(struct repository *r,
>         if (is_missing_file_error(errno)) {
>                 char *fullname = xstrfmt("%s%s", prefix, filename);
>
> -               if (!get_tree_entry_mode(r, tree_oid, fullname, &oid, &mode)) {
> +               if (!get_tree_entry_path(r, tree_oid, fullname, &oid)) {
>                         die(_("path '%s' exists, but not '%s'\n"
>                             "hint: Did you mean '%.*s:%s' aka '%.*s:./%s'?"),
>                             fullname,
> diff --git a/tree-walk.c b/tree-walk.c
> index 0ad3d80593e..83737634770 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -632,6 +632,17 @@ int get_tree_entry_all(struct repository *r,
>         return retval;
>  }
>
> +int get_tree_entry_path(struct repository *r,
> +                       const struct object_id *tree_oid,
> +                       const char *name,
> +                       struct object_id *oid)
> +{
> +       unsigned short mode;
> +       enum object_type object_type;
> +       return get_tree_entry_all(r, tree_oid, name, oid,
> +                                 &mode, &object_type);
> +}
> +
>  int get_tree_entry_mode(struct repository *r,
>                         const struct object_id *tree_oid,
>                         const char *name,
> diff --git a/tree-walk.h b/tree-walk.h
> index 5db38fcb575..1bfa839b275 100644
> --- a/tree-walk.h
> +++ b/tree-walk.h
> @@ -175,10 +175,13 @@ struct traverse_info {
>   * "struct name_entry" you'd like. You always need to pointer to an
>   * appropriate variable to fill in (NULL won't do!):
>   *
> + * get_tree_entry_path(): <no extra argument, just get the common 'path'>
>   * get_tree_entry_mode(): unsigned int mode
>   * get_tree_entry_type(): enum object_type
>   * get_tree_entry_all(): unsigned int mode, enum object_type
>   */
> +int get_tree_entry_path(struct repository *, const struct object_id *, const char *,
> +                       struct object_id *);
>  int get_tree_entry_mode(struct repository *, const struct object_id *, const char *,
>                         struct object_id *,
>                         unsigned short *);
> --
> 2.31.0.rc0.126.g04f22c5b82