Re: [PATCH 3/3] submodule--helper: add intern-git-dir function

From: Stefan Beller <sbeller@google.com>
Date: Nov 22, 2016, 02:09 UTC
Message-ID: <CAGZ79kb_4wWs_90AfsT932iPWbCXf6yRq875JUxoRZjUcsBW5A@mail.gmail.com>
In-Reply-To: <xmqqy40ch6wp.fsf@gitster.mtv.corp.google.com>

On Mon, Nov 21, 2016 at 1:14 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

>
> Does this format correctly?
>
> I somehow thought that second and subsequent paragraphs continued
> with "+" want no indentation before them.  See for example the
> Values section in config.txt and see how entries for boolean:: and
> color:: use multiple '+' paragraphs.
>
> If we do not have to refrain from indenting the second and
> subsequent paragraphs, that would be great for readability, but I
> take the existing practice as telling me that we cannot do that X-<.

Will fix and test in a resend.

Show 15 quoted lines

>
>> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '
>> +     git init sub2 &&
>> +     test_commit -C sub2 first &&
>> +     git add sub2 &&
>> +     git commit -m superproject
>> +'
>> +
>> +test_expect_success 'intern the git dir fails for incomplete submodules' '
>> +     test_must_fail git submodule interngitdirs &&
>> +     # check that we did not break the repository:
>> +     git status
>> +'
>
> It is not clear what the last "git status" wants to test.

Any errors that I ran into when manually truing to embed a submodules git dir, were fatal with `git status` already, e.g. missing or wrong call of connect_work_tree_and_git_dir were my main failure points.

So I guess we should test a bit more extensively, maybe

    git status >expect
    git submodule embedgitdirs
    git status >actual
    test_cmp expect actual
    # further testing via
    test -f ..
    test -d ..

Show 6 quoted lines

>  In the
> extreme, if the failed "git submodule" command did
>
>         rm -fr .git ?* && git init
>
> wouldn't "git status" still succeed?

    In that particular case you'd get
    $ git status
    fatal: Not a git repository (or any parent up to mount point ....)
    Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
    $ echo $?
    128

but I get the idea, which is why I propose the double check via status. That would detect any logical change for the repository, e.g. a change to the .gitmodules file.

Show 5 quoted lines

>
> What are the minimum things that we expect from "did not break" to
> see?  sub2/.git is still a directory and is a valid repository?  The
> contents of the .git/modules/* before and after the "git submodule"
> does not change?  Some other things?

I thought about making up a name for such a repo and creating that engry in .gitmodules. But I refrained from doing so, because it seems too much for this command.

I dunno, but I would suspect the double status is fine here, too?