git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 4/4] submodule deinit: unset core.worktree
Date: Sat, 08 Dec 2018 16:03:56 +0900
Message-ID: <xmqqo99w7b4z.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20181207235425.128568-5-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> This re-introduces 984cd77ddb (submodule deinit: unset core.worktree,
> 2018-06-18), which was reverted as part of f178c13fda (Revert "Merge
> branch 'sb/submodule-core-worktree'", 2018-09-07)
>
> The whole series was reverted as the offending commit e98317508c
> (submodule: ensure core.worktree is set after update, 2018-06-18)
> was relied on by other commits such as 984cd77ddb.
>
> Keep the offending commit reverted, but its functionality came back via
> 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17), such
> that we can reintroduce 984cd77ddb now.

None of the above three explains the most important thing directly,
so readers fail to grasp what the main theme of the three-patch
series is, without looking at the commits that were reverted
already.

Is the theme of the overall series to make sure core.worktree is set
to point at the working tree when submodule's working tree is
instantiated, and unset it when it is not?

2/4 was also explained (in the original) that it wants to unset and
did so when "move_head" gets called.  This one does the unset when a
submodule is deinited.  Are these the only two cases a submodule
loses its working tree?  If so, the log message for this step should
declare victory by ending with something like

	... as we covered the only other case in which a submodule
	loses its working tree in the earlier step (i.e. switching
	branches of top-level project to move to a commit that did
	not have the submodule), this makes the code always maintain
	core.worktree correctly unset when there is no working tree
	for a submodule.

Thanks.  I think I agree with what the series wants to do (if I read
what it wants to do correctly, that is).

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/submodule--helper.c | 2 ++
>  t/lib-submodule-update.sh   | 2 +-
>  t/t7400-submodule-basic.sh  | 5 +++++
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 31ac30cf2f..672b74db89 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1131,6 +1131,8 @@ static void deinit_submodule(const char *path, const char *prefix,
>  		if (!(flags & OPT_QUIET))
>  			printf(format, displaypath);
>  
> +		submodule_unset_core_worktree(sub);
> +
>  		strbuf_release(&sb_rm);
>  	}
>  
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index 51d4555549..5b56b23166 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -235,7 +235,7 @@ reset_work_tree_to_interested () {
>  	then
>  		mkdir -p submodule_update/.git/modules/sub1/modules &&
>  		cp -r submodule_update_repo/.git/modules/sub1/modules/sub2 submodule_update/.git/modules/sub1/modules/sub2
> -		GIT_WORK_TREE=. git -C submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree
> +		# core.worktree is unset for sub2 as it is not checked out
>  	fi &&
>  	# indicate we are interested in the submodule:
>  	git -C submodule_update config submodule.sub1.url "bogus" &&
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 76a7cb0af7..aba2d4d6ee 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -984,6 +984,11 @@ test_expect_success 'submodule deinit should remove the whole submodule section
>  	rmdir init
>  '
>  
> +test_expect_success 'submodule deinit should unset core.worktree' '
> +	test_path_is_file .git/modules/example/config &&
> +	test_must_fail git config -f .git/modules/example/config core.worktree
> +'
> +
>  test_expect_success 'submodule deinit from subdirectory' '
>  	git submodule update --init &&
>  	git config submodule.example.foo bar &&

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-07 23:54 [PATCH 0/4] Stefan Beller
2018-12-07 23:54 ` [PATCH 1/4] submodule update: add regression test with old style setups Stefan Beller
2018-12-09  0:11   ` Junio C Hamano
2018-12-07 23:54 ` [PATCH 2/4] submodule: unset core.worktree if no working tree is present Stefan Beller
2018-12-08  6:44   ` Junio C Hamano
2018-12-07 23:54 ` [PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree Stefan Beller
2018-12-08  6:55   ` Junio C Hamano
2018-12-12 22:46     ` Stefan Beller
2018-12-13  3:14       ` Junio C Hamano
2018-12-07 23:54 ` [PATCH 4/4] submodule deinit: unset core.worktree Stefan Beller
2018-12-08  7:03   ` Junio C Hamano [this message]
2018-12-08  5:57 ` [PATCH 0/4] Junio C Hamano
2018-12-12 22:35   ` Stefan Beller
2018-12-13  3:15     ` Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqo99w7b4z.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox