Re: [PATCHv5 4/4] rm: absorb a submodules git dir before deletion

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 27, 2016, 01:10 UTC
Message-ID: <xmqqmvfich2e.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161220232012.15997-5-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 18 quoted lines

> @@ -342,6 +313,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  			exit(0);
>  	}
>  
> +	submodules_absorb_gitdir_if_needed(prefix);
> +
>  	/*
>  	 * If not forced, the file, the index and the HEAD (if exists)
>  	 * must match; but the file can already been removed, since
> @@ -358,9 +331,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  			oidclr(&oid);
>  		if (check_local_mod(&oid, index_only))
>  			exit(1);
> -	} else if (!index_only) {
> -		if (check_submodules_use_gitfiles())
> -			exit(1);
>  	}
>

Hmph. It may be a bit strange to see an "index-only" remove to touch working tree, no? Yet submodules_absorb_gitdir_if_needed() is unconditionally called above, which feels somewhat unexpected.

Show 19 quoted lines

> @@ -389,32 +359,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  	 */
>  	if (!index_only) {
>  		int removed = 0, gitmodules_modified = 0;
>  		for (i = 0; i < list.nr; i++) {
>  			const char *path = list.entry[i].name;
>  			if (list.entry[i].is_submodule) {
> +				struct strbuf buf = STRBUF_INIT;
> +
> +				strbuf_addstr(&buf, path);
> +				if (remove_dir_recursively(&buf, 0))
> +					die(_("could not remove '%s'"), path);
> +				strbuf_release(&buf);
> +
> +				removed = 1;
> +				if (!remove_path_from_gitmodules(path))
> +					gitmodules_modified = 1;
> +				continue;
>  			}

I do not see any behaviour change from the original (not quoted here), but it is somewhat surprising that "git rm ./submodule" does not really check if the submodule has local modifications and files that are not even added before remove_dir_recursively() is called.

Or perhaps I am reading the code incorrectly and such a check is done elsewhere?