From: Stefan Beller <sbeller@google.com>
To: Brandon Williams <bmwill@google.com>
Cc: Junio C Hamano <gitster@pobox.com>,
"git@vger.kernel.org" <git@vger.kernel.org>,
Jeff King <peff@peff.net>
Subject: Re: [PATCH] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves
Date: Tue, 24 Jan 2017 14:13:53 -0800 [thread overview]
Message-ID: <CAGZ79kYKkx441bbU5Oy9Ernb1FmbcTybYbL_M_+yWG_ycfPwrA@mail.gmail.com> (raw)
In-Reply-To: <20170124215851.GA58021@google.com>
On Tue, Jan 24, 2017 at 1:58 PM, Brandon Williams <bmwill@google.com> wrote:
> On 01/24, Stefan Beller wrote:
>> + if (read_gitfile_gently(old_git_dir, &err_code) ||
>> + err_code == READ_GITFILE_ERR_NOT_A_REPO) {
>> + /*
>> + * If it is an actual gitfile, it doesn't need migration,
>> + * however in case of a recursively nested submodule, the
>> + * gitfile content may be stale, as its superproject
>> + * (which may be a submodule of another superproject)
>> + * may have been moved. So expect a bogus pointer to be read,
>> + * which materializes as error READ_GITFILE_ERR_NOT_A_REPO.
>> + */
>> + connect_work_tree_and_git_dir(path, real_new_git_dir);
>
> So connect_work_tree_and_git_dir() will update the .gitfile if it is
> stale.
>
>> + return;
>> + }
>> +
>> + if (submodule_uses_worktrees(path))
>> + die(_("relocate_gitdir for submodule '%s' with "
>> + "more than one worktree not supported"), path);
>
> No current support for worktrees (yet!).
>
>> +
>> if (!prefix)
>> prefix = get_super_prefix();
>>
>> @@ -1437,22 +1448,14 @@ void absorb_git_dir_into_superproject(const char *prefix,
>> const char *path,
>> unsigned flags)
>> {
>> - const char *sub_git_dir, *v;
>> - char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;
>> struct strbuf gitdir = STRBUF_INIT;
>> -
>> strbuf_addf(&gitdir, "%s/.git", path);
>> - sub_git_dir = resolve_gitdir(gitdir.buf);
>>
>> /* Not populated? */
>> - if (!sub_git_dir)
>> + if (!file_exists(gitdir.buf))
>> goto out;
>
> There should be a is_submodule_populated() function now, maybe
> we should start using it when performing population checks?
Yes I am aware of that, but the problem is we cannot use it here.
is_submodule_populated[1], just like the code here, uses
resolve_gitdir, which is
const char *resolve_gitdir(const char *suspect)
{
if (is_git_directory(suspect))
return suspect;
return read_gitfile(suspect);
}
And there you see the problem: read_gitfile will die on error.
we'd have to have use read_gitfile_gently(old_git_dir, &err_code),
and then allow READ_GITFILE_ERR_NOT_A_REPO to go through,
just as above.
And that is also the reason why we had to move submodule_uses_worktrees
down, as it also uses no gentle function to look for a git directory
(read: it would die as well). When you have bogus content in your
.git file, there is really nothing you can do to determine if the submodule
is part of a worktree setup, so it is fine to postpone the check until after we
fixed up the link.
So here is the bug you spotted: If it is a worktree already, then
read_gitfile_gently would work fine, no need to "fix" it.
I'll resend with logic as follows:
char *retvalue = read_gitfile_gently(old_git_dir, &err_code);
if (retvalue)
// return early; a worktree is fine here, no need to check
// because we do nothing
if (err_code == READ_GITFILE_ERR_NOT_A_REPO)
// connect; then check for worktree and return early;
// do the actual relocation.
[1] as found e.g. at
https://public-inbox.org/git/1481915002-162130-2-git-send-email-bmwill@google.com/
>
>>
>> - /* Is it already absorbed into the superprojects git dir? */
>> - real_sub_git_dir = real_pathdup(sub_git_dir);
>> - real_common_git_dir = real_pathdup(get_git_common_dir());
>> - if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))
>> - relocate_single_git_dir_into_superproject(prefix, path);
>> + relocate_single_git_dir_into_superproject(prefix, path);
>
> So the check was just pushed into the relocation function.
The check was pushed down, so we can use the
connect_work_tree_and_git_dir instead.
>
>>
>> if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
>> struct child_process cp = CHILD_PROCESS_INIT;
>> @@ -1481,6 +1484,4 @@ void absorb_git_dir_into_superproject(const char *prefix,
>>
>> out:
>> strbuf_release(&gitdir);
>> - free(real_sub_git_dir);
>> - free(real_common_git_dir);
>> }
>> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
>> index 1c47780e2b..e2bbb449b6 100755
>> --- a/t/t7412-submodule-absorbgitdirs.sh
>> +++ b/t/t7412-submodule-absorbgitdirs.sh
>> @@ -64,6 +64,33 @@ test_expect_success 'absorb the git dir in a nested submodule' '
>> test_cmp expect.2 actual.2
>> '
>>
>> +test_expect_success 're-setup nested submodule' '
>> + # un-absorb the direct submodule, to test if the nested submodule
>> + # is still correct (needs a rewrite of the gitfile only)
>> + rm -rf sub1/.git &&
>> + mv .git/modules/sub1 sub1/.git &&
>> + GIT_WORK_TREE=. git -C sub1 config --unset core.worktree &&
>> + # fixup the nested submodule
>> + echo "gitdir: ../.git/modules/nested" >sub1/nested/.git &&
>> + GIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \
>> + core.worktree "../../../nested" &&
>> + # make sure this re-setup is correct
>> + git status --ignore-submodules=none
>> +'
>> +
>> +test_expect_success 'absorb the git dir in a nested submodule' '
>> + git status >expect.1 &&
>> + git -C sub1/nested rev-parse HEAD >expect.2 &&
>> + git submodule absorbgitdirs &&
>> + test -f sub1/.git &&
>> + test -f sub1/nested/.git &&
>> + test -d .git/modules/sub1/modules/nested &&
>> + git status >actual.1 &&
>> + git -C sub1/nested rev-parse HEAD >actual.2 &&
>> + test_cmp expect.1 actual.1 &&
>> + test_cmp expect.2 actual.2
>> +'
>> +
>> test_expect_success 'setup a gitlink with missing .gitmodules entry' '
>> git init sub2 &&
>> test_commit -C sub2 first &&
>> --
>> 2.11.0.486.g67830dbe1c
>
>
> Aside from my one question the rest of this looks good to me.
>
> --
> Brandon Williams
next prev parent reply other threads:[~2017-01-24 22:20 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-24 21:03 [PATCH] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves Stefan Beller
2017-01-24 21:58 ` Brandon Williams
2017-01-24 22:13 ` Stefan Beller [this message]
2017-01-24 22:19 ` Brandon Williams
2017-01-24 23:56 ` [PATCHv2 0/3] fix recursive submodule absorbing Stefan Beller
2017-01-24 23:56 ` [PATCHv2 1/3] Add gentle version of resolve_git_dir Stefan Beller
2017-01-24 23:56 ` [PATCHv2 2/3] cache.h: expose the dying procedure for reading gitlinks Stefan Beller
2017-01-24 23:56 ` [PATCHv2 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves Stefan Beller
2017-01-25 0:46 ` Brandon Williams
2017-01-25 23:04 ` Stefan Beller
2017-01-25 22:54 ` Junio C Hamano
2017-01-25 23:04 ` [PATCHv3 " Stefan Beller
2017-01-25 23:39 ` Brandon Williams
Reply instructions:
You may reply publicly 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=CAGZ79kYKkx441bbU5Oy9Ernb1FmbcTybYbL_M_+yWG_ycfPwrA@mail.gmail.com \
--to=sbeller@google.com \
--cc=bmwill@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).