Re: [PATCHv12 0/7] Expose submodule parallelism to the user
Stefan Beller writes:
> On Thu, Feb 18, 2016 at 3:20 PM, Junio C Hamano wrote:
>> Stefan Beller writes:
>>
>>> On Thu, Feb 18, 2016 at 3:12 PM, Stefan Beller wrote:
> 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.
>>
>> Sorry, but I thought this "parsing update strategy" was all new
>> code.
>
> I meant previous patches or in my mind. That's why I was hesitant to
> throw out the skip_prefix.
I actually think the attached on top of your final version would be
the best. It would not make too big a difference in this codepath
that skips just one byte, the pattern naturally would apply to
prefix of any length, and this would serve as the BCP, ready to be
copied-and-pasted by others when writing new code.
And of course it does not waste an otherwise unnecessary temporary
variable ;-)
diff --git a/submodule.c b/submodule.c
index 911fa3b..8e08159 100644
--- a/submodule.c
+++ b/submodule.c
@@ -223,9 +223,9 @@ 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 (value[0] == '!') {
+ else if (skip_prefix(value, "!", &value)) {
dst->type = SM_UPDATE_COMMAND;
- dst->command = xstrdup(value + 1);
+ dst->command = xstrdup(value);
} else
return -1;
return 0;
--
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
Re: [PATCHv12 0/7] Expose submodule parallelism to the user
On Thu, Feb 18, 2016 at 3:20 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> On Thu, Feb 18, 2016 at 3:12 PM, Stefan Beller wrote:
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.
>
> Sorry, but I thought this "parsing update strategy" was all new
> code.
I meant previous patches or in my mind. That's why I was hesitant to
throw out the skip_prefix.
>
>>> Also it seems a bit more high level to me hence easier to read,
>>> (though I am biased). I'll use your suggestion.
>>
>> and it doesn't crash when passing in value == NULL.
>> (We don't do that currently, just a side observation)
>
> Hmph. If you pass str==NULL with prefix="!" to what we have below,
> I would think the first iteration would try to read from *str and do
> a bizarre thing.
>
> static inline int skip_prefix(const char *str, const char *prefix,
> const char **out)
> {
> do {
> if (!*prefix) {
> *out = str;
> return 1;
> }
> } while (*str++ == *prefix++);
> return 0;
> }
>
> Puzzled.
And there I was asserting properties about methods
without looking them up.
ok.
--
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
Re: [PATCHv12 0/7] Expose submodule parallelism to the user
Stefan Beller writes:
> On Thu, Feb 18, 2016 at 3:12 PM, Stefan Beller wrote:
>>> 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.
Sorry, but I thought this "parsing update strategy" was all new
code.
>> Also it seems a bit more high level to me hence easier to read,
>> (though I am biased). I'll use your suggestion.
>
> and it doesn't crash when passing in value == NULL.
> (We don't do that currently, just a side observation)
Hmph. If you pass str==NULL with prefix="!" to what we have below,
I would think the first iteration would try to read from *str and do
a bizarre thing.
static inline int skip_prefix(const char *str, const char *prefix,
const char **out)
{
do {
if (!*prefix) {
*out = str;
return 1;
}
} while (*str++ == *prefix++);
return 0;
}
Puzzled.
--
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
Re: [PATCHv12 0/7] Expose submodule parallelism to the user
On Thu, Feb 18, 2016 at 3:12 PM, Stefan Beller wrote: >> 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. and it doesn't crash when passing in value == NULL. (We don't do that currently, just a side observation) -- 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
Re: [PATCHv12 0/7] Expose submodule parallelism to the user
On Thu, Feb 18, 2016 at 2:55 PM, Junio C Hamano wrote:
> Stefan Beller 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
Re: [PATCHv12 0/7] Expose submodule parallelism to the user
Stefan Beller 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.
- 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.
> 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
[PATCHv12 0/7] Expose submodule parallelism to the user
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;
}
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(-)
--
2.7.0.rc0.34.g65aed89
--
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
