{"thread":{"id":"28196","subject":"[RFH] lifetime rule for url parameter to transport_get()?","startedAt":"2011-08-23T00:34:54Z","lastAt":"2011-08-23T17:04:09Z","messageCount":3,"participants":["Junio C Hamano","Daniel Barkalow"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"174058","messageId":"7vipppt175.fsf@alter.siamese.dyndns.org","threadId":"28196","inReplyTo":null,"subject":"[RFH] lifetime rule for url parameter to transport_get()?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-23T00:34:54Z","receivedAt":"2011-08-23T00:34:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Does anybody remember why we use a copied string of \"ref_git_copy\" in\nbuiltin/clone.c::setup_reference()?\n\n\tref_git = real_path(option_reference);\n\t...\n\tref_git_copy = xstrdup(ref_git);\n\tadd_to_alternates_file(ref_git_copy);\n\tremote = remote_get(ref_git_copy);\n\ttransport = transport_get(remote, ref_git_copy);\n\tfor (extra = transport_get_remote_refs(transport); extra;\n\t     extra = extra->next)\n\t\tadd_extra_ref(extra->name, extra->old_sha1, 0);\n\ttransport_disconnect(transport);\n\tfree(ref_git_copy);\n\nThe three functions add_to_alternates_file(), remote_get(), and\ntransport_get() all get \"const char *\" so I do not think the copy was done\nto avoid \"option_reference\" from getting clobbered by these functions. The\nonly thing I can think of is that transport_get() does this:\n\n    struct transport *transport_get(struct remote *remote, const char *url)\n    {\n            const char *helper;\n            struct transport *ret = xcalloc(1, sizeof(*ret));\n\n            ...\n            if (!url && remote->url)\n                    url = remote->url[0];\n            ret->url = url;\n\t    ...\n\t    return ret;\n    }\n\nholding onto \"url\" without making a copy for its own use. But then freeing\nthat copy by the caller after calling transport_disconnect() does not make\nmuch sense to me---we could have just gave it the original option_reference,\nhave transport use it while it runs ls-remote equivalent, and then called\ntransport_disconnect(), without using any extra copy.\n\nWhat I am missing?\n"},{"id":"174099","messageId":"7vsjosrs0w.fsf@alter.siamese.dyndns.org","threadId":"28196","inReplyTo":"7vipppt175.fsf@alter.siamese.dyndns.org","subject":"Re: [RFH] lifetime rule for url parameter to transport_get()?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-23T16:50:39Z","receivedAt":"2011-08-23T16:50:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Does anybody remember why we use a copied string of \"ref_git_copy\" in\n> builtin/clone.c::setup_reference()?\n>\n> \tref_git = real_path(option_reference);\n> \t...\n> \tref_git_copy = xstrdup(ref_git);\n\nIt didn't have anything to do with transport/remote layer.\n\nThis codepath uses real_path() and optionally mkpath(), both of which\nreturns a short-lived static buffer to return its findings, and long-term\nusers are expected to copy it away.\n\nI'll add a comment to that effect in the code.\n"},{"id":"174104","messageId":"alpine.LNX.2.00.1108231252520.2056@iabervon.org","threadId":"28196","inReplyTo":"7vsjosrs0w.fsf@alter.siamese.dyndns.org","subject":"Re: [RFH] lifetime rule for url parameter to transport_get()?","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2011-08-23T17:04:09Z","receivedAt":"2011-08-23T17:04:09Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 23 Aug 2011, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Does anybody remember why we use a copied string of \"ref_git_copy\" in\n> > builtin/clone.c::setup_reference()?\n> >\n> > \tref_git = real_path(option_reference);\n> > \t...\n> > \tref_git_copy = xstrdup(ref_git);\n> \n> It didn't have anything to do with transport/remote layer.\n> \n> This codepath uses real_path() and optionally mkpath(), both of which\n> returns a short-lived static buffer to return its findings, and long-term\n> users are expected to copy it away.\n\nYeah, that fits with my expectation, given the lack of a comment and the \nfact that you were asking about clone and not also fetch.\n\nAt least originally, the remote and transport data was expected to live \nuntil the process exits, since it's a small, bounded number of small \nobjects. If I'd included functions to free the structures, I'd have had \nthem free the strings they were passed.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}