Re: [PATCH] pull: respect submodule update configuration

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> From: Lars Schneider <larsxschneider@gmail.com>
>
> Do not override the submodule configuration in the call to update
> the submodules, but give a weaker default.
>
> Reported-by: Lars Schneider <larsxschneider@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>   
> Personally I dislike this patch, but I have no better idea for the time
> being.

The patch text from a cursory look seems reasonable to me.

It's not like you have 47 different codepaths that need to pay
attention to the .update config and they all have to pass the new
--default-update option, this is merely to fix one of them that
relates to the problem reported by Lars, and you need a similar fix
to other 46, right?

If you want the "--recurse-submodules" thing to always do the
"weaker default" thing in your project, you can choose not to set
.update to custom values in any of your submodules, so I do not
think the reason why you dislike this change is because it would
affect your use of submodules.

So I am a bit curious to learn which part of this change you dislike
and why.


>  builtin/pull.c             |  6 ++++--
>  git-submodule.sh           |  7 ++++++-
>  t/t7400-submodule-basic.sh | 22 ++++++++++++++++++++++
>  3 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 9b86e519b1..be4f74d764 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -553,7 +553,8 @@ static int rebase_submodules(void)
>  	cp.git_cmd = 1;
>  	cp.no_stdin = 1;
>  	argv_array_pushl(&cp.args, "submodule", "update",
> -				   "--recursive", "--rebase", NULL);
> +				   "--recursive", "--default-update",
> +				   "rebase", NULL);
>  
>  	return run_command(&cp);
>  }
> @@ -565,7 +566,8 @@ static int update_submodules(void)
>  	cp.git_cmd = 1;
>  	cp.no_stdin = 1;
>  	argv_array_pushl(&cp.args, "submodule", "update",
> -				   "--recursive", "--checkout", NULL);
> +				   "--recursive", "--default-update",
> +				   "checkout", NULL);
>  
>  	return run_command(&cp);
>  }
> diff --git a/git-submodule.sh b/git-submodule.sh
> index e131760eec..6dbc32e686 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -511,6 +511,7 @@ fetch_in_submodule () (
>  cmd_update()
>  {
>  	# parse $args after "submodule ... update".
> +	default_update="checkout"
>  	while test $# -ne 0
>  	do
>  		case "$1" in
> @@ -552,6 +553,10 @@ cmd_update()
>  		--checkout)
>  			update="checkout"
>  			;;
> +		--default-update)
> +			default_update="$2"
> +			shift
> +			;;
>  		--recommend-shallow)
>  			recommend_shallow="--recommend-shallow"
>  			;;
> @@ -619,7 +624,7 @@ cmd_update()
>  			update_module=$(git config submodule."$name".update)
>  			if test -z "$update_module"
>  			then
> -				update_module="checkout"
> +				update_module="$default_update"
>  			fi
>  		fi
>  
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index dcac364c5f..ff64bf8528 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -1289,4 +1289,26 @@ test_expect_success 'init properly sets the config' '
>  	test_must_fail git -C multisuper_clone config --get submodule.sub1.active
>  '
>  
> +test_expect_success 'submodule update and git pull with disabled submodule' '
> +	test_when_finished "rm -rf multisuper_clone" &&
> +	pwd=$(pwd) &&
> +	cat <<-\EOF >expect &&
> +	-sub0
> +	 sub1 (test2)
> +	 sub2 (test2)
> +	 sub3 (test2)
> +	 sub4 (test2)
> +	 sub5 (test2)
> +	EOF
> +	git clone file://"$pwd"/multisuper multisuper_clone &&
> +	(
> +		cd multisuper_clone &&
> +		git config --local submodule.sub0.update none &&
> +		git submodule update --init --recursive &&
> +		git pull --recurse-submodules &&
> +		git submodule status | cut -c 1,43- >actual
> +	) &&
> +	test_cmp expect multisuper_clone/actual
> +'
> +
>  test_done