threads / patch / 41048

v3, 3 partssubmodule: fix &&-chain breakage

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

## tl;dr

14 messages between Dec 20, 2015 and Mar 20, 2016. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

larsxschneider@gmail.com· Dec 20, 2015, 23:19 UTC · lore

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

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

larsxschneider@gmail.com· Dec 20, 2015, 23:19 UTC · re: larsxschneider@gmail.com · lore

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

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
Show changes to t/t7412-submodule-recursive.sh +52 −0
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
Stefan Beller· Mar 15, 2016, 19:50 UTC · re: larsxschneider@gmail.com · lore

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

On Sun, Dec 20, 2015 at 3:19 PM,  <larsxschneider@gmail.com> wrote:
Show 5 quoted lines
> 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

Show 18 quoted lines
> +       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
>
Junio C Hamano· Mar 15, 2016, 20:12 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 5 quoted lines
>> +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.

Lars Schneider· Mar 20, 2016, 17:05 UTC · re: Stefan Beller · lore

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

On 15 Mar 2016, at 20:50, Stefan Beller <sbeller@google.com> wrote:
Show 9 quoted lines
> 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?

Show 7 quoted lines
> 
> 
>> +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

Jeff King· Mar 20, 2016, 19:40 UTC · re: Lars Schneider · lore

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

On Sun, Mar 20, 2016 at 06:05:34PM +0100, Lars Schneider wrote:
Show 7 quoted lines
> >> +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
Stefan Beller· Mar 20, 2016, 21:27 UTC · re: Jeff King · lore

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

On 20.03.2016 12:40, Jeff King wrote:
Show 16 quoted lines
> 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

larsxschneider@gmail.com· Dec 20, 2015, 23:19 UTC · re: larsxschneider@gmail.com · lore
From: Lars Schneider <larsxschneider@gmail.com>
---
 t/t7406-submodule-update.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t7406-submodule-update.sh +1 −1
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
larsxschneider@gmail.com· Dec 20, 2015, 23:19 UTC · re: larsxschneider@gmail.com · lore

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

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(-)
Show changes to 2 files +35 −2

git-submodule.sh, t/t7406-submodule-update.sh

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
Stefan Beller· Jan 11, 2016, 16:26 UTC · re: larsxschneider@gmail.com · lore

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

On Sun, Dec 20, 2015 at 3:19 PM,  <larsxschneider@gmail.com> wrote:
Show 7 quoted lines
> +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?
Show 8 quoted lines
> +        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
>
Lars Schneider· Jan 12, 2016, 09:29 UTC · re: Stefan Beller · lore

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

On 11 Jan 2016, at 17:26, Stefan Beller <sbeller@google.com> wrote:
Show 11 quoted lines
> 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

Stefan Beller· Jan 12, 2016, 23:32 UTC · re: Lars Schneider · lore

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

On Tue, Jan 12, 2016 at 1:29 AM, Lars Schneider <larsxschneider@gmail.com> wrote:

Show 16 quoted lines
>
> 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.

Show 14 quoted lines
>
>>
>>> +        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
Lars Schneider· Jan 7, 2016, 21:50 UTC · re: larsxschneider@gmail.com · lore

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

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:
Show 28 quoted lines
> 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
>
Stefan Beller· Jan 11, 2016, 16:29 UTC · re: Lars Schneider · lore

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

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.
Show 36 quoted lines
>
> 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
>>
>

← back to recent threads