Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories
Hi, Stefan Beller wrote: > On Wed, Sep 5, 2018 at 12:18 PM Jonathan Nieder wrote: >> Stefan Beller wrote: >>> This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head >>> works with broken submodules, 2017-04-18), which tones down the case of >>> "broken submodule" in case of a missing git directory of the submodule to >>> be only a warning. [...] >> I don't understand what workflow this is a part of. >> >> If the submodule is missing, shouldn't we make it non-missing instead >> of producing a partial checkout that doesn't build? > > No. checkout and friends do not want to touch the network > (unless we are in a partial clone world; that is the user is fully > aware that commands can use the network at totally unexpected > times) > > So for that, all we can do is better error messages. Thanks. This patch doesn't just improve error messages, though, but it makes the operation report success instead of failing. Isn't that likely to produce more confusion when I run additional commands afterward? In other words, instead of $ git checkout --recurse-submodules -B master origin/new-fancy-branch Branch 'master' set up to track remote branch 'new-fancy-branch' from 'origin'. Switched to a new branch 'master' warning: Submodule 'new-fancy-submodule' is missing $ git status [some unclean state] I would prefer to experience $ git checkout --recurse-submodules -B master origin/new-fancy-branch fatal: missing submodule 'new-fancy-submodule' hint: run "git fetch --recurse-submodules" to fetch it $ git status [clean state] $ git fetch --recurse-submodules [...] $ git checkout --recurse-submodules -B master origin/new-fancy-branch Branch 'master' set up to track remote branch 'new-fancy-branch' from 'origin'. Switched to a new branch 'master' $ git status [clean state] Thanks, Jonathan
Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories
On Wed, Sep 5, 2018 at 12:18 PM Jonathan Nieder wrote: > > Hi, > > Stefan Beller wrote: > > > This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head > > works with broken submodules, 2017-04-18), which tones down the case of > > "broken submodule" in case of a missing git directory of the submodule to > > be only a warning. > > > > Signed-off-by: Stefan Beller > > --- > > submodule.c | 16 > > t/t2013-checkout-submodule.sh | 2 +- > > 2 files changed, 17 insertions(+), 1 deletion(-) > > I don't understand what workflow this is a part of. > > If the submodule is missing, shouldn't we make it non-missing instead > of producing a partial checkout that doesn't build? No. checkout and friends do not want to touch the network (unless we are in a partial clone world; that is the user is fully aware that commands can use the network at totally unexpected times) So for that, all we can do is better error messages. Stefan
Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories
Hi, Stefan Beller wrote: > This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head > works with broken submodules, 2017-04-18), which tones down the case of > "broken submodule" in case of a missing git directory of the submodule to > be only a warning. > > Signed-off-by: Stefan Beller > --- > submodule.c | 16 > t/t2013-checkout-submodule.sh | 2 +- > 2 files changed, 17 insertions(+), 1 deletion(-) I don't understand what workflow this is a part of. If the submodule is missing, shouldn't we make it non-missing instead of producing a partial checkout that doesn't build? Thanks, Jonathan
Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories
On Wed, Aug 29, 2018 at 10:17 AM Junio C Hamano wrote: > > Stefan Beller writes: > > > Not quite, as this is ... > > > > Looking at the test in the previous patch, I would think a reasonable > > workflow > > in the test is ... > > > >> The MOVE_HEAD_FORCE codepath that follows this hunk is, eh, already > >> forcing to correct the situation, so there is no need to touch that, > >> which makes sense, if I understand correctly. > > > > No, that is not executed for now as it depends on 'old_head'. > > All explanation worth having in the log message to help future > readers, don't you think? ok, will do. Thanks, Stefan
Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories
Stefan Beller writes: > Not quite, as this is ... > > Looking at the test in the previous patch, I would think a reasonable workflow > in the test is ... > >> The MOVE_HEAD_FORCE codepath that follows this hunk is, eh, already >> forcing to correct the situation, so there is no need to touch that, >> which makes sense, if I understand correctly. > > No, that is not executed for now as it depends on 'old_head'. All explanation worth having in the log message to help future readers, don't you think? Thanks.
Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories
On Tue, Aug 28, 2018 at 11:56 AM Junio C Hamano wrote: > > Stefan Beller writes: > > > This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head > > works with broken submodules, 2017-04-18), which tones down the case of > > "broken submodule" in case of a missing git directory of the submodule to > > be only a warning. > > After seeing this warning, as we do not do any remedial action in > this codepath, the user with a repository in this state will keep > seeing the 'missing' message. Wouldn't we want to give a hint in > addition (e.g. 'you can run "git submodule update $name" to > recover', or something like that)? Not quite, as this is only triggered in the case of 'old_head = NULL', which is when you have a superproject that is missing the submodule in the working tree before and wants to have it afterwards. Looking at the test in the previous patch, I would think a reasonable workflow in the test is git clone --recurse-submodules super1 super1_clone && cd super1_clone git checkout --recurse-submodules historic_state # see warning, but checkout proceeds git fetch --recurse-submodules # clones the submodule as it was configured active via the clone git checkout --recurse-submodules historic_state # this checkout will put the submodule in place # not sure if -f is needed here, though. I am currently working on the cloning of submodules that are not currently in the working tree while fetching in the superproject, which would address the latter part. > The MOVE_HEAD_FORCE codepath that follows this hunk is, eh, already > forcing to correct the situation, so there is no need to touch that, > which makes sense, if I understand correctly. No, that is not executed for now as it depends on 'old_head'. In case of FORCE we might want to die instead of just warn about that submodule. Stefan
Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories
Stefan Beller writes:
> This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head
> works with broken submodules, 2017-04-18), which tones down the case of
> "broken submodule" in case of a missing git directory of the submodule to
> be only a warning.
After seeing this warning, as we do not do any remedial action in
this codepath, the user with a repository in this state will keep
seeing the 'missing' message. Wouldn't we want to give a hint in
addition (e.g. 'you can run "git submodule update $name" to
recover', or something like that)?
The MOVE_HEAD_FORCE codepath that follows this hunk is, eh, already
forcing to correct the situation, so there is no need to touch that,
which makes sense, if I understand correctly.
> Signed-off-by: Stefan Beller
> ---
> submodule.c | 16
> t/t2013-checkout-submodule.sh | 2 +-
> 2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/submodule.c b/submodule.c
> index 50cbf5f13ed..689439a3d0c 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1641,6 +1641,22 @@ int submodule_move_head(const char *path,
> } else {
> char *gitdir = xstrfmt("%s/modules/%s",
> get_git_common_dir(), sub->name);
> +
> + if (!is_git_directory(gitdir)) {
> + /*
> + * It is safe to assume we could just clone
> + * the submodule here, as we passed the
> + * is_submodule_active test above (i.e. the
> + * user is interested in this submodule.
> + *
> + * However as this code path is exercised
> + * for operations that typically do not involve
> + * network operations, let's not do that for
> now.
> + */
> + warning(_("Submodule '%s' missing"), path);
> + free(gitdir);
> + return 0;
> + }
> connect_work_tree_and_git_dir(path, gitdir, 0);
> free(gitdir);
>
> diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
> index c69640fc341..82ef4576b91 100755
> --- a/t/t2013-checkout-submodule.sh
> +++ b/t/t2013-checkout-submodule.sh
> @@ -81,7 +81,7 @@ test_expect_success 'setup superproject with historic
> submodule' '
> test_must_be_empty super1/.gitmodules
> '
>
> -test_expect_failure 'checkout old state with deleted submodule' '
> +test_expect_success 'checkout old state with deleted submodule' '
> test_when_finished "rm -rf super1 sub1 super1_clone" &&
> git clone --recurse-submodules super1 super1_clone &&
> git -C super1_clone checkout --recurse-submodules historic_state
