Stefan Beller <sbeller@xxxxxxxxxx> writes:
> 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.