Re: [PATCH 2/7] grep: use submodule-ODB-as-alternate lazy-addition

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, Aug 10, 2021 at 11:28:40AM -0700, Jonathan Tan wrote:
> 
> In the parent commit, Git was taught to add submodule ODBs as alternates
> lazily, but grep does not use this because it computes the path to add
> directly, not going through add_submodule_odb(). Add an equivalent to
> add_submodule_odb() that takes the exact ODB path and teach grep to use
> it.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  builtin/grep.c | 2 +-
>  submodule.c    | 5 +++++
>  submodule.h    | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 7d2f8e5adb..87bcb934a2 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -450,7 +450,7 @@ static int grep_submodule(struct grep_opt *opt,
>  	 * store is no longer global and instead is a member of the repository
>  	 * object.
>  	 */
> -	add_to_alternates_memory(subrepo.objects->odb->path);
> +	add_submodule_odb_by_path(subrepo.objects->odb->path);

I had wondered whether we can entirely drop add_to_alternates_memory()
but I see on second reading that that's still used by
register_all_submodule_odb...(). I wonder if it can become static in
submodule.c to prevent users from dodging the envvar+BUG()?

>  	obj_read_unlock();
>  
>  	memcpy(&subopt, opt, sizeof(subopt));
> diff --git a/submodule.c b/submodule.c
> index 8fde90e906..8de1aecaeb 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -187,6 +187,11 @@ int add_submodule_odb(const char *path)
>  	return ret;
>  }
>  
> +void add_submodule_odb_by_path(const char *path)
> +{
> +	string_list_insert(&added_submodule_odb_paths, xstrdup(path));
> +}
> +
>  int register_all_submodule_odb_as_alternates(void)
>  {
>  	int i;
> diff --git a/submodule.h b/submodule.h
> index c252784bc2..17a06cc43b 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -104,6 +104,7 @@ int bad_to_remove_submodule(const char *path, unsigned flags);
>   * the_repository.
>   */
>  int add_submodule_odb(const char *path);
Curious whether add_submodule_odb() can be reduced by calling
add_submodule_odb_by_path() internally.
> +void add_submodule_odb_by_path(const char *path);
>  int register_all_submodule_odb_as_alternates(void);
>  
>  /*
> -- 
> 2.33.0.rc1.237.g0d66db33f3-goog
> 

These are all pretty minor comments; with or without them,

Reviewed-by: Emily Shaffer <emilyshaffer@google.com>