[PATCH 0/2] submodule update: allow '.' for branch value

[PATCH 0/2] submodule update: allow '.' for branch value

[Stefan Beller]

The meat is in patch 2 and helps Git and Gerrit work well together.

patch 1 looks unrelated but it is not, as when left out the broken test, breaks
with patch 2. This is because we add more commits in the submodule.

Thanks,
Stefan

Stefan Beller (2):
  t7406: correct depth test in shallow test
  submodule update: allow '.' for branch value

 git-submodule.sh            |  7 +++++++
 t/t7406-submodule-update.sh | 37 ++++++++++++++++++++++++++++++++++---
 2 files changed, 41 insertions(+), 3 deletions(-)

-- 
2.9.2.466.g8c6d1f9.dirty

[PATCH 1/2] t7406: correct depth test in shallow test

[Stefan Beller]

We used to ask for 3 changes and tested for having 1, so the test
seems broken.

Correct the test by using test_line_count that exists in the test suite.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7406-submodule-update.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 88e9750..bd261ac 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -846,9 +846,10 @@ test_expect_success 'submodule update clone shallow submodule' '
 	(cd super3 &&
 	 sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
 	 mv -f .gitmodules.tmp .gitmodules &&
-	 git submodule update --init --depth=3
+	 git submodule update --init --depth=2
 	 (cd submodule &&
-	  test 1 = $(git log --oneline | wc -l)
+	  git log --oneline >lines
+	  test_line_count = 2 lines
 	 )
 )
 '
-- 
2.9.2.466.g8c6d1f9.dirty

Re: [PATCH 1/2] t7406: correct depth test in shallow test

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> We used to ask for 3 changes and tested for having 1, so the test
> seems broken.

I am not sure what to think of "seems broken".

Asking for 3 and having 1 is broken in what way?  Should we be
expecting for 3 because we asked for that many?  Should we expect
less than three even though we asked for three because the upstream
side does not even have that many?  If the current test that asks
for 3 and gets only 1 is not failing, why should we expect that
asking for 2 would get 2?  In other words, why is it sane that
asking for fewer number of commits gives us more?

Also most of the lines in this subshell seem to be breaking
&&-chain.



> Correct the test by using test_line_count that exists in the test suite.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t7406-submodule-update.sh | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 88e9750..bd261ac 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -846,9 +846,10 @@ test_expect_success 'submodule update clone shallow submodule' '
>  	(cd super3 &&
>  	 sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
>  	 mv -f .gitmodules.tmp .gitmodules &&
> -	 git submodule update --init --depth=3
> +	 git submodule update --init --depth=2
>  	 (cd submodule &&
> -	  test 1 = $(git log --oneline | wc -l)
> +	  git log --oneline >lines
> +	  test_line_count = 2 lines
>  	 )
>  )
>  '

Re: [PATCH 1/2] t7406: correct depth test in shallow test

[Stefan Beller]

On Thu, Jul 28, 2016 at 11:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> We used to ask for 3 changes and tested for having 1, so the test
>> seems broken.
>
> I am not sure what to think of "seems broken".

When asking for depth 3, I would expect a result of 1,2, or 3 commits.

But when testing the depth argument with a history less than 3, and then
implying: "I got 1, which is less than 3, so the depth works", seems
to be a logical shortcut to me.

I would have expected a history of >3, then ask for 3 and confirm we did not
get 4 or 5 or 6, but 3 only.

>
> Asking for 3 and having 1 is broken in what way?  Should we be
> expecting for 3 because we asked for that many?  Should we expect
> less than three even though we asked for three because the upstream
> side does not even have that many?  If the current test that asks
> for 3 and gets only 1 is not failing, why should we expect that
> asking for 2 would get 2?  In other words, why is it sane that
> asking for fewer number of commits gives us more?

I think there is a subtle thing going on, that I did not examine properly but
it is hidden in the modernization from

    test 1 = $(something)
 to test_line_count = 2

I'll investigate the actual reason.

>
> Also most of the lines in this subshell seem to be breaking
> &&-chain.

Thanks for pointing that out, will fix while at it.

>
>
>
>> Correct the test by using test_line_count that exists in the test suite.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  t/t7406-submodule-update.sh | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index 88e9750..bd261ac 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -846,9 +846,10 @@ test_expect_success 'submodule update clone shallow submodule' '
>>       (cd super3 &&
>>        sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
>>        mv -f .gitmodules.tmp .gitmodules &&
>> -      git submodule update --init --depth=3
>> +      git submodule update --init --depth=2
>>        (cd submodule &&
>> -       test 1 = $(git log --oneline | wc -l)
>> +       git log --oneline >lines
>> +       test_line_count = 2 lines
>>        )
>>  )
>>  '

Re: [PATCH 1/2] t7406: correct depth test in shallow test

[Stefan Beller]

On Thu, Jul 28, 2016 at 11:47 AM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Jul 28, 2016 at 11:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> We used to ask for 3 changes and tested for having 1, so the test
>>> seems broken.
>>
>> I am not sure what to think of "seems broken".
>
> When asking for depth 3, I would expect a result of 1,2, or 3 commits.
>
> But when testing the depth argument with a history less than 3, and then
> implying: "I got 1, which is less than 3, so the depth works", seems
> to be a logical shortcut to me.
>
> I would have expected a history of >3, then ask for 3 and confirm we did not
> get 4 or 5 or 6, but 3 only.
>
>>
>> Asking for 3 and having 1 is broken in what way?  Should we be
>> expecting for 3 because we asked for that many?  Should we expect
>> less than three even though we asked for three because the upstream
>> side does not even have that many?  If the current test that asks
>> for 3 and gets only 1 is not failing, why should we expect that
>> asking for 2 would get 2?  In other words, why is it sane that
>> asking for fewer number of commits gives us more?
>
> I think there is a subtle thing going on, that I did not examine properly but
> it is hidden in the modernization from
>
>     test 1 = $(something)
>  to test_line_count = 2
>
> I'll investigate the actual reason.

So when I place a test_pause just before that check for the lines
we have the upstream submodule:
$ git log --oneline
6355002 upstream line4
820877d upstream line3
4301fd3 Commit 2
0c90624 upstream

which then allows fetching
6355002 upstream line4
820877d upstream line3
4301fd3 Commit 2

and "Commit 2" is recorded as the sha1, i.e.
when checking out "Commit 2" and running
(git log --oneline | wc -l) we get 1 as it cuts
off after that.

When adding a test (in the next patch) that adds
more commits to the submodule upstream, we
will fetch with depth 3 but will no longer see the sha1,
so we try a different approach. Current approach:
try fetching again with no depth, and then try again with sha1
given.

So one could say there is a bug as the fetching should
use the depth argument as well.

The depth of 2 I chose originally turns out to be a lucky
accident too, as the depth from "Commit 2" is 2,
so that we would observe the same depth no matter if
a --depth 2 was given and working or not.

I'll redo this test (as 2 tests, one is testing the situation as
we have now, i.e. the desired tip is reachable from within
the depth argument, the second test will test if it is not
reachable.)

>
>>
>> Also most of the lines in this subshell seem to be breaking
>> &&-chain.
>
> Thanks for pointing that out, will fix while at it.
>
>>
>>
>>
>>> Correct the test by using test_line_count that exists in the test suite.
>>>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>> ---
>>>  t/t7406-submodule-update.sh | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>>> index 88e9750..bd261ac 100755
>>> --- a/t/t7406-submodule-update.sh
>>> +++ b/t/t7406-submodule-update.sh
>>> @@ -846,9 +846,10 @@ test_expect_success 'submodule update clone shallow submodule' '
>>>       (cd super3 &&
>>>        sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
>>>        mv -f .gitmodules.tmp .gitmodules &&
>>> -      git submodule update --init --depth=3
>>> +      git submodule update --init --depth=2
>>>        (cd submodule &&
>>> -       test 1 = $(git log --oneline | wc -l)
>>> +       git log --oneline >lines
>>> +       test_line_count = 2 lines
>>>        )
>>>  )
>>>  '

Re: [PATCH 1/2] t7406: correct depth test in shallow test

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The depth of 2 I chose originally turns out to be a lucky
> accident too, as the depth from "Commit 2" is 2,
> so that we would observe the same depth no matter if
> a --depth 2 was given and working or not.
>
> I'll redo this test (as 2 tests, one is testing the situation as
> we have now, i.e. the desired tip is reachable from within
> the depth argument, the second test will test if it is not
> reachable.)

Good that I was puzzled ;-) Looking forward to see an improved test.

Thanks.

[PATCH 2/2] submodule update: allow '.' for branch value

[Stefan Beller]

Gerrit has a "superproject subscription" feature[1], that triggers a
commit in a superproject that is subscribed to its submodules.
Conceptually this Gerrit feature can be done on the client side with
Git via:

    git -C <superproject> submodule update --remote --force
    git -C <superproject> commit -a -m "Update submodules"
    git -C <superproject> push

for each branch in the superproject. To ease the configuration in Gerrit
a special value of "." has been introduced for the submodule.<name>.branch
to mean the same branch as the superproject[2], such that you can create a
new branch on both superproject and the submodule and this feature
continues to work on that new branch.

Now we have find projects in the wild with such a .gitmodules file.
To have Git working well with these, we imitate the behavior and
look up the superprojects branch name if the submodules branch is
configured to ".". In projects that do not use Gerrit, this value
whould be never configured as "." is not a valid branch name.

[1] introduced around 2012-01, e.g.
    https://gerrit-review.googlesource.com/#/c/30810/
[2] excerpt from [1]:
 > The branch value could be '.' if the submodule project branch
 > has the same name as the destination branch of the commit having
 > gitlinks/.gitmodules file.

CC: Avery Pennarun <apenwarr@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh            |  7 +++++++
 t/t7406-submodule-update.sh | 32 +++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 4ec7546..12bb9e8 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -591,6 +591,13 @@ cmd_update()
 		name=$(git submodule--helper name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
 		branch=$(get_submodule_config "$name" branch master)
+		if test "$branch" = "."
+		then
+			if ! branch=$(git symbolic-ref --short -q HEAD)
+			then
+				die "$(eval_gettext "submodule branch configured to inherit branch from superproject, but it's not on any branch")"
+			fi
+		fi
 		if ! test -z "$update"
 		then
 			update_module=$update
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index bd261ac..41d69e4 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -209,9 +209,39 @@ test_expect_success 'submodule update --remote should fetch upstream changes' '
 	)
 '
 
-test_expect_success 'local config should override .gitmodules branch' '
+test_expect_success 'submodule update --remote should fetch upstream changes with .' '
+	(cd super &&
+	 git config -f .gitmodules submodule."submodule".branch "." &&
+	 git add .gitmodules &&
+	 git commit -m "submodules: update from the respective superproject branch"
+	) &&
 	(cd submodule &&
+	 echo line4a >> file &&
+	 git add file &&
+	 test_tick &&
+	 git commit -m "upstream line4a" &&
 	 git checkout -b test-branch &&
+	 test_commit on-test-branch
+	) &&
+	(cd super &&
+	 git submodule update --remote --force submodule &&
+	 (cd submodule &&
+	  test "$(git log -1 --oneline)" = "$(GIT_DIR=../../submodule/.git git log -1 --oneline master)"
+	 ) &&
+	 git checkout -b test-branch &&
+	 git submodule update --remote --force submodule &&
+	 (cd submodule &&
+	  test "$(git log -1 --oneline)" = "$(GIT_DIR=../../submodule/.git git log -1 --oneline test-branch)"
+	 ) &&
+	 git checkout master &&
+	 git branch -d test-branch &&
+	 git revert HEAD
+	)
+'
+
+test_expect_success 'local config should override .gitmodules branch' '
+	(cd submodule &&
+	 git checkout test-branch &&
 	 echo line5 >> file &&
 	 git add file &&
 	 test_tick &&
-- 
2.9.2.466.g8c6d1f9.dirty

[PATCHv2 2/2] submodule update: allow '.' for branch value

[Stefan Beller]

Gerrit has a "superproject subscription" feature[1], that triggers a
commit in a superproject that is subscribed to its submodules.
Conceptually this Gerrit feature can be done on the client side with
Git via:

    git -C <superproject> submodule update --remote --force
    git -C <superproject> commit -a -m "Update submodules"
    git -C <superproject> push

for each branch in the superproject. To ease the configuration in Gerrit
a special value of "." has been introduced for the submodule.<name>.branch
to mean the same branch as the superproject[2], such that you can create a
new branch on both superproject and the submodule and this feature
continues to work on that new branch.

Now we have find projects in the wild with such a .gitmodules file.
To have Git working well with these, we imitate the behavior and
look up the superprojects branch name if the submodules branch is
configured to ".". In projects that do not use Gerrit, this value
whould be never configured as "." is not a valid branch name.

[1] introduced around 2012-01, e.g.
    https://gerrit-review.googlesource.com/#/c/30810/
[2] excerpt from [1]:
 > The branch value could be '.' if the submodule project branch
 > has the same name as the destination branch of the commit having
 > gitlinks/.gitmodules file.

CC: Avery Pennarun <apenwarr@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

 This comes with another test that I run into while using this code.
 Please replace patch 2 with this v2.
 
 Thanks,
 Stefan

 git-submodule.sh            |  9 ++++++++-
 t/t7406-submodule-update.sh | 42 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 4ec7546..1eb33ad 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -590,7 +590,6 @@ cmd_update()
 
 		name=$(git submodule--helper name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
-		branch=$(get_submodule_config "$name" branch master)
 		if ! test -z "$update"
 		then
 			update_module=$update
@@ -616,6 +615,14 @@ cmd_update()
 
 		if test -n "$remote"
 		then
+			branch=$(get_submodule_config "$name" branch master)
+			if test "$branch" = "."
+			then
+				if ! branch=$(git symbolic-ref --short -q HEAD)
+				then
+					die "$(eval_gettext "submodule branch configured to inherit branch from superproject, but it's not on any branch")"
+				fi
+			fi
 			if test -z "$nofetch"
 			then
 				# Fetch remote before determining tracking $sha1
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index bd261ac..953c486 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -209,9 +209,49 @@ test_expect_success 'submodule update --remote should fetch upstream changes' '
 	)
 '
 
-test_expect_success 'local config should override .gitmodules branch' '
+test_expect_success 'submodule update --remote should fetch upstream changes with .' '
+	(cd super &&
+	 git config -f .gitmodules submodule."submodule".branch "." &&
+	 git add .gitmodules &&
+	 git commit -m "submodules: update from the respective superproject branch"
+	) &&
 	(cd submodule &&
+	 echo line4a >> file &&
+	 git add file &&
+	 test_tick &&
+	 git commit -m "upstream line4a" &&
+	 git checkout -b test-branch &&
+	 test_commit on-test-branch
+	) &&
+	(cd super &&
+	 git submodule update --remote --force submodule &&
+	 (cd submodule &&
+	  test "$(git log -1 --oneline)" = "$(GIT_DIR=../../submodule/.git git log -1 --oneline master)"
+	 ) &&
 	 git checkout -b test-branch &&
+	 git submodule update --remote --force submodule &&
+	 (cd submodule &&
+	  test "$(git log -1 --oneline)" = "$(GIT_DIR=../../submodule/.git git log -1 --oneline test-branch)"
+	 ) &&
+	 git checkout master &&
+	 git branch -d test-branch
+	)
+'
+
+test_expect_success 'branch = . does not confuse the rest of update' '
+	(cd super &&
+	 git checkout --detach &&
+	 # update is not confused by branch="." even if the the superproject
+	 # is not on any branch currently
+	 git submodule update &&
+	 git revert HEAD &&
+	 git checkout master
+	)
+'
+
+test_expect_success 'local config should override .gitmodules branch' '
+	(cd submodule &&
+	 git checkout test-branch &&
 	 echo line5 >> file &&
 	 git add file &&
 	 test_tick &&
-- 
2.9.2.468.g3d9025b

Re: [PATCHv2 2/2] submodule update: allow '.' for branch value

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Gerrit has a "superproject subscription" feature[1], that triggers a
> commit in a superproject that is subscribed to its submodules.
> Conceptually this Gerrit feature can be done on the client side with
> Git via:
>
>     git -C <superproject> submodule update --remote --force
>     git -C <superproject> commit -a -m "Update submodules"
>     git -C <superproject> push
>
> for each branch in the superproject.

Which I imagine would be useful if you only have one submodule.  If
you work on submodules A and B and then want to give the updated
superproject that binds the latest in both A and B as an atomic
update, the three lines above would not quite work, no?

> To ease the configuration in Gerrit
> a special value of "." has been introduced for the submodule.<name>.branch
> to mean the same branch as the superproject[2], such that you can create a
> new branch on both superproject and the submodule and this feature
> continues to work on that new branch.
>
> Now we have find projects in the wild with such a .gitmodules file.

That's annoying.

> To have Git working well with these, we imitate the behavior and
> look up the superprojects branch name if the submodules branch is
> configured to ".". In projects that do not use Gerrit, this value
> whould be never configured as "." is not a valid branch name.

I find that the last sentence is somewhat misleading.  I agree it is
justifiable that using "." as the name to trigger a new (to us)
feature is safe, because such a setting wouldn't have meant anything
useful without this change, but I initially misread it and thought
you added "are we using Gerrit?  Error out if we are not" logic,
which is not the case here.

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 4ec7546..1eb33ad 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -590,7 +590,6 @@ cmd_update()
>  
>  		name=$(git submodule--helper name "$sm_path") || exit
>  		url=$(git config submodule."$name".url)
> -		branch=$(get_submodule_config "$name" branch master)
>  		if ! test -z "$update"
>  		then
>  			update_module=$update
> @@ -616,6 +615,14 @@ cmd_update()
>  
>  		if test -n "$remote"
>  		then
> +			branch=$(get_submodule_config "$name" branch master)
> +			if test "$branch" = "."
> +			then
> +				if ! branch=$(git symbolic-ref --short -q HEAD)
> +				then
> +					die "$(eval_gettext "submodule branch configured to inherit branch from superproject, but it's not on any branch")"
> +				fi
> +			fi

I see that you narrowed the scope of "$branch" (which is only used
when $remote exists), but it is a bit annoying to see that change
mixed with "now a dot means something different" change.

I wonder if the above 8-line block wants to be encapsulated to
become a part of "git submodule--helper" interface, though.  IOW,
branch=$(git submodule--helper branch "$name") or something?

> +test_expect_success 'submodule update --remote should fetch upstream changes with .' '
> +	(cd super &&
> +	 git config -f .gitmodules submodule."submodule".branch "." &&
> +	 git add .gitmodules &&
> +	 git commit -m "submodules: update from the respective superproject branch"
> +	) &&
>  	(cd submodule &&
> +	 echo line4a >> file &&
> +	 git add file &&
> +	 test_tick &&
> +	 git commit -m "upstream line4a" &&
> +	 git checkout -b test-branch &&
> +	 test_commit on-test-branch
> +	) &&
> +	(cd super &&
> +	 git submodule update --remote --force submodule &&
> +	 (cd submodule &&
> +	  test "$(git log -1 --oneline)" = "$(GIT_DIR=../../submodule/.git git log -1 --oneline master)"

A few issues:

  * A crash in "git log" would not be noticed with this.  Perhaps

	git log -1 --oneline $one_way_to_invoke >expect &&
        git log -1 --oneline $the_other_way >actual &&
        test_cmp expect actual

    would be better?

  * What exactly is this testing?  The current branch (in submodule)
    pointing at the same commit as the tip of 'master'?  Or the
    current branch _is_ 'master'?

  * What exactly is the reason why one has GIT_DIR=... and the other
    does not?  I do not think this a place to test that "gitdir: "
    in .git points at the right place, so it must be testing
    something else, but I cannot guess.

> +	 ) &&
>  	 git checkout -b test-branch &&
> +	 git submodule update --remote --force submodule &&
> +	 (cd submodule &&
> +	  test "$(git log -1 --oneline)" = "$(GIT_DIR=../../submodule/.git git log -1 --oneline test-branch)"
> +	 ) &&
> +	 git checkout master &&
> +	 git branch -d test-branch
> +	)
> +'
> +
> +test_expect_success 'branch = . does not confuse the rest of update' '
> +	(cd super &&
> +	 git checkout --detach &&
> +	 # update is not confused by branch="." even if the the superproject
> +	 # is not on any branch currently
> +	 git submodule update &&
> +	 git revert HEAD &&

"revert" is rather unusual thing to see in the test.  Also I am not
sure why cmd_update that now has an explicit check to die when
branch is set to "." and the head is detached is expected "not" to
be confused.  Perhaps I misread the main part of the patch?

Puzzled.

> +	 git checkout master
> +	)
> +'
> +
> +test_expect_success 'local config should override .gitmodules branch' '
> +	(cd submodule &&
> +	 git checkout test-branch &&
>  	 echo line5 >> file &&
>  	 git add file &&
>  	 test_tick &&

Re: [PATCHv2 2/2] submodule update: allow '.' for branch value

[Stefan Beller]

On Thu, Jul 28, 2016 at 12:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Gerrit has a "superproject subscription" feature[1], that triggers a
>> commit in a superproject that is subscribed to its submodules.
>> Conceptually this Gerrit feature can be done on the client side with
>> Git via:
>>
>>     git -C <superproject> submodule update --remote --force
>>     git -C <superproject> commit -a -m "Update submodules"
>>     git -C <superproject> push
>>
>> for each branch in the superproject.
>
> Which I imagine would be useful if you only have one submodule.  If
> you work on submodules A and B and then want to give the updated
> superproject that binds the latest in both A and B as an atomic
> update, the three lines above would not quite work, no?

When using Gerrit you can submit A,B together bound by a "topic"
and that will trigger a superproject update that contains one
atomic commit updating A and B at the same time.

To fully emulate that Gerrit feature you would need to put
the 3 lines above in an infinite loop that polls the remote
submodules all the time.

If you wanted to do that without that Gerrit feature,
this becomes racy as "submodule update --remote"
fetches the submodules racily (by design) and it may end
up putting A and B in the same commit or not.


>
>> To ease the configuration in Gerrit
>> a special value of "." has been introduced for the submodule.<name>.branch
>> to mean the same branch as the superproject[2], such that you can create a
>> new branch on both superproject and the submodule and this feature
>> continues to work on that new branch.
>>
>> Now we have find projects in the wild with such a .gitmodules file.

I'll fix the typo. (delete have or find)

>
> That's annoying.
>
>> To have Git working well with these, we imitate the behavior and
>> look up the superprojects branch name if the submodules branch is
>> configured to ".". In projects that do not use Gerrit, this value
>> whould be never configured as "." is not a valid branch name.
>
> I find that the last sentence is somewhat misleading.  I agree it is
> justifiable that using "." as the name to trigger a new (to us)
> feature is safe, because such a setting wouldn't have meant anything
> useful without this change, but I initially misread it and thought
> you added "are we using Gerrit?  Error out if we are not" logic,
> which is not the case here.

Well I wanted to express:

  The .gitmodules used in these Gerrit projects do not conform
  to Gits understanding of how .gitmodules should look like.
  Let's make Git understand this Gerrit corner case (which you
  would only need when using Gerrit).

  Adding special treatment of the "." value is safe as this is an
  invalid branch name in Git.


>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 4ec7546..1eb33ad 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -590,7 +590,6 @@ cmd_update()
>>
>>               name=$(git submodule--helper name "$sm_path") || exit
>>               url=$(git config submodule."$name".url)
>> -             branch=$(get_submodule_config "$name" branch master)
>>               if ! test -z "$update"
>>               then
>>                       update_module=$update
>> @@ -616,6 +615,14 @@ cmd_update()
>>
>>               if test -n "$remote"
>>               then
>> +                     branch=$(get_submodule_config "$name" branch master)
>> +                     if test "$branch" = "."
>> +                     then
>> +                             if ! branch=$(git symbolic-ref --short -q HEAD)
>> +                             then
>> +                                     die "$(eval_gettext "submodule branch configured to inherit branch from superproject, but it's not on any branch")"
>> +                             fi
>> +                     fi
>
> I see that you narrowed the scope of "$branch" (which is only used
> when $remote exists), but it is a bit annoying to see that change
> mixed with "now a dot means something different" change.
>
> I wonder if the above 8-line block wants to be encapsulated to
> become a part of "git submodule--helper" interface, though.  IOW,
> branch=$(git submodule--helper branch "$name") or something?

There is already get_submodule_config that we implement in shell,
which is also used in cmd_summary for reading the .ignore
field.

So having the "." check in that method (whether in shell or in C)
doesn't make sense to me.

We could of course have our own method specific for branches,
that take the "." into account. I think we can go with that.

>
>> +test_expect_success 'submodule update --remote should fetch upstream changes with .' '
>> +     (cd super &&
>> +      git config -f .gitmodules submodule."submodule".branch "." &&
>> +      git add .gitmodules &&
>> +      git commit -m "submodules: update from the respective superproject branch"
>> +     ) &&
>>       (cd submodule &&
>> +      echo line4a >> file &&
>> +      git add file &&
>> +      test_tick &&
>> +      git commit -m "upstream line4a" &&
>> +      git checkout -b test-branch &&
>> +      test_commit on-test-branch
>> +     ) &&
>> +     (cd super &&
>> +      git submodule update --remote --force submodule &&
>> +      (cd submodule &&
>> +       test "$(git log -1 --oneline)" = "$(GIT_DIR=../../submodule/.git git log -1 --oneline master)"
>
> A few issues:
>
>   * A crash in "git log" would not be noticed with this.  Perhaps
>
>         git log -1 --oneline $one_way_to_invoke >expect &&
>         git log -1 --oneline $the_other_way >actual &&
>         test_cmp expect actual
>
>     would be better?

Of course. I tried to blend in with the code after looking at the surrounding
code. Maybe I need to modernize that whole test file as a preparatory step.

>
>   * What exactly is this testing?  The current branch (in submodule)
>     pointing at the same commit as the tip of 'master'?  Or the
>     current branch _is_ 'master'?
>
>   * What exactly is the reason why one has GIT_DIR=... and the other
>     does not?  I do not think this a place to test that "gitdir: "
>     in .git points at the right place, so it must be testing
>     something else, but I cannot guess.

It is testing that the local state is at the same commit as the
master branch on the remote side.
(GIT_DIR=../../submodule/.git is saying to be "remote", and "master"
to check that branch. We need to have master here as we configured
"." and have the master branch checked out in the superproject.)

At least that is my understanding from the existing tests for
the  "--remote" flag

>> +     (cd super &&
>> +      git checkout --detach &&
>> +      # update is not confused by branch="." even if the the superproject
>> +      # is not on any branch currently
>> +      git submodule update &&
>> +      git revert HEAD &&
>
> "revert" is rather unusual thing to see in the test.

The tests are so long that I tried to get back in a state that is as least
different from before to not break the following tests.
(And except for the depth this worked well)

> Also I am not
> sure why cmd_update that now has an explicit check to die when
> branch is set to "." and the head is detached is expected "not" to
> be confused.  Perhaps I misread the main part of the patch?

Well you *only* explicitly die(..) when you ask for --remote.
If the superproject is on a detached HEAD and just wants the
sha1s as recorded (--no-remote), it doesn't care about the recorded
branch value and should *not* die.

Re: [PATCHv2 2/2] submodule update: allow '.' for branch value

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Well I wanted to express:
>
>   The .gitmodules used in these Gerrit projects do not conform
>   to Gits understanding of how .gitmodules should look like.
>   Let's make Git understand this Gerrit corner case (which you
>   would only need when using Gerrit).
>
>   Adding special treatment of the "." value is safe as this is an
>   invalid branch name in Git.

Yup, I got it after reading it twice.  My point was that you
shouldn't have to read it twice to get it.

>> I wonder if the above 8-line block wants to be encapsulated to
>> become a part of "git submodule--helper" interface, though.  IOW,
>> branch=$(git submodule--helper branch "$name") or something?
>
> There is already get_submodule_config that we implement in shell,
> which is also used in cmd_summary for reading the .ignore
> field.
>
> So having the "." check in that method (whether in shell or in C)
> doesn't make sense to me.

That's an excuse from the helper implementor's side, isn't it?  I
was coming from the opposite direction, i.e. potential caller of a
helper.  Whenever I want to know "is there a branch configured for
this submodule, and if so what is it?", wouldn't I be entitled to a
helper that consistently gets the real branch name with the magic
"." resolved for me?

>>> +       test "$(git log -1 --oneline)" = "$(GIT_DIR=../../submodule/.git git log -1 --oneline master)"
>>
>> A few issues:
>>
>>   * A crash in "git log" would not be noticed with this.  Perhaps
>>
>>         git log -1 --oneline $one_way_to_invoke >expect &&
>>         git log -1 --oneline $the_other_way >actual &&
>>         test_cmp expect actual
>>
>>     would be better?
>
> Of course. I tried to blend in with the code after looking at the surrounding
> code. Maybe I need to modernize that whole test file as a preparatory step.
>>
>>   * What exactly is this testing?  The current branch (in submodule)
>>     pointing at the same commit as the tip of 'master'?  Or the
>>     current branch _is_ 'master'?
>>
>>   * What exactly is the reason why one has GIT_DIR=... and the other
>>     does not?  I do not think this a place to test that "gitdir: "
>>     in .git points at the right place, so it must be testing
>>     something else, but I cannot guess.
>
> It is testing that the local state is at the same commit as the
> master branch on the remote side.

Ahh, OK, I totally misread that.  "git -C ../../submodule log" would
have been the more modern way to say that, I would guess, but now it
makes sense.

>>> +      # update is not confused by branch="." even if the the superproject
>>> +      # is not on any branch currently
>>> +      git submodule update &&
>>> +      git revert HEAD &&
>>
>> "revert" is rather unusual thing to see in the test.
>
> The tests are so long that I tried to get back in a state that is as least
> different from before to not break the following tests.

I guessed that much; I just expected to see "git reset --hard
$some_old_state" if you want to rewind to the previous state the
next test expects and "revert" looked unusual.

>> Also I am not
>> sure why cmd_update that now has an explicit check to die when
>> branch is set to "." and the head is detached is expected "not" to
>> be confused.  Perhaps I misread the main part of the patch?
>
> Well you *only* explicitly die(..) when you ask for --remote.

OK, I _did_ misread the patch, then.  It would help to have "when
giving no --remote, git submodule" before the comment that begins
with "update is not confused" to avoid the same confusion.

Thanks.