Re: [PATCH 2/3] submodule: ensure core.worktree is set after update

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Jun 16, 2018, 20:26 UTC
Message-ID: <20180616202628.9208-1-szeder.dev@gmail.com>
In-Reply-To: <20180612235825.12271-3-sbeller@google.com>

Show 8 quoted lines

> +static int connect_gitdir_workingtree(int argc, const char **argv, const char *prefix)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	const char *name, *path;
> +	char *sm_gitdir;
> +
> +	if (argc != 3)
> +		BUG("submodule--helper connect-gitdir-workingtree <name> <path>");

So this aborts when it's invoked with the wrong number of cmdline arguments.

Show 18 quoted lines

> +
> +	name = argv[1];
> +	path = argv[2];
> +
> +	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
> +	sm_gitdir = absolute_pathdup(sb.buf);
> +
> +	connect_work_tree_and_git_dir(path, sm_gitdir, 0);
> +
> +	strbuf_release(&sb);
> +	free(sm_gitdir);
> +
> +	return 0;
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>  
>  struct cmd_struct {

Show 11 quoted lines

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 78073cd87d1..6596a77c5ef 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -615,6 +615,11 @@ cmd_update()
>  			die "$(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
>  		fi
>  
> +		if ! $(git config -f "$(git rev-parse --git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null
> +		then
> +			git submodule--helper connect-gitdir-workingtree $name $sm_path

The path to the submodule, $sm_path, may contain spaces, so it must be quoted.

I'm sure you would have noticed this already, had you checked this 'submodule--helper's exit code :) In t7406 the test 'submodule update properly revives a moved submodule' does update a submodule with a space in its name, and thus executes 'submodule--helper' with one more argument than expected, causing it to abort, but since there is no error checking, 'git submodule update' continues anyway, and in the end the test tends to pass[1].

I think it would be prudent to check the exit code of all 'submodule--helper' executions.

[1] I wrote "tends to", because e.g. on Travis CI the aborting
    'submodule--helper' often dumps its core, and the extra 'core'
    file shows up in the output of 'git status' and 'test_cmp' notices
    it.

Show 9 quoted lines

> +		fi
> +
>  		if test "$subsha1" != "$sha1" || test -n "$force"
>  		then
>  			subforce=$force
> -- 
> 2.18.0.rc1.244.gcf134e6275-goog
> 
>