Re: [PATCH v8 4/8] submodule: allow add_submodule_odb to work even if path is not checked out

[Jacob Keller <jacob.keller@gmail.com>]

On Fri, Aug 19, 2016 at 1:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> diff --git a/path.c b/path.c
>> index 17551c483476..0cb30123e988 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -482,6 +482,11 @@ static void do_submodule_path(struct strbuf *buf, const char *path,
>>               strbuf_reset(buf);
>>               strbuf_addstr(buf, git_dir);
>>       }
>> +     if (!is_git_directory(buf->buf)) {
>> +             strbuf_reset(buf);
>> +             strbuf_git_path(buf, "%s/%s", "modules", path);
>> +     }
>> +
>>       strbuf_addch(buf, '/');
>>       strbuf_addbuf(&git_submodule_dir, buf);
>
> So, given submodule at $path, so far we have tried $path/.git and
> would have been happy if it was already a directory (i.e. an
> embedded repository in place), would have been happy if it was
> pointing at elsewhere (presumably at .git/modules/$name) that is a
> directory, and this is a fallback that covers the case where $path
> in the working tree of the superproject is missing.
>
> I _think_ "path" needs to be mapped to the "name" of the submodule
> that should be at the "path".  Other than that, this hunk looks
> correct to me.

How do we do that? I tried to find a way to do that, couldn't, and
decided it was probably already normalized. It seems to be, based on
my tests, where I run git-log from within a subdirectory that has the
submodule. I think we already have the complete path. Or is the name
*not* equivalent to the path?

>
>> diff --git a/submodule.c b/submodule.c
>> index 1b5cdfb7e784..e1a51b7506ff 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -348,7 +348,7 @@ void show_submodule_summary(FILE *f, const char *path,
>>       if (is_null_sha1(two))
>>               message = "(submodule deleted)";
>>       else if (add_submodule_odb(path))
>> -             message = "(not checked out)";
>> +             message = "(not initialized)";
>>       else if (is_null_sha1(one))
>>               message = "(new submodule)";
>>       else if (!(left = lookup_commit_reference(one)) ||
>
> This is a change unrelated to the fix you made above, and should be
> done in its own separate patch, I would think.
>

It is the same change. The first hunk makes it so that "not checked
out" is no longer what occurs. We change the behavior of
add_submodule_odb into "work even if there is no check out, but only
fail when the submodule isn't initialized at all, either in path/.git
or ./git/modules/path/" so we need to change the message because we've
changed behavior.

>> diff --git a/t/t4059-diff-submodule-not-initialized.sh b/t/t4059-diff-submodule-not-initialized.sh
>> new file mode 100755
>> index 000000000000..cc787454033a
>> --- /dev/null
>> +++ b/t/t4059-diff-submodule-not-initialized.sh
>> @@ -0,0 +1,105 @@
>> +#!/bin/sh
>> +#
>> +# Copyright (c) 2016 Jacob Keller, based on t4041 by Jens Lehmann
>> +#
>> +
>> +test_description='Test for submodule diff on non-checked out submodule
>> +
>> +This test tries to verify that add_submodule_odb works when the submodule was
>> +initialized previously but the checkout has since been removed.
>> +'
>> +
>> +TEST_NO_CREATE_REPO=1
>> +. ./test-lib.sh
>> +
>> +# Tested non-UTF-8 encoding
>> +test_encoding="ISO8859-1"
>> +
>> +# String "added" in German (translated with Google Translate), encoded in UTF-8,
>> +# used in sample commit log messages in add_file() function below.
>> +added=$(printf "hinzugef\303\274gt")
>
> Have an empty line here?
>

Makes sense I suppose.

>> +add_file () {
>> +     (
>> +             cd "$1" &&
>> +             shift &&
>> +             for name
>> +             do
>> +                     echo "$name" >"$name" &&
>> +                     git add "$name" &&
>> +                     test_tick &&
>> +                     # "git commit -m" would break MinGW, as Windows refuse to pass
>> +                     # $test_encoding encoded parameter to git.
>> +                     echo "Add $name ($added $name)" | iconv -f utf-8 -t $test_encoding |
>> +                     git -c "i18n.commitEncoding=$test_encoding" commit -F -
>> +             done >/dev/null &&
>> +             git rev-parse --short --verify HEAD
>> +     )
>> +}
>
> Have an empty line here?
>

There was no empty line in the place I copied from.

>> +commit_file () {
>> +     test_tick &&
>> +     git commit "$@" -m "Commit $*" >/dev/null
>> +}
>
> Surround these ...
>
>> +test_create_repo sm1 &&
>> +test_create_repo sm2 &&
>> +add_file sm1 foo >/dev/null &&
>> +add_file sm2 foo1 foo2 >/dev/null &&
>> +smhead1=$(cd sm2; git rev-parse --short --verify HEAD) &&
>
> ... inside its own "test_expect_success setup"?
>
> None of the ">/dev/null" we see in the above helper functions (and
> use of these helper functions) are necessary or desired, I would
> think.

If we put them in test_expect_success setup they aren't. They were
because add_file returns the sha1 of the rev-parse on HEAD after
adding.

>
> The last one can become "$(git -C sm2 rev-parse ...)".
>

Yea.

>> +cd sm1
>
> I can sort of see the attraction of doing it this way, but we avoid
> chdir'ing around outside subshells, especially in a newly added test
> script.  It is very easy to go down and forget to come back up, or
> worse yet, stop going down and forget to remove matching "cd .." and
> end up being outside the $TEST_DIRECTORY by mistake.
>

That would make a lot of tests problematic here.. but probably doable.

>> +test_expect_success 'setup - submodule directory' '
>> +...
>> +
>> +cd ..
>> +
>> +test_expect_success 'submodule not initialized in new clone' '
>> +...
>
> The tests themselves looked sensible.  Thanks.

Thanks,
Jake