On Thu, Feb 18, 2016 at 2:55 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>
>> 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.
The skip_prefix was there before, so it stuck there.
Also it seems a bit more high level to me hence easier to read,
(though I am biased). I'll use your suggestion.
>
> - Is dst->command always initialized to a NULL (otherwise the
> unconditional upfront free() makes it unsafe)?
Yes, although just currently. It seems hard to maintain going forward as
the struct submodule_update_strategy is part of the struct submodule
(as defined in submodule.h) as well as the struct submodule_update_clone
(as defined in submodule--helper.c) and both places take care of initializing
it to null.
>
> - 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.
Sure, I'll just reroll the series now.
>
> Thanks.
>
>> 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(-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html