threads / patch / 26535

v2, 2 partssubmodule: no [--merge|--rebase] when newly cloned

Subject: [PATCH 1/2 (v2)] submodule: no [--merge|--rebase] when newly cloned

## tl;dr

11 messages between Feb 17, 2011 and Feb 17, 2011. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Spencer E. Olson· Feb 17, 2011, 16:18 UTC · lore

Previously, when a new submodule is cloned by running "git submodule update [--merge|--rebase]", the newly cloned submodule does not get checked out and a rebase or merge is incorrectly attempted against an empty working directory. This patch ignores --rebase or --merge for new submodules and instead simply checks out the appropriate revision.

Signed-off-by: Spencer E. Olson <olsonse@umich.edu>
---
 git-submodule.sh |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

Show changes to git-submodule.sh +21 −0

diff --git a/git-submodule.sh b/git-submodule.sh
index 8b90589..7724885 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -365,6 +365,19 @@ cmd_init()
 }
 
 #
+# Test whether an element of the ';' separated list $2 matches $1
+#
+list_contains()
+{
+	case "$2;" in
+	*";$1;"*)
+		: yes ;;
+	*)
+		! : no ;;
+	esac
+}
+
+#
 # Update each submodule path to correct revision, using clone and checkout as needed
 #
 # $@ = requested paths (default to all)
@@ -423,6 +436,7 @@ cmd_update()
 		cmd_init "--" "$@" || return
 	fi
 
+	cloned_modules=
 	module_list "$@" |
 	while read mode sha1 stage path
 	do
@@ -442,6 +456,7 @@ cmd_update()
 		if ! test -d "$path"/.git -o -f "$path"/.git
 		then
 			module_clone "$path" "$url" "$reference"|| exit
+			cloned_modules="$cloned_modules;$name"
 			subsha1=
 		else
 			subsha1=$(clear_local_git_env; cd "$path" &&
@@ -469,6 +484,12 @@ cmd_update()
 				die "Unable to fetch in submodule path '$path'"
 			fi
 
+			list_contains "$name" "$cloned_modules"
+			if test "$?" = 0
+			then
+				update_module=
+			fi
+
 			case "$update_module" in
 			rebase)
 				command="git rebase"

-- 
1.7.4.1.42.g43f9f

Spencer E. Olson· Feb 17, 2011, 16:18 UTC · re: Spencer E. Olson · lore

[PATCH 2/2 (v2)] t7406: "git submodule update {--merge|--rebase]" with new submodules

This patch adds two new test cases in t7406 to ensure that the --merge/--rebase options are ignored for "git submodule update" with new modules. These test that a simple checkout is performed instead.

Signed-off-by: Spencer E. Olson <olsonse@umich.edu>
---
 t/t7406-submodule-update.sh |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

Show changes to t/t7406-submodule-update.sh +24 −0

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index bfb4975..141251c 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -203,4 +203,28 @@ test_expect_success 'submodule init picks up merge' '
 	)
 '
 
+test_expect_success 'submodule update --merge  - ignores --merge  for new submodules' '
+	(cd super &&
+	 rm -rf submodule &&
+	 git submodule update submodule &&
+	 git submodule status submodule >expect &&
+	 rm -rf submodule &&
+	 git submodule update --merge submodule &&
+	 git submodule status submodule >actual &&
+	 test_cmp expect actual
+	)
+'
+
+test_expect_success 'submodule update --rebase - ignores --rebase for new submodules' '
+	(cd super &&
+	 rm -rf submodule &&
+	 git submodule update submodule &&
+	 git submodule status submodule >expect &&
+	 rm -rf submodule &&
+	 git submodule update --rebase submodule &&
+	 git submodule status submodule >actual &&
+	 test_cmp expect actual
+	)
+'
+
 test_done

-- 
1.7.4.1.42.g43f9f

Jens Lehmann· Feb 17, 2011, 19:43 UTC · re: Spencer E. Olson · lore

Re: [PATCH 2/2 (v2)] t7406: "git submodule update {--merge|--rebase]" with new submodules

Am 17.02.2011 17:18, schrieb Spencer E. Olson:

Show 42 quoted lines

> This patch adds two new test cases in t7406 to ensure that the --merge/--rebase
> options are ignored for "git submodule update" with new modules.  These test
> that a simple checkout is performed instead.
> 
> Signed-off-by: Spencer E. Olson <olsonse@umich.edu>
> ---
>  t/t7406-submodule-update.sh |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index bfb4975..141251c 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -203,4 +203,28 @@ test_expect_success 'submodule init picks up merge' '
>  	)
>  '
>  
> +test_expect_success 'submodule update --merge  - ignores --merge  for new submodules' '
> +	(cd super &&
> +	 rm -rf submodule &&
> +	 git submodule update submodule &&
> +	 git submodule status submodule >expect &&
> +	 rm -rf submodule &&
> +	 git submodule update --merge submodule &&
> +	 git submodule status submodule >actual &&
> +	 test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'submodule update --rebase - ignores --rebase for new submodules' '
> +	(cd super &&
> +	 rm -rf submodule &&
> +	 git submodule update submodule &&
> +	 git submodule status submodule >expect &&
> +	 rm -rf submodule &&
> +	 git submodule update --rebase submodule &&
> +	 git submodule status submodule >actual &&
> +	 test_cmp expect actual
> +	)
> +'
> +
>  test_done

Thanks for writing these tests!

Unfortunately the first new test doesn't fail for me with current git. This should be fixed to be able to detect regressions in the future, so I came up with replacing the "git submodule status submodule" calls with "git status -s submodule", which did the trick for me.

Apart from that tests which cover the case when rebase or merge are configured via "submodule.<name>.update" would help, as that triggers the same problem.

So maybe you want to squash the following diff in, then feel free to add:
Acked-by: Jens Lehmann <Jens.Lehmann@web.de>

Show changes to t/t7406-submodule-update.sh +32 −4

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 141251c..fa9d23a 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -207,10 +207,10 @@ test_expect_success 'submodule update --merge  - ignores --merge  for new submod
 	(cd super &&
 	 rm -rf submodule &&
 	 git submodule update submodule &&
-	 git submodule status submodule >expect &&
+	 git status -s submodule >expect &&
 	 rm -rf submodule &&
 	 git submodule update --merge submodule &&
-	 git submodule status submodule >actual &&
+	 git status -s submodule >actual &&
 	 test_cmp expect actual
 	)
 '
@@ -219,10 +219,38 @@ test_expect_success 'submodule update --rebase - ignores --rebase for new submod
 	(cd super &&
 	 rm -rf submodule &&
 	 git submodule update submodule &&
-	 git submodule status submodule >expect &&
+	 git status -s submodule >expect &&
 	 rm -rf submodule &&
 	 git submodule update --rebase submodule &&
-	 git submodule status submodule >actual &&
+	 git status -s submodule >actual &&
+	 test_cmp expect actual
+	)
+'
+
+test_expect_success 'submodule update ignores update=merge config for new submodules' '
+	(cd super &&
+	 rm -rf submodule &&
+	 git submodule update submodule &&
+	 git status -s submodule >expect &&
+	 rm -rf submodule &&
+	 git config submodule.submodule.update merge &&
+	 git submodule update submodule &&
+	 git status -s submodule >actual &&
+	 git config --unset submodule.submodule.update &&
+	 test_cmp expect actual
+	)
+'
+
+test_expect_success 'submodule update ignores update=rebase config for new submodules' '
+	(cd super &&
+	 rm -rf submodule &&
+	 git submodule update submodule &&
+	 git status -s submodule >expect &&
+	 rm -rf submodule &&
+	 git config submodule.submodule.update rebase &&
+	 git submodule update submodule &&
+	 git status -s submodule >actual &&
+	 git config --unset submodule.submodule.update &&
 	 test_cmp expect actual
 	)
 '

-- 
1.7.4.1.109.g3d8f5

Junio C Hamano· Feb 17, 2011, 20:18 UTC · re: Jens Lehmann · lore

Re: [PATCH 2/2 (v2)] t7406: "git submodule update {--merge|--rebase]" with new submodules

Jens Lehmann <Jens.Lehmann@web.de> writes:

> This should be fixed to be able to detect regressions in the future, so
> I came up with replacing the "git submodule status submodule" calls with
> "git status -s submodule", which did the trick for me.

Hmph, is that a bug in "git submodule status" or is the subcommand deliberately designed to ignore this class of differences?

Jens Lehmann· Feb 17, 2011, 21:39 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2 (v2)] t7406: "git submodule update {--merge|--rebase]" with new submodules

Am 17.02.2011 21:18, schrieb Junio C Hamano:

Show 8 quoted lines

> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> This should be fixed to be able to detect regressions in the future, so
>> I came up with replacing the "git submodule status submodule" calls with
>> "git status -s submodule", which did the trick for me.
> 
> Hmph, is that a bug in "git submodule status" or is the subcommand
> deliberately designed to ignore this class of differences?

The latter, it doesn't care about working tree modifications inside of submodules as It uses the "--ignore-submodules=dirty" option when calling "git diff-files" (I added that in 18076502). The reason for one test to succeed and the other to fail is that the merge succeeds even though the work tree is empty while rebase errors out. So "git submodule status" is the wrong command to detect work tree differences, that's why we have to use "git status" here.

Jens Lehmann· Feb 17, 2011, 19:41 UTC · re: Spencer E. Olson · lore

Re: [PATCH 1/2 (v2)] submodule: no [--merge|--rebase] when newly cloned

Am 17.02.2011 17:18, schrieb Spencer E. Olson:

Show 5 quoted lines

> Previously, when a new submodule is cloned by running "git submodule update
> [--merge|--rebase]", the newly cloned submodule does not get checked out and a
> rebase or merge is incorrectly attempted against an empty working directory.
> This patch ignores --rebase or --merge for new submodules and instead simply
> checks out the appropriate revision.

Nice work, thanks!

Just a small thing: This problem also happens when the configuration "submodule.<name>.update" is set to either "rebase" or "merge", not only when using the command line options. So perhaps you could reword the topic to something like "submodule: don't merge or rebase when newly cloned" and adjust both commit messages a bit?

Apart from that:
Acked-by: Jens Lehmann <Jens.Lehmann@web.de>

Junio C Hamano· Feb 17, 2011, 20:25 UTC · re: Jens Lehmann · lore

Re: [PATCH 1/2 (v2)] submodule: no [--merge|--rebase] when newly cloned

Jens Lehmann <Jens.Lehmann@web.de> writes:

Show 14 quoted lines

> Am 17.02.2011 17:18, schrieb Spencer E. Olson:
>> Previously, when a new submodule is cloned by running "git submodule update
>> [--merge|--rebase]", the newly cloned submodule does not get checked out and a
>> rebase or merge is incorrectly attempted against an empty working directory.
>> This patch ignores --rebase or --merge for new submodules and instead simply
>> checks out the appropriate revision.
>
> Nice work, thanks!
>
> Just a small thing: This problem also happens when the configuration
> "submodule.<name>.update" is set to either "rebase" or "merge", not only
> when using the command line options. So perhaps you could reword the topic
> to something like "submodule: don't merge or rebase when newly cloned" and
> adjust both commit messages a bit?

I tentatively queued with this rewrite (the patch text is the same as the one I sent previously).

    From: Spencer E. Olson <olsonse@umich.edu>
    Date: Thu, 17 Feb 2011 09:18:45 -0700
    Subject: [PATCH] submodule: no [--merge|--rebase] when newly cloned

    "git submodule update" can be run with either the "--merge" or "--rebase"
    option, or submodule.<name>.update configuration variable can be set to
    "merge" or "rebase, to cause local work to get integrated when updating
    the submodule.

    When a submodule is newly cloned, however, it does not have a check out
    when a rebase or merge is attempted, leading to a failure.  For newly
    cloned submodules, simply check out the appropriate revision.  There is no
    local work to integrate with for them.

    Signed-off-by: Spencer E. Olson <olsonse@umich.edu>
    Acked-by: Jens Lehmann <Jens.Lehmann@web.de>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Spencer E. Olson· Feb 17, 2011, 22:17 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2 (v2)] submodule: no [--merge|--rebase] when newly cloned

This looks fine by me.

What about the testing patch? It'll be a few hours before I can resubmit with "git submodule status submodule" changed to "git status -s submodule".

On Thursday 17 February 2011 13:25, Junio C Hamano wrote:

Show 36 quoted lines

> Jens Lehmann <Jens.Lehmann@web.de> writes:
> > Am 17.02.2011 17:18, schrieb Spencer E. Olson:
> >> Previously, when a new submodule is cloned by running "git submodule
> >> update [--merge|--rebase]", the newly cloned submodule does not get
> >> checked out and a rebase or merge is incorrectly attempted against an
> >> empty working directory. This patch ignores --rebase or --merge for new
> >> submodules and instead simply checks out the appropriate revision.
> >
> > Nice work, thanks!
> >
> > Just a small thing: This problem also happens when the configuration
> > "submodule.<name>.update" is set to either "rebase" or "merge", not only
> > when using the command line options. So perhaps you could reword the
> > topic to something like "submodule: don't merge or rebase when newly
> > cloned" and adjust both commit messages a bit?
>
> I tentatively queued with this rewrite (the patch text is the same as the
> one I sent previously).
>
>     From: Spencer E. Olson <olsonse@umich.edu>
>     Date: Thu, 17 Feb 2011 09:18:45 -0700
>     Subject: [PATCH] submodule: no [--merge|--rebase] when newly cloned
>
>     "git submodule update" can be run with either the "--merge" or
> "--rebase" option, or submodule.<name>.update configuration variable can be
> set to "merge" or "rebase, to cause local work to get integrated when
> updating the submodule.
>
>     When a submodule is newly cloned, however, it does not have a check out
>     when a rebase or merge is attempted, leading to a failure.  For newly
>     cloned submodules, simply check out the appropriate revision.  There is
> no local work to integrate with for them.
>
>     Signed-off-by: Spencer E. Olson <olsonse@umich.edu>
>     Acked-by: Jens Lehmann <Jens.Lehmann@web.de>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>

Junio C Hamano· Feb 17, 2011, 23:37 UTC · re: Spencer E. Olson · lore

Re: [PATCH 1/2 (v2)] submodule: no [--merge|--rebase] when newly cloned

"Spencer E. Olson" <olsonse@umich.edu> writes:

> This looks fine by me.
>
> What about the testing patch?  It'll be a few hours before I can resubmit 
> with "git submodule status submodule" changed to "git status -s submodule".

I just squashed the patch from Jens in, with Ack from him.

Spencer E. Olson· Feb 17, 2011, 23:50 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2 (v2)] submodule: no [--merge|--rebase] when newly cloned

great, and thanks

On Thursday 17 February 2011 16:37, Junio C Hamano wrote:

Show 8 quoted lines

> "Spencer E. Olson" <olsonse@umich.edu> writes:
> > This looks fine by me.
> >
> > What about the testing patch?  It'll be a few hours before I can resubmit
> > with "git submodule status submodule" changed to "git status -s
> > submodule".
>
> I just squashed the patch from Jens in, with Ack from him.

Junio C Hamano· Feb 17, 2011, 20:02 UTC · re: Spencer E. Olson · lore

Re: [PATCH 1/2 (v2)] submodule: no [--merge|--rebase] when newly cloned

"Spencer E. Olson" <olsonse@umich.edu> writes:

Show 10 quoted lines

> @@ -469,6 +484,12 @@ cmd_update()
>  				die "Unable to fetch in submodule path '$path'"
>  			fi
>  
> +			list_contains "$name" "$cloned_modules"
> +			if test "$?" = 0
> +			then
> +				update_module=
> +			fi
> +

It would be a lot easier to read if you said this:

	if list_contains "$name" "$cloned_modules"
        then
		update_module=
	fi

or just open code the idiom:

	case ";list-of-stuff-with-delimiter;" in
        *";i-want-to-see-this;"*) : happy ;;
	esac

without introducing "list_contains" that is called only once.

But otherwise the patch looks good.

Thanks.

-- >8 --
From: Spencer E. Olson <olsonse@umich.edu>
Date: Thu, 17 Feb 2011 09:18:45 -0700
Subject: [PATCH] submodule: no [--merge|--rebase] when newly cloned

Signed-off-by: Spencer E. Olson <olsonse@umich.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-submodule.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

Show changes to git-submodule.sh +9 −0

diff --git a/git-submodule.sh b/git-submodule.sh
index 8b90589..5e989b7 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -423,6 +423,7 @@ cmd_update()
 		cmd_init "--" "$@" || return
 	fi
 
+	cloned_modules=
 	module_list "$@" |
 	while read mode sha1 stage path
 	do
@@ -442,6 +443,7 @@ cmd_update()
 		if ! test -d "$path"/.git -o -f "$path"/.git
 		then
 			module_clone "$path" "$url" "$reference"|| exit
+			cloned_modules="$cloned_modules;$name"
 			subsha1=
 		else
 			subsha1=$(clear_local_git_env; cd "$path" &&
@@ -469,6 +471,13 @@ cmd_update()
 				die "Unable to fetch in submodule path '$path'"
 			fi
 
+			# Is this something we just cloned?
+			case ";$cloned_modules;" in
+			*";$name;"*)
+				# then there is no local change to integrate
+				update_module= ;;
+			esac
+
 			case "$update_module" in
 			rebase)
 				command="git rebase"

-- 
1.7.4.1.107.ga7184