Glen Choo [off-list ref] writes:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 73717be957c..23ab9c7e349 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -1511,6 +1511,7 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
 struct module_clone_data {
 	const char *prefix;
 	const char *path;
+	char *path_free;
 	const char *name;
 	const char *url;
 	const char *depth;

@@ -1527,6 +1528,11 @@ struct module_clone_data {
 	.single_branch = -1, \
 }
 
+static void module_clone_data_release(struct module_clone_data *cd)
+{
+	free(cd->path_free);
+}
+
 struct submodule_alternate_setup {
 	const char *submodule_name;
 	enum SUBMODULE_ALTERNATE_ERROR_MODE {

@@ -1651,9 +1657,9 @@ static int clone_submodule(struct module_clone_data *clone_data)
 
 	if (!is_absolute_path(clone_data->path)) {
 		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path);
-		clone_data->path = strbuf_detach(&sb, NULL);
+		clone_data->path = clone_data->path_free = strbuf_detach(&sb, NULL);
 	} else {
-		clone_data->path = xstrdup(clone_data->path);
+		clone_data->path = clone_data->path_free = xstrdup(clone_data->path);
 	}

Hm, having .path_free doesn't seem necessary. If I'm reading the code
correctly,

- in module_clone() we set clone_data.path from argv
- then we unconditionally call clone_submodule(), which does all of the
  hard work
- in clone_submodule(), we always hit this conditional, which means that
  past this point, clone_data.path is always free()-able.

which suggests that we don't need clone_data.path_free at all. I suspect
that just this

   static void module_clone_data_release(struct module_clone_data *cd)
   {
   	free(cd->path);
   }

is enough to plug the leak (though admittedly, I haven't properly tested
the leak because it's been a PITA to set up locally).

That's a pretty hacky suggestion though, since we're still using the
same member to hold free()-able and non-free()-able memory....

Ah, here's a wacky idea (whose feasibility I haven't thought about at
all) that would make things a lot cleaner. If we had something like
OPT_STRING_DUP, that xstrdup()-s the value from argv when we parse it,
then we could drop the "const" from clone_data.path and just free() it.