Re: [PATCH] use child_process_init() to initialize struct child_process variables

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 29, 2014, 19:16 UTC
Message-ID: <xmqqlhnyy9e2.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20141029172109.GA32234@peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

> On Tue, Oct 28, 2014 at 09:52:34PM +0100, René Scharfe wrote:
>
>> --- a/bundle.c
>> +++ b/bundle.c
>> @@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path,
>>  	write_or_die(bundle_fd, "\n", 1);
>>  
>>  	/* write pack */
>> -	memset(&rls, 0, sizeof(rls));
>> +	child_process_init(&rls);
>>  	argv_array_pushl(&rls.args,
>>  			 "pack-objects", "--all-progress-implied",
>>  			 "--stdout", "--thin", "--delta-base-offset",
>
> I wondered if this one could use CHILD_PROCESS_INIT in the declaration
> instead. And indeed, we _do_ use CHILD_PROCESS_INIT there, but we use
> the same variable twice for two different child processes in the same
> function. Besides variable reuse being slightly confusing, the name
> "rls" (which presumably stands for "rev-list" for the first child) means
> nothing here, where we are calling "pack-objects". Maybe it would be
> cleaner to introduce a second variable?

It has been this way since day one at b1daf300 (Replace fork_with_pipe in bundle with run_command, 2007-03-12); I agree that two variables might make things less confusing.

> I also suspect the function would be a lot more readable broken into two
> sub-functions (reading from rev-list and writing to pack-objects), but I
> did not look closely enough to see whether there were any complicating
> factors.

Probably three helper functions:

 - The first is to find tops and bottoms (this translates fuzzy
   specifications such as "--since 30.days" into a more concrete
   revision range "^A ^B ... Z" to establish bundle prerequisites),
   which is done by running a "rev-list --boundary".

 - The second is to show refs, while paying attention to things like
   "--10 maint master" which may result in the tip of 'maint' not
   being shown at all.  I am not sure if this part can/should take
   advantage of revs.cmdline, though.

 - The last is to create the actual pack data.

I agree with your analysis on the change in column.c and trailer.c

Thanks.