Re: [PATCH v3] Teach rm to remove submodules unless they contain a git directory

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 11, 2012, 19:41 UTC
Message-ID: <7vhar4gxdq.fsf@alter.siamese.dyndns.org>
In-Reply-To: <504F8427.1020507@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

Show 17 quoted lines

>> * jl/submodule-rm (2012-08-27) 1 commit
>>  - Teach rm to remove submodules unless they contain a git directory
>> 
>> "git rm submodule" cannot blindly remove a submodule directory as
>> its working tree may have local changes, and worse yet, it may even
>> have its repository embedded in it.  Teach it some special cases
>> where it is safe to remove a submodule, specifically, when there is
>> no local changes in the submodule working tree, and its repository
>> is not embedded in its working tree but is elsewhere and uses the
>> gitfile mechanism to point at it.
>> 
>> I lost track; what is the doneness of the discussion on this patch?
>
> The review of v2 revealed that in case of submodule merge conflicts
> the necessary checks weren't done. This (and the minor issues raised
> in http://permalink.gmane.org/gmane.comp.version-control.git/204370)
> is fixed in this version.

Thanks. I wish all others paid attention to "What's cooking" like you did here.

And if it is hard to do so for whatever reason, suggest a better way for me to publish "What's cooking" or an equivalent (I am interested in finding the least bureaucratic way to help people and keep the balls rolling).

Show 20 quoted lines

> +static int check_submodules_use_gitfiles(void)
> +{
> +	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)
> +			pos = -pos-1;
> +		ce = active_cache[pos];
> +
> +		if (!S_ISGITLINK(ce->ce_mode) ||
> +		    (lstat(ce->name, &st) < 0) ||
> +		    is_empty_dir(name))
> +			continue;

If the name doesn't exist in the index (i.e. "list" has names that do not exist in the index for whatever reason), a negative pos is returned to tell you where it _would_ be inserted if you said "git add" the path. But these names in the "list" are guaranteed to exist in the index in _some_ form, so for a negative pos, (-pos-1) will have the conflicted entry at the lowest stage (typically the common ancestor's version). I am not sure checking only that one is sufficient, though. Wouldn't you want to at least check stage #2 (ours, which should most resemble the working tree)? If this were "common ancestor had it as a submodule, our side removed it and created something else, their side updated the submodule" conflict, the stage #2 would not be a gitlink (it would be a blob if that something else is a file, or may be missing if the submodule was replaced with a directory), and the path ce->name would definitely not be a submodule.

Show 31 quoted lines

> +		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;
> +}
> +
>  static int check_local_mod(unsigned char *head, int index_only)
>  {
>  	/*
> @@ -37,15 +72,23 @@ static int check_local_mod(unsigned char *head, int index_only)
>  		struct stat st;
>  		int pos;
>  		struct cache_entry *ce;
> -		const char *name = list.name[i];
> +		const char *name = list.entry[i].name;
>  		unsigned char sha1[20];
>  		unsigned mode;
>  		int local_changes = 0;
>  		int staged_changes = 0;
>
>  		pos = cache_name_pos(name, strlen(name));
> -		if (pos < 0)
> -			continue; /* removing unmerged entry */
> +		if (pos < 0) {
> +			/*
> +			 * Skip unmerged entries except for populated submodules
> +			 * that could loose history when removed.

s/loose/lose/

Show 6 quoted lines

> +			 */
> +			pos = -pos-1;
> +			if (!S_ISGITLINK(active_cache[pos]->ce_mode) ||
> +			    is_empty_dir(name))
> +				continue;
> +		}

Lilewise. It may make sense to introduce a helper function to tell if it is a submodule on our side by checking only the stage #2 entry when you see a nagetive pos returned from cache_name_pos() and call it "is_ours_submodule?()" or something.