[PATCH v3 0/3] add test to demonstrate that shallow recursive clones fail

[PATCH v3 0/3] add test to demonstrate that shallow recursive clones fail

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

diff to v2:
* remove workaround tests as suggested by Peff [1]
* fix chain breakage introduced in 275cd18
* add hints to the user if a submodule checkout fails while using the
  depth argument [2]

Thanks,
Lars

[1] http://article.gmane.org/gmane.comp.version-control.git/281237
[2] http://article.gmane.org/gmane.comp.version-control.git/281420

Lars Schneider (3):
  submodule: add test to demonstrate that shallow recursive clones fail
  submodule: fix &&-chain breakage
  submodule: extend die message on failed checkout with depth argument

 git-submodule.sh               |  4 ++++
 t/t7406-submodule-update.sh    | 35 +++++++++++++++++++++++++---
 t/t7412-submodule-recursive.sh | 52 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 3 deletions(-)
 create mode 100755 t/t7412-submodule-recursive.sh

--
2.5.1

[PATCH v3 1/3] submodule: add test to demonstrate that shallow recursive clones fail

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

"git clone --recursive --depth 1 --single-branch <url>" clones the
submodules successfully. However, it does not obey "--depth 1" for
submodule cloning.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/t7412-submodule-recursive.sh | 52 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100755 t/t7412-submodule-recursive.sh

diff --git a/t/t7412-submodule-recursive.sh b/t/t7412-submodule-recursive.sh
new file mode 100755
index 0000000..58b3eb9
--- /dev/null
+++ b/t/t7412-submodule-recursive.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+
+test_description='Test shallow cloning of repos with submodules'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	git checkout -b master &&
+	echo file >file &&
+	git add file &&
+	test_tick &&
+	git commit -m "master commit 1" &&
+
+	git checkout -b branch &&
+	echo file >branch-file &&
+	git add branch-file &&
+	test_tick &&
+	git commit -m "branch commit 1" &&
+
+	git checkout master &&
+	git clone . repo &&
+	(
+		cd repo &&
+		git checkout master &&
+		git submodule add ../. submodule &&
+		(
+			cd submodule &&
+			git checkout branch
+		) &&
+		git add submodule &&
+		test_tick &&
+		git commit -m "master commit 2"
+	)
+'
+
+test_expect_failure shallow-clone-recursive '
+	URL="file://$(pwd | sed "s/[[:space:]]/%20/g")/repo" &&
+	echo $URL &&
+	git clone --recursive --depth 1 --single-branch $URL clone-recursive &&
+	(
+		cd "clone-recursive" &&
+		git log --oneline >lines &&
+		test_line_count = 1 lines
+	) &&
+	(
+		cd "clone-recursive/submodule" &&
+		git log --oneline >lines &&
+		test_line_count = 1 lines
+	)
+'
+
+test_done
-- 
2.5.1

[PATCH v3 2/3] submodule: fix &&-chain breakage

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

---
 t/t7406-submodule-update.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index dda3929..b5bd976 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -758,7 +758,7 @@ 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=3 &&
 	 (cd submodule &&
 	  test 1 = $(git log --oneline | wc -l)
 	 )
-- 
2.5.1

[PATCH v3 3/3] submodule: extend die message on failed checkout with depth argument

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

A submodule checkout might fail due missing hashes caused by a shallow
fetch operation triggered with the "--depth" argument. Make the user
aware of this with an extended die message.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 git-submodule.sh            |  4 ++++
 t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9bc5c5f..b555d87 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -764,6 +764,10 @@ Maybe you want to use 'update --init'?")"
 				command="git checkout $subforce -q"
 				die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule path '\$displaypath'")"
 				say_msg="$(eval_gettext "Submodule path '\$displaypath': checked out '\$sha1'")"
+				if ! test -z "$depth"
+				then
+					die_msg="$die_msg $(eval_gettext "Commit is probably not on the default branch. Try to remove the '\$depth' argument!")"
+				fi
 				;;
 			rebase)
 				command="git rebase"
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index b5bd976..52b7120 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -31,6 +31,14 @@ test_expect_success 'setup a submodule tree' '
 	git clone super rebasing &&
 	git clone super merging &&
 	git clone super none &&
+	git clone super non-default-branch &&
+	(cd non-default-branch &&
+	 git checkout -b non-default &&
+	 echo "non-default" >file &&
+	 git add file &&
+	 git commit -m "Commit on non-default branch" &&
+	 git checkout master
+	) &&
 	(cd super &&
 	 git submodule add ../submodule submodule &&
 	 test_tick &&
@@ -63,6 +71,15 @@ test_expect_success 'setup a submodule tree' '
 	 git submodule add ../none none &&
 	 test_tick &&
 	 git commit -m "none"
+	) &&
+	(cd super &&
+	 git submodule add ../non-default-branch non-default-branch &&
+	 (cd non-default-branch &&
+	  git checkout non-default
+	 ) &&
+     git add non-default-branch &&
+	 test_tick &&
+	 git commit -m "non-default-branch"
 	)
 '
 
@@ -752,17 +769,29 @@ test_expect_success SYMLINKS 'submodule update can handle symbolic links in pwd'
 	)
 '
 
-test_expect_success 'submodule update clone shallow submodule' '
+test_expect_success 'submodule update clone shallow submodule on default branch' '
 	git clone cloned super3 &&
 	pwd=$(pwd) &&
 	(cd super3 &&
 	 sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
 	 mv -f .gitmodules.tmp .gitmodules &&
+	 git config submodule.non-default-branch.update none &&
 	 git submodule update --init --depth=3 &&
 	 (cd submodule &&
 	  test 1 = $(git log --oneline | wc -l)
 	 )
-)
+	)
+'
+
+test_expect_success 'submodule update clone shallow submodule on non-default branch' '
+	git clone cloned super4 &&
+	pwd=$(pwd) &&
+	(cd super4 &&
+	 sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
+	 mv -f .gitmodules.tmp .gitmodules &&
+	 test_must_fail git submodule update --init --depth=3 2>submodule.out &&
+	 test_i18ngrep --count "Commit is probably not on the default branch." submodule.out
+    )
 '
 
 test_expect_success 'submodule update --recursive drops module name before recursing' '
-- 
2.5.1

Re: [PATCH v3 0/3] add test to demonstrate that shallow recursive clones fail

[Lars Schneider]

Hi,

does anyone have a few free cycles to take a look at this patch series?
I wonder if you deem it as not interesting or if it got lost.

Thank you,
Lars


On 21 Dec 2015, at 00:19, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> diff to v2:
> * remove workaround tests as suggested by Peff [1]
> * fix chain breakage introduced in 275cd18
> * add hints to the user if a submodule checkout fails while using the
>  depth argument [2]
> 
> Thanks,
> Lars
> 
> [1] http://article.gmane.org/gmane.comp.version-control.git/281237
> [2] http://article.gmane.org/gmane.comp.version-control.git/281420
> 
> Lars Schneider (3):
>  submodule: add test to demonstrate that shallow recursive clones fail
>  submodule: fix &&-chain breakage
>  submodule: extend die message on failed checkout with depth argument
> 
> git-submodule.sh               |  4 ++++
> t/t7406-submodule-update.sh    | 35 +++++++++++++++++++++++++---
> t/t7412-submodule-recursive.sh | 52 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 88 insertions(+), 3 deletions(-)
> create mode 100755 t/t7412-submodule-recursive.sh
> 
> --
> 2.5.1
> 

Re: [PATCH v3 3/3] submodule: extend die message on failed checkout with depth argument

[Stefan Beller]

On Sun, Dec 20, 2015 at 3:19 PM,  <larsxschneider@gmail.com> wrote:

> +test_expect_success 'submodule update clone shallow submodule on non-default branch' '
> +       git clone cloned super4 &&
> +       pwd=$(pwd) &&
> +       (cd super4 &&
> +        sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
> +        mv -f .gitmodules.tmp .gitmodules &&
> +        test_must_fail git submodule update --init --depth=3 2>submodule.out &&

Why do we choose a depth of 3 here?

> +        test_i18ngrep --count "Commit is probably not on the default branch." submodule.out
> +    )
>  '
>
>  test_expect_success 'submodule update --recursive drops module name before recursing' '
> --
> 2.5.1
>

Re: [PATCH v3 0/3] add test to demonstrate that shallow recursive clones fail

[Stefan Beller]

On Thu, Jan 7, 2016 at 1:50 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
> Hi,
>
> does anyone have a few free cycles to take a look at this patch series?
> I wonder if you deem it as not interesting or if it got lost.

Patch 1&2 look good to me.

>
> Thank you,
> Lars
>
>
> On 21 Dec 2015, at 00:19, larsxschneider@gmail.com wrote:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> diff to v2:
>> * remove workaround tests as suggested by Peff [1]
>> * fix chain breakage introduced in 275cd18
>> * add hints to the user if a submodule checkout fails while using the
>>  depth argument [2]
>>
>> Thanks,
>> Lars
>>
>> [1] http://article.gmane.org/gmane.comp.version-control.git/281237
>> [2] http://article.gmane.org/gmane.comp.version-control.git/281420
>>
>> Lars Schneider (3):
>>  submodule: add test to demonstrate that shallow recursive clones fail
>>  submodule: fix &&-chain breakage
>>  submodule: extend die message on failed checkout with depth argument
>>
>> git-submodule.sh               |  4 ++++
>> t/t7406-submodule-update.sh    | 35 +++++++++++++++++++++++++---
>> t/t7412-submodule-recursive.sh | 52 ++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 88 insertions(+), 3 deletions(-)
>> create mode 100755 t/t7412-submodule-recursive.sh
>>
>> --
>> 2.5.1
>>
>

Re: [PATCH v3 3/3] submodule: extend die message on failed checkout with depth argument

[Lars Schneider]

On 11 Jan 2016, at 17:26, Stefan Beller <sbeller@google.com> wrote:

> On Sun, Dec 20, 2015 at 3:19 PM,  <larsxschneider@gmail.com> wrote:
> 
>> +test_expect_success 'submodule update clone shallow submodule on non-default branch' '
>> +       git clone cloned super4 &&
>> +       pwd=$(pwd) &&
>> +       (cd super4 &&
>> +        sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
>> +        mv -f .gitmodules.tmp .gitmodules &&
>> +        test_must_fail git submodule update --init --depth=3 2>submodule.out &&
> 
> Why do we choose a depth of 3 here?
I just copied this code from the test above. Let's change that to "--depth=1" to avoid confusion.
Should I reroll or can this be fixed in the merge?

> 
>> +        test_i18ngrep --count "Commit is probably not on the default branch." submodule.out
I was just reading this and wonder if we should remove "--count". My initial intention was to
make sure this error message only happens once. However, this is not checked here anyways.

>> +    )
>> '
>> 
>> test_expect_success 'submodule update --recursive drops module name before recursing' '


Thanks,
Lars

Re: [PATCH v3 3/3] submodule: extend die message on failed checkout with depth argument

[Stefan Beller]

On Tue, Jan 12, 2016 at 1:29 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
> On 11 Jan 2016, at 17:26, Stefan Beller <sbeller@google.com> wrote:
>
>> On Sun, Dec 20, 2015 at 3:19 PM,  <larsxschneider@gmail.com> wrote:
>>
>>> +test_expect_success 'submodule update clone shallow submodule on non-default branch' '
>>> +       git clone cloned super4 &&
>>> +       pwd=$(pwd) &&
>>> +       (cd super4 &&
>>> +        sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
>>> +        mv -f .gitmodules.tmp .gitmodules &&
>>> +        test_must_fail git submodule update --init --depth=3 2>submodule.out &&
>>
>> Why do we choose a depth of 3 here?
> I just copied this code from the test above. Let's change that to "--depth=1" to avoid confusion.
> Should I reroll or can this be fixed in the merge?

I noticed that too after reviewing 1&2 that 3 seems to be the standard
depth in the tests,
so I'd think 3 is fine here, too. Which makes it not worth to reroll,
so I'd be fine with it as is.

>
>>
>>> +        test_i18ngrep --count "Commit is probably not on the default branch." submodule.out
> I was just reading this and wonder if we should remove "--count". My initial intention was to
> make sure this error message only happens once. However, this is not checked here anyways.
>
>>> +    )
>>> '
>>>
>>> test_expect_success 'submodule update --recursive drops module name before recursing' '
>
>
> Thanks,
> Lars

Re: [PATCH v3 1/3] submodule: add test to demonstrate that shallow recursive clones fail

[Stefan Beller]

On Sun, Dec 20, 2015 at 3:19 PM,  <larsxschneider@gmail.com> wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> "git clone --recursive --depth 1 --single-branch <url>" clones the
> submodules successfully. However, it does not obey "--depth 1" for
> submodule cloning.

I am about to resend "[RFC/PATCH] clone: add `--shallow-submodules` flag"
which would need tests and I thought about this series as tests.

I assume patch 2 (fixing a broken && chain in tests) made it through,
but patch 1 and 3 did not? I may pick up ideas from here as it will be
slightly different tests I'd guess.


> +test_expect_failure shallow-clone-recursive '
> +       URL="file://$(pwd | sed "s/[[:space:]]/%20/g")/repo" &&

This would break if the test suite is in a path containing any other white space
than U+0020 such as a tab? (Not that I am encouraging using such paths...)

Thanks,
Stefan


> +       echo $URL &&
> +       git clone --recursive --depth 1 --single-branch $URL clone-recursive &&
> +       (
> +               cd "clone-recursive" &&
> +               git log --oneline >lines &&
> +               test_line_count = 1 lines
> +       ) &&
> +       (
> +               cd "clone-recursive/submodule" &&
> +               git log --oneline >lines &&
> +               test_line_count = 1 lines
> +       )
> +'
> +
> +test_done
> --
> 2.5.1
>

Re: [PATCH v3 1/3] submodule: add test to demonstrate that shallow recursive clones fail

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> +test_expect_failure shallow-clone-recursive '
>> +       URL="file://$(pwd | sed "s/[[:space:]]/%20/g")/repo" &&
>
> This would break if the test suite is in a path containing any other white space
> than U+0020 such as a tab? (Not that I am encouraging using such paths...)

Good spotting.

I thought use of [[:named character classes:]] is explicitly
disallowed by CodingGuidelines.

Re: [PATCH v3 1/3] submodule: add test to demonstrate that shallow recursive clones fail

[Lars Schneider]

On 15 Mar 2016, at 20:50, Stefan Beller <sbeller@google.com> wrote:

> On Sun, Dec 20, 2015 at 3:19 PM,  <larsxschneider@gmail.com> wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> "git clone --recursive --depth 1 --single-branch <url>" clones the
>> submodules successfully. However, it does not obey "--depth 1" for
>> submodule cloning.
> 
> I am about to resend "[RFC/PATCH] clone: add `--shallow-submodules` flag"
> which would need tests and I thought about this series as tests.
Sounds good, thanks!


> I assume patch 2 (fixing a broken && chain in tests) made it through,
> but patch 1 and 3 did not? I may pick up ideas from here as it will be
> slightly different tests I'd guess.
Unfortunately patch 2 ($gmane/282776) did not made it in (I just checked 
next 104e649). How should we proceed?

> 
> 
>> +test_expect_failure shallow-clone-recursive '
>> +       URL="file://$(pwd | sed "s/[[:space:]]/%20/g")/repo" &&
> 
> This would break if the test suite is in a path containing any other white space
> than U+0020 such as a tab? (Not that I am encouraging using such paths...)
True. I wonder if we really need to use the "file://" URL format here. Maybe
a relative would be easier?!


Thanks,
Lars

Re: [PATCH v3 1/3] submodule: add test to demonstrate that shallow recursive clones fail

[Jeff King]

On Sun, Mar 20, 2016 at 06:05:34PM +0100, Lars Schneider wrote:

> >> +test_expect_failure shallow-clone-recursive '
> >> +       URL="file://$(pwd | sed "s/[[:space:]]/%20/g")/repo" &&
> > 
> > This would break if the test suite is in a path containing any other white space
> > than U+0020 such as a tab? (Not that I am encouraging using such paths...)
> True. I wonder if we really need to use the "file://" URL format here. Maybe
> a relative would be easier?!

I did not look closely at the patch or the tests, but generally we skip
shallow-cloning for local-path repositories, as we don't do a regular
object transfer at all (and we turn that optimization off for file://
URLs). So presumably it would defeat the purpose of the test.

-Peff

Re: [PATCH v3 1/3] submodule: add test to demonstrate that shallow recursive clones fail

[Stefan Beller]

On 20.03.2016 12:40, Jeff King wrote:
> On Sun, Mar 20, 2016 at 06:05:34PM +0100, Lars Schneider wrote:
> 
>>>> +test_expect_failure shallow-clone-recursive '
>>>> +       URL="file://$(pwd | sed "s/[[:space:]]/%20/g")/repo" &&
>>>
>>> This would break if the test suite is in a path containing any other white space
>>> than U+0020 such as a tab? (Not that I am encouraging using such paths...)
>> True. I wonder if we really need to use the "file://" URL format here. Maybe
>> a relative would be easier?!
> 
> I did not look closely at the patch or the tests, but generally we skip
> shallow-cloning for local-path repositories, as we don't do a regular
> object transfer at all (and we turn that optimization off for file://
> URLs). So presumably it would defeat the purpose of the test.
> 
> -Peff

Please see proposed patches at $gmane/288940

By passing along the --[no-]local option we can have tests, without
using file:// explicitely.

Thanks,
Stefan