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

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

[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.

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

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

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

[Spencer E. Olson]

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
-- 
1.7.4.1.42.g43f9f

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

[Jens Lehmann]

Am 17.02.2011 17:18, schrieb Spencer E. Olson:
> 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>


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

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

[Junio C Hamano]

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?

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

[Jens Lehmann]

Am 17.02.2011 21:18, schrieb Junio C Hamano:
> 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.

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

[Jens Lehmann]

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?

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

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

[Junio C Hamano]

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>

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

[Spencer E. Olson]

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:
> 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>

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

[Junio C Hamano]

"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.

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

[Spencer E. Olson]

great, and thanks


On Thursday 17 February 2011 16:37, Junio C Hamano wrote:
> "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.

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

[Junio C Hamano]

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

> @@ -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

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-submodule.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

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