Re: [PATCH/RFC v3 04/16] Connect fast-import to the remote-helper via pipe, adding 'bidi-import' capability.

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 15, 2012, 17:41 UTC
Message-ID: <7vboic6oik.fsf@alter.siamese.dyndns.org>
In-Reply-To: <2444647.P1AdWcSsQk@flomedio>

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

Show 19 quoted lines

>> The updated code frees argv[] immediately after start_command()
>> returns, and it may happen to be safe to do so with the current
>> implementation of start_command() and friends, but I think it is a
>> bad taste to free argv[] (or env[] for that matter) before calling
>> finish_command().  These pieces of memory are still pointed by the
>> child_process structure, and users of the structure may want to use
>> contents of them (especially, argv[0]) for reporting errors and
>> various other purposes, e.g.
>> 
>> 	child = get_helper();
>> 
>>         trace("started %s\n", child->argv[0]);
>> 
>> 	if (finish_command(child))
>>         	return error("failed to cleanly finish %s", child->argv[0]);
>
> Yes, sounds reasonable. The present of immedate clearing has the advantage 
> that I don't have to store the struct argv_array, as struct child_process only 
> has a member for const char **argv.

And updated code shouldn't have to store struct argv_array either. If you just give the ownership of argv_array.argv to child_process and clean it as part of destroying the child_process, you do not have to worry about argv_array at all.

In order to cleanly support that use case at the API level, we may want to introduce argv_array_detach() that is similar in spirit to strbuf_detach(), which transfers ownership of the underlying memory to the caller.