On Thu, Feb 18, 2016 at 2:55 PM, Junio C Hamano <gits...@pobox.com> wrote: > Stefan Beller <sbel...@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. > > - 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html