Re: [PATCHv12 0/7] Expose submodule parallelism to the user
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 18, 2016, 22:55 UTC
- Message-ID
- <xmqqr3g9eii9.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1455833964-3629-1-git-send-email-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 32 quoted lines
> Thanks Junio for a review of v11!
>
> I addressed the memory issue with the interdiff (in patch 1/7) as follows:
> Interdiff to v11:
>
> diff --git a/submodule.c b/submodule.c
> index 263cb2a..45d0967 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -219,6 +219,9 @@ void gitmodules_config(void)
> int parse_submodule_update_strategy(const char *value,
> struct submodule_update_strategy *dst)
> {
> + const char *com;
> +
> + free((void*)dst->command);
> dst->command = NULL;
> if (!strcmp(value, "none"))
> dst->type = SM_UPDATE_NONE;
> @@ -228,9 +231,10 @@ int parse_submodule_update_strategy(const char *value,
> dst->type = SM_UPDATE_REBASE;
> else if (!strcmp(value, "merge"))
> dst->type = SM_UPDATE_MERGE;
> - else if (skip_prefix(value, "!", &dst->command))
> + else if (skip_prefix(value, "!", &com)) {
> dst->type = SM_UPDATE_COMMAND;
> - else
> + dst->command = xstrdup(com);
> + } else
> return -1;
> return 0;
> }Unless you count "I want to write differently from what was suggested" is a desirable thing to do, I do not see a point in favouring the above that uses an extra variable and skip_prefix() over what I gave you as "how about" patch. But whatever.
- Is dst->command always initialized to a NULL (otherwise the unconditional upfront free() makes it unsafe)?
- Is there a global free_something() or something_clear() function that are used to release the resource held by a structure that has submodule_update_strategy structure embedded in it? If so dst->command needs to be freed there as well.
Thanks.
Show 24 quoted lines
> Stefan Beller (7): > submodule-config: keep update strategy around > submodule-config: drop check against NULL > fetching submodules: respect `submodule.fetchJobs` config option > submodule update: direct error message to stderr > git submodule update: have a dedicated helper for cloning > submodule update: expose parallelism to the user > clone: allow an explicit argument for parallel submodule clones > > Documentation/config.txt | 6 + > Documentation/git-clone.txt | 6 +- > Documentation/git-submodule.txt | 7 +- > builtin/clone.c | 19 +++- > builtin/fetch.c | 2 +- > builtin/submodule--helper.c | 239 ++++++++++++++++++++++++++++++++++++++++ > git-submodule.sh | 54 ++++----- > submodule-config.c | 18 ++- > submodule-config.h | 2 + > submodule.c | 39 ++++++- > submodule.h | 18 +++ > t/t5526-fetch-submodules.sh | 14 +++ > t/t7400-submodule-basic.sh | 4 +- > t/t7406-submodule-update.sh | 27 +++++ > 14 files changed, 406 insertions(+), 49 deletions(-)