Prathamesh Chavan <pc44...@gmail.com> writes:
> + /* remove the submodule work tree (unless the user already did it) */
> + if (is_directory(path)) {
> + struct strbuf sb_rm = STRBUF_INIT;
> + const char *format;
> +
> + /*
> + * protect submodules containing a .git directory
> + * NEEDSWORK: instead of dying, automatically call
> + * absorbgitdirs and (possibly) warn.
> + */
> + if (is_directory(sub_git_dir))
> + die(_("Submodule work tree '%s' contains a .git "
> + "directory (use 'rm -rf' if you really want "
> + "to remove it including all of its history)"),
> + displaypath);
> +
> + if (!(flags & OPT_FORCE)) {
> + struct child_process cp_rm = CHILD_PROCESS_INIT;
> + cp_rm.git_cmd = 1;
> + argv_array_pushl(&cp_rm.args, "rm", "-qn",
> + path, NULL);
> +
> + if (run_command(&cp_rm))
> + die(_("Submodule work tree '%s' contains local "
> + "modifications; use '-f' to discard
> them"),
> + displaypath);
> + }
> +
> + strbuf_addstr(&sb_rm, path);
> +
> + if (!remove_dir_recursively(&sb_rm, 0))
> + format = _("Cleared directory '%s'\n");
> + else
> + format = _("Could not remove submodule work tree
> '%s'\n");
> +
> + if (!(flags & OPT_QUIET))
> + printf(format, displaypath);
> +
> + strbuf_release(&sb_rm);
> + }
> +
> + if (mkdir(path, 0777))
> + die_errno(_("could not create empty submodule directory %s"),
> + displaypath);
If path was a directory (which presumably is the normal case) and
recursive removal fails (i.e. when the code says "Could not remove"),
this mkdir() would also fail with EEXIST.
In such a case, the original code did not die and instead continued
to remove the entries for the submodule from the configuration.
This "rewritten" version dies, leaving the stale configuration for
the submodule we failed to get rid of from the working tree.
I offhand do not know which one of these error case behaviours is
more useful; the user needs to do something (e.g. loosening the perm
in some paths in the submodule that prevented "rm -rf" from working
with "chmod u+w sub/some/path" and removing it manually) to recover
in either case, and cleaning as much as possible by removing the
configuration entries even when this mkdir() fails would probably be
a better behaviour, as long as the command as a whole exits with non
zero status to signal an error.
