git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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.

      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).