From: Jens Lehmann <Jens.Lehmann@web.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
"Michał Górny" <mgorny@gentoo.org>,
"Phil Hord" <phil.hord@gmail.com>,
"Heiko Voigt" <hvoigt@hvoigt.net>
Subject: Re: [RFC v2 PATCH] Teach rm to remove submodules unless they contain a git directory
Date: Tue, 28 Aug 2012 20:29:29 +0200 [thread overview]
Message-ID: <503D0E09.8010605@web.de> (raw)
In-Reply-To: <7v1uisqcef.fsf@alter.siamese.dyndns.org>
Am 27.08.2012 22:59, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>> +{
>> + int i;
>> + int errs = 0;
>> +
>> + for (i = 0; i < list.nr; i++) {
>> + const char *name = list.entry[i].name;
>> + int pos;
>> + struct cache_entry *ce;
>> + struct stat st;
>> +
>> + pos = cache_name_pos(name, strlen(name));
>> + if (pos < 0)
>> + continue; /* ignore unmerged entry */
>
> Would this cause "git rm -f path" for an unmerged submodule bypass
> the safety check?
Oops, thanks for spotting that. So replacing the "continue;" with
"pos = -pos-1;" should do the trick here, right? Will add some
tests for unmerged submodules ...
>> + ce = active_cache[pos];
>> +
>> + if (!S_ISGITLINK(ce->ce_mode) ||
>> + (lstat(ce->name, &st) < 0) ||
>> + is_empty_dir(name))
>> + continue;
>> +
>> + if (!submodule_uses_gitfile(name))
>> + errs = error(_("submodule '%s' (or one of its nested "
>> + "submodules) uses a .git directory\n"
>> + "(use 'rm -rf' if you really want to remove "
>> + "it including all of its history)"), name);
>> + }
>> +
>> + return errs;
>> +}
>
> The call to this function comes at the very end and gives us yes/no
> for the entire set of paths. After getting this error for one
> submodule and bunch of other non-submodule paths, what is the
> procedure for the user to remove it that we want to recommend in our
> documentation? Would it go like this?
>
> $ git rm path1 path2 sub path3
> ... get the above error ...
> $ git submodule --to-gitfile sub
> $ rm -fr sub
> $ git rm sub
> ... then finally ...
> $ git rm path1 path2 path3
With current git I'd recommend:
$ git rm path1 path2 sub path3
... get the above error ...
$ rm -fr sub
... try again ...
$ git rm path1 path2 sub path3
Maybe I should add the hint to repeat the git rm after removing the
submodule to the error output?
Once we implemented "git submodule --to-gitfile" it could be used
instead of "rm -fr sub" to preserve the submodule's repo if the user
wants to.
BTW: I added the same message twice, here for the forced case and in
check_local_mod() when not forced. Is there a recommended way to assign
a localized message to a static variable, so I could define it only once
and reuse it?
>> @@ -80,8 +116,11 @@ static int check_local_mod(unsigned char *head, int index_only)
>>
>> /*
>> * Is the index different from the file in the work tree?
>> + * If it's a submodule, is its work tree modified?
>> */
>> - if (ce_match_stat(ce, &st, 0))
>> + if (ce_match_stat(ce, &st, 0) ||
>> + (S_ISGITLINK(ce->ce_mode) &&
>> + !ok_to_remove_submodule(ce->name)))
>> local_changes = 1;
>
> As noted before, because we also skip these "does it match the
> index? does it match the HEAD?" checks for unmerged paths in this
> function, a submodule that has local changes or new files is
> eligible for removal during a conflicted merge. I have a feeling
> that this should be tightened a bit; wouldn't we want to check at
> least in the checked out version (i.e. stage #2 in the index) if the
> path were a submodule, even if we are in the middle of a conflicted
> merge? After all, the top level merge shouldn't have touched the
> submodule working tree, so the local modes and new files must have
> come from the end user action that was done _before_ the conflicted
> merge started, and not expendable, no?
Right, I'll change that.
prev parent reply other threads:[~2012-08-28 18:29 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-08-27 19:42 [RFC v2 PATCH] Teach rm to remove submodules unless they contain a git directory Jens Lehmann
2012-08-27 20:59 ` Junio C Hamano
2012-08-28 18:29 ` Jens Lehmann [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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=503D0E09.8010605@web.de \
--to=jens.lehmann@web.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=hvoigt@hvoigt.net \
--cc=mgorny@gentoo.org \
--cc=phil.hord@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).