Re: Old submodules broken in 2.19rc1 and 2.19rc2

From: Jonathan Nieder <jrnieder@gmail.com>
To: Stefan Beller <sbeller@google.com>
Cc: allan.jensen@qt.io, git <git@vger.kernel.org>
Subject: Re: Old submodules broken in 2.19rc1 and 2.19rc2
Date: Fri, 7 Sep 2018 15:35:15 -0700	[thread overview]
Message-ID: <20180907223515.GD103699@aiede.svl.corp.google.com> (raw)
In-Reply-To: <CAGZ79kaVQ0T=acpviOoD+8XVxYsefNkO7c5d+d0Wc0iCbr2Evw@mail.gmail.com>

Stefan Beller wrote:
> On Fri, Sep 7, 2018 at 2:53 AM Allan Sandfeld Jensen <allan.jensen@qt.io> wrote:

>> Submodules checked out with older versions of git not longer works in the
>> latest 2.19 releases. A "git submodule update --recursive" command wil fail
>> for each submodule with a line saying "fatal: could not open
>> '<submodule>/.git' for writing> Is a directory.
[...]
> I have the suspicion that e98317508c0 (submodule:
> ensure core.worktree is set after update, 2018-06-18)
> might be the offender.

I still was not able to reproduce it, but after a bit of staring at
the code, I'm pretty sure I just did something wrong in the
reproduction process.  That commit is indeed the offender.

It introduces the following code (rewrapped for clarity) in
git-submodule.sh:

	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"
	fi

Staring at it for a while, you can see one problem: the 'if ! $(git
config)' should be simply 'if ! git config'.  This ends up trying to
run the core.worktree value as a command, which would usually fail.

That brings us into connect_work_tree_and_git_dir, which does

	/* Prepare .git file */
	strbuf_addf(&gitfile_sb, "%s/.git", work_tree_);
	if (safe_create_leading_directories_const(gitfile_sb.buf))
		die(_("could not create directories for %s"), gitfile_sb.buf);

	/* Prepare config file */
	strbuf_addf(&cfg_sb, "%s/config", git_dir_);
	if (safe_create_leading_directories_const(cfg_sb.buf))
		die(_("could not create directories for %s"), cfg_sb.buf);

	git_dir = real_pathdup(git_dir_, 1);
	work_tree = real_pathdup(work_tree_, 1);

	/* Write .git file */
	write_file(gitfile_sb.buf, "gitdir: %s",
		   relative_path(git_dir, work_tree, &rel_path));

The write_file runs into .git already existing as a directory, failing
with the message Allan saw.

This would happen in at least two cases:

- if the submodule exists both in .git/modules/ *and* in the worktree
  (due to flipping between Git versions and branches with and without
  the submodule), the above will happen

- likewise if the submodule exists only in the worktree, like for Allan.

In "next" there is 74d4731d (submodule--helper: replace
connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13) which
uses robust helpers in C that handle this much better.  I think we
should revert e98317508c0 in "master" (for 2.19) and keep making use
of that 'second try' in "next" (for 2.20).

I'll try to pin down a reproduction case and send a revert + testsuite
patch.

Thanks again,
Jonathan