From: Stefan Beller <sbeller@google.com>
To: vi0oss <vi0oss@gmail.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
Stefan Beller <stefanbeller@gmail.com>
Subject: Re: [PATCH 1/2] submodule--helper: set alternateLocation for cloned submodules
Date: Wed, 7 Dec 2016 16:02:41 -0800 [thread overview]
Message-ID: <CAGZ79kaSVcf1zk4P1hWsU=aJG7G99qohhavBftmPxnrkjW+nfQ@mail.gmail.com> (raw)
In-Reply-To: <20161207224948.7957-1-vi0oss@gmail.com>
On Wed, Dec 7, 2016 at 2:49 PM, <vi0oss@gmail.com> wrote:
> Notes:
> Resolved issues pointed by Stefan Beller except of
> the one about loosened path check, which he aggreed
> to be relaxed for this case.
I am sorry to have given an incomplete review at the first time. :/
More below.
> + /* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
> + git_config_get_string("submodule.alternateLocation", &sm_alternate);
> + if (sm_alternate) {
> + git_config_set_in_file(p, "submodule.alternateLocation",
> + sm_alternate);
> + }
For a single command, we usually omit the braces for
an if clause, i.e.
if (foo)
bar(...);
/* code goes on */
> + git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
> + if (error_strategy) {
> + git_config_set_in_file(p, "submodule.alternateErrorStrategy",
> + error_strategy);
> + }
> +
> + free(sm_alternate);
> + free(error_strategy);
> +
The indentation seems a bit off for the free here?
(The main nit that motivated me writing the email)
> strbuf_release(&sb);
> strbuf_release(&rel_path);
> free(sm_gitdir);
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> index 1c1e289..ef7771b 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -125,4 +125,76 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm
> )
> '
>
> +test_expect_success 'preparing second superproject with a nested submodule' '
> + test_create_repo supersuper &&
> + (
> + cd supersuper &&
> + echo "I am super super." >file &&
> + git add file &&
> + git commit -m B-super-super-initial
> + git submodule add "file://$base_dir/super" subwithsub &&
> + git commit -m B-super-super-added &&
> + git submodule update --init --recursive &&
> + git repack -ad
> + )
> +'
> +
> +# At this point there are three root-level positories: A, B, super and super2
> +
> +test_expect_success 'nested submodule alternate in works and is actually used' '
> + test_when_finished "rm -rf supersuper-clone" &&
> + git clone --recursive --reference supersuper supersuper supersuper-clone &&
> + (
> + cd supersuper-clone &&
> + # test superproject has alternates setup correctly
> + test_alternate_is_used .git/objects/info/alternates . &&
> + # immediate submodule has alternate:
> + test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
> + # nested submodule also has alternate:
> + test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
> + )
> +'
> +
> +test_expect_success 'missing nested submodule alternate fails clone and submodule update' '
> + test_when_finished "rm -rf supersuper-clone supersuper2" &&
> + git clone supersuper supersuper2 &&
> + (
> + cd supersuper2 &&
> + git submodule update --init
> + ) &&
> + test_must_fail git clone --recursive --reference supersuper2 supersuper2 supersuper-clone &&
> + (
> + cd supersuper-clone &&
> + # test superproject has alternates setup correctly
> + test_alternate_is_used .git/objects/info/alternates . &&
> + # update of the submodule fails
> + test_must_fail git submodule update --init --recursive &&
> + # immediate submodule has alternate:
> + test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
> + # but nested submodule has no alternate:
> + test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
> + )
> +'
> +
> +test_expect_success 'missing nested submodule alternate in --reference-if-able mode' '
> + test_when_finished "rm -rf supersuper-clone supersuper2" &&
> + git clone supersuper supersuper2 &&
> + (
> + cd supersuper2 &&
> + git submodule update --init
> + ) &&
> + git clone --recursive --reference-if-able supersuper2 supersuper2 supersuper-clone &&
> + (
> + cd supersuper-clone &&
> + # test superproject has alternates setup correctly
> + test_alternate_is_used .git/objects/info/alternates . &&
> + # update of the submodule fails
> + test_must_fail git submodule update --init --recursive &&
> + # immediate submodule has alternate:
> + test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
> + # but nested submodule has no alternate:
> + test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
> + )
Both the first and the last part are the same in the last two tests,
the only difference is the line with git clone --reference ...
Maybe we can use a function somehow to make this a bit more
obvious?
Otherwise the tests look good to me.
Thanks,
Stefan
prev parent reply other threads:[~2016-12-08 0:02 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-07 22:49 [PATCH 1/2] submodule--helper: set alternateLocation for cloned submodules vi0oss
2016-12-07 22:49 ` [PATCH 2/2] mailmap: Update my e-mail address vi0oss
2016-12-08 0:02 ` Stefan Beller [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAGZ79kaSVcf1zk4P1hWsU=aJG7G99qohhavBftmPxnrkjW+nfQ@mail.gmail.com' \
--to=sbeller@google.com \
--cc=git@vger.kernel.org \
--cc=stefanbeller@gmail.com \
--cc=vi0oss@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).