[RFC PATCH v3 0/4] implement fetching of moved submodules

[RFC PATCH v3 0/4] implement fetching of moved submodules

[Heiko Voigt]

The last iteration can be found here:

https://public-inbox.org/git/20170817105349.GC52233@book.hvoigt.net/

This is mainly a status update and to let people know that I am still
working on this.

I struggled quite a bit with reviving my original test for the path
based recursive fetch (first patch). The behavior seems to haved changed
and simply setting the submodule configuration in .git/config without
one in .gitmodules does not work anymore. I did not have time to
investigate whether this was a deliberate change or a maybe a bug?

So the solution for now is that I write my fake configuration (to avoid
skipping submodule handling altogether) into a .gitmodules file.

The second patch (cleanup of a submodule push testcase) was written
because that currently is the only test failing. It is not meant for
inclusion but rather as a demonstration of what might be happening when
we cleanup testcases: Because of the behavioral change above, on first
sight, it seemed like there was a shortcut in fetch and so on-demand
fetch without submodule configuration would not be supported anymore.

IIRC there were a lot more tests failing before when I implemented my
patch without the fallback on paths. So my guess is that some tests have
been cleaned up to use proper (.gitmodules) submodule setup.

So the thing here is: If we want to make sure that we stay backwards
compatible by supporting the setup with gitlinks without configuration.
Then we also should keep tests around that have the plain manual setup
without .gitmodules files. Just something, I think, we should keep in
mind.

Apart from the tests nothing has been added in this iteration. Since I
finally have a working test now I will continue with reviving the
fallback to paths.

Cheers Heiko

Heiko Voigt (4):
  fetch: add test to make sure we stay backwards compatible
  change submodule push test to use proper repository setup
  implement fetching of moved submodules
  submodule: simplify decision tree whether to or not to fetch

 submodule.c                    | 155 ++++++++++++++++++++++-------------------
 t/t5526-fetch-submodules.sh    |  77 +++++++++++++++++++-
 t/t5531-deep-submodule-push.sh |  29 ++++----
 3 files changed, 174 insertions(+), 87 deletions(-)

-- 
2.10.0.129.g35f6318

[RFC PATCH 1/4] fetch: add test to make sure we stay backwards compatible

[Heiko Voigt]

The current implementation of submodules supports on-demand fetch if
there is no .gitmodules entry for a submodule. Let's add a test to
document this behavior.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 t/t5526-fetch-submodules.sh | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 42251f7..43a22f6 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -478,7 +478,47 @@ test_expect_success "don't fetch submodule when newly recorded commits are alrea
 		git fetch >../actual.out 2>../actual.err
 	) &&
 	! test -s actual.out &&
-	test_i18ncmp expect.err actual.err
+	test_i18ncmp expect.err actual.err &&
+	(
+		cd submodule &&
+		git checkout -q master
+	)
+'
+
+test_expect_success "'fetch.recurseSubmodules=on-demand' works also without .gitmodule entry" '
+	(
+		cd downstream &&
+		git fetch --recurse-submodules
+	) &&
+	add_upstream_commit &&
+	head1=$(git rev-parse --short HEAD) &&
+	git add submodule &&
+	git rm .gitmodules &&
+	git commit -m "new submodule without .gitmodules" &&
+	printf "" >expect.out &&
+	head2=$(git rev-parse --short HEAD) &&
+	echo "From $pwd/." >expect.err.2 &&
+	echo "   $head1..$head2  master     -> origin/master" >>expect.err.2 &&
+	head -3 expect.err >>expect.err.2 &&
+	(
+		cd downstream &&
+		rm .gitmodules &&
+		git config fetch.recurseSubmodules on-demand &&
+		# fake submodule configuration to avoid skipping submodule handling
+		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 &&
+		git fetch >../actual.out 2>../actual.err &&
+		# cleanup
+		git config --unset fetch.recurseSubmodules &&
+		git reset --hard
+	) &&
+	test_i18ncmp expect.out actual.out &&
+	test_i18ncmp expect.err.2 actual.err &&
+	git checkout HEAD^ -- .gitmodules &&
+	git add .gitmodules &&
+	git commit -m "new submodule restored .gitmodules"
 '
 
 test_expect_success 'fetching submodules respects parallel settings' '
-- 
2.10.0.129.g35f6318

[RFC PATCH 2/4] change submodule push test to use proper repository setup

[Heiko Voigt]

NOTE: The argument in this message is not correct, see description in
cover letter.

The setup of the repositories in this test is using gitlinks without the
.gitmodules infrastructure. It is however testing convenience features
like --recurse-submodules=on-demand. These features are already not
supported by fetch without a .gitmodules file. This leads us to the
conclusion that it is not really used here as well.

Let's use the usual submodule commands to setup the repository in a
typical way. This also has the advantage that we are testing with a
repository structure that is more similar to one we could expect on a
users setup.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---

As mentioned in the cover letter. This seems to be the only test that
ensures that we stay compatible with setups without .gitmodules. Maybe
we should add/revive some?

Cheers Heiko

 t/t5531-deep-submodule-push.sh | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 39cb2c1..a4a2c6a 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -8,22 +8,26 @@ test_expect_success setup '
 	mkdir pub.git &&
 	GIT_DIR=pub.git git init --bare &&
 	GIT_DIR=pub.git git config receive.fsckobjects true &&
+	mkdir submodule &&
+	(
+		cd submodule &&
+		git init &&
+		git config push.default matching &&
+		>junk &&
+		git add junk &&
+		git commit -m "Initial junk"
+	) &&
+	git clone --bare submodule submodule.git &&
 	mkdir work &&
 	(
 		cd work &&
 		git init &&
 		git config push.default matching &&
-		mkdir -p gar/bage &&
-		(
-			cd gar/bage &&
-			git init &&
-			git config push.default matching &&
-			>junk &&
-			git add junk &&
-			git commit -m "Initial junk"
-		) &&
-		git add gar/bage &&
+		mkdir gar &&
+		git submodule add ../submodule.git gar/bage &&
 		git commit -m "Initial superproject"
+		cd gar/bage &&
+		git remote rm origin
 	)
 '
 
@@ -51,11 +55,10 @@ test_expect_success 'push if submodule has no remote' '
 
 test_expect_success 'push fails if submodule commit not on remote' '
 	(
-		cd work/gar &&
-		git clone --bare bage ../../submodule.git &&
-		cd bage &&
+		cd work/gar/bage &&
 		git remote add origin ../../../submodule.git &&
 		git fetch &&
+		git push --set-upstream origin master &&
 		>junk3 &&
 		git add junk3 &&
 		git commit -m "Third junk"
-- 
2.10.0.129.g35f6318

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Stefan Beller]

On Fri, Oct 6, 2017 at 3:32 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> NOTE: The argument in this message is not correct, see description in
> cover letter.
>
> The setup of the repositories in this test is using gitlinks without the
> .gitmodules infrastructure. It is however testing convenience features
> like --recurse-submodules=on-demand. These features are already not
> supported by fetch without a .gitmodules file. This leads us to the
> conclusion that it is not really used here as well.
>
> Let's use the usual submodule commands to setup the repository in a
> typical way. This also has the advantage that we are testing with a
> repository structure that is more similar to one we could expect on a
> users setup.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
>
> As mentioned in the cover letter. This seems to be the only test that
> ensures that we stay compatible with setups without .gitmodules. Maybe
> we should add/revive some?

An interesting discussion covering this topic is found at
https://public-inbox.org/git/20170606035650.oykbz2uc4xkr3cr2@sigill.intra.peff.net/

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Heiko Voigt]

Hi,

On Mon, Oct 09, 2017 at 11:20:51AM -0700, Stefan Beller wrote:
> On Fri, Oct 6, 2017 at 3:32 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > NOTE: The argument in this message is not correct, see description in
> > cover letter.
> >
> > The setup of the repositories in this test is using gitlinks without the
> > .gitmodules infrastructure. It is however testing convenience features
> > like --recurse-submodules=on-demand. These features are already not
> > supported by fetch without a .gitmodules file. This leads us to the
> > conclusion that it is not really used here as well.
> >
> > Let's use the usual submodule commands to setup the repository in a
> > typical way. This also has the advantage that we are testing with a
> > repository structure that is more similar to one we could expect on a
> > users setup.
> >
> > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> > ---
> >
> > As mentioned in the cover letter. This seems to be the only test that
> > ensures that we stay compatible with setups without .gitmodules. Maybe
> > we should add/revive some?
> 
> An interesting discussion covering this topic is found at
> https://public-inbox.org/git/20170606035650.oykbz2uc4xkr3cr2@sigill.intra.peff.net/

Thanks for that pointer. So in that discussion Junio said that the
recursive operations should succeed if we have everything necessary at
hand. I kind of agree because why should we limit usage when not
necessary. On the other hand we want git to be easy to use. And that
example from Peff is perfect as a demonstration of a incosistency we
currently have:

git clone git://some.where.git/submodule.git
git add submodule

is an operation I remember, I did, when first getting in contact with
submodules (many years back), since that is one intuitive way. And the
thing is: It works, kind of... Only later I discovered that one actually
needs to us a special submodule command to get everything approriately
setup to work together with others.

If everyone agrees that submodules are the default way of handling
repositories insided repositories, IMO, 'git add' should also alter
.gitmodules by default. We could provide a switch to avoid doing that.

An intermediate solution would be to warn but in the long run my goal
for submodules is and always was: Make them behave as close to files as
possible. And why should a 'git add submodule' not magically do
everything it can to make submodules just work? I can look into a patch
for that if people agree here...

Regarding handling of gitlinks with or without .gitmodules:

Currently we are actually in some intermediate state:

 * If there is no .gitmodules file: No submodule processing on any
   gitlinks (AFAIK)
 * If there is a .gitmodules files with some submodule configured: Do
   recursive fetch and push as far as possible on gitlinks.

So I am not sure whether there are actually many users (knowingly)
using a mix of some submodules configured and some not and then relying
on the submodule infrastructure.

I would rather expect two sorts of users:

  1. Those that do use .gitmodules

  2. Those that do *not* use .gitmodules

Users that do not use any .gitmodules file will currently (AFAIK) not
get any submodule handling. So the question is are there really many
"mixed users"? My guess would be no.
Because without those using this mixed we could switch to saying: "You
need to have a .gitmodules file for submodule handling" without much
fallout from breaking users use cases.

Maybe we can test this out somehow? My patch series would be ready in
that case, just had to drop the first patch and adjust the commit
message of this one.

Cheers Heiko

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Stefan Beller]

On Tue, Oct 10, 2017 at 6:03 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:

>> > As mentioned in the cover letter. This seems to be the only test that
>> > ensures that we stay compatible with setups without .gitmodules. Maybe
>> > we should add/revive some?
>>
>> An interesting discussion covering this topic is found at
>> https://public-inbox.org/git/20170606035650.oykbz2uc4xkr3cr2@sigill.intra.peff.net/
>
> Thanks for that pointer. So in that discussion Junio said that the
> recursive operations should succeed if we have everything necessary at
> hand. I kind of agree because why should we limit usage when not
> necessary. On the other hand we want git to be easy to use. And that
> example from Peff is perfect as a demonstration of a incosistency we
> currently have:
>
> git clone git://some.where.git/submodule.git
> git add submodule
>
> is an operation I remember, I did, when first getting in contact with
> submodules (many years back), since that is one intuitive way. And the
> thing is: It works, kind of... Only later I discovered that one actually
> needs to us a special submodule command to get everything approriately
> setup to work together with others.

I agree that we ought to not block off users "because we can", but rather
perform the operation if possible with the data at hand.

Note that the result of the discussion `jk/warn-add-gitlink actually`
warns about adding a raw gitlink now, such that we hint at using
"git submodule add", directly.

So you propose to make git-add behave like "git submodule add"
(i.e. also add the .gitmodules entry for name/path/URL), which I
like from a submodule perspective.

However other users of gitlinks might be confused[1], which is why
I refrained from "making every gitlink into a submodule". Specifically
the more powerful a submodule operation is (the more fluff adds),
the harder it should be for people to mis-use it.

[1] https://github.com/git-series/git-series/blob/master/INTERNALS.md
     "git-series uses gitlinks to store pointer to commits in its own repo."

> If everyone agrees that submodules are the default way of handling
> repositories insided repositories, IMO, 'git add' should also alter
> .gitmodules by default. We could provide a switch to avoid doing that.

I wonder if that switch should be default-on (i.e. not treat a gitlink as
a submodule initially, behavior as-is, and then eventually we will
die() on unconfigured repos, expecting the user to make the decision)

> An intermediate solution would be to warn

That is already implemented by Peff.

> but in the long run my goal
> for submodules is and always was: Make them behave as close to files as
> possible. And why should a 'git add submodule' not magically do
> everything it can to make submodules just work? I can look into a patch
> for that if people agree here...

I'd love to see this implemented. I cc'd Josh (the author of git-series), who
may disagree with this, or has some good input how to go forward without
breaking git-series.

> Regarding handling of gitlinks with or without .gitmodules:
>
> Currently we are actually in some intermediate state:
>
>  * If there is no .gitmodules file: No submodule processing on any
>    gitlinks (AFAIK)

AFAIK this is true.

>  * If there is a .gitmodules files with some submodule configured: Do
>    recursive fetch and push as far as possible on gitlinks.

* If submodule.recurse is set, then we also treat submodules like files
  for checkout, reset, read-tree.

> So I am not sure whether there are actually many users (knowingly)
> using a mix of some submodules configured and some not and then relying
> on the submodule infrastructure.
>
> I would rather expect two sorts of users:
>
>   1. Those that do use .gitmodules

Those want to reap all benefits of good submodules.

>
>   2. Those that do *not* use .gitmodules

As said above, we don't know if those users are
"holding submodules wrong" or are using gitlinks for
magic tricks (unrelated to submodules).

>
> Users that do not use any .gitmodules file will currently (AFAIK) not
> get any submodule handling. So the question is are there really many
> "mixed users"? My guess would be no.

I hope that there are few (if any) users of these mixed setups.

> Because without those using this mixed we could switch to saying: "You
> need to have a .gitmodules file for submodule handling" without much
> fallout from breaking users use cases.

That seems reasonable to me, actually.

> Maybe we can test this out somehow? My patch series would be ready in
> that case, just had to drop the first patch and adjust the commit
> message of this one.

I wonder how we would test this, though? Do you have any idea
(even vague) how we'd accomplish such a measurement?
I fear we'll have to go this way blindly.

Cheers,
Stefan

>
> Cheers Heiko

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So you propose to make git-add behave like "git submodule add"
> (i.e. also add the .gitmodules entry for name/path/URL), which I
> like from a submodule perspective.
>
> However other users of gitlinks might be confused[1], which is why
> I refrained from "making every gitlink into a submodule". Specifically
> the more powerful a submodule operation is (the more fluff adds),
> the harder it should be for people to mis-use it.

A few questions that come to mind are:

 - Does "git add sub/" have enough information to populate
   .gitmodules?  If we have reasonable "default" values for
   .gitmodules entries (e.g. missing URL means we won't fetch when
   asked to go recursively fetch), perhaps we can leave everything
   other than "submodule.$name.path" undefined.

 - Can't we help those who have gitlinks without .gitmodules entries
   exactly the same way as above, i.e. when we see a gitlink and try
   to treat it as a submodule, we'd first try to look it up from
   .gitmodules (by going from path to name and then to
   submodule.$name.$var); the above "'git add sub/' would add an
   entry for .gitmodules" wish is based on the assumption that there
   are reasonable "default" values for each of these $var--so by
   basing on the same assumption, we can "pretend" as if these
   submodule.$name.$var were in .gitmodules file when we see
   gitlinks without .gitmodules entries.  IOW, if "git add sub/" can
   add .gitmodules to help people without having to type "git
   submodule add sub/", then we can give exactly the same degree of
   help without even modifying .gitmodules when "git add sub/" is
   run.

 - Even if we could solve it with "git add sub/" that adds to
   .gitmodules, is it a good solution, when we can solve the same
   thing without having to do so?

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Stefan Beller]

On Tue, Oct 10, 2017 at 4:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So you propose to make git-add behave like "git submodule add"
>> (i.e. also add the .gitmodules entry for name/path/URL), which I
>> like from a submodule perspective.
>>
>> However other users of gitlinks might be confused[1], which is why
>> I refrained from "making every gitlink into a submodule". Specifically
>> the more powerful a submodule operation is (the more fluff adds),
>> the harder it should be for people to mis-use it.
>
> A few questions that come to mind are:
>
>  - Does "git add sub/" have enough information to populate
>    .gitmodules?  If we have reasonable "default" values for
>    .gitmodules entries (e.g. missing URL means we won't fetch when
>    asked to go recursively fetch), perhaps we can leave everything
>    other than "submodule.$name.path" undefined.

I think we would want to populate path and URL only.

>
>  - Can't we help those who have gitlinks without .gitmodules entries
>    exactly the same way as above, i.e. when we see a gitlink and try
>    to treat it as a submodule, we'd first try to look it up from
>    .gitmodules (by going from path to name and then to
>    submodule.$name.$var); the above "'git add sub/' would add an
>    entry for .gitmodules" wish is based on the assumption that there
>    are reasonable "default" values for each of these $var--so by
>    basing on the same assumption, we can "pretend" as if these
>    submodule.$name.$var were in .gitmodules file when we see
>    gitlinks without .gitmodules entries.  IOW, if "git add sub/" can
>    add .gitmodules to help people without having to type "git
>    submodule add sub/", then we can give exactly the same degree of
>    help without even modifying .gitmodules when "git add sub/" is
>    run.

I do not understand the gist of this paragraph, other then:

  "When git-add <repository> encounters a section submodule.<name>.*,
   do not modify it; We can assume it is sane already."

>  - Even if we could solve it with "git add sub/" that adds to
>    .gitmodules, is it a good solution, when we can solve the same
>    thing without having to do so?

I am confused even more.

So you suggest that "git add [--gitlink=submodule]" taking on the
responsibilities of "git submodule add" is a bad idea?

I thought we had the same transition from "git remote update" to
"git fetch", which eventually superseded the former.

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> A few questions that come to mind are:
>
>  - Does "git add sub/" have enough information to populate
>    .gitmodules?  If we have reasonable "default" values for
>    .gitmodules entries (e.g. missing URL means we won't fetch when
>    asked to go recursively fetch), perhaps we can leave everything
>    other than "submodule.$name.path" undefined.
> ...
>  - ...  IOW, if "git add sub/" can
>    add .gitmodules to help people without having to type "git
>    submodule add sub/", then we can give exactly the same degree of
>    help without even modifying .gitmodules when "git add sub/" is
>    run.

Answering my own questions (aka correcting my own stupidity), there
is a big leap/gap between the two that came from my forgetting an
important point: a local repository has a lot richer information
than others that are clones of it.

"git add sub/" could look at sub/.git/config and use that
information when considering what values to populate .gitmodules
with.  It can learn where its origin remote is, for example.

And while this can do that at look-up time locally (i.e. removing
the need to do .gitmodules), those who pull from this local
repository, of those who pull from a shared central repository this
local repository pushes into, will not have the same information
available to them, _unless_ this local repository records it in the
.gitmodules file for them to use.

So, I think "git add sub/" that adds to .gitmodules would work
(unless the sub/ repository originates locally without pushing
out--in which case, submodule.$name.url cannot be populated with a
value suitable for other people, and we should continue warning),
while doing the same at look-up time would not be a good solution.

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Heiko Voigt]

On Wed, Oct 11, 2017 at 08:31:37AM +0900, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
> 
> > So you propose to make git-add behave like "git submodule add"
> > (i.e. also add the .gitmodules entry for name/path/URL), which I
> > like from a submodule perspective.
> >
> > However other users of gitlinks might be confused[1], which is why
> > I refrained from "making every gitlink into a submodule". Specifically
> > the more powerful a submodule operation is (the more fluff adds),
> > the harder it should be for people to mis-use it.
> 
> A few questions that come to mind are:
> 
>  - Does "git add sub/" have enough information to populate
>    .gitmodules?  If we have reasonable "default" values for
>    .gitmodules entries (e.g. missing URL means we won't fetch when
>    asked to go recursively fetch), perhaps we can leave everything
>    other than "submodule.$name.path" undefined.

My suggestion would be: If we do not have them we do not populate them.
We could even go further and say: If we do not have the set "git
submodule add" would populate then we do not add anything to .gitmodules
and warn the user.

>  - Can't we help those who have gitlinks without .gitmodules entries
>    exactly the same way as above, i.e. when we see a gitlink and try
>    to treat it as a submodule, we'd first try to look it up from
>    .gitmodules (by going from path to name and then to
>    submodule.$name.$var); the above "'git add sub/' would add an
>    entry for .gitmodules" wish is based on the assumption that there
>    are reasonable "default" values for each of these $var--so by
>    basing on the same assumption, we can "pretend" as if these
>    submodule.$name.$var were in .gitmodules file when we see
>    gitlinks without .gitmodules entries.  IOW, if "git add sub/" can
>    add .gitmodules to help people without having to type "git
>    submodule add sub/", then we can give exactly the same degree of
>    help without even modifying .gitmodules when "git add sub/" is
>    run.

This "default" value thing got me thinking in a different direction. We
could use a scheme like that to get names (and values) for submodules
that are missing from the .gitmodules file. If we decide that we need to
handle them.

Cheers Heiko

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> This "default" value thing got me thinking in a different direction. We
> could use a scheme like that to get names (and values) for submodules
> that are missing from the .gitmodules file. If we decide that we need to
> handle them.

Yes, I suspect that would improve things quite a bit in a repository
where it added a new submodule by filling the gap between the time
when a gitlink is added and an entry in .gitmodules is added.  The
latter needs to happen if the result of the work done in that
repository is pushed out elsewhere---otherwise it won't usable by
other people.

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Heiko Voigt]

On Tue, Oct 10, 2017 at 11:39:21AM -0700, Stefan Beller wrote:
> So you propose to make git-add behave like "git submodule add"
> (i.e. also add the .gitmodules entry for name/path/URL), which I
> like from a submodule perspective.

Well more like: clone and add will behave like "git submodule add" but
basically yes.

> However other users of gitlinks might be confused[1], which is why
> I refrained from "making every gitlink into a submodule". Specifically
> the more powerful a submodule operation is (the more fluff adds),
> the harder it should be for people to mis-use it.
> 
> [1] https://github.com/git-series/git-series/blob/master/INTERNALS.md
>      "git-series uses gitlinks to store pointer to commits in its own repo."

But would those users use

    git add

to add a gitlink? From the description in that file I read that it
points to commits in its own repository. Will there also be files
checked out like submodules at that location?

Otherwise I would propose that 'git add' could detect whether a gitlink
is a submodule by trying to read its git configuration. If we do not
find that we simply do not do anything.

> > If everyone agrees that submodules are the default way of handling
> > repositories insided repositories, IMO, 'git add' should also alter
> > .gitmodules by default. We could provide a switch to avoid doing that.
> 
> I wonder if that switch should be default-on (i.e. not treat a gitlink as
> a submodule initially, behavior as-is, and then eventually we will
> die() on unconfigured repos, expecting the user to make the decision)
> 
> > An intermediate solution would be to warn
> 
> That is already implemented by Peff.

Ah ok, thanks I suspected so when I realized that this discussion was
older.

> > but in the long run my goal
> > for submodules is and always was: Make them behave as close to files as
> > possible. And why should a 'git add submodule' not magically do
> > everything it can to make submodules just work? I can look into a patch
> > for that if people agree here...
> 
> I'd love to see this implemented. I cc'd Josh (the author of git-series), who
> may disagree with this, or has some good input how to go forward without
> breaking git-series.

Yeah, lets see if, as described above, that actually would break
git-series.

> > Regarding handling of gitlinks with or without .gitmodules:
> >
> > Currently we are actually in some intermediate state:
> >
> >  * If there is no .gitmodules file: No submodule processing on any
> >    gitlinks (AFAIK)
> 
> AFAIK this is true.
> 
> >  * If there is a .gitmodules files with some submodule configured: Do
> >    recursive fetch and push as far as possible on gitlinks.
> 
> * If submodule.recurse is set, then we also treat submodules like files
>   for checkout, reset, read-tree.

To clarify: If submodule.recurse is set but there is no .gitmodules file
we do submodule processing for the above commands?

> > So I am not sure whether there are actually many users (knowingly)
> > using a mix of some submodules configured and some not and then relying
> > on the submodule infrastructure.
> >
> > I would rather expect two sorts of users:
> >
> >   1. Those that do use .gitmodules
> 
> Those want to reap all benefits of good submodules.
> 
> >
> >   2. Those that do *not* use .gitmodules
> 
> As said above, we don't know if those users are
> "holding submodules wrong" or are using gitlinks for
> magic tricks (unrelated to submodules).

I did not want to say that they are "holding submodules wrong" but
rather that if they do not use .gitmodules they do that knowingly and
thus consistently not use .gitmodules for any gitlink.

> > Users that do not use any .gitmodules file will currently (AFAIK) not
> > get any submodule handling. So the question is are there really many
> > "mixed users"? My guess would be no.
> 
> I hope that there are few (if any) users of these mixed setups.

That sounds promising.

> > Because without those using this mixed we could switch to saying: "You
> > need to have a .gitmodules file for submodule handling" without much
> > fallout from breaking users use cases.
> 
> That seems reasonable to me, actually.

Nice.

> > Maybe we can test this out somehow? My patch series would be ready in
> > that case, just had to drop the first patch and adjust the commit
> > message of this one.
> 
> I wonder how we would test this, though? Do you have any idea
> (even vague) how we'd accomplish such a measurement?
> I fear we'll have to go this way blindly.

One idea would be to expose this somewhere to a limited amount of users.
I remember Jonathan was suggesting, back when Jens was working on the
recursive checkout, that he could add the series to the debian package
and see what happens. Or we could use Junios next branch? Something like
that. If we get complaints we know the assumption was wrong and we need
a fallback.

Cheers Heiko

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Josh Triplett]

On Tue, Oct 10, 2017 at 11:39:21AM -0700, Stefan Beller wrote:
> On Tue, Oct 10, 2017 at 6:03 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > but in the long run my goal
> > for submodules is and always was: Make them behave as close to files as
> > possible. And why should a 'git add submodule' not magically do
> > everything it can to make submodules just work? I can look into a patch
> > for that if people agree here...
> 
> I'd love to see this implemented. I cc'd Josh (the author of git-series), who
> may disagree with this, or has some good input how to go forward without
> breaking git-series.

git-series doesn't use the git-submodule command at all, nor does it
construct series trees using git-add or any other git command-line tool;
it constructs gitlinks directly. Most of the time, it doesn't even make
sense to `git checkout` a series branch. Modifying commands like git-add
and similar to automatically manage .gitmodules won't cause any issue at
all, as long as git itself doesn't start rejecting or complaining about
repositories that have gitlinks without a .gitmodules file.

- Josh Triplett

Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup

[Brandon Williams]

On 10/11, Josh Triplett wrote:
> On Tue, Oct 10, 2017 at 11:39:21AM -0700, Stefan Beller wrote:
> > On Tue, Oct 10, 2017 at 6:03 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > > but in the long run my goal
> > > for submodules is and always was: Make them behave as close to files as
> > > possible. And why should a 'git add submodule' not magically do
> > > everything it can to make submodules just work? I can look into a patch
> > > for that if people agree here...
> > 
> > I'd love to see this implemented. I cc'd Josh (the author of git-series), who
> > may disagree with this, or has some good input how to go forward without
> > breaking git-series.
> 
> git-series doesn't use the git-submodule command at all, nor does it
> construct series trees using git-add or any other git command-line tool;
> it constructs gitlinks directly. Most of the time, it doesn't even make
> sense to `git checkout` a series branch. Modifying commands like git-add
> and similar to automatically manage .gitmodules won't cause any issue at
> all, as long as git itself doesn't start rejecting or complaining about
> repositories that have gitlinks without a .gitmodules file.

That's good to know!  And from what I remember, with the commands we've
begun teaching to understand submodules we have been requiring a
.gitmodules entry for a submodule in order to do the recursion, and a
gitlink without a .gitmodules entry would simply be ignored or skipped.

-- 
Brandon Williams

[RFC PATCH v3 3/4] implement fetching of moved submodules

[Heiko Voigt]

We store the changed submodules paths to calculate which submodule needs
fetching. This does not work for moved submodules since their paths do
not stay the same in case of a moved submodules. In case of new
submodules we do not have a path in the current checkout, since they
just appeared in this fetch.

It is more general to collect the submodule names for changes instead of
their paths to include the above cases.

With the change described above we implement 'on-demand' fetching of
changes in moved submodules.

Note: This does only work when repositories have a .gitmodules file. In
other words: We now require a name for a submodule repository.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---

No changes from the previous version here.

 submodule.c                 | 91 +++++++++++++++++++++++++--------------------
 t/t5526-fetch-submodules.sh | 35 +++++++++++++++++
 2 files changed, 85 insertions(+), 41 deletions(-)

diff --git a/submodule.c b/submodule.c
index 63e7094..0c586a0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -21,7 +21,7 @@
 #include "parse-options.h"
 
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;
+static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
 static int initialized_fetch_ref_tips;
 static struct oid_array ref_tips_before_fetch;
 static struct oid_array ref_tips_after_fetch;
@@ -674,11 +674,11 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)
 }
 
 static struct oid_array *submodule_commits(struct string_list *submodules,
-					   const char *path)
+					   const char *name)
 {
 	struct string_list_item *item;
 
-	item = string_list_insert(submodules, path);
+	item = string_list_insert(submodules, name);
 	if (item->util)
 		return (struct oid_array *) item->util;
 
@@ -687,39 +687,34 @@ static struct oid_array *submodule_commits(struct string_list *submodules,
 	return (struct oid_array *) item->util;
 }
 
+struct collect_changed_submodules_cb_data {
+	struct string_list *changed;
+	const struct object_id *commit_oid;
+};
+
 static void collect_changed_submodules_cb(struct diff_queue_struct *q,
 					  struct diff_options *options,
 					  void *data)
 {
+	struct collect_changed_submodules_cb_data *me = data;
+	struct string_list *changed = me->changed;
+	const struct object_id *commit_oid = me->commit_oid;
 	int i;
-	struct string_list *changed = data;
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		struct oid_array *commits;
+		const struct submodule *submodule;
+
 		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.
-			 */
-			commits = submodule_commits(changed, p->two->path);
-			oid_array_append(commits, &p->two->oid);
-		} else {
-			/* Submodule is new or was moved here */
-			/*
-			 * NEEDSWORK: When the .git directories of submodules
-			 * live inside the superprojects .git directory some
-			 * day we should fetch new submodules directly into
-			 * that location too when config or options request
-			 * that so they can be checked out from there.
-			 */
+		submodule = submodule_from_path(commit_oid, p->two->path);
+		if (!submodule)
 			continue;
-		}
+
+		commits = submodule_commits(changed, submodule->name);
+		oid_array_append(commits, &p->two->oid);
 	}
 }
 
@@ -742,11 +737,14 @@ static void collect_changed_submodules(struct string_list *changed,
 
 	while ((commit = get_revision(&rev))) {
 		struct rev_info diff_rev;
+		struct collect_changed_submodules_cb_data data;
+		data.changed = changed;
+		data.commit_oid = &commit->object.oid;
 
 		init_revisions(&diff_rev, NULL);
 		diff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 		diff_rev.diffopt.format_callback = collect_changed_submodules_cb;
-		diff_rev.diffopt.format_callback_data = changed;
+		diff_rev.diffopt.format_callback_data = &data;
 		diff_tree_combined_merge(commit, 1, &diff_rev);
 	}
 
@@ -894,7 +892,7 @@ int find_unpushed_submodules(struct oid_array *commits,
 		const char *remotes_name, struct string_list *needs_pushing)
 {
 	struct string_list submodules = STRING_LIST_INIT_DUP;
-	struct string_list_item *submodule;
+	struct string_list_item *name;
 	struct argv_array argv = ARGV_ARRAY_INIT;
 
 	/* argv.argv[0] will be ignored by setup_revisions */
@@ -905,12 +903,16 @@ int find_unpushed_submodules(struct oid_array *commits,
 
 	collect_changed_submodules(&submodules, &argv);
 
-	for_each_string_list_item(submodule, &submodules) {
-		struct oid_array *commits = submodule->util;
-		const char *path = submodule->string;
+	for_each_string_list_item(name, &submodules) {
+		struct oid_array *commits = name->util;
+		const struct submodule *submodule;
+
+		submodule = submodule_from_name(&null_oid, name->string);
+		if (!submodule)
+			continue;
 
-		if (submodule_needs_pushing(path, commits))
-			string_list_insert(needs_pushing, path);
+		if (submodule_needs_pushing(submodule->path, commits))
+			string_list_insert(needs_pushing, submodule->path);
 	}
 
 	free_submodules_oids(&submodules);
@@ -1065,7 +1067,7 @@ static void calculate_changed_submodule_paths(void)
 {
 	struct argv_array argv = ARGV_ARRAY_INIT;
 	struct string_list changed_submodules = STRING_LIST_INIT_DUP;
-	const struct string_list_item *item;
+	const struct string_list_item *name;
 
 	/* No need to check if there are no submodules configured */
 	if (!submodule_from_path(NULL, NULL))
@@ -1080,16 +1082,20 @@ static void calculate_changed_submodule_paths(void)
 
 	/*
 	 * Collect all submodules (whether checked out or not) for which new
-	 * commits have been recorded upstream in "changed_submodule_paths".
+	 * commits have been recorded upstream in "changed_submodule_names".
 	 */
 	collect_changed_submodules(&changed_submodules, &argv);
 
-	for_each_string_list_item(item, &changed_submodules) {
-		struct oid_array *commits = item->util;
-		const char *path = item->string;
+	for_each_string_list_item(name, &changed_submodules) {
+		struct oid_array *commits = name->util;
+		const struct submodule *submodule;
+
+		submodule = submodule_from_name(&null_oid, name->string);
+		if (!submodule)
+			continue;
 
-		if (!submodule_has_commits(path, commits))
-			string_list_append(&changed_submodule_paths, path);
+		if (!submodule_has_commits(submodule->path, commits))
+			string_list_append(&changed_submodule_names, name->string);
 	}
 
 	free_submodules_oids(&changed_submodules);
@@ -1175,7 +1181,8 @@ static int get_next_submodule(struct child_process *cp,
 				if (fetch_recurse == RECURSE_SUBMODULES_OFF)
 					continue;
 				if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {
-					if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
+					if (!unsorted_string_list_lookup(&changed_submodule_names,
+									 submodule->name))
 						continue;
 					default_argv = "on-demand";
 				}
@@ -1183,13 +1190,15 @@ static int get_next_submodule(struct child_process *cp,
 				if (spf->default_option == RECURSE_SUBMODULES_OFF)
 					continue;
 				if (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {
-					if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
+					if (!unsorted_string_list_lookup(&changed_submodule_names,
+									  submodule->name))
 						continue;
 					default_argv = "on-demand";
 				}
 			}
 		} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {
-			if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
+			if (!unsorted_string_list_lookup(&changed_submodule_names,
+							 submodule->name))
 				continue;
 			default_argv = "on-demand";
 		}
@@ -1282,7 +1291,7 @@ int fetch_populated_submodules(const struct argv_array *options,
 
 	argv_array_clear(&spf.args);
 out:
-	string_list_clear(&changed_submodule_paths, 1);
+	string_list_clear(&changed_submodule_names, 1);
 	return spf.result;
 }
 
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 43a22f6..a552ad4 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -570,4 +570,39 @@ test_expect_success 'fetching submodule into a broken repository' '
 	test_must_fail git -C dst fetch --recurse-submodules
 '
 
+test_expect_success "fetch new commits when submodule got renamed" '
+	git clone . downstream_rename &&
+	(
+		cd downstream_rename &&
+		git submodule update --init &&
+# NEEDSWORK: we omitted --recursive for the submodule update here since
+# that does not work. See test 7001 for mv "moving nested submodules"
+# for details. Once that is fixed we should add the --recursive option
+# here.
+		git checkout -b rename &&
+		git mv submodule submodule_renamed &&
+		(
+			cd submodule_renamed &&
+			git checkout -b rename_sub &&
+			echo a >a &&
+			git add a &&
+			git commit -ma &&
+			git push origin rename_sub &&
+			git rev-parse HEAD >../../expect
+		) &&
+		git add submodule_renamed &&
+		git commit -m "update renamed submodule" &&
+		git push origin rename
+	) &&
+	(
+		cd downstream &&
+		git fetch --recurse-submodules=on-demand &&
+		(
+			cd submodule &&
+			git rev-parse origin/rename_sub >../../actual
+		)
+	) &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.10.0.129.g35f6318

[RFC PATCH v3 4/4] submodule: simplify decision tree whether to or not to fetch

[Heiko Voigt]

To make extending this logic later easier.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---

This should also be the same as in the previous version.

 submodule.c | 74 ++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/submodule.c b/submodule.c
index 0c586a0..c7b32c6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1142,6 +1142,31 @@ struct submodule_parallel_fetch {
 };
 #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
 
+static int get_fetch_recurse_config(const struct submodule *submodule,
+				    struct submodule_parallel_fetch *spf)
+{
+	if (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)
+		return spf->command_line_option;
+
+	if (submodule) {
+		char *key;
+		const char *value;
+
+		int fetch_recurse = submodule->fetch_recurse;
+		key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
+		if (!repo_config_get_string_const(the_repository, key, &value)) {
+			fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
+		}
+		free(key);
+
+		if (fetch_recurse != RECURSE_SUBMODULES_NONE)
+			/* local config overrules everything except commandline */
+			return fetch_recurse;
+	}
+
+	return spf->default_option;
+}
+
 static int get_next_submodule(struct child_process *cp,
 			      struct strbuf *err, void *data, void **task_cb)
 {
@@ -1161,46 +1186,21 @@ static int get_next_submodule(struct child_process *cp,
 
 		submodule = submodule_from_path(&null_oid, ce->name);
 
-		default_argv = "yes";
-		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
-			int fetch_recurse = RECURSE_SUBMODULES_NONE;
-
-			if (submodule) {
-				char *key;
-				const char *value;
-
-				fetch_recurse = submodule->fetch_recurse;
-				key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
-				if (!repo_config_get_string_const(the_repository, key, &value)) {
-					fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
-				}
-				free(key);
-			}
-
-			if (fetch_recurse != RECURSE_SUBMODULES_NONE) {
-				if (fetch_recurse == RECURSE_SUBMODULES_OFF)
-					continue;
-				if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {
-					if (!unsorted_string_list_lookup(&changed_submodule_names,
-									 submodule->name))
-						continue;
-					default_argv = "on-demand";
-				}
-			} else {
-				if (spf->default_option == RECURSE_SUBMODULES_OFF)
-					continue;
-				if (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {
-					if (!unsorted_string_list_lookup(&changed_submodule_names,
-									  submodule->name))
-						continue;
-					default_argv = "on-demand";
-				}
-			}
-		} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {
-			if (!unsorted_string_list_lookup(&changed_submodule_names,
+		switch (get_fetch_recurse_config(submodule, spf))
+		{
+		default:
+		case RECURSE_SUBMODULES_DEFAULT:
+		case RECURSE_SUBMODULES_ON_DEMAND:
+			if (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,
 							 submodule->name))
 				continue;
 			default_argv = "on-demand";
+			break;
+		case RECURSE_SUBMODULES_ON:
+			default_argv = "yes";
+			break;
+		case RECURSE_SUBMODULES_OFF:
+			continue;
 		}
 
 		strbuf_addf(&submodule_path, "%s/%s", spf->work_tree, ce->name);
-- 
2.10.0.129.g35f6318

Re: [RFC PATCH v3 0/4] implement fetching of moved submodules

[Stefan Beller]

On Fri, Oct 6, 2017 at 3:25 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> The last iteration can be found here:
>
> https://public-inbox.org/git/20170817105349.GC52233@book.hvoigt.net/
>
> This is mainly a status update and to let people know that I am still
> working on this.

Cool. :)

> I struggled quite a bit with reviving my original test for the path
> based recursive fetch (first patch). The behavior seems to haved changed
> and simply setting the submodule configuration in .git/config without
> one in .gitmodules does not work anymore. I did not have time to
> investigate whether this was a deliberate change or a maybe a bug?

I think it is deliberate. (We wrote this man page "gitsubmodules"[1] and there
was so much discussion on "What is a submodule?". Key take away is this:
* a gitlink alone is not a submodule.
* a submodule consists of at least
  -> the gitlink in the superproject
  -> a mapping of path -> name via
      $(git config -f .gitmodules submodule.<name>.path)
  -> Depending on config in .git/config or the existence of its git directory,
      it may be [in]active or [de]initialized.

[1] not to be confused with "gitmodules" or "git-submodule"

Sometimes we accept a plain git-link without the config in .gitmodules,
(a) due to historic reasons or (b) because it seems sane even for
a repo "that just happens to exist at the gitlinks location"
(example git-diff)

> So the solution for now is that I write my fake configuration (to avoid
> skipping submodule handling altogether) into a .gitmodules file.

I'll try to spot what is fake about the config.

> The second patch (cleanup of a submodule push testcase) was written
> because that currently is the only test failing. It is not meant for
> inclusion but rather as a demonstration of what might be happening when
> we cleanup testcases: Because of the behavioral change above, on first
> sight, it seemed like there was a shortcut in fetch and so on-demand
> fetch without submodule configuration would not be supported anymore.
>
> IIRC there were a lot more tests failing before when I implemented my
> patch without the fallback on paths. So my guess is that some tests have
> been cleaned up to use proper (.gitmodules) submodule setup.

I don't remember any large recent activity for submodule things lately.

> So the thing here is: If we want to make sure that we stay backwards
> compatible by supporting the setup with gitlinks without configuration.
> Then we also should keep tests around that have the plain manual setup
> without .gitmodules files. Just something, I think, we should keep in
> mind.

But do we want this?

Without the name<->path mapping, we can only have the "old style"
submodules, that have their git repo inside its tree instead of inside
the superprojects git dir.
So renaming/moving "old style with no name<->path mapping" will not
work. (That may be an acceptable trade off. But then again, just providing
the mapping, such that the superproject can absorb the git directory
of the submodule for this use case, doesn't seem like a big deal to me.
Something you want to have anyway, for ease of use of the superproject
w.r.t. cloning for example)

So while I do not try to deliberately break these old behaviors, I'd rather
want us to go forward with a saner model than "if we happen to have
enough data around, the operation succeeds", i.e. ignore anything
that is not following the rather strict definition of a submodule.

FYI: Once upon a time I found "fake submodules"
http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb
Last time this was discussed on list, this was considered a bug not
worth fixing instead of a feature IIRC. (Personally I think this is
a rather cool hack, which we may want to abuse ourselves for
things like "convert a subtree into a submodule and back again",
but we could also go without this hack)

> Apart from the tests nothing has been added in this iteration. Since I
> finally have a working test now I will continue with reviving the
> fallback to paths.

I'll have a look.

Cheers,
Stefan

Re: [RFC PATCH v3 0/4] implement fetching of moved submodules

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> So the thing here is: If we want to make sure that we stay backwards
> compatible by supporting the setup with gitlinks without configuration.
> Then we also should keep tests around that have the plain manual setup
> without .gitmodules files. Just something, I think, we should keep in
> mind.
>
> Apart from the tests nothing has been added in this iteration. Since I
> finally have a working test now I will continue with reviving the
> fallback to paths.

Thanks for an update.