On Wed, Jul 13 2022, Glen Choo wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>> [...]
>> What do you think?
>
> Frankly I'm ok with moving things around; I think the code could use
> a little cleaning up :)
Yeah, but this whole part of it is something we'll be throwing away
entirely anyway, so I wanted to leave it at just fixing the memory leaks
as narrowly as possible for now.
I.e. we invoke "clone" from submodule--helper itself, which we don't
need to do if we just invoke it as a function, in which case this whole
argv v.s. dynamically generated difference goes away.
Well, we'll have a constant string in some cases, but we'll likely
either strdup them all with a strvec, or more likely pass the arguments
with custom "options" struct or something.
> But yeah, I think my suggestion isn't so great -
> it's a bit weird to keep around an auto variable that exists only to be
> dup-ed to the thing we care about. We can forget about that.
>
> I do think that it's worth avoiding the "sometimes dup, sometimes not"
> pattern if we can, though (of course these are just my non-C instincts
> talking), and we can do that here if we just choose not to assign back
> to .path. Something like:
>
> struct module_clone_data {
> const char *prefix;
> - const char *path;
> + char *path;
> + const char *path_argv;
>
> ...
>
> if (!is_absolute_path(clone_data->path)) {
> - strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path);
> + strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path_argv);
> + clone_data->path = strbuf_detach(&sb, NULL);
> } else {
> - clone_data->path = xstrdup(clone_data->path);
> + clone_data->path = xstrdup(clone_data->path_argv);
> }
>
> would be clearer to me since the const pointer never points to something
> that the struct actually owns.
I think that actually makes a lot of sense, I'll probably just change it
to that. I'll mull over this again when I get to re-rolling this
(depending on future comments), thanks!
> But if the "= .to_free = " idiom is well-understood and accepted to the
> point that we don't need to actively avoid "sometimes dup, sometimes
> not", then we should drop my suggestion and just go with your patch :)
FWIW "git grep ' = to_free = ' finds a fair bit of them, but luckily we
usually don't need to play that particular game...
