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

[Junio C Hamano <gitster@pobox.com>]

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.

> 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.

> 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?

> +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?

> +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.

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

> +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.

> +test_expect_success 'setup - submodule directory' '
> +...
> +
> +cd ..
> +
> +test_expect_success 'submodule not initialized in new clone' '
> +...

The tests themselves looked sensible.  Thanks.