Jens Lehmann <Jens.Lehmann@xxxxxx> writes:
> diff --git a/submodule.c b/submodule.c
> index 6f1c107..c8c3a99 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -152,6 +153,20 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt,
> ...
> +int parse_fetch_recurse_submodules_arg(const char *arg)
> +{
> + switch (git_config_maybe_bool("", arg)) {
It's a bit unusual to see "" as the variable name; doesn't config-maybe-bool
barf when arg is not what it likes, with the name as part of its message?
> @@ -248,11 +263,73 @@ void set_config_fetch_recurse_submodules(int value)
> ...
> +static void submodule_collect_changed_cb(struct diff_queue_struct *q,
> + struct diff_options *options,
> + void *data)
> +{
> + int i;
> + for (i = 0; i < q->nr; i++) {
> + struct diff_filepair *p = q->queue[i];
> + if (!S_ISGITLINK(p->two->mode))
> + continue;
> +
> + if (S_ISGITLINK(p->one->mode)) {
> + /* NEEDSWORK: We should honor the name configured in
> + * the .gitmodules file of the commit we are examining
> + * here to be able to correctly follow submodules
> + * being moved around. */
> + struct string_list_item *path;
> + path = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);
> + if (!path)
> + string_list_append(&changed_submodule_paths, xstrdup(p->two->path));
I wondered why there is no mention of "data" in the implementation of this
function; changed_submodule_paths global is used instead here.
I do not see anywhere the global string_list is initialized, freed, nor
re-initialized for reuse. Are you guaranteeing that the caller only calls
the check_for_new_submodule_commits() function once, and if so how? The
function is called from update_local_ref() in builtin/fetch.c, which in
turn gets called number of times during a fetch. IOW, does the code do
the right thing when you are fetching more than one refs?
--
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