Re: [PATCH 2/4] submodule: unset core.worktree if no working tree is present
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 8, 2018, 06:44 UTC
- Message-ID
- <xmqqwook7c1k.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20181207235425.128568-3-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 10 quoted lines
> 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.
Show 59 quoted lines
> 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.