Jeff King <peff@xxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html