On Fri, Jul 29 2022, Glen Choo wrote:

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

quoted

Add "const" to the "struct module_clone_data" that we pass to
clone_submodule(), which makes the ownership clear, and stops us from
clobbering the "clone_data->path".

We still need to add to the "reference" member, which is a "struct
string_list". Let's do this by having clone_submodule() create its
own, and copy the contents over, allowing us to pass it as a
separate parameter.

I can't help but think that this would be easier to review as part of
the leaks series since:

- Outside of leaks, I don't think we really care about ownership (though
  please please correct me if I'm off base).

- The ownership of "reference" is still quite messy (downstream code
  might append to it, but its members are sometimes free()-able and
  sometimes not), so it's hard to visualize what we're getting out of
  this change without seeing the corresponding leak fix.

I'll amend the commit message. I'll leave this in this series, as
starting to split "is this really just for the leak fix?" out of this
will generally lead to the slippery slope of bundling most of this up
again.

I think the addition of "const" helps things along independently of
that.

quoted

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6cedcc5b239..e235acce985 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -1569,18 +1567,20 @@ static char *clone_submodule_sm_gitdir(const char *name)
 	return sm_gitdir;
 }
 
-static int clone_submodule(struct module_clone_data *clone_data)
+static int clone_submodule(const struct module_clone_data *clone_data,
+			   struct string_list *reference)
 {
 	char *p;
 	char *sm_gitdir = clone_submodule_sm_gitdir(clone_data->name);
 	char *sm_alternate = NULL, *error_strategy = NULL;
 	struct child_process cp = CHILD_PROCESS_INIT;
+	const char *clone_data_path;
 
 	if (!is_absolute_path(clone_data->path))
-		clone_data->path = xstrfmt("%s/%s", get_git_work_tree(),
-					   clone_data->path);
+		clone_data_path = xstrfmt("%s/%s", get_git_work_tree(),
+					  clone_data->path);

- (this is pretty minor) It feels weird to see that we're intentionally
  leaking clone_data_path at its inception. We aren't introducing any
  new leaks, but moving this to the leaks series makes it clearer that
  we eventually do the right thing.

Here we're just preserving the status quo, memory leaks aren't really
tied to variable scope (except as a reporting convenience in some
tools). We're leaking in the same way before & after this. I think it's
better to fix the leak in the follow-up series to separate the concerns
here.