From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: "René Scharfe" <l.s.r@web•de>, "Git Mailing List" <git@vger•kernel.org>
Subject: Re: [PATCH] use child_process_init() to initialize struct child_process variables
Date: Wed, 29 Oct 2014 12:16:05 -0700 [thread overview]
Message-ID: <xmqqlhnyy9e2.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20141029172109.GA32234@peff.net> (Jeff King's message of "Wed, 29 Oct 2014 13:21:09 -0400")
Jeff King <peff@peff•net> writes:
> 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.
next prev parent reply other threads:[~2014-10-29 19:16 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-28 20:52 [PATCH] use child_process_init() to initialize struct child_process variables René Scharfe
2014-10-28 21:58 ` mike.gorchak.qnx
2014-10-29 17:21 ` Jeff King
2014-10-29 19:16 ` Junio C Hamano [this message]
2014-10-30 18:07 ` Junio C Hamano
2014-10-30 21:25 ` Jeff King
2014-10-30 18:08 ` [PATCH] bundle: split out a helper function to compute and write prerequisites Junio C Hamano
2014-10-30 21:26 ` Jeff King
2014-10-30 21:35 ` [PATCH] use child_process_init() to initialize struct child_process variables Jeff King
2014-10-31 0:19 ` Philip Oakley
2014-10-31 21:48 ` Junio C Hamano
2014-11-01 3:33 ` Jeff King
2014-11-02 22:54 ` Philip Oakley
2014-11-03 18:26 ` Junio C Hamano
2014-11-03 22:04 ` Jeff King
2014-11-03 23:42 ` Junio C Hamano
2014-11-04 21:56 ` Junio C Hamano
2014-11-04 23:32 ` Jeff King
2014-11-05 0:32 ` Junio C Hamano
2014-11-05 13:41 ` Philip Oakley
2014-11-05 13:35 ` Philip Oakley
2014-11-05 19:35 ` Jeff King
2014-11-05 23:50 ` Philip Oakley
2014-11-09 13:49 ` René Scharfe
2014-11-10 7:14 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqlhnyy9e2.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=l.s.r@web$(echo .)de \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox