Re: [PATCH v5 1/2] submodule refactor: use git_path_submodule() in add_submodule_odb()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 3, 2015, 21:29 UTC
- Message-ID
- <xmqqmvy86oy0.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1438635836-7857-2-git-send-email-max@max630.net>
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.
Show 16 quoted lines
> 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.
Show 46 quoted lines
> 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;
> }