Re: [PATCH v3 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

Am 30.07.2013 22:15, schrieb Fredrik Gustafsson:

On Tue, Jul 30, 2013 at 09:51:51PM +0200, Jens Lehmann wrote:

quoted

+/*
+ * Try to remove the "submodule.<name>" section from .gitmodules where the given
+ * path is configured. Return 0 only if a .gitmodules file was found, a section
+ * with the correct path=<path> setting was found and we could remove it.
+ */
+int remove_path_from_gitmodules(const char *path)
+{
+	struct strbuf sect = STRBUF_INIT;
+	struct string_list_item *path_option;
+
+	if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */
+		return -1;
+
+	if (gitmodules_is_unmerged)
+		die(_("Cannot change unmerged .gitmodules, resolve merge conflicts first"));
+
+	path_option = unsorted_string_list_lookup(&config_name_for_path, path);
+	if (!path_option) {
+		warning(_("Could not find section in .gitmodules where path=%s"), path);
+		return -1;
+	}
+	strbuf_addstr(&sect, "submodule.");
+	strbuf_addstr(&sect, path_option->util);
+	if (git_config_rename_section_in_file(".gitmodules", sect.buf, NULL) < 0) {
+		/* Maybe the user already did that, don't error out here */
+		warning(_("Could not remove .gitmodules entry for %s"), path);
+		return -1;
+	}
+	strbuf_release(&sect);
+	return 0;
+}

This question applies for this function and a few more functions in this
patch that has the same characteristics.

If we're in a state when we need to return non-zero, we don't do any
cleaning (that is strbuf_release()). Since this file is in the part
called libgit AFAIK, shouldn't we always clean after us?

Right you are, thanks for bringing that up. The last return needs a
strbuf_release(), will fix that in v4.

Would it make sense to have different return values for different
errors?

I don't think so. The caller only wants to know if the modification
of the .gitmodules file happened or not, so he can decide if that
needs to be staged.