[RFH] lifetime rule for url parameter to transport_get()?

[RFH] lifetime rule for url parameter to transport_get()?

[Junio C Hamano]

Does anybody remember why we use a copied string of "ref_git_copy" in
builtin/clone.c::setup_reference()?

	ref_git = real_path(option_reference);
	...
	ref_git_copy = xstrdup(ref_git);
	add_to_alternates_file(ref_git_copy);
	remote = remote_get(ref_git_copy);
	transport = transport_get(remote, ref_git_copy);
	for (extra = transport_get_remote_refs(transport); extra;
	     extra = extra->next)
		add_extra_ref(extra->name, extra->old_sha1, 0);
	transport_disconnect(transport);
	free(ref_git_copy);

The three functions add_to_alternates_file(), remote_get(), and
transport_get() all get "const char *" so I do not think the copy was done
to avoid "option_reference" from getting clobbered by these functions. The
only thing I can think of is that transport_get() does this:

    struct transport *transport_get(struct remote *remote, const char *url)
    {
            const char *helper;
            struct transport *ret = xcalloc(1, sizeof(*ret));

            ...
            if (!url && remote->url)
                    url = remote->url[0];
            ret->url = url;
	    ...
	    return ret;
    }

holding onto "url" without making a copy for its own use. But then freeing
that copy by the caller after calling transport_disconnect() does not make
much sense to me---we could have just gave it the original option_reference,
have transport use it while it runs ls-remote equivalent, and then called
transport_disconnect(), without using any extra copy.

What I am missing?

Re: [RFH] lifetime rule for url parameter to transport_get()?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Does anybody remember why we use a copied string of "ref_git_copy" in
> builtin/clone.c::setup_reference()?
>
> 	ref_git = real_path(option_reference);
> 	...
> 	ref_git_copy = xstrdup(ref_git);

It didn't have anything to do with transport/remote layer.

This codepath uses real_path() and optionally mkpath(), both of which
returns a short-lived static buffer to return its findings, and long-term
users are expected to copy it away.

I'll add a comment to that effect in the code.

Re: [RFH] lifetime rule for url parameter to transport_get()?

[Daniel Barkalow]

On Tue, 23 Aug 2011, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Does anybody remember why we use a copied string of "ref_git_copy" in
> > builtin/clone.c::setup_reference()?
> >
> > 	ref_git = real_path(option_reference);
> > 	...
> > 	ref_git_copy = xstrdup(ref_git);
> 
> It didn't have anything to do with transport/remote layer.
> 
> This codepath uses real_path() and optionally mkpath(), both of which
> returns a short-lived static buffer to return its findings, and long-term
> users are expected to copy it away.

Yeah, that fits with my expectation, given the lack of a comment and the 
fact that you were asking about clone and not also fetch.

At least originally, the remote and transport data was expected to live 
until the process exits, since it's a small, bounded number of small 
objects. If I'd included functions to free the structures, I'd have had 
them free the strings they were passed.

	-Daniel
*This .sig left intentionally blank*