Re: [PATCHv2 5/5] rm: add absorb a submodules git dir before deletion

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 14, 2016, 18:55 UTC
Message-ID: <xmqq7f72xrvw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161213205622.841-6-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 38 quoted lines

> diff --git a/builtin/rm.c b/builtin/rm.c
> index fdd7183f61..8c9c535a88 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -392,28 +392,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  		for (i = 0; i < list.nr; i++) {
>  			const char *path = list.entry[i].name;
>  			if (list.entry[i].is_submodule) {
> -				if (is_empty_dir(path)) {
> -					if (!rmdir(path)) {
> -						removed = 1;
> -						if (!remove_path_from_gitmodules(path))
> -							gitmodules_modified = 1;
> -						continue;
> -					}
> -				} else {
> -					strbuf_reset(&buf);
> -					strbuf_addstr(&buf, path);
> -					if (!remove_dir_recursively(&buf, 0)) {
> -						removed = 1;
> -						if (!remove_path_from_gitmodules(path))
> -							gitmodules_modified = 1;
> -						strbuf_release(&buf);
> -						continue;
> -					} else if (!file_exists(path))
> -						/* Submodule was removed by user */
> -						if (!remove_path_from_gitmodules(path))
> -							gitmodules_modified = 1;
> -					/* Fallthrough and let remove_path() fail. */
> -				}
> +				if (is_empty_dir(path) && rmdir(path))
> +					die_errno("git rm: '%s'", path);
> +				if (file_exists(path))
> +					depopulate_submodule(path);
> +				removed = 1;
> +				if (!remove_path_from_gitmodules(path))
> +					gitmodules_modified = 1;
> +				continue;

The updated code structure is somewhat nicer for understanding the flow than the original, but it can further be improved.

A step "If empty directory, we try to rmdir and we check its return status and die ourselves if we couldn't remove it" reads very sensible, but coming immediately after that, "if it still exists, call depop()" looks a bit strange. I would have expected a similar construct, i.e.

	if (directory_exists(path) && depop_submodlue(path))
		die("git rm: '%s' submodule still populated", path);

there.  Also,

	if (is_empty_dir(path)) {
		if (rmdir(path))
			die_errno(...);
	} else if (is_nonempty_dir(path)) {
		if (depop_subm(path))
                	die(...);
	}

would have clarified the structure even further.

Yes, I know that you made depop_subm() to die on error, and the above is questioning if that is a sensible design choice.