Re: [PATCH v3 1/9] tree: do not use the_repository for tree traversal methods.

From: Glen Choo <chooglen@google.com>
Date: Oct 26, 2022, 22:33 UTC
Message-ID: <kl6lo7tyqkph.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <79959a54eb4c1a0812b1f4643530069a63e549f4.1665973401.git.gitgitgadget@gmail.com>

"Alphadelta14 via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 22 quoted lines

> diff --git a/tree.h b/tree.h
> index 6efff003e21..cc6402e4738 100644
> --- a/tree.h
> +++ b/tree.h
> @@ -18,15 +18,21 @@ struct tree *lookup_tree(struct repository *r, const struct object_id *oid);
>  
>  int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size);
>  
> -int parse_tree_gently(struct tree *tree, int quiet_on_missing);
> -static inline int parse_tree(struct tree *tree)
> +int repo_parse_tree_gently(struct repository *r, struct tree *tree, int quiet_on_missing);
> +static inline int repo_parse_tree(struct repository *r, struct tree *tree)
>  {
> -	return parse_tree_gently(tree, 0);
> +	return repo_parse_tree_gently(r, tree, 0);
>  }
> +
> +#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
> +#define parse_tree(tree) repo_parse_tree(the_repository, tree)
> +#define parse_tree_gently(tree, quiet_on_missing) repo_parse_tree_gently(the_repository, tree, quiet_on_missing)
> +#define parse_tree_indirect(oid) repo_parse_tree_indirect(the_repository, oid)
> +#endif

Typically, when we have repo_* and non-repo_* variants, we use a "static inline" function, e.g. from refs.h:

  int repo_dwim_ref(struct repository *r, const char *str, int len,
        struct object_id *oid, char **ref, int nonfatal_dangling_mark);

  static inline int dwim_ref(const char *str, int len, struct object_id *oid,
          char **ref, int nonfatal_dangling_mark)
  {
    return repo_dwim_ref(the_repository, str, len, oid, ref,
            nonfatal_dangling_mark);
  }

I think we should do the same here, instead of using "#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS".

From I can gather from "git log -S NO_THE_REPOSITORY_COMPATIBILITY_MACROS", that macro was introduced in e675765235 (diff.c: remove implicit dependency on the_index, 2018-09-21) and all instances of that macro were introduced around that time. At the time, there was an effort to get rid of the_repository and the_index almost everywhere (except builtins), and the macro would ensure that we did this successfully.

We did such a good job with the_index that we flipped the default from NO_THE_INDEX_COMPATIBILITY_MACROS to USE_THE_INDEX_COMPATIBILITY_MACROS (f8adbec9fe (cache.h: flip NO_THE_INDEX_COMPATIBILITY_MACROS switch, 2019-01-24)) but it looks like we never got there with the_repository. I couldn't find any instances of "#define NO_THE_REPOSITORY_COMPATIBILITY_MACROS", so I think we should just use "static inline" instead.

Alternatively, one could get rid of the non-repo_* variant and adjust all existing callers to use "struct repository", but that's a lot of churn and may conflict with other in-flight series, so that's probably left for another time.

Show 10 quoted lines

>  void free_tree_buffer(struct tree *tree);
>  
>  /* Parses and returns the tree in the given ent, chasing tags and commits. */
> -struct tree *parse_tree_indirect(const struct object_id *oid);
> +struct tree *repo_parse_tree_indirect(struct repository *r, const struct object_id *oid);
>  
>  int cmp_cache_name_compare(const void *a_, const void *b_);
>  
> -- 
> gitgitgadget