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

[Stefan Beller <sbeller@google.com>]

On Tue, Aug 4, 2015 at 3:05 PM, Max Kirillov <max@max630.net> wrote:
> Functions which directly operate submodule's object database do not handle the
> case when the submodule is linked worktree (which are introduced in
> c7b3a3d2fe). Instead of fixing the path calculation use already existing
> git_path_submodule() function without changing overall behavior. Then it will
> be possible to modify only that function whenever we need to change real
> location of submodule's repository content.
>
> 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..70d18ec 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;
>         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/");

git_path_submodule is going away with  jk/git-path (2015-08-10)
So you may want to use git_pathdup_submodule and free the
result of that function before returning.

> +       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;
>  }
>

This looks good, though I am diving into the submodule code base
myself just now,
so it's not a strong review. :)

> --
> 2.3.4.2801.g3d0809b
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html