On Fri, Aug 19, 2016 at 1:06 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Jacob Keller <jacob.e.keller@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html