Re: [PATCH v8 4/8] submodule: allow add_submodule_odb to work even if path is not checked out
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 19, 2016, 20:06 UTC
- Message-ID
- <xmqqvayw1q1z.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160819000031.24854-5-jacob.e.keller@intel.com>
Jacob Keller <jacob.e.keller@intel.com> writes:
Show 15 quoted lines
> 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.
Show 13 quoted lines
> 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.
Show 26 quoted lines
> 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?
Show 17 quoted lines
> +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 ...
Show 5 quoted lines
> +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.
Show 7 quoted lines
> +test_expect_success 'setup - submodule directory' ' > +... > + > +cd .. > + > +test_expect_success 'submodule not initialized in new clone' ' > +...
The tests themselves looked sensible. Thanks.