[RFC PATCH 0/7] Unset the submodule URL in the superproject when no longer needed

[RFC PATCH 0/7] Unset the submodule URL in the superproject when no longer needed

[Stefan Beller]

This is available as

  git fetch //github.com/stefanbeller/git unsetsubmoduleurl
  
and was hinted at in
https://public-inbox.org/git/CAGZ79kYfoK9hfXM2-VMAZLPpqBOFQYKtyYuYJb8twzz6Oz5ymQ@mail.gmail.com/

  Originally we have had the url in the config, (a) that we can change
  the URLs after the "git submodule init" and "git submodule update"
  step that actually clones the submodule if not present and much more
  importantly (b) to know which submodule "was initialized/active".
  
  Now that we have the submodule.active or even
  submodule.<name>.active flags, we do not need (b) any more.
  So the URL turns into a useless piece of cruft that just is unneeded
  and might confuse the user.

Opinions?

Thanks,
Stefan

Stefan Beller (7):
  t7410: update to new style
  builtin/submodule--helper: remove stray new line
  submodule: is_submodule_active to differentiate between new and old
    mode
  submodule sync: omit setting submodule URL in config if possible
  submodule--helper: factor out allocation of callback cookie
  submodule--helper, update_clone: store index to update_clone instead
    of ce
  builtin/submodule--helper: unset submodule url if possible

 builtin/submodule--helper.c      | 82 ++++++++++++++++++--------
 submodule.c                      |  5 +-
 submodule.h                      |  6 ++
 t/t5526-fetch-submodules.sh      |  2 +-
 t/t7406-submodule-update.sh      |  8 +++
 t/t7410-submodule-checkout-to.sh | 99 +++++++++++++++++++-------------
 6 files changed, 131 insertions(+), 71 deletions(-)

-- 
2.18.0.265.g16de1b435c9.dirty

[PATCH 1/7] t7410: update to new style

[Stefan Beller]

While at it fix a typo (s/independed/independent) and
make sure git is not in a chain of pipes.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7410-submodule-checkout-to.sh | 99 +++++++++++++++++++-------------
 1 file changed, 58 insertions(+), 41 deletions(-)

diff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh
index 1acef32647a..f1b492ebc46 100755
--- a/t/t7410-submodule-checkout-to.sh
+++ b/t/t7410-submodule-checkout-to.sh
@@ -6,55 +6,72 @@ test_description='Combination of submodules and multiple workdirs'
 
 base_path=$(pwd -P)
 
-test_expect_success 'setup: make origin' \
-    'mkdir -p origin/sub && ( cd origin/sub && git init &&
-	echo file1 >file1 &&
-	git add file1 &&
-	git commit -m file1 ) &&
-    mkdir -p origin/main && ( cd origin/main && git init &&
-	git submodule add ../sub &&
-	git commit -m "add sub" ) &&
-    ( cd origin/sub &&
-	echo file1updated >file1 &&
-	git add file1 &&
-	git commit -m "file1 updated" ) &&
-    ( cd origin/main/sub && git pull ) &&
-    ( cd origin/main &&
-	git add sub &&
-	git commit -m "sub updated" )'
-
-test_expect_success 'setup: clone' \
-    'mkdir clone && ( cd clone &&
-	git clone --recursive "$base_path/origin/main")'
+test_expect_success 'setup: make origin'  '
+	mkdir -p origin/sub &&
+	(
+		cd origin/sub && git init &&
+		echo file1 >file1 &&
+		git add file1 &&
+		git commit -m file1
+	) &&
+	mkdir -p origin/main &&
+	(
+		cd origin/main && git init &&
+		git submodule add ../sub &&
+		git commit -m "add sub"
+	) &&
+	(
+		cd origin/sub &&
+		echo file1updated >file1 &&
+		git add file1 &&
+		git commit -m "file1 updated"
+	) &&
+	git -C origin/main/sub pull &&
+	(
+		cd origin/main &&
+		git add sub &&
+		git commit -m "sub updated"
+	)
+'
+
+test_expect_success 'setup: clone' '
+	mkdir clone &&
+	git -C clone clone --recursive "$base_path/origin/main"
+'
 
 rev1_hash_main=$(git --git-dir=origin/main/.git show --pretty=format:%h -q "HEAD~1")
 rev1_hash_sub=$(git --git-dir=origin/sub/.git show --pretty=format:%h -q "HEAD~1")
 
-test_expect_success 'checkout main' \
-    'mkdir default_checkout &&
-    (cd clone/main &&
-	git worktree add "$base_path/default_checkout/main" "$rev1_hash_main")'
+test_expect_success 'checkout main' '
+	mkdir default_checkout &&
+	git -C clone/main worktree add "$base_path/default_checkout/main" "$rev1_hash_main"
+'
 
-test_expect_failure 'can see submodule diffs just after checkout' \
-    '(cd default_checkout/main && git diff --submodule master"^!" | grep "file1 updated")'
+test_expect_failure 'can see submodule diffs just after checkout' '
+	git -C default_checkout/main diff --submodule master"^!" >out &&
+	grep "file1 updated" out
+'
 
-test_expect_success 'checkout main and initialize independed clones' \
-    'mkdir fully_cloned_submodule &&
-    (cd clone/main &&
-	git worktree add "$base_path/fully_cloned_submodule/main" "$rev1_hash_main") &&
-    (cd fully_cloned_submodule/main && git submodule update)'
+test_expect_success 'checkout main and initialize independent clones' '
+	mkdir fully_cloned_submodule &&
+	git -C clone/main worktree add "$base_path/fully_cloned_submodule/main" "$rev1_hash_main" &&
+	git -C fully_cloned_submodule/main submodule update
+'
 
-test_expect_success 'can see submodule diffs after independed cloning' \
-    '(cd fully_cloned_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
+test_expect_success 'can see submodule diffs after independent cloning' '
+	git -C fully_cloned_submodule/main diff --submodule master"^!" >out &&
+	grep "file1 updated" out
+'
 
-test_expect_success 'checkout sub manually' \
-    'mkdir linked_submodule &&
-    (cd clone/main &&
-	git worktree add "$base_path/linked_submodule/main" "$rev1_hash_main") &&
-    (cd clone/main/sub &&
-	git worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub")'
+test_expect_success 'checkout sub manually' '
+	mkdir linked_submodule &&
+	git -C clone/main worktree add "$base_path/linked_submodule/main" "$rev1_hash_main" &&
+	git -C clone/main/sub worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub"
+'
 
-test_expect_success 'can see submodule diffs after manual checkout of linked submodule' \
-    '(cd linked_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
+test_expect_success 'can see submodule diffs after manual checkout of linked submodule' '
+	git -C linked_submodule/main diff --submodule master"^!" >out &&
+	grep "file1 updated" out
+'
 
 test_done
-- 
2.18.0.265.g16de1b435c9.dirty

Re: [PATCH 1/7] t7410: update to new style

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> While at it fix a typo (s/independed/independent) and
> make sure git is not in a chain of pipes.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t7410-submodule-checkout-to.sh | 99 +++++++++++++++++++-------------
>  1 file changed, 58 insertions(+), 41 deletions(-)

Makes sense and the result does read easier.

> diff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh
> index 1acef32647a..f1b492ebc46 100755
> --- a/t/t7410-submodule-checkout-to.sh
> +++ b/t/t7410-submodule-checkout-to.sh
> @@ -6,55 +6,72 @@ test_description='Combination of submodules and multiple workdirs'
>  
>  base_path=$(pwd -P)
>  
> -test_expect_success 'setup: make origin' \
> -    'mkdir -p origin/sub && ( cd origin/sub && git init &&
> -	echo file1 >file1 &&
> -	git add file1 &&
> -	git commit -m file1 ) &&
> -    mkdir -p origin/main && ( cd origin/main && git init &&
> -	git submodule add ../sub &&
> -	git commit -m "add sub" ) &&
> -    ( cd origin/sub &&
> -	echo file1updated >file1 &&
> -	git add file1 &&
> -	git commit -m "file1 updated" ) &&
> -    ( cd origin/main/sub && git pull ) &&
> -    ( cd origin/main &&
> -	git add sub &&
> -	git commit -m "sub updated" )'
> -
> -test_expect_success 'setup: clone' \
> -    'mkdir clone && ( cd clone &&
> -	git clone --recursive "$base_path/origin/main")'
> +test_expect_success 'setup: make origin'  '
> +	mkdir -p origin/sub &&
> +	(
> +		cd origin/sub && git init &&
> +		echo file1 >file1 &&
> +		git add file1 &&
> +		git commit -m file1
> +	) &&
> +	mkdir -p origin/main &&
> +	(
> +		cd origin/main && git init &&
> +		git submodule add ../sub &&
> +		git commit -m "add sub"
> +	) &&
> +	(
> +		cd origin/sub &&
> +		echo file1updated >file1 &&
> +		git add file1 &&
> +		git commit -m "file1 updated"
> +	) &&
> +	git -C origin/main/sub pull &&
> +	(
> +		cd origin/main &&
> +		git add sub &&
> +		git commit -m "sub updated"
> +	)
> +'
> +
> +test_expect_success 'setup: clone' '
> +	mkdir clone &&
> +	git -C clone clone --recursive "$base_path/origin/main"
> +'
>  
>  rev1_hash_main=$(git --git-dir=origin/main/.git show --pretty=format:%h -q "HEAD~1")
>  rev1_hash_sub=$(git --git-dir=origin/sub/.git show --pretty=format:%h -q "HEAD~1")
>  
> -test_expect_success 'checkout main' \
> -    'mkdir default_checkout &&
> -    (cd clone/main &&
> -	git worktree add "$base_path/default_checkout/main" "$rev1_hash_main")'
> +test_expect_success 'checkout main' '
> +	mkdir default_checkout &&
> +	git -C clone/main worktree add "$base_path/default_checkout/main" "$rev1_hash_main"
> +'
>  
> -test_expect_failure 'can see submodule diffs just after checkout' \
> -    '(cd default_checkout/main && git diff --submodule master"^!" | grep "file1 updated")'
> +test_expect_failure 'can see submodule diffs just after checkout' '
> +	git -C default_checkout/main diff --submodule master"^!" >out &&
> +	grep "file1 updated" out
> +'
>  
> -test_expect_success 'checkout main and initialize independed clones' \
> -    'mkdir fully_cloned_submodule &&
> -    (cd clone/main &&
> -	git worktree add "$base_path/fully_cloned_submodule/main" "$rev1_hash_main") &&
> -    (cd fully_cloned_submodule/main && git submodule update)'
> +test_expect_success 'checkout main and initialize independent clones' '
> +	mkdir fully_cloned_submodule &&
> +	git -C clone/main worktree add "$base_path/fully_cloned_submodule/main" "$rev1_hash_main" &&
> +	git -C fully_cloned_submodule/main submodule update
> +'
>  
> -test_expect_success 'can see submodule diffs after independed cloning' \
> -    '(cd fully_cloned_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
> +test_expect_success 'can see submodule diffs after independent cloning' '
> +	git -C fully_cloned_submodule/main diff --submodule master"^!" >out &&
> +	grep "file1 updated" out
> +'
>  
> -test_expect_success 'checkout sub manually' \
> -    'mkdir linked_submodule &&
> -    (cd clone/main &&
> -	git worktree add "$base_path/linked_submodule/main" "$rev1_hash_main") &&
> -    (cd clone/main/sub &&
> -	git worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub")'
> +test_expect_success 'checkout sub manually' '
> +	mkdir linked_submodule &&
> +	git -C clone/main worktree add "$base_path/linked_submodule/main" "$rev1_hash_main" &&
> +	git -C clone/main/sub worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub"
> +'
>  
> -test_expect_success 'can see submodule diffs after manual checkout of linked submodule' \
> -    '(cd linked_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
> +test_expect_success 'can see submodule diffs after manual checkout of linked submodule' '
> +	git -C linked_submodule/main diff --submodule master"^!" >out &&
> +	grep "file1 updated" out
> +'
>  
>  test_done

[PATCH 2/7] builtin/submodule--helper: remove stray new line

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5c9d1fb496d..2f20bd4abdc 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1024,7 +1024,6 @@ static void sync_submodule_cb(const struct cache_entry *list_item, void *cb_data
 {
 	struct sync_cb *info = cb_data;
 	sync_submodule(list_item->name, info->prefix, info->flags);
-
 }
 
 static int module_sync(int argc, const char **argv, const char *prefix)
-- 
2.18.0.265.g16de1b435c9.dirty

[PATCH 3/7] submodule: is_submodule_active to differentiate between new and old mode

[Stefan Beller]

The change a086f921a72 (submodule: decouple url and submodule interest,
2017-03-17) enables us to do more than originally thought.
As the url setting was used both to actually set the url where to
obtain the submodule from, as well as used as a boolean flag later
to see if it was active, we would need to keep the url around.

Now that submodules can be activated using the submodule.[<name>.]active
setting, we could remove the url if the submodule is activated via that
setting.

In preparation to do so, pave the way by providing an easy way to see
if a submodule is considered active via the new .active setting or via
the old .url setting.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 5 +----
 submodule.h | 6 ++++++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index 6e14547e9e0..d56350ed094 100644
--- a/submodule.c
+++ b/submodule.c
@@ -221,9 +221,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
 	return 0;
 }
 
-/*
- * Determine if a submodule has been initialized at a given 'path'
- */
 int is_submodule_active(struct repository *repo, const char *path)
 {
 	int ret = 0;
@@ -267,7 +264,7 @@ int is_submodule_active(struct repository *repo, const char *path)
 
 	/* fallback to checking if the URL is set */
 	key = xstrfmt("submodule.%s.url", module->name);
-	ret = !repo_config_get_string(repo, key, &value);
+	ret = !repo_config_get_string(repo, key, &value) ? 2 : 0;
 
 	free(value);
 	free(key);
diff --git a/submodule.h b/submodule.h
index 4644683e6cb..bfc070e4629 100644
--- a/submodule.h
+++ b/submodule.h
@@ -45,6 +45,12 @@ extern int git_default_submodule_config(const char *var, const char *value, void
 struct option;
 int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
 						     const char *arg, int unset);
+/*
+ * Determine if a submodule has been initialized at a given 'path'.
+ * Returns 1 if it is considered active via the submodule.[<name>.]active
+ * setting, or return 2 if it is active via the older submodule.url setting.
+ */
+#define SUBMODULE_ACTIVE_VIA_URL 2
 extern int is_submodule_active(struct repository *repo, const char *path);
 /*
  * Determine if a submodule has been populated at a given 'path' by checking if
-- 
2.18.0.265.g16de1b435c9.dirty

Re: [PATCH 3/7] submodule: is_submodule_active to differentiate between new and old mode

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The change a086f921a72 (submodule: decouple url and submodule interest,
> 2017-03-17) enables us to do more than originally thought.
> As the url setting was used both to actually set the url where to
> obtain the submodule from, as well as used as a boolean flag later
> to see if it was active, we would need to keep the url around.
>
> Now that submodules can be activated using the submodule.[<name>.]active
> setting, we could remove the url if the submodule is activated via that
> setting.

... as the cloned submodule repository has $GIT_DIR/config which
knows its own origin, we do not need to record URL in superproject's
$GIT_DIR/config.  Back before we started using a directory under
$GIT_DIR/modules/ as a more permanent location to store submodule,
and point at it by a gitdir file in $path/.git to allow removal of a
submodule from the working tree of the superproject without having
to reclone when resurrecting the same submodule, it may have been
useful to keep custom URL somewhere in the superproject, but that no
longer is the case.

> In preparation to do so, pave the way by providing an easy way to see
> if a submodule is considered active via the new .active setting or via
> the old .url setting.

Makes sense.

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c | 5 +----
>  submodule.h | 6 ++++++
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 6e14547e9e0..d56350ed094 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -221,9 +221,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
>  	return 0;
>  }
>  
> -/*
> - * Determine if a submodule has been initialized at a given 'path'
> - */
>  int is_submodule_active(struct repository *repo, const char *path)
>  {
>  	int ret = 0;
> @@ -267,7 +264,7 @@ int is_submodule_active(struct repository *repo, const char *path)
>  
>  	/* fallback to checking if the URL is set */
>  	key = xstrfmt("submodule.%s.url", module->name);
> -	ret = !repo_config_get_string(repo, key, &value);
> +	ret = !repo_config_get_string(repo, key, &value) ? 2 : 0;
>
>  	free(value);
>  	free(key);
> diff --git a/submodule.h b/submodule.h
> index 4644683e6cb..bfc070e4629 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -45,6 +45,12 @@ extern int git_default_submodule_config(const char *var, const char *value, void
>  struct option;
>  int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
>  						     const char *arg, int unset);
> +/*
> + * Determine if a submodule has been initialized at a given 'path'.
> + * Returns 1 if it is considered active via the submodule.[<name>.]active
> + * setting, or return 2 if it is active via the older submodule.url setting.
> + */
> +#define SUBMODULE_ACTIVE_VIA_URL 2
>  extern int is_submodule_active(struct repository *repo, const char *path);
>  /*
>   * Determine if a submodule has been populated at a given 'path' by checking if

This change assumes that all the existing return sites in the
is_submodule_active() function signals true with 1 (or at least some
value that is different from 2).

But the part that uses submodule.active as pathspec list calls
match_pathspec() and uses its return value to signal if the module
is active.  match_pathspec() in turn uses dir.c::do_match_pathspec()
which signals _how_ the set of pathspec list elements matched to the
given name by returning various forms of true, like MATCHED_FNMATCH,
etc.

So I think this patch is insufficient, and needs to at least change
the "submodule.active" codepath to return !!ret; otherwise, a caller
that now expects 0 (not active), 1 (active but can lose URL) and 2
(active and the presence of URL makes it so) will be confused when
one of the MATCHED_* constants from dir.h comes back.

Re: [PATCH 3/7] submodule: is_submodule_active to differentiate between new and old mode

[Stefan Beller]

On Thu, Aug 16, 2018 at 10:37 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> > The change a086f921a72 (submodule: decouple url and submodule interest,
> > 2017-03-17) enables us to do more than originally thought.
> > As the url setting was used both to actually set the url where to
> > obtain the submodule from, as well as used as a boolean flag later
> > to see if it was active, we would need to keep the url around.
> >
> > Now that submodules can be activated using the submodule.[<name>.]active
> > setting, we could remove the url if the submodule is activated via that
> > setting.
>
> ... as the cloned submodule repository has $GIT_DIR/config which
> knows its own origin, we do not need to record URL in superproject's
> $GIT_DIR/config.  Back before we started using a directory under
> $GIT_DIR/modules/ as a more permanent location to store submodule,
> and point at it by a gitdir file in $path/.git to allow removal of a
> submodule from the working tree of the superproject without having
> to reclone when resurrecting the same submodule, it may have been
> useful to keep custom URL somewhere in the superproject, but that no
> longer is the case.
>
> > In preparation to do so, pave the way by providing an easy way to see
> > if a submodule is considered active via the new .active setting or via
> > the old .url setting.
>
> Makes sense.
>
> >
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
> >  submodule.c | 5 +----
> >  submodule.h | 6 ++++++
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/submodule.c b/submodule.c
> > index 6e14547e9e0..d56350ed094 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -221,9 +221,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
> >       return 0;
> >  }
> >
> > -/*
> > - * Determine if a submodule has been initialized at a given 'path'
> > - */
> >  int is_submodule_active(struct repository *repo, const char *path)
> >  {
> >       int ret = 0;
> > @@ -267,7 +264,7 @@ int is_submodule_active(struct repository *repo, const char *path)
> >
> >       /* fallback to checking if the URL is set */
> >       key = xstrfmt("submodule.%s.url", module->name);
> > -     ret = !repo_config_get_string(repo, key, &value);
> > +     ret = !repo_config_get_string(repo, key, &value) ? 2 : 0;
> >
> >       free(value);
> >       free(key);
> > diff --git a/submodule.h b/submodule.h
> > index 4644683e6cb..bfc070e4629 100644
> > --- a/submodule.h
> > +++ b/submodule.h
> > @@ -45,6 +45,12 @@ extern int git_default_submodule_config(const char *var, const char *value, void
> >  struct option;
> >  int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
> >                                                    const char *arg, int unset);
> > +/*
> > + * Determine if a submodule has been initialized at a given 'path'.
> > + * Returns 1 if it is considered active via the submodule.[<name>.]active
> > + * setting, or return 2 if it is active via the older submodule.url setting.
> > + */
> > +#define SUBMODULE_ACTIVE_VIA_URL 2
> >  extern int is_submodule_active(struct repository *repo, const char *path);
> >  /*
> >   * Determine if a submodule has been populated at a given 'path' by checking if
>
> This change assumes that all the existing return sites in the
> is_submodule_active() function signals true with 1 (or at least some
> value that is different from 2).

Yes.

> So I think this patch is insufficient, and needs to at least change
> the "submodule.active" codepath to return !!ret; otherwise, a caller
> that now expects 0 (not active), 1 (active but can lose URL) and 2
> (active and the presence of URL makes it so) will be confused when
> one of the MATCHED_* constants from dir.h comes back.

Yes.

I'll resend when appropriately.

Thanks,
Stefan

Re: [PATCH 3/7] submodule: is_submodule_active to differentiate between new and old mode

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> So I think this patch is insufficient, and needs to at least change
>> the "submodule.active" codepath to return !!ret; otherwise, a caller
>> that now expects 0 (not active), 1 (active but can lose URL) and 2
>> (active and the presence of URL makes it so) will be confused when
>> one of the MATCHED_* constants from dir.h comes back.
>
> Yes.
>
> I'll resend when appropriately.

Thanks.

[PATCH 4/7] submodule sync: omit setting submodule URL in config if possible

[Stefan Beller]

We do not need to update the submodule url in the superprojects config
if the url is not used to keep the submodule active.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 2f20bd4abdc..639d0bb20a1 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -922,8 +922,10 @@ static void sync_submodule(const char *path, const char *prefix,
 	struct strbuf sb = STRBUF_INIT;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	char *sub_config_path = NULL;
+	int active, r;
 
-	if (!is_submodule_active(the_repository, path))
+	active = is_submodule_active(the_repository, path);
+	if (!active)
 		return;
 
 	sub = submodule_from_path(the_repository, &null_oid, path);
@@ -983,13 +985,15 @@ static void sync_submodule(const char *path, const char *prefix,
 	strbuf_strip_suffix(&sb, "\n");
 	remote_key = xstrfmt("remote.%s.url", sb.buf);
 
+	if (active == SUBMODULE_ACTIVE_VIA_URL)
+		FREE_AND_NULL(sub_origin_url);
 	strbuf_reset(&sb);
 	submodule_to_gitdir(&sb, path);
 	strbuf_addstr(&sb, "/config");
-
-	if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))
-		die(_("failed to update remote for submodule '%s'"),
-		      path);
+	if ((r = git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url)))
+		if (sub_origin_url || r != CONFIG_NOTHING_SET)
+			die(_("failed to update remote for submodule '%s'"),
+			      path);
 
 	if (flags & OPT_RECURSIVE) {
 		struct child_process cpr = CHILD_PROCESS_INIT;
-- 
2.18.0.265.g16de1b435c9.dirty

[PATCH 5/7] submodule--helper: factor out allocation of callback cookie

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 639d0bb20a1..1c9a12781fd 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1688,6 +1688,13 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	return needs_cloning;
 }
 
+static void *update_clone_alloc_cb(int i)
+{
+	int *p = xmalloc(sizeof(*p));
+	*p = i;
+	return p;
+}
+
 static int update_clone_get_next_task(struct child_process *child,
 				      struct strbuf *err,
 				      void *suc_cb,
@@ -1700,9 +1707,7 @@ static int update_clone_get_next_task(struct child_process *child,
 	for (; suc->current < suc->list.nr; suc->current++) {
 		ce = suc->list.entries[suc->current];
 		if (prepare_to_clone_next_submodule(ce, child, suc, err)) {
-			int *p = xmalloc(sizeof(*p));
-			*p = suc->current;
-			*idx_task_cb = p;
+			*idx_task_cb = update_clone_alloc_cb(suc->current);
 			suc->current++;
 			return 1;
 		}
@@ -1715,7 +1720,6 @@ static int update_clone_get_next_task(struct child_process *child,
 	 */
 	index = suc->current - suc->list.nr;
 	if (index < suc->failed_clones_nr) {
-		int *p;
 		ce = suc->failed_clones[index];
 		if (!prepare_to_clone_next_submodule(ce, child, suc, err)) {
 			suc->current ++;
@@ -1724,9 +1728,7 @@ static int update_clone_get_next_task(struct child_process *child,
 					   "any more?\n");
 			return 0;
 		}
-		p = xmalloc(sizeof(*p));
-		*p = suc->current;
-		*idx_task_cb = p;
+		*idx_task_cb = update_clone_alloc_cb(suc->current);
 		suc->current ++;
 		return 1;
 	}
-- 
2.18.0.265.g16de1b435c9.dirty

[PATCH 6/7] submodule--helper, update_clone: store index to update_clone instead of ce

[Stefan Beller]

In update_submodules, we use the run_processes_parallel(get_task, finished)
API, which allows to pass around a task specific callback cookie from the
get_next function to the finish function. That finish function in turn may
alter generic callback cookie to have the next call of get_task come up
with another new task.

Up to now we passed around the index into a list of cache entries,
which was stored in the generic callback cookie which is a struct
submodule_update_clone.

Change this to an index into 'update_clone' array, which is the potential
output of this helper. This will allow for a future change to make
use of the data the update_clone_data struct.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1c9a12781fd..36de64902ec 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1511,8 +1511,10 @@ static int module_update_module_mode(int argc, const char **argv, const char *pr
 
 struct update_clone_data {
 	const struct submodule *sub;
+	const struct cache_entry *ce;
 	struct object_id oid;
 	unsigned just_cloned;
+	unsigned retried;
 };
 
 struct submodule_update_clone {
@@ -1541,8 +1543,8 @@ struct submodule_update_clone {
 	/* If we want to stop as fast as possible and return an error */
 	unsigned quickstop : 1;
 
-	/* failed clones to be retried again */
-	const struct cache_entry **failed_clones;
+	/* failed clones to be retried again, indexes into update_clone */
+	int *failed_clones;
 	int failed_clones_nr, failed_clones_alloc;
 
 	int max_jobs;
@@ -1649,6 +1651,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	oidcpy(&suc->update_clone[suc->update_clone_nr].oid, &ce->oid);
 	suc->update_clone[suc->update_clone_nr].just_cloned = needs_cloning;
 	suc->update_clone[suc->update_clone_nr].sub = sub;
+	suc->update_clone[suc->update_clone_nr].retried = 0;
+	suc->update_clone[suc->update_clone_nr].ce = ce;
 	suc->update_clone_nr++;
 
 	if (!needs_cloning)
@@ -1707,7 +1711,8 @@ static int update_clone_get_next_task(struct child_process *child,
 	for (; suc->current < suc->list.nr; suc->current++) {
 		ce = suc->list.entries[suc->current];
 		if (prepare_to_clone_next_submodule(ce, child, suc, err)) {
-			*idx_task_cb = update_clone_alloc_cb(suc->current);
+			*idx_task_cb = update_clone_alloc_cb(
+				suc->update_clone_nr - 1);
 			suc->current++;
 			return 1;
 		}
@@ -1720,7 +1725,9 @@ static int update_clone_get_next_task(struct child_process *child,
 	 */
 	index = suc->current - suc->list.nr;
 	if (index < suc->failed_clones_nr) {
-		ce = suc->failed_clones[index];
+		int ucd_index = suc->failed_clones[index];
+		struct update_clone_data *ucd = &suc->update_clone[ucd_index];
+		ce = ucd->ce;
 		if (!prepare_to_clone_next_submodule(ce, child, suc, err)) {
 			suc->current ++;
 			strbuf_addstr(err, "BUG: submodule considered for "
@@ -1728,7 +1735,7 @@ static int update_clone_get_next_task(struct child_process *child,
 					   "any more?\n");
 			return 0;
 		}
-		*idx_task_cb = update_clone_alloc_cb(suc->current);
+		*idx_task_cb = update_clone_alloc_cb(ucd_index);
 		suc->current ++;
 		return 1;
 	}
@@ -1750,31 +1757,31 @@ static int update_clone_task_finished(int result,
 				      void *suc_cb,
 				      void *idx_task_cb)
 {
-	const struct cache_entry *ce;
 	struct submodule_update_clone *suc = suc_cb;
+	struct update_clone_data *ucd;
 
 	int *idxP = idx_task_cb;
 	int idx = *idxP;
+	ucd = &suc->update_clone[idx];
 	free(idxP);
 
 	if (!result)
 		return 0;
 
-	if (idx < suc->list.nr) {
-		ce  = suc->list.entries[idx];
+	if (!ucd->retried) {
+		ucd->retried = 1;
 		strbuf_addf(err, _("Failed to clone '%s'. Retry scheduled"),
-			    ce->name);
+			    ucd->ce->name);
 		strbuf_addch(err, '\n');
 		ALLOC_GROW(suc->failed_clones,
 			   suc->failed_clones_nr + 1,
 			   suc->failed_clones_alloc);
-		suc->failed_clones[suc->failed_clones_nr++] = ce;
+		suc->failed_clones[suc->failed_clones_nr++] = idx;
 		return 0;
 	} else {
 		idx -= suc->list.nr;
-		ce  = suc->failed_clones[idx];
 		strbuf_addf(err, _("Failed to clone '%s' a second time, aborting"),
-			    ce->name);
+			    ucd->ce->name);
 		strbuf_addch(err, '\n');
 		suc->quickstop = 1;
 		return 1;
-- 
2.18.0.265.g16de1b435c9.dirty

[PATCH 7/7] builtin/submodule--helper: unset submodule url if possible

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c      | 24 ++++++++++++++++++++++--
 t/t5526-fetch-submodules.sh      |  2 +-
 t/t7406-submodule-update.sh      |  8 ++++++++
 t/t7410-submodule-checkout-to.sh |  2 +-
 4 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 36de64902ec..3aa385bce5c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1515,6 +1515,7 @@ struct update_clone_data {
 	struct object_id oid;
 	unsigned just_cloned;
 	unsigned retried;
+	unsigned cleanup_url;
 };
 
 struct submodule_update_clone {
@@ -1590,7 +1591,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	struct strbuf displaypath_sb = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	const char *displaypath = NULL;
-	int needs_cloning = 0;
+	int needs_cloning = 0, active;
 
 	if (ce_stage(ce)) {
 		if (suc->recursive_prefix)
@@ -1632,7 +1633,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	}
 
 	/* Check if the submodule has been initialized. */
-	if (!is_submodule_active(the_repository, ce->name)) {
+	active = is_submodule_active(the_repository, ce->name);
+	if (!active) {
 		next_submodule_warn_missing(suc, out, displaypath);
 		goto cleanup;
 	}
@@ -1653,6 +1655,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	suc->update_clone[suc->update_clone_nr].sub = sub;
 	suc->update_clone[suc->update_clone_nr].retried = 0;
 	suc->update_clone[suc->update_clone_nr].ce = ce;
+	suc->update_clone[suc->update_clone_nr].cleanup_url =
+		(active != SUBMODULE_ACTIVE_VIA_URL);
 	suc->update_clone_nr++;
 
 	if (!needs_cloning)
@@ -1801,6 +1805,22 @@ static int git_update_clone_config(const char *var, const char *value,
 
 static void update_submodule(struct update_clone_data *ucd)
 {
+	if (ucd->cleanup_url) {
+		struct strbuf cfg = STRBUF_INIT;
+		struct strbuf submodule_url = STRBUF_INIT;
+		int r;
+
+		strbuf_addf(&submodule_url, "submodule.%s.url", ucd->sub->name);
+		strbuf_repo_git_path(&cfg, the_repository, "config");
+
+		r = git_config_set_in_file_gently(cfg.buf, submodule_url.buf, NULL);
+		if (r && r != CONFIG_NOTHING_SET)
+			die(_("failed to remove '%s'"), submodule_url.buf);
+
+		strbuf_release(&cfg);
+		strbuf_release(&submodule_url);
+	}
+
 	fprintf(stdout, "dummy %s %d\t%s\n",
 		oid_to_hex(&ucd->oid),
 		ucd->just_cloned,
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 0f730d77815..cd1bd131b59 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -508,7 +508,7 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' works also without .git
 		git config -f .gitmodules submodule.fake.path fake &&
 		git config -f .gitmodules submodule.fake.url fakeurl &&
 		git add .gitmodules &&
-		git config --unset submodule.submodule.url &&
+		test_might_fail git config --unset submodule.submodule.url &&
 		git fetch >../actual.out 2>../actual.err &&
 		# cleanup
 		git config --unset fetch.recurseSubmodules &&
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f604ef7a729..f581fea28e0 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -84,6 +84,14 @@ test_expect_success 'submodule update detaching the HEAD ' '
 	)
 '
 
+test_expect_success 'active submodule leaves no URL config in superproject' '
+	# relies on previous test
+	(
+		cd super &&
+		test_must_fail git config -f .git/config submodule.submodule.url
+	)
+'
+
 test_expect_success 'submodule update from subdirectory' '
 	(cd super/submodule &&
 	 git reset --hard HEAD~1
diff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh
index f1b492ebc46..683e957934b 100755
--- a/t/t7410-submodule-checkout-to.sh
+++ b/t/t7410-submodule-checkout-to.sh
@@ -55,7 +55,7 @@ test_expect_failure 'can see submodule diffs just after checkout' '
 test_expect_success 'checkout main and initialize independent clones' '
 	mkdir fully_cloned_submodule &&
 	git -C clone/main worktree add "$base_path/fully_cloned_submodule/main" "$rev1_hash_main" &&
-	git -C fully_cloned_submodule/main submodule update
+	git -C fully_cloned_submodule/main submodule update --init
 '
 
 test_expect_success 'can see submodule diffs after independent cloning' '
-- 
2.18.0.265.g16de1b435c9.dirty

Re: [RFC PATCH 0/7] Unset the submodule URL in the superproject when no longer needed

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>   Originally we have had the url in the config, (a) that we can change
>   the URLs after the "git submodule init" and "git submodule update"
>   step that actually clones the submodule if not present and much more
>   importantly (b) to know which submodule "was initialized/active".
>   
>   Now that we have the submodule.active or even
>   submodule.<name>.active flags, we do not need (b) any more.

Up to that point the description is sane.

>   So the URL turns into a useless piece of cruft that just is unneeded
>   and might confuse the user.
>
> Opinions?

You spelled out why you do not need for (b) but not for (a) and
worse it is is unclear if you never need it for (a) or under what
condition you need it for (a).  So there isn't enough information to
form an opinion in the above.  Sorry--readers need to go to the real
patches.

Re: [RFC PATCH 0/7] Unset the submodule URL in the superproject when no longer needed

[Stefan Beller]

On Thu, Aug 16, 2018 at 8:12 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> >   Originally we have had the url in the config, (a) that we can change
> >   the URLs after the "git submodule init" and "git submodule update"
> >   step that actually clones the submodule if not present and much more
> >   importantly (b) to know which submodule "was initialized/active".
> >
> >   Now that we have the submodule.active or even
> >   submodule.<name>.active flags, we do not need (b) any more.
>
> Up to that point the description is sane.
>
> >   So the URL turns into a useless piece of cruft that just is unneeded
> >   and might confuse the user.
> >
> > Opinions?
>
> You spelled out why you do not need for (b) but not for (a) and
> worse it is is unclear if you never need it for (a) or under what
> condition you need it for (a).  So there isn't enough information to
> form an opinion in the above.  Sorry--readers need to go to the real
> patches.

Regarding (a): Once the submodule is cloned, you either need
to change the remote in the submodule or you can use "git submodule sync"
which can bypass the superproject config, too (that copies the URL from
.gitmodules to the submodules config/remote)

I don't think (a) is needed after the clone of a submodule is done.