Re: [PATCH v5 1/2] submodule refactor: use git_path_submodule() in add_submodule_odb()

[Junio C Hamano <gitster@pobox.com>]

Max Kirillov <max@max630.net> writes:

Here is a space to describe why this change is a good thing.  Is
this a fix to change the behaviour, and if so how is the behaviour
different with and without the patch?  Or is this just to drop the
block of code from here and replace it with a call to an existing
helper that does exactly the same thing?  I _suspect_ that it is the
latter, but please do not force reviewers to guess.

> Signed-off-by: Max Kirillov <max@max630.net>
> ---
>  submodule.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 15e90d1..f6afe0a 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -122,43 +122,35 @@ void stage_updated_gitmodules(void)
>  
>  static int add_submodule_odb(const char *path)
>  {
> -	struct strbuf objects_directory = STRBUF_INIT;
>  	struct alternate_object_database *alt_odb;
> +	const char* objects_directory;

Style; asterisk sticks to the variable, not the type.  I think you
need to fix this in multiple places (not just 1/2 but also in 2/2).

Other than that I think it is a sensible "replace bulk of code with
identical helper that already exists" rewrite.

Thanks.

>  	int ret = 0;
> -	const char *git_dir;
>  
> -	strbuf_addf(&objects_directory, "%s/.git", path);
> -	git_dir = read_gitfile(objects_directory.buf);
> -	if (git_dir) {
> -		strbuf_reset(&objects_directory);
> -		strbuf_addstr(&objects_directory, git_dir);
> -	}
> -	strbuf_addstr(&objects_directory, "/objects/");
> -	if (!is_directory(objects_directory.buf)) {
> +	objects_directory = git_path_submodule(path, "objects/");
> +	if (!is_directory(objects_directory)) {
>  		ret = -1;
>  		goto done;
>  	}
> +
>  	/* avoid adding it twice */
>  	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
> -		if (alt_odb->name - alt_odb->base == objects_directory.len &&
> -				!strncmp(alt_odb->base, objects_directory.buf,
> -					objects_directory.len))
> +		if (alt_odb->name - alt_odb->base == strlen(objects_directory) &&
> +				!strcmp(alt_odb->base, objects_directory))
>  			goto done;
>  
> -	alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
> +	alt_odb = xmalloc(strlen(objects_directory) + 42 + sizeof(*alt_odb));
>  	alt_odb->next = alt_odb_list;
> -	strcpy(alt_odb->base, objects_directory.buf);
> -	alt_odb->name = alt_odb->base + objects_directory.len;
> +	strcpy(alt_odb->base, objects_directory);
> +	alt_odb->name = alt_odb->base + strlen(objects_directory);
>  	alt_odb->name[2] = '/';
>  	alt_odb->name[40] = '\0';
>  	alt_odb->name[41] = '\0';
>  	alt_odb_list = alt_odb;
>  
>  	/* add possible alternates from the submodule */
> -	read_info_alternates(objects_directory.buf, 0);
> +	read_info_alternates(objects_directory, 0);
>  	prepare_alt_odb();
>  done:
> -	strbuf_release(&objects_directory);
>  	return ret;
>  }