Submodule regression in 2.14?

Submodule regression in 2.14?

[Lars Schneider]

Hi,

I think we discovered a regression in Git 2.14.1 today.
It looks like as if "git submodule update --init --recursive" removes
the "skip submodules" config.

Consider the following steps:

    git clone https://server/repo.git
    cd repo
    git config --local submodule.some/other/repo.update none
    git submodule update --init --recursive
    git pull --recurse-submodules

With Git 2.14 the last "git pull" will clone the "some/other/repo"
submodule. This did not happen with Git 2.13.

Bug or feature? I don't have anymore time for Git today. I am happy to
provide a proper test case tomorrow, though.

Cheers,
Lars

Re: Submodule regression in 2.14?

[Stefan Beller]

On Wed, Aug 16, 2017 at 11:35 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
> Hi,
>
> I think we discovered a regression in Git 2.14.1 today.
> It looks like as if "git submodule update --init --recursive" removes
> the "skip submodules" config.
>
> Consider the following steps:
>
>     git clone https://server/repo.git
>     cd repo
>     git config --local submodule.some/other/repo.update none
>     git submodule update --init --recursive
>     git pull --recurse-submodules
>
> With Git 2.14 the last "git pull" will clone the "some/other/repo"
> submodule. This did not happen with Git 2.13.
>
> Bug or feature? I don't have anymore time for Git today. I am happy to
> provide a proper test case tomorrow, though.

$ git log --oneline v2.13.0..v2.14.1 -- git-submodule.sh
532139940c add: warn when adding an embedded repository
(I am confident this is not the suspect, let's keep searching.
Not a lot happened in submodule land apparently)

Looking through all commits v2.13..v2.14 doesn't have me
suspect any of them.

Any chance the "did not happen with 2.13" was not
freshly cloned but tested on an existing repo? If so a hot
candidate for suspicion is a93dcb0a56 (Merge branch
'bw/submodule-is-active', 2017-03-30), IMHO, just
gut feeling, though.

Oh, wait.
$ git log --oneline v2.13.0..v2.14.1 -- builtin/pull.c
c9c63ee558 Merge branch 'sb/pull-rebase-submodule'
a6d7eb2c7a pull: optionally rebase submodules (remote submodule changes only)
could also be a culprit. Do you have pull.rebase set?

>
> Cheers,
> Lars

Re: Submodule regression in 2.14?

[Stefan Beller]

On Wed, Aug 16, 2017 at 11:51 AM, Stefan Beller <sbeller@google.com> wrote:

> Any chance the "did not happen with 2.13" was not
> freshly cloned but tested on an existing repo? If so a hot
> candidate for suspicion is a93dcb0a56 (Merge branch
> 'bw/submodule-is-active', 2017-03-30), IMHO, just
> gut feeling, though.

which makes it a feature, I should add.

Re: Submodule regression in 2.14?

[Lars Schneider]

> On 16 Aug 2017, at 20:51, Stefan Beller <sbeller@google.com> wrote:
> 
> On Wed, Aug 16, 2017 at 11:35 AM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
>> Hi,
>> 
>> I think we discovered a regression in Git 2.14.1 today.
>> It looks like as if "git submodule update --init --recursive" removes
>> the "skip submodules" config.
>> 
>> Consider the following steps:
>> 
>>    git clone https://server/repo.git
>>    cd repo
>>    git config --local submodule.some/other/repo.update none
>>    git submodule update --init --recursive
>>    git pull --recurse-submodules
>> 
>> With Git 2.14 the last "git pull" will clone the "some/other/repo"
>> submodule. This did not happen with Git 2.13.
>> 
>> Bug or feature? I don't have anymore time for Git today. I am happy to
>> provide a proper test case tomorrow, though.
> 
> $ git log --oneline v2.13.0..v2.14.1 -- git-submodule.sh
> 532139940c add: warn when adding an embedded repository
> (I am confident this is not the suspect, let's keep searching.
> Not a lot happened in submodule land apparently)
> 
> Looking through all commits v2.13..v2.14 doesn't have me
> suspect any of them.
> 
> Any chance the "did not happen with 2.13" was not
> freshly cloned but tested on an existing repo? If so a hot
> candidate for suspicion is a93dcb0a56 (Merge branch
> 'bw/submodule-is-active', 2017-03-30), IMHO, just
> gut feeling, though.
> 
> Oh, wait.
> $ git log --oneline v2.13.0..v2.14.1 -- builtin/pull.c
> c9c63ee558 Merge branch 'sb/pull-rebase-submodule'
> a6d7eb2c7a pull: optionally rebase submodules (remote submodule changes only)
> could also be a culprit. Do you have pull.rebase set?

I bisected the problem today and "a6d7eb2c7a pull: optionally rebase submodules 
(remote submodule changes only)" is indeed the culprit.

The commit seems to break the following test case.

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index dcac364c5f..24f9729015 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1289,4 +1289,19 @@ test_expect_success 'init properly sets the config' '
 	test_must_fail git -C multisuper_clone config --get submodule.sub1.active
 '
 
+test_expect_success 'submodule update and git pull with disabled submodule' '
+	test_when_finished "rm -rf multisuper_clone" &&
+	pwd=$(pwd) &&
+	git clone file://"$pwd"/multisuper multisuper_clone &&
+	(
+		cd multisuper_clone &&
+		git config --local submodule.sub0.update none &&
+		git submodule update --init --recursive &&
+		git pull --recurse-submodules &&
+		git submodule status | cut -c 1,43- >actual
+	) &&
+	ls &&
+	test_cmp expect multisuper_clone/actual
+'
+
 test_done


I am not familiar with the code. Does anyone see the problem
right away?

Thanks,
Lars

Re: Submodule regression in 2.14?

[Stefan Beller]

On Thu, Aug 17, 2017 at 2:21 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>> Oh, wait.
>> $ git log --oneline v2.13.0..v2.14.1 -- builtin/pull.c
>> c9c63ee558 Merge branch 'sb/pull-rebase-submodule'
>> a6d7eb2c7a pull: optionally rebase submodules (remote submodule changes only)
>> could also be a culprit. Do you have pull.rebase set?
>
> I bisected the problem today and "a6d7eb2c7a pull: optionally rebase submodules
> (remote submodule changes only)" is indeed the culprit.
>
> The commit seems to break the following test case.
>
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index dcac364c5f..24f9729015 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -1289,4 +1289,19 @@ test_expect_success 'init properly sets the config' '
>         test_must_fail git -C multisuper_clone config --get submodule.sub1.active
>  '
>
> +test_expect_success 'submodule update and git pull with disabled submodule' '
> +       test_when_finished "rm -rf multisuper_clone" &&
> +       pwd=$(pwd) &&
> +       git clone file://"$pwd"/multisuper multisuper_clone &&
> +       (
> +               cd multisuper_clone &&
> +               git config --local submodule.sub0.update none &&
> +               git submodule update --init --recursive &&
> +               git pull --recurse-submodules &&
> +               git submodule status | cut -c 1,43- >actual
> +       ) &&
> +       ls &&
> +       test_cmp expect multisuper_clone/actual
> +'

Thanks for providing this test.

cd trash directory.t7400-submodule-basic/multisuper_clone
cat .git/config
[submodule "sub0"]
  update = none
  active = true
  url = file:///.../t/trash directory.t7400-submodule-basic/sub1


submodule.<name>.update
    The default update procedure for a submodule.
    This variable is populated by git submodule init
    from the gitmodules(5) file. See description of
    update command in git-submodule(1).

The first sentence of .update is misleading IMHO as the
these settings should strictly apply to the "submodule update"
command. So "pull --recurse-submodules" ought to ignore it,
instead the pull can do whatever it wants, namely treat the
submodule roughly like a tree and either merge/rebase
inside the submodule as well. The user *asked* for recursive
pull after all.

Are you saying this might be a design mistake and
the .update ought to be respected by all the other
commands? For example
    git reset --recurse-submodules
should ignore the .update= none?

When designing these new recursive submodule functionality
outside the "submodule" command, I'd want submodules
to behave as much as possible like trees.

ideas?

Thanks,
Stefan

> +
>  test_done
>
>
> I am not familiar with the code. Does anyone see the problem
> right away?
>
> Thanks,
> Lars
>
>

Re: Submodule regression in 2.14?

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Are you saying this might be a design mistake and
> the .update ought to be respected by all the other
> commands? For example
>     git reset --recurse-submodules
> should ignore the .update= none?

I have been under the impression that that has been the traditional
desire of what .update ought to mean.  I personally do not have a
strong opinion---at least not yet.

> When designing these new recursive submodule functionality
> outside the "submodule" command, I'd want submodules
> to behave as much as possible like trees.

I think that is sensible as long as the user does not explicitly say
"this and that submodule behave differently" by giving configuration
variables.  Perhaps .update is one of those that should countermand
the default behaviour of "--recurse-submodules"?

Re: Submodule regression in 2.14?

[Stefan Beller]

On Thu, Aug 17, 2017 at 7:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Are you saying this might be a design mistake and
>> the .update ought to be respected by all the other
>> commands? For example
>>     git reset --recurse-submodules
>> should ignore the .update= none?
>
> I have been under the impression that that has been the traditional
> desire of what .update ought to mean.  I personally do not have a
> strong opinion---at least not yet.

In this context note v2.14.0-rc1-34-g7463e2ec3
(bw/submodule-config-cleanup~7, "unpack-trees:
don't respect submodule.update") that is going opposite of
your impression.

>> When designing these new recursive submodule functionality
>> outside the "submodule" command, I'd want submodules
>> to behave as much as possible like trees.
>
> I think that is sensible as long as the user does not explicitly say
> "this and that submodule behave differently" by giving configuration
> variables.  Perhaps .update is one of those that should countermand
> the default behaviour of "--recurse-submodules"?

Maybe, I'll think about it. However there is no such
equivalent for trees (and AFAICT never came up) to
treat a specific directory other than the rest in worktree
operations.

The problem with the issue in question is however:
git-pull is a combination of two other high level commands
(fetch/merge), the fetch component already had
a recursive behavior, and that commit in question
added a bit for the merge component, so the UX
is hard to get right for both of them:

    git pull --recurse=fetch-only
    git pull --recurse=merge-respects-update-strategy

is what I'd want to avoid.

So maybe we can just respect the update strategy
before starting the local part.

Stefan

Re: Submodule regression in 2.14?

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Thu, Aug 17, 2017 at 7:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> Are you saying this might be a design mistake and
>>> the .update ought to be respected by all the other
>>> commands? For example
>>>     git reset --recurse-submodules
>>> should ignore the .update= none?
>>
>> I have been under the impression that that has been the traditional
>> desire of what .update ought to mean.  I personally do not have a
>> strong opinion---at least not yet.
>
> In this context note v2.14.0-rc1-34-g7463e2ec3
> (bw/submodule-config-cleanup~7, "unpack-trees:
> don't respect submodule.update") that is going opposite of
> your impression.

Exactly.  We are in agreement that recent developments seem to go
against the traditional desire and it is understandable Lars sees
this as a regression.  I still do not have a strong opinion either
way, if this is a regression or a progress.

> Maybe, I'll think about it. However there is no such
> equivalent for trees (and AFAICT never came up) to
> treat a specific directory other than the rest in worktree
> operations.

I am not sure if I follow.  Submodules are not trees and one of the
reasons people may want to separate things into different modules is
so that they can treat them differently.  If submodules allow you
a richer set of operations than a tree that is part of a monolithic
project, is that necessarily a bad thing?

Re: Submodule regression in 2.14?

[Stefan Beller]

On Fri, Aug 18, 2017 at 9:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I am not sure if I follow.  Submodules are not trees and one of the
> reasons people may want to separate things into different modules is
> so that they can treat them differently.  If submodules allow you
> a richer set of operations than a tree that is part of a monolithic
> project, is that necessarily a bad thing?

It is not a bad thing on its own, but we have to consider which
additional actions are useful.

Jonathan brought up the following very long term vision:
Eventually the everyday git commands do not treat submodules
any special than trees, even the submodules git directory
may be non existent (everything is absorbed into the superproject);
so it really feels like a monorepo.
When you want to work on a submodule individually, you have to
make a new working tree.

Re: Submodule regression in 2.14?

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Jonathan brought up the following very long term vision:
> Eventually the everyday git commands do not treat submodules
> any special than trees, even the submodules git directory
> may be non existent (everything is absorbed into the superproject);
> so it really feels like a monorepo.

That may be one valid option to have but I do not see a reason why
it needs to be the only valid option.  So I do not see why you are
bringing it up in this thread.

But that is a good starting point to discuss one possible future.
Let's think aloud how that world would look like.

 * When you "git clone" a superproject (perhaps implicitly with the
   "--recurse-submodules" option), the top-level project and all of
   its submodules will be checked out on the same branch (presumably
   the 'master' branch, which is the default).

 * Your attempt to "git commit", "git branch", "git checkout -b",
   etc. inside a submodule will either fail, or will implicitly
   chdir up to the top-level superproject and turn into the
   corresponding command with "--recurse-submodules".

 * "git commit --recurse-submodules -a" from the top-level will grab
   all the local changes, depth-first and recursively, in
   submodules, makes a commit, binds the new commit to the index of
   the superproject that immediately contains the submodule and
   makes a commit in it, until it percolates all the way up to the
   superproject.  When working in this mode, branches in submodules
   do not really matter; the gitlink in the superproject is the only
   thing that matters.

 * It naturally follows that between two adjacent commits C and C~1
   in the superproject's history, the commit in a submodule bound to
   C and the commit in a submodule bound to C~1 are either the same
   (i.e. superproject made a commit but there was no change in the
   submodule), or they are in direct parent-child relationship
   (i.e. the local changes made to the submodule was recorded as a
   single commit when the superproject made the commit).

 * "git push --recurse-submodules" from the top-level will push the
   history of the superproject out, together with the history of the
   submodules.

I think it is doable, but a mechanism to enumerate all the commits
bound from submodules to a range of superproject's commits needs to
be invented to drive the pack-objects for efficient object transfer.
Having it would also help in fsck and gc---as branches are immaterial
in the submodule repositories, commits in superprojects that are
reachable from refs will have to serve as the connectivity anchors
for commit DAG in the submodule histories.

As long as we are talking about idealized future world (well, at
least an idea of somebody's "ideal", not necessarily shared by
everybody), I wonder if there is even any need to have commits in
submodules in such a world.  To realize such a "monorepo" world, you
might be better off allowing a gitlink in the superproject to
directly point at a tree object in a submodule repository (making
them physically a single repository is an optional implementation
detail I choose to ignore in this discussion).

Re: Submodule regression in 2.14?

[Heiko Voigt]

On Fri, Aug 18, 2017 at 11:51:13PM -0700, Junio C Hamano wrote:
> As long as we are talking about idealized future world (well, at
> least an idea of somebody's "ideal", not necessarily shared by
> everybody), I wonder if there is even any need to have commits in
> submodules in such a world.  To realize such a "monorepo" world, you
> might be better off allowing a gitlink in the superproject to
> directly point at a tree object in a submodule repository (making
> them physically a single repository is an optional implementation
> detail I choose to ignore in this discussion).

IMO this is one step to far. One main use of submodules are shared
repositories that are used by many superprojects. The reason you want to
have commits in the submodule are so that you can push them
independently and all other users can pick up the changes. You could get
by by Using the superproject commits for the submodule once you push or
something but those do not necessarily make sense in the context of the
submodule.

So I think it is important that there are commits in the submodule so
its history makes sense independently for others.

Or how would you push out the history in the submodule in your idea?
Maybe I am missing something? What would be your use case with gitlinks
pointing to trees?

Cheers Heiko

Re: Submodule regression in 2.14?

[Stefan Beller]

On Mon, Aug 21, 2017 at 9:05 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> On Fri, Aug 18, 2017 at 11:51:13PM -0700, Junio C Hamano wrote:
>> As long as we are talking about idealized future world (well, at
>> least an idea of somebody's "ideal", not necessarily shared by
>> everybody), I wonder if there is even any need to have commits in
>> submodules in such a world.  To realize such a "monorepo" world, you
>> might be better off allowing a gitlink in the superproject to
>> directly point at a tree object in a submodule repository (making
>> them physically a single repository is an optional implementation
>> detail I choose to ignore in this discussion).
>
> IMO this is one step to far. One main use of submodules are shared
> repositories that are used by many superprojects. The reason you want to
> have commits in the submodule are so that you can push them
> independently and all other users can pick up the changes. You could get
> by by Using the superproject commits for the submodule once you push or
> something but those do not necessarily make sense in the context of the
> submodule.
>
> So I think it is important that there are commits in the submodule so
> its history makes sense independently for others.
>
> Or how would you push out the history in the submodule in your idea?
> Maybe I am missing something? What would be your use case with gitlinks
> pointing to trees?

Well there are still commits, but in the superproject the UX feels more
as if the submodules were special trees. So if you want to interact with
the submodule specifically, you could do things like

    git add /path/inside/sub
    # works seamlessly from the superproject tree

    git commit --submodule-commit-only
    # When the flag is not give, you may get an editor
    # asking for two commit messages, (sub+super)

    git fetch --submodule
    # When the flag is not given, we'd fetch superproject and
    # on-demand

    # You feel the superproject is in the way?
    git worktree add --for-submodule <path/to/sub> ...
    # The new submodule worktree puts the
    # submodule only UX first. so it feels like its own
    # repository, no need for specific flags.


>
> Cheers Heiko

Re: Submodule regression in 2.14?

[Heiko Voigt]

On Mon, Aug 21, 2017 at 09:42:54AM -0700, Stefan Beller wrote:
> On Mon, Aug 21, 2017 at 9:05 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > On Fri, Aug 18, 2017 at 11:51:13PM -0700, Junio C Hamano wrote:
> >> As long as we are talking about idealized future world (well, at
> >> least an idea of somebody's "ideal", not necessarily shared by
> >> everybody), I wonder if there is even any need to have commits in
> >> submodules in such a world.  To realize such a "monorepo" world, you
> >> might be better off allowing a gitlink in the superproject to
> >> directly point at a tree object in a submodule repository (making
> >> them physically a single repository is an optional implementation
> >> detail I choose to ignore in this discussion).
> >
> > IMO this is one step to far. One main use of submodules are shared
> > repositories that are used by many superprojects. The reason you want to
> > have commits in the submodule are so that you can push them
> > independently and all other users can pick up the changes. You could get
> > by by Using the superproject commits for the submodule once you push or
> > something but those do not necessarily make sense in the context of the
> > submodule.
> >
> > So I think it is important that there are commits in the submodule so
> > its history makes sense independently for others.
> >
> > Or how would you push out the history in the submodule in your idea?
> > Maybe I am missing something? What would be your use case with gitlinks
> > pointing to trees?
> 
> Well there are still commits, but in the superproject the UX feels more
> as if the submodules were special trees.

Ah ok then I misunderstood. So they only feel like trees.

> So if you want to interact with
> the submodule specifically, you could do things like
> 
>     git add /path/inside/sub
>     # works seamlessly from the superproject tree

Would that mean that we need to loosen/keep the requirement loose for a
name from .gitmodules? I am asking because of my series for on-demand
fetch of renamed submodules. For the full functionality I would require
a name.

Would that be in a scenario where the user would then e.g. push the
submodule into the superproject?

Ah wait I misunderstood again. You mean a file in an existing
submodule right? Not adding submodule from a repository a user moved
there?

>     git commit --submodule-commit-only
>     # When the flag is not give, you may get an editor
>     # asking for two commit messages, (sub+super)

Or maybe something like

    git commit --submodule path/to/submodule

so the user can specify which submodule she wants. I first wrote it
without the switch but but that collides with listing files which should
be added. IMO this shorter option is also more intuitive to understand
what it does (for this case).

>     git fetch --submodule
>     # When the flag is not given, we'd fetch superproject and
>     # on-demand

Yes like above we should add the path to the submodule right?

>     # You feel the superproject is in the way?
>     git worktree add --for-submodule <path/to/sub> ...
>     # The new submodule worktree puts the
>     # submodule only UX first. so it feels like its own
>     # repository, no need for specific flags.

I am not sure I understand this one. What would that do? Put a worktree
for submodule path/to/sub to ...?

Overall I like the direction of these ideas.

Cheers Heiko

Re: Submodule regression in 2.14?

[Stefan Beller]

On Tue, Aug 22, 2017 at 8:33 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> On Mon, Aug 21, 2017 at 09:42:54AM -0700, Stefan Beller wrote:
>> On Mon, Aug 21, 2017 at 9:05 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
>> > On Fri, Aug 18, 2017 at 11:51:13PM -0700, Junio C Hamano wrote:
>> >> As long as we are talking about idealized future world (well, at
>> >> least an idea of somebody's "ideal", not necessarily shared by
>> >> everybody), I wonder if there is even any need to have commits in
>> >> submodules in such a world.  To realize such a "monorepo" world, you
>> >> might be better off allowing a gitlink in the superproject to
>> >> directly point at a tree object in a submodule repository (making
>> >> them physically a single repository is an optional implementation
>> >> detail I choose to ignore in this discussion).
>> >
>> > IMO this is one step to far. One main use of submodules are shared
>> > repositories that are used by many superprojects. The reason you want to
>> > have commits in the submodule are so that you can push them
>> > independently and all other users can pick up the changes. You could get
>> > by by Using the superproject commits for the submodule once you push or
>> > something but those do not necessarily make sense in the context of the
>> > submodule.
>> >
>> > So I think it is important that there are commits in the submodule so
>> > its history makes sense independently for others.
>> >
>> > Or how would you push out the history in the submodule in your idea?
>> > Maybe I am missing something? What would be your use case with gitlinks
>> > pointing to trees?
>>
>> Well there are still commits, but in the superproject the UX feels more
>> as if the submodules were special trees.
>
> Ah ok then I misunderstood. So they only feel like trees.
>
>> So if you want to interact with
>> the submodule specifically, you could do things like
>>
>>     git add /path/inside/sub
>>     # works seamlessly from the superproject tree
>
> Would that mean that we need to loosen/keep the requirement loose for a
> name from .gitmodules? I am asking because of my series for on-demand
> fetch of renamed submodules. For the full functionality I would require
> a name.
>
> Would that be in a scenario where the user would then e.g. push the
> submodule into the superproject?
>
> Ah wait I misunderstood again. You mean a file in an existing
> submodule right? Not adding submodule from a repository a user moved
> there?

Assuming the submodule is at  /path in this example, the effect of
that command could be achieved today via

    git -C /path add inside/sub

(i.e. for git-add we "just" detect that there is a submodule boundary
and run the git-add inside the submodule)

>
>>     git commit --submodule-commit-only
>>     # When the flag is not give, you may get an editor
>>     # asking for two commit messages, (sub+super)
>
> Or maybe something like
>
>     git commit --submodule path/to/submodule

Yes. In todays UX, you do

    git -C path/to/submodule commit

for the command that you proposed. For a plain
git-commit in the superproject, we could envision
this sequence of todays commands:

    git -C submodule commit
    git add submodule
    git commit

>
> so the user can specify which submodule she wants. I first wrote it
> without the switch but but that collides with listing files which should
> be added. IMO this shorter option is also more intuitive to understand
> what it does (for this case).
>
>>     git fetch --submodule
>>     # When the flag is not given, we'd fetch superproject and
>>     # on-demand
>
> Yes like above we should add the path to the submodule right?

yes.

>
>>     # You feel the superproject is in the way?
>>     git worktree add --for-submodule <path/to/sub> ...
>>     # The new submodule worktree puts the
>>     # submodule only UX first. so it feels like its own
>>     # repository, no need for specific flags.
>
> I am not sure I understand this one. What would that do? Put a worktree
> for submodule path/to/sub to ...?

Yes, and at "..." you would have the UX of the submodule being
its own repository, no interaction with the superproject.

>
> Overall I like the direction of these ideas.
>
> Cheers Heiko

Re: Submodule regression in 2.14?

[Heiko Voigt]

On Tue, Aug 22, 2017 at 11:10:52AM -0700, Stefan Beller wrote:
> On Tue, Aug 22, 2017 at 8:33 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > On Mon, Aug 21, 2017 at 09:42:54AM -0700, Stefan Beller wrote:
> >> On Mon, Aug 21, 2017 at 9:05 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> >> > On Fri, Aug 18, 2017 at 11:51:13PM -0700, Junio C Hamano wrote:
> >>     # You feel the superproject is in the way?
> >>     git worktree add --for-submodule <path/to/sub> ...
> >>     # The new submodule worktree puts the
> >>     # submodule only UX first. so it feels like its own
> >>     # repository, no need for specific flags.
> >
> > I am not sure I understand this one. What would that do? Put a worktree
> > for submodule path/to/sub to ...?
> 
> Yes, and at "..." you would have the UX of the submodule being
> its own repository, no interaction with the superproject.

Are you sure that is what Junio meant? IMO that would be 'git worktree
clone' or 'git worktree checkout', no?  In todays git terminology an
'add' is something that puts data into the repository / the index. That
is why I was/am confused.

Cheers Heiko

Re: Submodule regression in 2.14?

[Stefan Beller]

On Fri, Aug 25, 2017 at 2:10 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> On Tue, Aug 22, 2017 at 11:10:52AM -0700, Stefan Beller wrote:
>> On Tue, Aug 22, 2017 at 8:33 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
>> > On Mon, Aug 21, 2017 at 09:42:54AM -0700, Stefan Beller wrote:
>> >> On Mon, Aug 21, 2017 at 9:05 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
>> >> > On Fri, Aug 18, 2017 at 11:51:13PM -0700, Junio C Hamano wrote:
>> >>     # You feel the superproject is in the way?
>> >>     git worktree add --for-submodule <path/to/sub> ...
>> >>     # The new submodule worktree puts the
>> >>     # submodule only UX first. so it feels like its own
>> >>     # repository, no need for specific flags.
>> >
>> > I am not sure I understand this one. What would that do? Put a worktree
>> > for submodule path/to/sub to ...?
>>
>> Yes, and at "..." you would have the UX of the submodule being
>> its own repository, no interaction with the superproject.
>
> Are you sure that is what Junio meant?

I am not sure at all, what Junio thought about this.

> IMO that would be 'git worktree
> clone' or 'git worktree checkout', no?  In todays git terminology an
> 'add' is something that puts data into the repository / the index. That
> is why I was/am confused.

I went with current gits:
$ git worktree
usage: git worktree add [<options>] <path> [<branch>]
   or: git worktree list [<options>]
   or: git worktree lock [<options>] <path>
   or: git worktree prune [<options>]
   or: git worktree unlock <path>

where the 'add' adds a new worktree (my mental emphasis was on the
option that says: "Make the new worktree for the submodule, such
that in that worktree the submodule feels like the toplevel
repository". If 'add' is a bad naming choice, we may want to
discuss&deprecate it in a new thread as that is related to the
worktree area?)

Thanks,
Stefan

>
> Cheers Heiko

Re: Submodule regression in 2.14?

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> On Tue, Aug 22, 2017 at 11:10:52AM -0700, Stefan Beller wrote:
>> On Tue, Aug 22, 2017 at 8:33 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
>> > On Mon, Aug 21, 2017 at 09:42:54AM -0700, Stefan Beller wrote:
>> >> On Mon, Aug 21, 2017 at 9:05 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
>> >> > On Fri, Aug 18, 2017 at 11:51:13PM -0700, Junio C Hamano wrote:
>> >>     # You feel the superproject is in the way?
>> >>     git worktree add --for-submodule <path/to/sub> ...
>> >>     # The new submodule worktree puts the
>> >>     # submodule only UX first. so it feels like its own
>> >>     # repository, no need for specific flags.
>> >
>> > I am not sure I understand this one. What would that do? Put a worktree
>> > for submodule path/to/sub to ...?
>> 
>> Yes, and at "..." you would have the UX of the submodule being
>> its own repository, no interaction with the superproject.
>
> Are you sure that is what Junio meant?

I am sure it was not.  

If adding a separate worktree only for one submodule, you should be
able to "cd $path-to-submodule" and run "git worktree add" to create
a new worktree, and I do not see any reason for the superproject to
get involved in that process.  Does it have more information ready
than the end user has to help the process of creating a new worktree?

While I can understand how --for-submodule above may be implemented,
I do not see the point of adding such an option.

Re: Submodule regression in 2.14?

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> So I think it is important that there are commits in the submodule so
> its history makes sense independently for others.

I was trying to push the "just like trees" to the logical conclusion
in order to see see if it leads to an absurd conclusions, or some
useful scenario.  I do not necessarily subscribed to Jonathan's
"vision" (I do not object to it as one possible mode of operation,
either).

> Or how would you push out the history in the submodule in your idea?
> Maybe I am missing something? What would be your use case with gitlinks
> pointing to trees?

Not my idea.  But Stefan's message I was responding to said that
object database for the superproject is the same as and mixed with
object databases for the submodules, so it is plausible that push
always happens at the superproject, and a mechanism to be invented
(I mentioned the need for it in the message you are responding to)
to enumerate all the commits bound from submodules to a range of
superproject's commits would identify these trees to be pushed out.

In essense, "just like trees" folks want to use the gitlinks in the
superproject to only specify the tree from the submodule project
that should sit there.  And my point is that such a world view would
lead to no need for branches in the submodule repository, no need
for commits in the submodule repository, and no need for history in
the submodule repository.  Which may or may not be a bad thing.

Re: Submodule regression in 2.14?

[Heiko Voigt]

On Mon, Aug 21, 2017 at 09:48:51AM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > So I think it is important that there are commits in the submodule so
> > its history makes sense independently for others.
> 
> I was trying to push the "just like trees" to the logical conclusion
> in order to see see if it leads to an absurd conclusions, or some
> useful scenario.  I do not necessarily subscribed to Jonathan's
> "vision" (I do not object to it as one possible mode of operation,
> either).
> 
> > Or how would you push out the history in the submodule in your idea?
> > Maybe I am missing something? What would be your use case with gitlinks
> > pointing to trees?
> 
> Not my idea.  But Stefan's message I was responding to said that
> object database for the superproject is the same as and mixed with
> object databases for the submodules, so it is plausible that push
> always happens at the superproject, and a mechanism to be invented
> (I mentioned the need for it in the message you are responding to)
> to enumerate all the commits bound from submodules to a range of
> superproject's commits would identify these trees to be pushed out.
> 
> In essense, "just like trees" folks want to use the gitlinks in the
> superproject to only specify the tree from the submodule project
> that should sit there.  And my point is that such a world view would
> lead to no need for branches in the submodule repository, no need
> for commits in the submodule repository, and no need for history in
> the submodule repository.  Which may or may not be a bad thing.

The problem I see here is: How do we seperate the submodule from the
superproject? Without the history (commits and trees) of the
superproject the submodule trees do not make sense anymore. The reason
users have submodules is because the projects are seperate somehow. With
just trees in the submodule it becomes tied quite tightly to the
superproject, IMO.

One step further: Why use gitlinks to point to trees? Let's just use
normal tree links instead, we already have that implemented :)

Cheers Heiko

Re: Submodule regression in 2.14?

[Stefan Beller]

On Fri, Aug 18, 2017 at 11:51 PM, Junio C Hamano <gitster@pobox.com> wrote:

> As long as we are talking about idealized future world (well, at
> least an idea of somebody's "ideal", not necessarily shared by
> everybody), I wonder if there is even any need to have commits in
> submodules in such a world.  To realize such a "monorepo" world, you
> might be better off allowing a gitlink in the superproject to
> directly point at a tree object in a submodule repository (making
> them physically a single repository is an optional implementation
> detail I choose to ignore in this discussion).

Then the sharing between superprojects (e.g. send an Android's linux
patch upstream or to another distro that also uses a superproject),
becomes cumbersome as the commit messages are missing and
potentially not specific to that subtree.

Re: Submodule regression in 2.14?

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Fri, Aug 18, 2017 at 11:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> As long as we are talking about idealized future world (well, at
>> least an idea of somebody's "ideal", not necessarily shared by
>> everybody), I wonder if there is even any need to have commits in
>> submodules in such a world.  To realize such a "monorepo" world, you
>> might be better off allowing a gitlink in the superproject to
>> directly point at a tree object in a submodule repository (making
>> them physically a single repository is an optional implementation
>> detail I choose to ignore in this discussion).
>
> Then the sharing between superprojects (e.g. send an Android's linux
> patch upstream or to another distro that also uses a superproject),
> becomes cumbersome as the commit messages are missing and
> potentially not specific to that subtree.

Indeed.  That is a problem "git commit --recurse-submodules" has.
Socratic method seem to have worked well to convince you that it is
not necessarily a good idea to make submodules "just like a tree".

Re: Submodule regression in 2.14?

[Lars Schneider]

> On 17 Aug 2017, at 23:55, Stefan Beller <sbeller@google.com> wrote:
> 
> On Thu, Aug 17, 2017 at 2:21 PM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
>> 
>>> Oh, wait.
>>> $ git log --oneline v2.13.0..v2.14.1 -- builtin/pull.c
>>> c9c63ee558 Merge branch 'sb/pull-rebase-submodule'
>>> a6d7eb2c7a pull: optionally rebase submodules (remote submodule changes only)
>>> could also be a culprit. Do you have pull.rebase set?
>> 
>> I bisected the problem today and "a6d7eb2c7a pull: optionally rebase submodules
>> (remote submodule changes only)" is indeed the culprit.
>> 
>> The commit seems to break the following test case.
>> 
>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>> index dcac364c5f..24f9729015 100755
>> --- a/t/t7400-submodule-basic.sh
>> +++ b/t/t7400-submodule-basic.sh
>> @@ -1289,4 +1289,19 @@ test_expect_success 'init properly sets the config' '
>>        test_must_fail git -C multisuper_clone config --get submodule.sub1.active
>> '
>> 
>> +test_expect_success 'submodule update and git pull with disabled submodule' '
>> +       test_when_finished "rm -rf multisuper_clone" &&
>> +       pwd=$(pwd) &&
>> +       git clone file://"$pwd"/multisuper multisuper_clone &&
>> +       (
>> +               cd multisuper_clone &&
>> +               git config --local submodule.sub0.update none &&
>> +               git submodule update --init --recursive &&
>> +               git pull --recurse-submodules &&
>> +               git submodule status | cut -c 1,43- >actual
>> +       ) &&
>> +       ls &&
>> +       test_cmp expect multisuper_clone/actual
>> +'
> 
> Thanks for providing this test.
> 
> cd trash directory.t7400-submodule-basic/multisuper_clone
> cat .git/config
> [submodule "sub0"]
>  update = none
>  active = true
>  url = file:///.../t/trash directory.t7400-submodule-basic/sub1
> 
> 
> submodule.<name>.update
>    The default update procedure for a submodule.
>    This variable is populated by git submodule init
>    from the gitmodules(5) file. See description of
>    update command in git-submodule(1).
> 
> The first sentence of .update is misleading IMHO as the
> these settings should strictly apply to the "submodule update"
> command. So "pull --recurse-submodules" ought to ignore it,
> instead the pull can do whatever it wants, namely treat the
> submodule roughly like a tree and either merge/rebase
> inside the submodule as well. The user *asked* for recursive
> pull after all.
> 
> Are you saying this might be a design mistake and
> the .update ought to be respected by all the other
> commands? For example
>    git reset --recurse-submodules
> should ignore the .update= none?
> 
> When designing these new recursive submodule functionality
> outside the "submodule" command, I'd want submodules
> to behave as much as possible like trees.

In the past "submodule.<name>.update=none" was an easy way
to selectively disable certain Submodules.

How would I do this with Git 2.14?

My gut feeling is that all commands should respect the
"submodule.<name>.update=none" setting.

- Lars

Re: Submodule regression in 2.14?

[Stefan Beller]

> In the past "submodule.<name>.update=none" was an easy way
> to selectively disable certain Submodules.
>
> How would I do this with Git 2.14?

    submodule.<name>.active = false

> My gut feeling is that all commands should respect the
> "submodule.<name>.update=none" setting.

Well my gut feeling was that the "update" part of the name
reponds to the subcommand, not the generic action.

For example when you set update=none, git-status,
recursive git-diff still reported the submodule.

>
> - Lars

Re: Submodule regression in 2.14?

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> In the past "submodule.<name>.update=none" was an easy way
>> to selectively disable certain Submodules.
>>
>> How would I do this with Git 2.14?
>
>     submodule.<name>.active = false
>
>> My gut feeling is that all commands should respect the
>> "submodule.<name>.update=none" setting.
>
> Well my gut feeling was that the "update" part of the name
> reponds to the subcommand, not the generic action.
>
> For example when you set update=none, git-status,
> recursive git-diff still reported the submodule.

Both status and diff are read-only operations, so this smells like a
bit bogus argument made by comparing apples and oranges.

I think Lars is more interested in operations that actually affects
the state of submodules by updating them---"submodule update" may be
a prime example as it goes down to run fetch, pull and/or checkout.
It may have been the only thing that affected the state of
submodules before the "--recurse-submodules" option was added to
commands that affect the state of the (super)project, but I would
think that it is not so wrong to expect that these state-affecting
operations running in the "recurse into submodules" mode to honor
"do not update this submodule" that used to be honored only by
"submodule update".

[PATCH] pull: respect submodule update configuration

[Stefan Beller]

From: Lars Schneider <larsxschneider@gmail.com>

Do not override the submodule configuration in the call to update
the submodules, but give a weaker default.

Reported-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
  
Personally I dislike this patch, but I have no better idea for the time
being.

Thanks,
Stefan 

 builtin/pull.c             |  6 ++++--
 git-submodule.sh           |  7 ++++++-
 t/t7400-submodule-basic.sh | 22 ++++++++++++++++++++++
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 9b86e519b1..be4f74d764 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -553,7 +553,8 @@ static int rebase_submodules(void)
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;
 	argv_array_pushl(&cp.args, "submodule", "update",
-				   "--recursive", "--rebase", NULL);
+				   "--recursive", "--default-update",
+				   "rebase", NULL);
 
 	return run_command(&cp);
 }
@@ -565,7 +566,8 @@ static int update_submodules(void)
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;
 	argv_array_pushl(&cp.args, "submodule", "update",
-				   "--recursive", "--checkout", NULL);
+				   "--recursive", "--default-update",
+				   "checkout", NULL);
 
 	return run_command(&cp);
 }
diff --git a/git-submodule.sh b/git-submodule.sh
index e131760eec..6dbc32e686 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -511,6 +511,7 @@ fetch_in_submodule () (
 cmd_update()
 {
 	# parse $args after "submodule ... update".
+	default_update="checkout"
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -552,6 +553,10 @@ cmd_update()
 		--checkout)
 			update="checkout"
 			;;
+		--default-update)
+			default_update="$2"
+			shift
+			;;
 		--recommend-shallow)
 			recommend_shallow="--recommend-shallow"
 			;;
@@ -619,7 +624,7 @@ cmd_update()
 			update_module=$(git config submodule."$name".update)
 			if test -z "$update_module"
 			then
-				update_module="checkout"
+				update_module="$default_update"
 			fi
 		fi
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index dcac364c5f..ff64bf8528 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1289,4 +1289,26 @@ test_expect_success 'init properly sets the config' '
 	test_must_fail git -C multisuper_clone config --get submodule.sub1.active
 '
 
+test_expect_success 'submodule update and git pull with disabled submodule' '
+	test_when_finished "rm -rf multisuper_clone" &&
+	pwd=$(pwd) &&
+	cat <<-\EOF >expect &&
+	-sub0
+	 sub1 (test2)
+	 sub2 (test2)
+	 sub3 (test2)
+	 sub4 (test2)
+	 sub5 (test2)
+	EOF
+	git clone file://"$pwd"/multisuper multisuper_clone &&
+	(
+		cd multisuper_clone &&
+		git config --local submodule.sub0.update none &&
+		git submodule update --init --recursive &&
+		git pull --recurse-submodules &&
+		git submodule status | cut -c 1,43- >actual
+	) &&
+	test_cmp expect multisuper_clone/actual
+'
+
 test_done
-- 
2.14.0.rc0.3.g6c2e499285

Re: [PATCH] pull: respect submodule update configuration

[Stefan Beller]

On Fri, Aug 18, 2017 at 3:04 PM, Stefan Beller <sbeller@google.com> wrote:
> From: Lars Schneider <larsxschneider@gmail.com>

eh, that is what I get for amending to Lars patch.

Re: [PATCH] pull: respect submodule update configuration

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Fri, Aug 18, 2017 at 3:04 PM, Stefan Beller <sbeller@google.com> wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>
> eh, that is what I get for amending to Lars patch.

Sorry, I do not understand this remark.  

If you started from a patch by Lars (I do not recall seeing it but
the list is high volume so it is entirely plausible that I may have
missed it) and tweaked it, it is more than OK to keep the original
author and record it in an in-body From: header like you did,
instead of taking authorship over.

Re: [PATCH] pull: respect submodule update configuration

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> From: Lars Schneider <larsxschneider@gmail.com>
>
> Do not override the submodule configuration in the call to update
> the submodules, but give a weaker default.
>
> Reported-by: Lars Schneider <larsxschneider@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>   
> Personally I dislike this patch, but I have no better idea for the time
> being.

The patch text from a cursory look seems reasonable to me.

It's not like you have 47 different codepaths that need to pay
attention to the .update config and they all have to pass the new
--default-update option, this is merely to fix one of them that
relates to the problem reported by Lars, and you need a similar fix
to other 46, right?

If you want the "--recurse-submodules" thing to always do the
"weaker default" thing in your project, you can choose not to set
.update to custom values in any of your submodules, so I do not
think the reason why you dislike this change is because it would
affect your use of submodules.

So I am a bit curious to learn which part of this change you dislike
and why.


>  builtin/pull.c             |  6 ++++--
>  git-submodule.sh           |  7 ++++++-
>  t/t7400-submodule-basic.sh | 22 ++++++++++++++++++++++
>  3 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 9b86e519b1..be4f74d764 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -553,7 +553,8 @@ static int rebase_submodules(void)
>  	cp.git_cmd = 1;
>  	cp.no_stdin = 1;
>  	argv_array_pushl(&cp.args, "submodule", "update",
> -				   "--recursive", "--rebase", NULL);
> +				   "--recursive", "--default-update",
> +				   "rebase", NULL);
>  
>  	return run_command(&cp);
>  }
> @@ -565,7 +566,8 @@ static int update_submodules(void)
>  	cp.git_cmd = 1;
>  	cp.no_stdin = 1;
>  	argv_array_pushl(&cp.args, "submodule", "update",
> -				   "--recursive", "--checkout", NULL);
> +				   "--recursive", "--default-update",
> +				   "checkout", NULL);
>  
>  	return run_command(&cp);
>  }
> diff --git a/git-submodule.sh b/git-submodule.sh
> index e131760eec..6dbc32e686 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -511,6 +511,7 @@ fetch_in_submodule () (
>  cmd_update()
>  {
>  	# parse $args after "submodule ... update".
> +	default_update="checkout"
>  	while test $# -ne 0
>  	do
>  		case "$1" in
> @@ -552,6 +553,10 @@ cmd_update()
>  		--checkout)
>  			update="checkout"
>  			;;
> +		--default-update)
> +			default_update="$2"
> +			shift
> +			;;
>  		--recommend-shallow)
>  			recommend_shallow="--recommend-shallow"
>  			;;
> @@ -619,7 +624,7 @@ cmd_update()
>  			update_module=$(git config submodule."$name".update)
>  			if test -z "$update_module"
>  			then
> -				update_module="checkout"
> +				update_module="$default_update"
>  			fi
>  		fi
>  
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index dcac364c5f..ff64bf8528 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -1289,4 +1289,26 @@ test_expect_success 'init properly sets the config' '
>  	test_must_fail git -C multisuper_clone config --get submodule.sub1.active
>  '
>  
> +test_expect_success 'submodule update and git pull with disabled submodule' '
> +	test_when_finished "rm -rf multisuper_clone" &&
> +	pwd=$(pwd) &&
> +	cat <<-\EOF >expect &&
> +	-sub0
> +	 sub1 (test2)
> +	 sub2 (test2)
> +	 sub3 (test2)
> +	 sub4 (test2)
> +	 sub5 (test2)
> +	EOF
> +	git clone file://"$pwd"/multisuper multisuper_clone &&
> +	(
> +		cd multisuper_clone &&
> +		git config --local submodule.sub0.update none &&
> +		git submodule update --init --recursive &&
> +		git pull --recurse-submodules &&
> +		git submodule status | cut -c 1,43- >actual
> +	) &&
> +	test_cmp expect multisuper_clone/actual
> +'
> +
>  test_done

Re: [PATCH] pull: respect submodule update configuration

[Heiko Voigt]

On Fri, Aug 18, 2017 at 11:24:47PM -0700, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
> 
> > From: Lars Schneider <larsxschneider@gmail.com>
> >
> > Do not override the submodule configuration in the call to update
> > the submodules, but give a weaker default.
> >
> > Reported-by: Lars Schneider <larsxschneider@gmail.com>
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
> >   
> > Personally I dislike this patch, but I have no better idea for the time
> > being.
> 
> The patch text from a cursory look seems reasonable to me.
> 
> It's not like you have 47 different codepaths that need to pay
> attention to the .update config and they all have to pass the new
> --default-update option, this is merely to fix one of them that
> relates to the problem reported by Lars, and you need a similar fix
> to other 46, right?
> 
> If you want the "--recurse-submodules" thing to always do the
> "weaker default" thing in your project, you can choose not to set
> .update to custom values in any of your submodules, so I do not
> think the reason why you dislike this change is because it would
> affect your use of submodules.
> 
> So I am a bit curious to learn which part of this change you dislike
> and why.

I am also curious. Isn't this the same strategy we are using in other
places?

Cheers Heiko

Re: [PATCH] pull: respect submodule update configuration

[Stefan Beller]

On Mon, Aug 21, 2017 at 9:20 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:

>> So I am a bit curious to learn which part of this change you dislike
>> and why.
>
> I am also curious. Isn't this the same strategy we are using in other
> places?
>

I dislike it because the UX feels crude.  When reading the documentation,
it seems to me as if submodule.<name> can be one of the following

    (none, checkout, rebase, merge, !<custom-command>)

This is perfect for "submodule-update", whose primary goal is
to update submodules *somehow*. However other commands

    git rebase --recurse
    git merge --recurse
    git checkout --recurse

have a different primary mode of operation (note how their name
is one of the modes from the set above), so it may get confusing
for a user.

'none'  and '!<custom-command>' seem like they would be okay
for any of the commands above but then:

    git config submodule.<name>.update "!..."
    git reset --hard --recurse
    git status
    # submodule is reported, because "!..." did not 'reset'.

Anyway. That dislike is just a minor gut feeling about the UX/UI
being horrible. I wrote the patch to keep the conversation going,
and if it fixes Lars problem, let's take it for now.

Thanks,
Stefan

Re: [PATCH] pull: respect submodule update configuration

[Lars Schneider]

> On 21 Aug 2017, at 18:55, Stefan Beller <sbeller@google.com> wrote:
> 
> On Mon, Aug 21, 2017 at 9:20 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> 
>>> So I am a bit curious to learn which part of this change you dislike
>>> and why.
>> 
>> I am also curious. Isn't this the same strategy we are using in other
>> places?
>> 
> 
> I dislike it because the UX feels crude.  When reading the documentation,
> it seems to me as if submodule.<name> can be one of the following
> 
>    (none, checkout, rebase, merge, !<custom-command>)
> 
> This is perfect for "submodule-update", whose primary goal is
> to update submodules *somehow*. However other commands
> 
>    git rebase --recurse
>    git merge --recurse
>    git checkout --recurse
> 
> have a different primary mode of operation (note how their name
> is one of the modes from the set above), so it may get confusing
> for a user.
> 
> 'none'  and '!<custom-command>' seem like they would be okay
> for any of the commands above but then:
> 
>    git config submodule.<name>.update "!..."
>    git reset --hard --recurse
>    git status
>    # submodule is reported, because "!..." did not 'reset'.
> 
> Anyway. That dislike is just a minor gut feeling about the UX/UI
> being horrible. I wrote the patch to keep the conversation going,
> and if it fixes Lars problem, let's take it for now.

Well, I need just a way to disable certain Submodules completely.
If you show me how "git config --local submodule.sub.active false"
works then I don't need this patch.

I tried to make it work here:
https://public-inbox.org/git/89AB8AA3-8E19-46BA-B169-D1EA4CF4ABE7@gmail.com/

Thanks,
Lars

Re: [PATCH] pull: respect submodule update configuration

[Stefan Beller]

On Mon, Aug 21, 2017 at 10:20 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>> On 21 Aug 2017, at 18:55, Stefan Beller <sbeller@google.com> wrote:
>>
>> On Mon, Aug 21, 2017 at 9:20 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
>>
>>>> So I am a bit curious to learn which part of this change you dislike
>>>> and why.
>>>
>>> I am also curious. Isn't this the same strategy we are using in other
>>> places?
>>>
>>
>> I dislike it because the UX feels crude.  When reading the documentation,
>> it seems to me as if submodule.<name> can be one of the following
>>
>>    (none, checkout, rebase, merge, !<custom-command>)
>>
>> This is perfect for "submodule-update", whose primary goal is
>> to update submodules *somehow*. However other commands
>>
>>    git rebase --recurse
>>    git merge --recurse
>>    git checkout --recurse
>>
>> have a different primary mode of operation (note how their name
>> is one of the modes from the set above), so it may get confusing
>> for a user.
>>
>> 'none'  and '!<custom-command>' seem like they would be okay
>> for any of the commands above but then:
>>
>>    git config submodule.<name>.update "!..."
>>    git reset --hard --recurse
>>    git status
>>    # submodule is reported, because "!..." did not 'reset'.
>>
>> Anyway. That dislike is just a minor gut feeling about the UX/UI
>> being horrible. I wrote the patch to keep the conversation going,
>> and if it fixes Lars problem, let's take it for now.
>
> Well, I need just a way to disable certain Submodules completely.
> If you show me how "git config --local submodule.sub.active false"
> works then I don't need this patch.
>
> I tried to make it work here:
> https://public-inbox.org/git/89AB8AA3-8E19-46BA-B169-D1EA4CF4ABE7@gmail.com/

(A) you need to set expect there as well, to have sub{2,4,5} be expected
there as well.

(B) That may hint at another (UX) bug.

The test case there uses "git submodule update --init".
The init flag will set all submodules to active.

Maybe you want

    git config submodule.active ":(exclude)sub0"
    git config --add submodule.active ":(exclude)sub2"
    git config --add submodule.active "."
    # Read: anything except sub0 and sub2 are interesting

    git submodule update
    # no init flag, needed even for new submodules IIUC

Re: [PATCH] pull: respect submodule update configuration

[Brandon Williams]

On 08/21, Stefan Beller wrote:
> On Mon, Aug 21, 2017 at 10:20 AM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
> >
> >> On 21 Aug 2017, at 18:55, Stefan Beller <sbeller@google.com> wrote:
> >>
> >> On Mon, Aug 21, 2017 at 9:20 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> >>
> >>>> So I am a bit curious to learn which part of this change you dislike
> >>>> and why.
> >>>
> >>> I am also curious. Isn't this the same strategy we are using in other
> >>> places?
> >>>
> >>
> >> I dislike it because the UX feels crude.  When reading the documentation,
> >> it seems to me as if submodule.<name> can be one of the following
> >>
> >>    (none, checkout, rebase, merge, !<custom-command>)
> >>
> >> This is perfect for "submodule-update", whose primary goal is
> >> to update submodules *somehow*. However other commands
> >>
> >>    git rebase --recurse
> >>    git merge --recurse
> >>    git checkout --recurse
> >>
> >> have a different primary mode of operation (note how their name
> >> is one of the modes from the set above), so it may get confusing
> >> for a user.
> >>
> >> 'none'  and '!<custom-command>' seem like they would be okay
> >> for any of the commands above but then:
> >>
> >>    git config submodule.<name>.update "!..."
> >>    git reset --hard --recurse
> >>    git status
> >>    # submodule is reported, because "!..." did not 'reset'.
> >>
> >> Anyway. That dislike is just a minor gut feeling about the UX/UI
> >> being horrible. I wrote the patch to keep the conversation going,
> >> and if it fixes Lars problem, let's take it for now.
> >
> > Well, I need just a way to disable certain Submodules completely.
> > If you show me how "git config --local submodule.sub.active false"
> > works then I don't need this patch.

Yeah if you want to completely disable a submodule (as in not even check
it out) then setting .active to false would do that.  But as stefan
pointed out and IIRC 'submodule update --init' with no pathspec sets all
submodules to be active.  Perhaps it should only init submodules who
don't already have an explicit active flag set.

> >
> > I tried to make it work here:
> > https://public-inbox.org/git/89AB8AA3-8E19-46BA-B169-D1EA4CF4ABE7@gmail.com/
> 
> (A) you need to set expect there as well, to have sub{2,4,5} be expected
> there as well.
> 
> (B) That may hint at another (UX) bug.
> 
> The test case there uses "git submodule update --init".
> The init flag will set all submodules to active.
> 
> Maybe you want
> 
>     git config submodule.active ":(exclude)sub0"
>     git config --add submodule.active ":(exclude)sub2"
>     git config --add submodule.active "."
>     # Read: anything except sub0 and sub2 are interesting
> 
>     git submodule update
>     # no init flag, needed even for new submodules IIUC

-- 
Brandon Williams

Re: [PATCH] pull: respect submodule update configuration

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

>> > I tried to make it work here:
>> > https://public-inbox.org/git/89AB8AA3-8E19-46BA-B169-D1EA4CF4ABE7@gmail.com/
>> 
>> (A) you need to set expect there as well, to have sub{2,4,5} be expected
>> there as well.
>> 
>> (B) That may hint at another (UX) bug.
>> 
>> The test case there uses "git submodule update --init".
>> The init flag will set all submodules to active.
>> 
>> Maybe you want
>> 
>>     git config submodule.active ":(exclude)sub0"
>>     git config --add submodule.active ":(exclude)sub2"
>>     git config --add submodule.active "."
>>     # Read: anything except sub0 and sub2 are interesting
>> 
>>     git submodule update
>>     # no init flag, needed even for new submodules IIUC

Sounds like a good solution.  I do think what Stefan said about
"reset --hard --recurse-submodules" makes tons of sense, and we
should do a hard reset in submodules as long as they are marked
as active, regardless of what the .update option says.  I have
not thought deeply enough about other operations like "pull" in
his message to be able to comment, though.

Thanks.

Re: [PATCH] pull: respect submodule update configuration

[Lars Schneider]

> On 21 Aug 2017, at 20:21, Brandon Williams <bmwill@google.com> wrote:
> 
> On 08/21, Stefan Beller wrote:
>> On Mon, Aug 21, 2017 at 10:20 AM, Lars Schneider
>> <larsxschneider@gmail.com> wrote:
>>> 
>>>> On 21 Aug 2017, at 18:55, Stefan Beller <sbeller@google.com> wrote:
>>>> 
>>>> On Mon, Aug 21, 2017 at 9:20 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
>>>> 
>>>>>> So I am a bit curious to learn which part of this change you dislike
>>>>>> and why.
>>>>> 
>>>>> I am also curious. Isn't this the same strategy we are using in other
>>>>> places?
>>>>> 
>>>> 
>>>> I dislike it because the UX feels crude.  When reading the documentation,
>>>> it seems to me as if submodule.<name> can be one of the following
>>>> 
>>>>   (none, checkout, rebase, merge, !<custom-command>)
>>>> 
>>>> This is perfect for "submodule-update", whose primary goal is
>>>> to update submodules *somehow*. However other commands
>>>> 
>>>>   git rebase --recurse
>>>>   git merge --recurse
>>>>   git checkout --recurse
>>>> 
>>>> have a different primary mode of operation (note how their name
>>>> is one of the modes from the set above), so it may get confusing
>>>> for a user.
>>>> 
>>>> 'none'  and '!<custom-command>' seem like they would be okay
>>>> for any of the commands above but then:
>>>> 
>>>>   git config submodule.<name>.update "!..."
>>>>   git reset --hard --recurse
>>>>   git status
>>>>   # submodule is reported, because "!..." did not 'reset'.
>>>> 
>>>> Anyway. That dislike is just a minor gut feeling about the UX/UI
>>>> being horrible. I wrote the patch to keep the conversation going,
>>>> and if it fixes Lars problem, let's take it for now.
>>> 
>>> Well, I need just a way to disable certain Submodules completely.
>>> If you show me how "git config --local submodule.sub.active false"
>>> works then I don't need this patch.
> 
> Yeah if you want to completely disable a submodule (as in not even check
> it out) then setting .active to false would do that.  But as stefan
> pointed out and IIRC 'submodule update --init' with no pathspec sets all
> submodules to be active.  Perhaps it should only init submodules who
> don't already have an explicit active flag set.

OK. I change my scripts to use ".active" and it seems to work nicely.

I noticed one oddity, though:

If I clone a repo using `git clone --recursive <url>` then the local
Git config of the repo gets the following entry:

[submodule]
	active = .

Is this intentional? Something in the git/git test harness seems to prevent
that. I was not able to write a test to replicate the issue.

Any idea?

Thanks,
Lars

Re: [PATCH] pull: respect submodule update configuration

[Stefan Beller]

On Tue, Aug 22, 2017 at 7:50 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
> OK. I change my scripts to use ".active" and it seems to work nicely.
>
> I noticed one oddity, though:
>
> If I clone a repo using `git clone --recursive <url>` then the local
> Git config of the repo gets the following entry:
>
> [submodule]
>         active = .

bb62e0a99f (clone: teach --recurse-submodules to optionally take a
pathspec, 2017-03-17) makes it clear that this is intentional for
--recurse-submodules, but doesn't exactly state that --recurse will
behave the same. The idea here is that at clone time you can already
give

    git clone --recurse=:(exclude)sub0 <url> <path>

and have your desired set of submodules there.
Combined with the changes in the attr system, b0db704652
(pathspec: allow querying for attributes, 2017-03-13)
you could make up things like this:

  $ cat .gitattributes
  /sub0 label0
  /sub1
  /sub2 label1 label2
  /sub3 label1
  /platform-specifc-subs/* label1 label2

and then get a clone via

    git clone --recurse=:(attr:label2). <url> <path>

for example. The labeling via the attributes allows for
complex patterns, but a relatively easy command line, that you
can share with coworkers.

> Is this intentional? Something in the git/git test harness seems to prevent
> that. I was not able to write a test to replicate the issue.
>
> Any idea?

I do not seem to understand the perceived bug?
The setting of submodule.active=<pathspec> seems intentional to me,
but how would you not reproduce it? Maybe Brandon has an idea.

Thanks,
Stefan

Re: [PATCH] pull: respect submodule update configuration

[Brandon Williams]

On 08/22, Stefan Beller wrote:
> On Tue, Aug 22, 2017 at 7:50 AM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
> >
> > OK. I change my scripts to use ".active" and it seems to work nicely.
> >
> > I noticed one oddity, though:
> >
> > If I clone a repo using `git clone --recursive <url>` then the local
> > Git config of the repo gets the following entry:
> >
> > [submodule]
> >         active = .
> 
> bb62e0a99f (clone: teach --recurse-submodules to optionally take a
> pathspec, 2017-03-17) makes it clear that this is intentional for
> --recurse-submodules, but doesn't exactly state that --recurse will
> behave the same. The idea here is that at clone time you can already
> give
> 
>     git clone --recurse=:(exclude)sub0 <url> <path>
> 
> and have your desired set of submodules there.
> Combined with the changes in the attr system, b0db704652
> (pathspec: allow querying for attributes, 2017-03-13)
> you could make up things like this:
> 
>   $ cat .gitattributes
>   /sub0 label0
>   /sub1
>   /sub2 label1 label2
>   /sub3 label1
>   /platform-specifc-subs/* label1 label2
> 
> and then get a clone via
> 
>     git clone --recurse=:(attr:label2). <url> <path>
> 
> for example. The labeling via the attributes allows for
> complex patterns, but a relatively easy command line, that you
> can share with coworkers.
> 
> > Is this intentional? Something in the git/git test harness seems to prevent
> > that. I was not able to write a test to replicate the issue.
> >
> > Any idea?
> 
> I do not seem to understand the perceived bug?
> The setting of submodule.active=<pathspec> seems intentional to me,
> but how would you not reproduce it? Maybe Brandon has an idea.
> 

When adding '.active' we wanted it to be as flexible as possible.  So
you can either use a pathspec with 'submodule.active' to catch multiple
submodules as being active or you can turn on/off individual submodules
with 'submodule.<name>.active' (this has precedent over the more general
'submodule.active' config).

The intent was if a user supplies --recurse-submodules (I believe i
removed the docs for --recursive in order to make the CLI more consistent
with other commands, so --recursive is just a synonym for
--recurse-submodules) then they clearly wanted all the submodules cloned
and checked out.  With the '.active' config the way to specify this
is to make 'submodule.active = .'.  In the old world every submodule
would need to have its URL copied into the config.  This way the config
is kept cleaner as it only has a single entry added.

As stefan mentioned you can specify a value for 'submodule.active' to
take as an arg to --recurse-submodules (the default being '.' or all
submodules) so you can do clever things like group submodules using
attributes, you can even repeat the flag to provided a more complex
pathspec.

Hopefully that answers your question :D

-- 
Brandon Williams

Re: Submodule regression in 2.14?

[Lars Schneider]

> On 18 Aug 2017, at 19:16, Stefan Beller <sbeller@google.com> wrote:
> 
>> In the past "submodule.<name>.update=none" was an easy way
>> to selectively disable certain Submodules.
>> 
>> How would I do this with Git 2.14?
> 
>    submodule.<name>.active = false

That's what I thought after your first response. However,
this test case fails for me, too:


diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index dcac364c5f..24f9729015 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1289,4 +1289,19 @@ test_expect_success 'init properly sets the config' '
	test_must_fail git -C multisuper_clone config --get submodule.sub1.active
'

+test_expect_success 'submodule update and git pull with disabled submodule' '
+	test_when_finished "rm -rf multisuper_clone" &&
+	pwd=$(pwd) &&
+	git clone file://"$pwd"/multisuper multisuper_clone &&
+	(
+		cd multisuper_clone &&
+		git config --local submodule.sub0.update none &&
+		git config --local submodule.sub0.active false &&
+		git submodule update --init --recursive &&
+		git pull --recurse-submodules &&
+		git submodule status | cut -c 1,43- >actual
+	) &&
+	ls &&
+	test_cmp expect multisuper_clone/actual
+'
+
test_done


Here is the relevant part of the Git config:

	[submodule "sub0"]
		update = none
		active = false

Is this a bug?


>> My gut feeling is that all commands should respect the
>> "submodule.<name>.update=none" setting.
> 
> Well my gut feeling was that the "update" part of the name
> reponds to the subcommand, not the generic action.

I see. But wouldn't that be inconsistent with the config
"active" then? Following that logic "active" would only
respond to the (non-existent) "active" subcommand, no?


> For example when you set update=none, git-status,
> recursive git-diff still reported the submodule.

My understanding is this:

(1) "active" controls if a submodule is considered by
    Git at all.

(2) "update" controls how and if the submodule pointer
    modified

Is this your intention? What would be the use case for
"active=true" and "update=none"? 


- Lars