Re: [PATCHv12 0/7] Expose submodule parallelism to the user
- From
Stefan Beller <sbeller@google.com>
- Date
- Feb 18, 2016, 23:12 UTC
- Message-ID
- <CAGZ79kZZRENjVhfrhSfPmSNNQ_qrwpzWk=pocQ4GY6kBos1r6Q@mail.gmail.com>
- In-Reply-To
- <xmqqr3g9eii9.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 18, 2016 at 2:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 39 quoted lines
> Stefan Beller <sbeller@google.com> 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.
Show 5 quoted lines
> > - 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.
Show 27 quoted lines
> > 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(-)