Re: [PATCH 2/4] submodule: unset core.worktree if no working tree is present

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

Stefan Beller <sbeller@google.com> writes:

> This reintroduces 4fa4f90ccd (submodule: unset core.worktree if no working
> tree is present, 2018-06-12), which was reverted as part of f178c13fda
> (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07).
>
> 4fa4f90ccd was reverted as its followup commit was faulty, but without
> the accompanying change of the followup, we'd have an incomplete workflow
> of setting `core.worktree` again, when it is needed such as checking out
> a revision that contains a submodule.
>
> So re-introduce that commit as-is, focusing on fixing up the followup

I was hoping to hear (given what 0/4 claimed) a clearer explanation
of what this change wants to achieve, but that is lacking.

No need to grumble about an earlier work was that turned out to be
inappropriate for the codebase back then.  Repeatedly saying "this
is needed" without giving further explaining why it is so, or
anything like that, would help readers.

Just pretend that the ealier commits and their reversion never
happened, and further pretend that we are doing the best thing that
should happen to our codebase.  How would we explain this change,
what the problem it tries to solve and what the solution looks like
in the larger picture?

	When removing a working tree of a submodule (e.g. we may be
	switching back to an earlier commit in the superproject that
	did not have the submodule in question yet), we failed to
	unset core.worktree of the submodule's repository.  That
	caused this and that issues, exhibited by a few new tests
	this commit adds.

	Make sure that core.worktree gets unset so that a leftover
	setting won't cause these issues.

or something like that?  I am just guessing by looking at the old
commit's text, as the above two paragraphs and one sentence does not
say much.

> diff --git a/submodule.c b/submodule.c
> index 6415cc5580..d393e947e6 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1561,6 +1561,18 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
>  	return ret;
>  }
>  
> +void submodule_unset_core_worktree(const struct submodule *sub)
> +{
> +	char *config_path = xstrfmt("%s/modules/%s/config",
> +				    get_git_common_dir(), sub->name);
> +
> +	if (git_config_set_in_file_gently(config_path, "core.worktree", NULL))
> +		warning(_("Could not unset core.worktree setting in submodule '%s'"),
> +			  sub->path);
> +
> +	free(config_path);
> +}
> +
>  static const char *get_super_prefix_or_empty(void)
>  {
>  	const char *s = get_super_prefix();
> @@ -1726,6 +1738,8 @@ int submodule_move_head(const char *path,
>  
>  			if (is_empty_dir(path))
>  				rmdir_or_warn(path);
> +
> +			submodule_unset_core_worktree(sub);
>  		}
>  	}
>  out:
> diff --git a/submodule.h b/submodule.h
> index a680214c01..9e18e9b807 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -131,6 +131,8 @@ int submodule_move_head(const char *path,
>  			const char *new_head,
>  			unsigned flags);
>  
> +void submodule_unset_core_worktree(const struct submodule *sub);
> +
>  /*
>   * Prepare the "env_array" parameter of a "struct child_process" for executing
>   * a submodule by clearing any repo-specific environment variables, but
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index 016391723c..51d4555549 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -709,7 +709,8 @@ test_submodule_recursing_with_args_common() {
>  			git branch -t remove_sub1 origin/remove_sub1 &&
>  			$command remove_sub1 &&
>  			test_superproject_content origin/remove_sub1 &&
> -			! test -e sub1
> +			! test -e sub1 &&
> +			test_must_fail git config -f .git/modules/sub1/config core.worktree
>  		)
>  	'
>  	# ... absorbing a .git directory along the way.