Re: sb/submodule-parallel-fetch, was: Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[Stefan Beller <sbeller@google.com>]

On Fri, Dec 11, 2015 at 1:37 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>
> Generally, I'm already quite satisfied with the state of the
> infrastructure at the tip of the branch.
>

I was about the rework the patch series.

> Nevertheless, I have a few niggles.

If you don't mind I'll split your patch into chunks and
squash these where appropriate and resend the series
with the suggestions included without the intermediate stages
of non compiling code for Windows.

>
> The primary one is that we are using a global variable of type
> struct parallel_processes to keep track of the processes. Fortunately,
> most functions already take a pointer (I gather you did anticipate an
> object oriented use of the functions). The only exception is pp_init():
> It returns a pointer to the global object, which is then passed around
> to the other functions. This does not conform to our usual style,
> however, where the initializer function takes a pointer to the object,
> too.

Noted. I thought about the init function as a constructor,
such as

    foo *pp = new foo();

in C++ would be just syntactic sugar for what I did there.
I'll adhere to Git style then instead.

>
> After converting pp_init, we can have a nicely object oriented
> collection of functions and get rid of the global object ... almost.
>
> We still need a global variable for the signal handler. But since
> signals and their handlers are a global resource, it is not that bad to
> have a global variable that is dedicated to signal handling.
>
> Another small nit is that I found it confusing that the closure
> parameters are arranged differently in the callback functions. Granted,
> in get_next_task one of them is an out parameter, but that is actually
> an argument to move it to the end of the parameter list, IMHO.
>
> On top of that I found an error or two in the documentation, and I have
> a few suggestions for improvements.
>
> All this is summarized in the patch below.

Thanks for the improvements!

Stefan