From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Mugdha Pattnaik via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Bagas Sanjaya <bagasdotme@gmail.com>,
Atharva Raykar <raykar.ath@gmail.com>,
Junio C Hamano <gitster@pobox.com>,
mugdha <mugdhapattnaik@gmail.com>
Subject: Re: [PATCH v4] submodule: absorb git dir instead of dying on deinit
Date: Tue, 28 Sep 2021 11:53:45 +0200 [thread overview]
Message-ID: <87sfxpvxcs.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <pull.1078.v4.git.git.1630090303445.gitgitgadget@gmail.com>
On Fri, Aug 27 2021, Mugdha Pattnaik via GitGitGadget wrote:
> From: mugdha <mugdhapattnaik@gmail.com>
>
> Currently, running 'git submodule deinit' on repos where the
> submodule's '.git' is a directory, aborts with a message that is not
> exactly user friendly. Let's change this to instead warn the user
> to rerun the command with '--force'.
>
> This internally calls 'absorb_git_dir_into_superproject()', which
> moves the git dir into the superproject and replaces it with
> a '.git' file. The rest of the deinit function can operate as it
> already does with new-style submodules.
>
> We also edit a test case such that it matches the new behaviour of
> deinit.
>
> Suggested-by: Atharva Raykar <raykar.ath@gmail.com>
> Signed-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com>
> ---
> submodule: absorb git dir instead of dying on deinit
>
> Changes since v3:
>
> * Replaced 1 instance of the word "folder" with "directory"
> * Fixed tab spacing
>
> Thank you, Mugdha
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v4
> Pull-Request: https://github.com/git/git/pull/1078
>
> Range-diff vs v3:
>
> 1: 1ac65b2458b ! 1: 7460fc0e12a submodule: absorb git dir instead of dying on deinit
> @@ builtin/submodule--helper.c: static void deinit_submodule(const char *path, cons
> + if (is_directory(sub_git_dir)) {
> + if (!(flags & OPT_FORCE))
> + die(_("Submodule work tree '%s' contains a "
> -+ ".git directory.\nUse --force if you want "
> -+ "to move its contents to superproject's "
> -+ "module folder and convert .git to a file "
> -+ "and then proceed with deinit."),
> -+ displaypath);
> ++ ".git directory.\nUse --force if you want "
> ++ "to move its contents to superproject's "
> ++ "module directory and convert .git to a file "
> ++ "and then proceed with deinit."),
> ++ displaypath);
> +
> + if (!(flags & OPT_QUIET))
> + warning(_("Submodule work tree '%s' contains a .git "
> -+ "directory. This will be replaced with a "
> -+ ".git file by using absorbgitdirs."),
> -+ displaypath);
> ++ "directory. This will be replaced with a "
> ++ ".git file by using absorbgitdirs."),
> ++ displaypath);
> +
> + absorb_git_dir_into_superproject(displaypath, flags);
> +
>
>
> builtin/submodule--helper.c | 28 ++++++++++++++++++----------
> t/t7400-submodule-basic.sh | 10 +++++-----
> 2 files changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index ef2776a9e45..040b26f149d 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1539,16 +1539,24 @@ static void deinit_submodule(const char *path, const char *prefix,
> struct strbuf sb_rm = STRBUF_INIT;
> const char *format;
>
> - /*
> - * protect submodules containing a .git directory
> - * NEEDSWORK: instead of dying, automatically call
> - * absorbgitdirs and (possibly) warn.
> - */
> - if (is_directory(sub_git_dir))
> - die(_("Submodule work tree '%s' contains a .git "
> - "directory (use 'rm -rf' if you really want "
> - "to remove it including all of its history)"),
> - displaypath);
> + if (is_directory(sub_git_dir)) {
> + if (!(flags & OPT_FORCE))
> + die(_("Submodule work tree '%s' contains a "
> + ".git directory.\nUse --force if you want "
> + "to move its contents to superproject's "
> + "module directory and convert .git to a file "
> + "and then proceed with deinit."),
> + displaypath);
> +
> + if (!(flags & OPT_QUIET))
> + warning(_("Submodule work tree '%s' contains a .git "
> + "directory. This will be replaced with a "
> + ".git file by using absorbgitdirs."),
> + displaypath);
> +
> + absorb_git_dir_into_superproject(displaypath, flags);
> +
> + }
>
> if (!(flags & OPT_FORCE)) {
> struct child_process cp_rm = CHILD_PROCESS_INIT;
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index cb1b8e35dbf..3df71478d06 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -1182,18 +1182,18 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su
> rmdir init example2
> '
>
> -test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '
> +test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' '
> git submodule update --init &&
> (
> cd init &&
> rm .git &&
> - cp -R ../.git/modules/example .git &&
> + mv ../.git/modules/example .git &&
> GIT_WORK_TREE=. git config --unset core.worktree
> ) &&
> test_must_fail git submodule deinit init &&
> - test_must_fail git submodule deinit -f init &&
> - test -d init/.git &&
> - test -n "$(git config --get-regexp "submodule\.example\.")"
> + git submodule deinit -f init &&
> + ! test -d init/.git &&
> + test -z "$(git config --get-regexp "submodule\.example\.")"
I see we don't have a "test_dir_is_missing" for the pre-image, but
shouldn't the post-image "! test -d" just be a test_path_is_missing?
I.e. should this pass if .git is a file? Probably not...
next prev parent reply other threads:[~2021-09-28 9:54 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-26 18:33 [PATCH] submodule: absorb git dir instead of dying on deinit Mugdha Pattnaik via GitGitGadget
2021-08-27 6:03 ` [PATCH v2] " Mugdha Pattnaik via GitGitGadget
2021-08-27 13:20 ` Atharva Raykar
2021-08-27 17:28 ` Junio C Hamano
2021-08-27 18:10 ` [PATCH v3] " Mugdha Pattnaik via GitGitGadget
2021-08-27 18:51 ` [PATCH v4] " Mugdha Pattnaik via GitGitGadget
2021-09-28 7:30 ` Christian Couder
2021-09-28 9:53 ` Ævar Arnfjörð Bjarmason [this message]
2021-10-06 11:45 ` Mugdha Pattnaik
2021-10-06 12:02 ` [PATCH v5] " Mugdha Pattnaik via GitGitGadget
2021-10-06 12:24 ` [PATCH v6] " Mugdha Pattnaik via GitGitGadget
2021-10-07 19:41 ` Junio C Hamano
2021-11-16 18:20 ` Mugdha Pattnaik
2021-11-16 18:54 ` Junio C Hamano
2021-11-17 5:55 ` Mugdha Pattnaik
2021-11-19 10:56 ` [PATCH v7] " Mugdha Pattnaik via GitGitGadget
2021-08-27 7:51 ` [PATCH] " Bagas Sanjaya
2021-08-27 13:13 ` Mugdha Pattnaik
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87sfxpvxcs.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=bagasdotme@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=mugdhapattnaik@gmail.com \
--cc=raykar.ath@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.
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 NNTP newsgroup(s).