[PATCHv3 03/11] run-command: omit setting file descriptors to non blocking in Windows

Stefan Beller· Nov 4, 2015, 00:37 UTC · lore

[PATCHv3 00/11] Expose the submodule parallelism to the user

 * 3 new patches (make it compile in Windows, better warnings in posix environment
   for setting fds to non blocking, drop check against NULL)
 * adressed reviews by Eric for readability. :) 
 * addressed Junios comments for the new clone helper function

Stefan Beller (11):
  run_processes_parallel: delimit intermixed task output
  run-command: report failure for degraded output just once
  run-command: omit setting file descriptors to non blocking in Windows
  submodule-config: keep update strategy around
  submodule-config: drop check against NULL
  submodule-config: remove name_and_item_from_var
  submodule-config: introduce parse_generic_submodule_config
  fetching submodules: respect `submodule.jobs` config option
  git submodule update: have a dedicated helper for cloning
  submodule update: expose parallelism to the user
  clone: allow an explicit argument for parallel submodule clones

 Documentation/config.txt        |   7 ++
 Documentation/git-clone.txt     |   6 +-
 Documentation/git-submodule.txt |   7 +-
 builtin/clone.c                 |  19 +++-
 builtin/fetch.c                 |   2 +-
 builtin/submodule--helper.c     | 239 ++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh                |  54 ++++-----
 run-command.c                   |  26 ++++-
 submodule-config.c              | 109 +++++++++++-------
 submodule-config.h              |   3 +
 submodule.c                     |   5 +
 t/t5526-fetch-submodules.sh     |  14 +++
 t/t7400-submodule-basic.sh      |   4 +-
 t/t7406-submodule-update.sh     |  27 +++++
 14 files changed, 433 insertions(+), 89 deletions(-)

-- 
2.6.1.247.ge8f2a41.dirty

Stefan Beller· Nov 4, 2015, 00:37 UTC · re: Stefan Beller · lore

[PATCHv3 01/11] run_processes_parallel: delimit intermixed task output

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/run-command.c b/run-command.c
index 0a3c24e..7c00c21 100644
--- a/run-command.c
+++ b/run-command.c
@@ -959,6 +959,9 @@ static struct parallel_processes *pp_init(int n,
 		n = online_cpus();
 
 	pp->max_processes = n;
+
+	trace_printf("run_processes_parallel: preparing to run up to %d tasks", n);
+
 	pp->data = data;
 	if (!get_next_task)
 		die("BUG: you need to specify a get_next_task function");
@@ -988,6 +991,7 @@ static void pp_cleanup(struct parallel_processes *pp)
 {
 	int i;
 
+	trace_printf("run_processes_parallel: done");
 	for (i = 0; i < pp->max_processes; i++) {
 		strbuf_release(&pp->children[i].err);
 		child_process_clear(&pp->children[i].process);

-- 
2.6.1.247.ge8f2a41.dirty

Stefan Beller· Nov 4, 2015, 00:37 UTC · re: Stefan Beller · lore

[PATCHv3 02/11] run-command: report failure for degraded output just once

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/run-command.c b/run-command.c
index 7c00c21..3ae563f 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1012,13 +1012,21 @@ static void pp_cleanup(struct parallel_processes *pp)
 
 static void set_nonblocking(int fd)
 {
+	static int reported_degrade = 0;
 	int flags = fcntl(fd, F_GETFL);
-	if (flags < 0)
-		warning("Could not get file status flags, "
-			"output will be degraded");
-	else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
-		warning("Could not set file status flags, "
-			"output will be degraded");
+	if (flags < 0) {
+		if (!reported_degrade) {
+			warning("Could not get file status flags, "
+				"output will be degraded");
+			reported_degrade = 1;
+		}
+	} else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK)) {
+		if (!reported_degrade) {
+			warning("Could not set file status flags, "
+				"output will be degraded");
+			reported_degrade = 1;
+		}
+	}
 }
 
 /* returns

-- 
2.6.1.247.ge8f2a41.dirty

Junio C Hamano· Nov 4, 2015, 18:14 UTC · re: Stefan Beller · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

Stefan Beller <sbeller@google.com> writes:

> The warning message is cluttering the output itself,
> so just report it once.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  run-command.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 7c00c21..3ae563f 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1012,13 +1012,21 @@ static void pp_cleanup(struct parallel_processes *pp)
>  
>  static void set_nonblocking(int fd)
>  {
> +	static int reported_degrade = 0;
>  	int flags = fcntl(fd, F_GETFL);
> -	if (flags < 0)
> -		warning("Could not get file status flags, "
> -			"output will be degraded");
> -	else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
> -		warning("Could not set file status flags, "
> -			"output will be degraded");
> +	if (flags < 0) {
> +		if (!reported_degrade) {
> +			warning("Could not get file status flags, "
> +				"output will be degraded");
> +			reported_degrade = 1;
> +		}
> +	} else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK)) {
> +		if (!reported_degrade) {
> +			warning("Could not set file status flags, "
> +				"output will be degraded");
> +			reported_degrade = 1;
> +		}
> +	}
>  }

Stefan Beller· Nov 4, 2015, 20:14 UTC · re: Junio C Hamano · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

On Wed, Nov 4, 2015 at 10:14 AM, Junio C Hamano <gitster@pobox.com> wrote:

> Stefan Beller <sbeller@google.com> writes:
>
>> The warning message is cluttering the output itself,
>> so just report it once.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  run-command.c | 20 ++++++++++++++------
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 7c00c21..3ae563f 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -1012,13 +1012,21 @@ static void pp_cleanup(struct parallel_processes *pp)
>>
>>  static void set_nonblocking(int fd)
>>  {
>> +     static int reported_degrade = 0;
>>       int flags = fcntl(fd, F_GETFL);
>> -     if (flags < 0)
>> -             warning("Could not get file status flags, "
>> -                     "output will be degraded");
>> -     else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
>> -             warning("Could not set file status flags, "
>> -                     "output will be degraded");
>> +     if (flags < 0) {
>> +             if (!reported_degrade) {
>> +                     warning("Could not get file status flags, "
>> +                             "output will be degraded");
>> +                     reported_degrade = 1;
>> +             }
>> +     } else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK)) {
>> +             if (!reported_degrade) {
>> +                     warning("Could not set file status flags, "
>> +                             "output will be degraded");
>> +                     reported_degrade = 1;
>> +             }
>> +     }
>>  }
>
> Imagine that we are running two things A and B at the same time.  We
> ask poll(2) and it says both A and B have some data ready to be
> read, and we try to read from A.  strbuf_read_once() would try to
> read up to 8K, relying on the fact that you earlier set the IO to be
> nonblock.  It will get stuck reading from A without allowing output
> from B to drain.  B's write may get stuck because we are not reading
> from it, and would cause B to stop making progress.
>
> What if the other sides of the connection from A and B are talking
> with each other,

> and B's non-progress caused the processing for A on
> the other side of the connection to block, causing it not to produce
> more output to allow us to make progress reading from A (so that
> eventually we can give B a chance to drain its output)?  Imagine A
> and B are pushes to the same remote, B may be pushing a change to a
> submodule while A may be pushing a matching change to its
> superproject, and the server may be trying to make sure that the
> submodule update completes and updates the ref before making the
> superproject's tree that binds that updated submodule's commit
> availble, for example?  Can we make any progress from that point?
>
> I am not convinced that the failure to set nonblock IO is merely
> "output will be degraded".  It feels more like a fatal error if we
> are driving more than one task at the same time.
>

    if (set_nonblocking(pp->children[i].process.err) < 0) {
        pp->children[i].process.err = 2;
        degraded_parallelism = 1;
    }

Johannes Sixt· Nov 4, 2015, 20:36 UTC · re: Stefan Beller · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

Am 04.11.2015 um 21:14 schrieb Stefan Beller:

> On Wed, Nov 4, 2015 at 10:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Imagine that we are running two things A and B at the same time.  We
>> ask poll(2) and it says both A and B have some data ready to be
>> read, and we try to read from A.  strbuf_read_once() would try to
>> read up to 8K, relying on the fact that you earlier set the IO to be
>> nonblock.  It will get stuck reading from A without allowing output
>> from B to drain.  B's write may get stuck because we are not reading
>> from it, and would cause B to stop making progress.
>>
>> What if the other sides of the connection from A and B are talking
>> with each other,
>
> I am not sure if we want to allow this ever. How would that work with
> jobs==1? How do we guarantee to have A and B running at the same time?

-- Hannes

Junio C Hamano· Nov 4, 2015, 21:01 UTC · re: Johannes Sixt · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

Johannes Sixt <j6t@kdbg.org> writes:

> I think that a scenario where A and B are communicating is rather
> far-fetched. We are talking about parallelizing independent tasks. I
> would not worry.

Jeff King· Nov 4, 2015, 22:56 UTC · re: Junio C Hamano · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

On Wed, Nov 04, 2015 at 01:01:53PM -0800, Junio C Hamano wrote:

> But the symptom does not have to be as severe as a total deadlock to
> be problematic.  If we block B (and other tasks) by not reading from
> them quickly because we are blocked on reading from A, which may
> take forever (in timescale of B and other tasks) to feed us enough
> to satisfy strbuf_read_once(), we are wasting resource by spawning B
> (and other tasks) early when we are not prepared to service them
> well, on both our end and on the other side of the connection.

What am I missing?

-Peff

Junio C Hamano· Nov 5, 2015, 02:05 UTC · re: Jeff King · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

Jeff King <peff@peff.net> writes:

> So I'm not sure I see why we need to be non-blocking at all here, if we
> are correctly hitting poll() and doing a single read on anybody who
> claims to be ready (rather than trying to soak up all of their available
> data), then we should never block, and we should never starve one
> process (even without blocking, we could be in a busy loop slurping from
> A and starve B, but by hitting the descriptors in round-robin for each
> poll(), we make sure they all progress).
>
> What am I missing?

Jeff King· Nov 5, 2015, 06:51 UTC · re: Junio C Hamano · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

On Wed, Nov 04, 2015 at 06:05:17PM -0800, Junio C Hamano wrote:

> I've always assumed that the original reason why we wanted to set
> the fd to nonblock was because poll(2) only tells us there is
> something to read (even a single byte), and the xread_nonblock()
> call strbuf_read_once() makes with the default size of 8KB is
> allowed to consume all available bytes and then get stuck waiting
> for the remainder of 8KB before returning.
> 
> If the read(2) in xread_nonblock() always returns as soon as we
> receive as much as there is data available without waiting for any
> more, ignoring the size of the buffer (rather, taking the size of
> the buffer only as the upper bound), then there is no need for
> nonblock anywhere.

It's easy to verify experimentally. E.g.:

  perl -e 'while(1) { syswrite(STDOUT, "a", 1); sleep(1); }' |
  strace perl -e 'while(1) { sysread(STDIN, my $buf, 1024) }'

POSIX implies it is the case in the definition of read[2] in two ways:

  1. The O_NONBLOCK behavior for pipes is mentioned only when dealing
     with empty pipes.

  2. Later, it says:

       The value returned may be less than nbyte if the number of bytes
       left in the file is less than nbyte, if the read() request was
       interrupted by a signal, or if the file is a pipe or FIFO or
       special file and has fewer than nbyte bytes immediately available
       for reading.

     That is not explicit, but the "immediately" there seems to imply
     it.

> So perhaps the original reasoning of doing nonblock was faulty, you
> are saying?

-Peff

Junio C Hamano· Nov 5, 2015, 07:32 UTC · re: Jeff King · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

Jeff King <peff@peff.net> writes:

> POSIX implies it is the case in the definition of read[2] in two ways:
>
>   1. The O_NONBLOCK behavior for pipes is mentioned only when dealing
>      with empty pipes.
>
>   2. Later, it says:
>
>        The value returned may be less than nbyte if the number of bytes
>        left in the file is less than nbyte, if the read() request was
>        interrupted by a signal, or if the file is a pipe or FIFO or
>        special file and has fewer than nbyte bytes immediately available
>        for reading.
>
>      That is not explicit, but the "immediately" there seems to imply
>      it.

Having said that,...

>> So perhaps the original reasoning of doing nonblock was faulty, you
>> are saying?
>
> Exactly. And therefore a convenient way to deal with the portability
> issue is to get rid of it. :)

Stefan Beller· Nov 5, 2015, 17:37 UTC · re: Junio C Hamano · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

On Wed, Nov 4, 2015 at 11:32 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Jeff King <peff@peff.net> writes:
>
>> POSIX implies it is the case in the definition of read[2] in two ways:
>>
>>   1. The O_NONBLOCK behavior for pipes is mentioned only when dealing
>>      with empty pipes.
>>
>>   2. Later, it says:
>>
>>        The value returned may be less than nbyte if the number of bytes
>>        left in the file is less than nbyte, if the read() request was
>>        interrupted by a signal, or if the file is a pipe or FIFO or
>>        special file and has fewer than nbyte bytes immediately available
>>        for reading.
>>
>>      That is not explicit, but the "immediately" there seems to imply
>>      it.
>
> We were reading the same book, but I was more worried about that
> "may" there; it merely tells the caller of read(2) not to be alarmed
> when the call returned without filling the entire buffer, without
> mandating the implementation of read(2) never to block.
>
> Having said that,...
>
>>> So perhaps the original reasoning of doing nonblock was faulty, you
>>> are saying?

>>
>> Exactly. And therefore a convenient way to deal with the portability
>> issue is to get rid of it. :)
>
> ... I do like the simplification you alluded to in the other
> message.  Not having to worry about the nonblock (at least until it
> is found problematic in the real world) is a very good first step,
> especially because the approach allows us to collectively make
> progress by letting all of us in various platforms build and
> experiment with "something that works".

After that I continue with the update series.

Junio C Hamano· Nov 4, 2015, 20:42 UTC · re: Stefan Beller · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

Stefan Beller <sbeller@google.com> writes:

> Another approach would be to test if we can set to non blocking and if
> that is not possible, do not buffer it, but redirect the subcommand
> directly to stderr of the calling process.
>
>     if (set_nonblocking(pp->children[i].process.err) < 0) {
>         pp->children[i].process.err = 2;
>         degraded_parallelism = 1;
>     }
>
> and once we observe the degraded_parallelism flag, we can only
> schedule a maximum of one job at a time, having direct output?

1. If we can arrange to omit the intermediary buffer processing
   without butchering the flow of the main logic with many
   #ifdef..#endif, then that would make a lot of sense to do so, and
   running the processes in parallel with mixed output might be OK.
   It may not be very nice, but should be an acceptable compromise.

2. If we need to sprinkle conditional compilation all over the place
   to do so, then I do not think it is worth it.  Instead, we should
   keep a single code structure, and forbid setting numtasks to more
   than one, which would also remove the need for nonblock IO.

Stefan Beller· Nov 4, 2015, 21:04 UTC · re: Junio C Hamano · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

On Wed, Nov 4, 2015 at 12:42 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Stefan Beller <sbeller@google.com> writes:
>
>> Another approach would be to test if we can set to non blocking and if
>> that is not possible, do not buffer it, but redirect the subcommand
>> directly to stderr of the calling process.
>>
>>     if (set_nonblocking(pp->children[i].process.err) < 0) {
>>         pp->children[i].process.err = 2;
>>         degraded_parallelism = 1;
>>     }
>>
>> and once we observe the degraded_parallelism flag, we can only
>> schedule a maximum of one job at a time, having direct output?
>
> I would even say that on a platform that is _capable_ of setting fd
> non-blocking, we should signal a grave error and die if an attempt
> to do so fails, period.

So more like:

    if (platform_capable_non_blocking_IO())
        set_nonblocking_or_die(&pp->children[i].process.err);
    else
        pp->children[i].process.err = 2; /* ugly intermixed output is possible*/

>
> On the other hand, on a platform that is known to be incapable
> (e.g. lacks SETFL or NONBLOCK), we have two options.
>
> 1. If we can arrange to omit the intermediary buffer processing
>    without butchering the flow of the main logic with many
>    #ifdef..#endif, then that would make a lot of sense to do so, and
>    running the processes in parallel with mixed output might be OK.
>    It may not be very nice, but should be an acceptable compromise.

>From what I hear this kind of output is very annoying. (One of the

main complaints of repo users beside missing atomic fetch transactions)

>
> 2. If we need to sprinkle conditional compilation all over the place
>    to do so, then I do not think it is worth it.  Instead, we should
>    keep a single code structure, and forbid setting numtasks to more
>    than one, which would also remove the need for nonblock IO.

    if (!platform_capable_non_blocking_IO())
        pp.max_processes = 1;

>
> Either way, bringing "parallelism with sequential output" to
> platforms without nonblock IO can be left for a later day, when we
> find either (1) a good approach that does not require nonblock IO to
> do this, or (2) a good approach to do a nonblock IO on these
> platforms (we know about Windows, but there may be others; I dunno).
>

Junio C Hamano· Nov 4, 2015, 21:19 UTC · re: Stefan Beller · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

Stefan Beller <sbeller@google.com> writes:

> So more like:
>
>     if (platform_capable_non_blocking_IO())
>         set_nonblocking_or_die(&pp->children[i].process.err);
>     else
>         pp->children[i].process.err = 2; /* ugly intermixed output is possible*/

>> On the other hand, on a platform that is known to be incapable
>> (e.g. lacks SETFL or NONBLOCK), we have two options.
>>
>> 1. If we can arrange to omit the intermediary buffer processing
>>    without butchering the flow of the main logic with many
>>    #ifdef..#endif, then that would make a lot of sense to do so, and
>>    running the processes in parallel with mixed output might be OK.
>>    It may not be very nice, but should be an acceptable compromise.
>
> From what I hear this kind of output is very annoying. (One of the
> main complaints of repo users beside missing atomic fetch transactions)

>> Either way, bringing "parallelism with sequential output" to
>> platforms without nonblock IO can be left for a later day, when we
>> find either (1) a good approach that does not require nonblock IO to
>> do this, or (2) a good approach to do a nonblock IO on these
>> platforms (we know about Windows, but there may be others; I dunno).

Stefan Beller· Nov 4, 2015, 21:41 UTC · re: Junio C Hamano · lore

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

On Wed, Nov 4, 2015 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Stefan Beller <sbeller@google.com> writes:
>
>> So more like:
>>
>>     if (platform_capable_non_blocking_IO())
>>         set_nonblocking_or_die(&pp->children[i].process.err);
>>     else
>>         pp->children[i].process.err = 2; /* ugly intermixed output is possible*/
>
> When I mentioned #if..#endif, I didn't mean it as a dogmatic
> "conditional compilation is wrong" sense.  It was more about "the
> high-level flow of logic should not have to know too much about
> platform peculiarities".  As platform_capable_non_blocking_IO()
> function would be a constant function after you compile it for a
> single platform, if you add 10 instances of such if/else in a patch
> that adds 250 lines, unless the change is to add a set of lowest
> level helpers to be called from the higher-level flow of logic so
> that the callers do not have to know about the platform details,
> that's just as bad as adding 10 instances of #if..#endif.

Right. Yeah I was just outlining the program flow as I understood it.

Specially:
 * It's fine if the platform doesn't support non blocking IO
 * but if the platform claims to support it and fails to, this is a hard error.
    How is this worse than the platform claiming to not support it at all?

>
>>> On the other hand, on a platform that is known to be incapable
>>> (e.g. lacks SETFL or NONBLOCK), we have two options.
>>>
>>> 1. If we can arrange to omit the intermediary buffer processing
>>>    without butchering the flow of the main logic with many
>>>    #ifdef..#endif, then that would make a lot of sense to do so, and
>>>    running the processes in parallel with mixed output might be OK.
>>>    It may not be very nice, but should be an acceptable compromise.
>>
>> From what I hear this kind of output is very annoying. (One of the
>> main complaints of repo users beside missing atomic fetch transactions)
>
> When (1) "parallelism with sequential output" is the desired
> outcome, (2) on some platforms we haven't found a way to achieve
> both, and (3) a non-sequential output is unacceptable, then
> parallelism has to give :-(.

Ok, then I will go that route.

>
> I was getting an impression from your "not buffer" suggestion that
> "sequential output" would be the one that can be sacrificed, but
> that is OK.  Until we find a way to achieve both at the same time,
> achieving only either one or the other is better than achieving
> nothing.

>
>>> Either way, bringing "parallelism with sequential output" to
>>> platforms without nonblock IO can be left for a later day, when we
>>> find either (1) a good approach that does not require nonblock IO to
>>> do this, or (2) a good approach to do a nonblock IO on these
>>> platforms (we know about Windows, but there may be others; I dunno).

Stefan Beller· Nov 4, 2015, 00:37 UTC · re: Stefan Beller · lore

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/run-command.c b/run-command.c
index 3ae563f..8db7df8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1012,6 +1012,7 @@ static void pp_cleanup(struct parallel_processes *pp)
 
 static void set_nonblocking(int fd)
 {
+#ifndef GIT_WINDOWS_NATIVE
 	static int reported_degrade = 0;
 	int flags = fcntl(fd, F_GETFL);
 	if (flags < 0) {
@@ -1027,6 +1028,7 @@ static void set_nonblocking(int fd)
 			reported_degrade = 1;
 		}
 	}
+#endif
 }
 
 /* returns

-- 
2.6.1.247.ge8f2a41.dirty

Stefan Beller· Nov 4, 2015, 00:37 UTC · re: Stefan Beller · lore

[PATCHv3 04/11] submodule-config: keep update strategy around

We need the submodule update strategies in a later patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 submodule-config.c | 11 +++++++++++
 submodule-config.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index afe0ea8..4239b0e 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -194,6 +194,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
 
 	submodule->path = NULL;
 	submodule->url = NULL;
+	submodule->update = NULL;
 	submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
 	submodule->ignore = NULL;
 
@@ -311,6 +312,16 @@ static int parse_config(const char *var, const char *value, void *data)
 			free((void *) submodule->url);
 			submodule->url = xstrdup(value);
 		}
+	} else if (!strcmp(item.buf, "update")) {
+		if (!value)
+			ret = config_error_nonbool(var);
+		else if (!me->overwrite && submodule->update != NULL)
+			warn_multiple_config(me->commit_sha1, submodule->name,
+					     "update");
+		else {
+			free((void *) submodule->update);
+			submodule->update = xstrdup(value);
+		}
 	}
 
 	strbuf_release(&name);
diff --git a/submodule-config.h b/submodule-config.h
index 9061e4e..f9e2a29 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -14,6 +14,7 @@ struct submodule {
 	const char *url;
 	int fetch_recurse;
 	const char *ignore;
+	const char *update;
 	/* the sha1 blob id of the responsible .gitmodules file */
 	unsigned char gitmodules_sha1[20];
 };

-- 
2.6.1.247.ge8f2a41.dirty

Stefan Beller· Nov 4, 2015, 00:37 UTC · re: Stefan Beller · lore

[PATCHv3 05/11] submodule-config: drop check against NULL

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule-config.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 4239b0e..6d01941 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -265,7 +265,7 @@ static int parse_config(const char *var, const char *value, void *data)
 	if (!strcmp(item.buf, "path")) {
 		if (!value)
 			ret = config_error_nonbool(var);
-		else if (!me->overwrite && submodule->path != NULL)
+		else if (!me->overwrite && submodule->path)
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					"path");
 		else {
@@ -289,7 +289,7 @@ static int parse_config(const char *var, const char *value, void *data)
 	} else if (!strcmp(item.buf, "ignore")) {
 		if (!value)
 			ret = config_error_nonbool(var);
-		else if (!me->overwrite && submodule->ignore != NULL)
+		else if (!me->overwrite && submodule->ignore)
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					"ignore");
 		else if (strcmp(value, "untracked") &&
@@ -305,7 +305,7 @@ static int parse_config(const char *var, const char *value, void *data)
 	} else if (!strcmp(item.buf, "url")) {
 		if (!value) {
 			ret = config_error_nonbool(var);
-		} else if (!me->overwrite && submodule->url != NULL) {
+		} else if (!me->overwrite && submodule->url) {
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					"url");
 		} else {
@@ -315,7 +315,7 @@ static int parse_config(const char *var, const char *value, void *data)
 	} else if (!strcmp(item.buf, "update")) {
 		if (!value)
 			ret = config_error_nonbool(var);
-		else if (!me->overwrite && submodule->update != NULL)
+		else if (!me->overwrite && submodule->update)
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					     "update");
 		else {

-- 
2.6.1.247.ge8f2a41.dirty

Stefan Beller· Nov 4, 2015, 00:37 UTC · re: Stefan Beller · lore

[PATCHv3 06/11] submodule-config: remove name_and_item_from_var

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule-config.c | 48 ++++++++++++++++--------------------------------
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 6d01941..b826841 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -161,31 +161,17 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,
 	return NULL;
 }
 
-static int name_and_item_from_var(const char *var, struct strbuf *name,
-				  struct strbuf *item)
-{
-	const char *subsection, *key;
-	int subsection_len, parse;
-	parse = parse_config_key(var, "submodule", &subsection,
-			&subsection_len, &key);
-	if (parse < 0 || !subsection)
-		return 0;
-
-	strbuf_add(name, subsection, subsection_len);
-	strbuf_addstr(item, key);
-
-	return 1;
-}
-
 static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
-		const unsigned char *gitmodules_sha1, const char *name)
+						  const unsigned char *gitmodules_sha1,
+						  const char *name_ptr, int name_len)
 {
 	struct submodule *submodule;
 	struct strbuf name_buf = STRBUF_INIT;
+	char *name = xmemdupz(name_ptr, name_len);
 
 	submodule = cache_lookup_name(cache, gitmodules_sha1, name);
 	if (submodule)
-		return submodule;
+		goto out;
 
 	submodule = xmalloc(sizeof(*submodule));
 
@@ -201,7 +187,8 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
 	hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
 
 	cache_add(cache, submodule);
-
+out:
+	free(name);
 	return submodule;
 }
 
@@ -251,18 +238,18 @@ static int parse_config(const char *var, const char *value, void *data)
 {
 	struct parse_config_parameter *me = data;
 	struct submodule *submodule;
-	struct strbuf name = STRBUF_INIT, item = STRBUF_INIT;
-	int ret = 0;
+	int subsection_len, ret = 0;
+	const char *subsection, *key;
 
-	/* this also ensures that we only parse submodule entries */
-	if (!name_and_item_from_var(var, &name, &item))
+	if (parse_config_key(var, "submodule", &subsection,
+			     &subsection_len, &key) < 0 || !subsection_len)
 		return 0;
 
 	submodule = lookup_or_create_by_name(me->cache,
 					     me->gitmodules_sha1,
-					     name.buf);
+					     subsection, subsection_len);
 
-	if (!strcmp(item.buf, "path")) {
+	if (!strcmp(key, "path")) {
 		if (!value)
 			ret = config_error_nonbool(var);
 		else if (!me->overwrite && submodule->path)
@@ -275,7 +262,7 @@ static int parse_config(const char *var, const char *value, void *data)
 			submodule->path = xstrdup(value);
 			cache_put_path(me->cache, submodule);
 		}
-	} else if (!strcmp(item.buf, "fetchrecursesubmodules")) {
+	} else if (!strcmp(key, "fetchrecursesubmodules")) {
 		/* when parsing worktree configurations we can die early */
 		int die_on_error = is_null_sha1(me->gitmodules_sha1);
 		if (!me->overwrite &&
@@ -286,7 +273,7 @@ static int parse_config(const char *var, const char *value, void *data)
 			submodule->fetch_recurse = parse_fetch_recurse(
 								var, value,
 								die_on_error);
-	} else if (!strcmp(item.buf, "ignore")) {
+	} else if (!strcmp(key, "ignore")) {
 		if (!value)
 			ret = config_error_nonbool(var);
 		else if (!me->overwrite && submodule->ignore)
@@ -302,7 +289,7 @@ static int parse_config(const char *var, const char *value, void *data)
 			free((void *) submodule->ignore);
 			submodule->ignore = xstrdup(value);
 		}
-	} else if (!strcmp(item.buf, "url")) {
+	} else if (!strcmp(key, "url")) {
 		if (!value) {
 			ret = config_error_nonbool(var);
 		} else if (!me->overwrite && submodule->url) {
@@ -312,7 +299,7 @@ static int parse_config(const char *var, const char *value, void *data)
 			free((void *) submodule->url);
 			submodule->url = xstrdup(value);
 		}
-	} else if (!strcmp(item.buf, "update")) {
+	} else if (!strcmp(key, "update")) {
 		if (!value)
 			ret = config_error_nonbool(var);
 		else if (!me->overwrite && submodule->update)
@@ -324,9 +311,6 @@ static int parse_config(const char *var, const char *value, void *data)
 		}
 	}
 
-	strbuf_release(&name);
-	strbuf_release(&item);
-
 	return ret;
 }
 

-- 
2.6.1.247.ge8f2a41.dirty

Stefan Beller· Nov 4, 2015, 00:37 UTC · re: Stefan Beller · lore

[PATCHv3 07/11] submodule-config: introduce parse_generic_submodule_config

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule-config.c | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index b826841..29e21b2 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -234,17 +234,22 @@ struct parse_config_parameter {
 	int overwrite;
 };
 
-static int parse_config(const char *var, const char *value, void *data)
+static int parse_generic_submodule_config(const char *key,
+					  const char *var,
+					  const char *value,
+					  struct parse_config_parameter *me)
 {
-	struct parse_config_parameter *me = data;
-	struct submodule *submodule;
-	int subsection_len, ret = 0;
-	const char *subsection, *key;
-
-	if (parse_config_key(var, "submodule", &subsection,
-			     &subsection_len, &key) < 0 || !subsection_len)
-		return 0;
+	return 0;
+}
 
+static int parse_specific_submodule_config(const char *subsection, int subsection_len,
+					   const char *key,
+					   const char *var,
+					   const char *value,
+					   struct parse_config_parameter *me)
+{
+	int ret = 0;
+	struct submodule *submodule;
 	submodule = lookup_or_create_by_name(me->cache,
 					     me->gitmodules_sha1,
 					     subsection, subsection_len);
@@ -314,6 +319,24 @@ static int parse_config(const char *var, const char *value, void *data)
 	return ret;
 }
 
+static int parse_config(const char *var, const char *value, void *data)
+{
+	struct parse_config_parameter *me = data;
+	int subsection_len;
+	const char *subsection, *key;
+
+	if (parse_config_key(var, "submodule", &subsection,
+			     &subsection_len, &key) < 0)
+		return 0;
+
+	if (!subsection_len)
+		return parse_generic_submodule_config(key, var, value, me);
+	else
+		return parse_specific_submodule_config(subsection,
+						       subsection_len, key,
+						       var, value, me);
+}
+
 static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
 				      unsigned char *gitmodules_sha1)
 {

-- 
2.6.1.247.ge8f2a41.dirty

Stefan Beller· Nov 4, 2015, 00:37 UTC · re: Stefan Beller · lore

[PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/config.txt    |  7 +++++++
 builtin/fetch.c             |  2 +-
 submodule-config.c          | 15 +++++++++++++++
 submodule-config.h          |  2 ++
 submodule.c                 |  5 +++++
 t/t5526-fetch-submodules.sh | 14 ++++++++++++++
 6 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..70e1b88 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2643,6 +2643,13 @@ submodule.<name>.ignore::
 	"--ignore-submodules" option. The 'git submodule' commands are not
 	affected by this setting.
 
+submodule.jobs::
+	This is used to determine how many submodules can be operated on in
+	parallel. Specifying a positive integer allows up to that number
+	of submodules being fetched in parallel. This is used in fetch
+	and clone operations only. A value of 0 will give some reasonable
+	configuration. It defaults to 1.
+
 tag.sort::
 	This variable controls the sort ordering of tags when displayed by
 	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9cc1c9d..60e6797 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */
 static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow;
-static int max_children = 1;
+static int max_children = -1;
 static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
diff --git a/submodule-config.c b/submodule-config.c
index 29e21b2..475551a 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -32,6 +32,7 @@ enum lookup_type {
 
 static struct submodule_cache cache;
 static int is_cache_init;
+static int parallel_jobs = -1;
 
 static int config_path_cmp(const struct submodule_entry *a,
 			   const struct submodule_entry *b,
@@ -239,6 +240,15 @@ static int parse_generic_submodule_config(const char *key,
 					  const char *value,
 					  struct parse_config_parameter *me)
 {
+	if (!strcmp(key, "jobs")) {
+		parallel_jobs = strtol(value, NULL, 10);
+		if (parallel_jobs < 0) {
+			warning("submodule.jobs not allowed to be negative.");
+			parallel_jobs = 1;
+			return 1;
+		}
+	}
+
 	return 0;
 }
 
@@ -482,3 +492,8 @@ void submodule_free(void)
 	cache_free(&cache);
 	is_cache_init = 0;
 }
+
+int config_parallel_submodules(void)
+{
+	return parallel_jobs;
+}
diff --git a/submodule-config.h b/submodule-config.h
index f9e2a29..d9bbf9a 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -27,4 +27,6 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
 		const char *path);
 void submodule_free(void);
 
+int config_parallel_submodules(void);
+
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index 0257ea3..188ba02 100644
--- a/submodule.c
+++ b/submodule.c
@@ -752,6 +752,11 @@ int fetch_populated_submodules(const struct argv_array *options,
 	argv_array_push(&spf.args, "--recurse-submodules-default");
 	/* default value, "--submodule-prefix" and its value are added later */
 
+	if (max_parallel_jobs < 0)
+		max_parallel_jobs = config_parallel_submodules();
+	if (max_parallel_jobs < 0)
+		max_parallel_jobs = 1;
+
 	calculate_changed_submodule_paths();
 	run_processes_parallel(max_parallel_jobs,
 			       get_next_submodule,
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 1b4ce69..5c3579c 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -470,4 +470,18 @@ test_expect_success "don't fetch submodule when newly recorded commits are alrea
 	test_i18ncmp expect.err actual.err
 '
 
+test_expect_success 'fetching submodules respects parallel settings' '
+	git config fetch.recurseSubmodules true &&
+	(
+		cd downstream &&
+		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&
+		grep "7 children" trace.out &&
+		git config submodule.jobs 8 &&
+		GIT_TRACE=$(pwd)/trace.out git fetch &&
+		grep "8 children" trace.out &&
+		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&
+		grep "9 children" trace.out
+	)
+'
+
 test_done

-- 
2.6.1.247.ge8f2a41.dirty

Jens Lehmann· Nov 10, 2015, 22:21 UTC · re: Stefan Beller · lore

Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

Am 04.11.2015 um 01:37 schrieb Stefan Beller:

> This allows to configure fetching and updating in parallel
> without having the command line option.
>
> This moved the responsibility to determine how many parallel processes
> to start from builtin/fetch to submodule.c as we need a way to communicate
> "The user did not specify the number of parallel processes in the command
> line options" in the builtin fetch. The submodule code takes care of
> the precedence (CLI > config > default)
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>   Documentation/config.txt    |  7 +++++++
>   builtin/fetch.c             |  2 +-
>   submodule-config.c          | 15 +++++++++++++++
>   submodule-config.h          |  2 ++
>   submodule.c                 |  5 +++++
>   t/t5526-fetch-submodules.sh | 14 ++++++++++++++
>   6 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 391a0c3..70e1b88 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2643,6 +2643,13 @@ submodule.<name>.ignore::
>   	"--ignore-submodules" option. The 'git submodule' commands are not
>   	affected by this setting.
>
> +submodule.jobs::
> +	This is used to determine how many submodules can be operated on in
> +	parallel. Specifying a positive integer allows up to that number
> +	of submodules being fetched in parallel. This is used in fetch
> +	and clone operations only. A value of 0 will give some reasonable
> +	configuration. It defaults to 1.
> +

>   tag.sort::
>   	This variable controls the sort ordering of tags when displayed by
>   	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 9cc1c9d..60e6797 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */
>   static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;
>   static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
>   static int tags = TAGS_DEFAULT, unshallow, update_shallow;
> -static int max_children = 1;
> +static int max_children = -1;
>   static const char *depth;
>   static const char *upload_pack;
>   static struct strbuf default_rla = STRBUF_INIT;
> diff --git a/submodule-config.c b/submodule-config.c
> index 29e21b2..475551a 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -32,6 +32,7 @@ enum lookup_type {
>
>   static struct submodule_cache cache;
>   static int is_cache_init;
> +static int parallel_jobs = -1;
>
>   static int config_path_cmp(const struct submodule_entry *a,
>   			   const struct submodule_entry *b,
> @@ -239,6 +240,15 @@ static int parse_generic_submodule_config(const char *key,
>   					  const char *value,
>   					  struct parse_config_parameter *me)
>   {
> +	if (!strcmp(key, "jobs")) {
> +		parallel_jobs = strtol(value, NULL, 10);
> +		if (parallel_jobs < 0) {
> +			warning("submodule.jobs not allowed to be negative.");
> +			parallel_jobs = 1;
> +			return 1;
> +		}
> +	}
> +
>   	return 0;
>   }
>
> @@ -482,3 +492,8 @@ void submodule_free(void)
>   	cache_free(&cache);
>   	is_cache_init = 0;
>   }
> +
> +int config_parallel_submodules(void)
> +{
> +	return parallel_jobs;
> +}
> diff --git a/submodule-config.h b/submodule-config.h
> index f9e2a29..d9bbf9a 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -27,4 +27,6 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
>   		const char *path);
>   void submodule_free(void);
>
> +int config_parallel_submodules(void);
> +
>   #endif /* SUBMODULE_CONFIG_H */
> diff --git a/submodule.c b/submodule.c
> index 0257ea3..188ba02 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -752,6 +752,11 @@ int fetch_populated_submodules(const struct argv_array *options,
>   	argv_array_push(&spf.args, "--recurse-submodules-default");
>   	/* default value, "--submodule-prefix" and its value are added later */
>
> +	if (max_parallel_jobs < 0)
> +		max_parallel_jobs = config_parallel_submodules();
> +	if (max_parallel_jobs < 0)
> +		max_parallel_jobs = 1;
> +
>   	calculate_changed_submodule_paths();
>   	run_processes_parallel(max_parallel_jobs,
>   			       get_next_submodule,
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index 1b4ce69..5c3579c 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -470,4 +470,18 @@ test_expect_success "don't fetch submodule when newly recorded commits are alrea
>   	test_i18ncmp expect.err actual.err
>   '
>
> +test_expect_success 'fetching submodules respects parallel settings' '
> +	git config fetch.recurseSubmodules true &&
> +	(
> +		cd downstream &&
> +		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&
> +		grep "7 children" trace.out &&
> +		git config submodule.jobs 8 &&
> +		GIT_TRACE=$(pwd)/trace.out git fetch &&
> +		grep "8 children" trace.out &&
> +		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&
> +		grep "9 children" trace.out
> +	)
> +'
> +
>   test_done
>

Stefan Beller· Nov 10, 2015, 22:29 UTC · re: Jens Lehmann · lore

Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

On Tue, Nov 10, 2015 at 2:21 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:

>> +submodule.jobs::
>> +       This is used to determine how many submodules can be operated on in
>> +       parallel. Specifying a positive integer allows up to that number
>> +       of submodules being fetched in parallel. This is used in fetch
>> +       and clone operations only. A value of 0 will give some reasonable
>> +       configuration. It defaults to 1.
>> +
>
>
> Just curious (and sorry if this has already been discussed and I missed
> it, but the volume of your output is too much for my current git time
> budget ;-): While this config is for fetching only, do I recall correctly
> that you have plans to do submodule work tree updates in parallel too?
> If so, would it make sense to have different settings for fetching and
> updating?

Jens Lehmann· Nov 11, 2015, 19:55 UTC · re: Stefan Beller · lore

Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

Am 10.11.2015 um 23:29 schrieb Stefan Beller:

> On Tue, Nov 10, 2015 at 2:21 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>> +submodule.jobs::
>>> +       This is used to determine how many submodules can be operated on in
>>> +       parallel. Specifying a positive integer allows up to that number
>>> +       of submodules being fetched in parallel. This is used in fetch
>>> +       and clone operations only. A value of 0 will give some reasonable
>>> +       configuration. It defaults to 1.
>>> +
>>
>>
>> Just curious (and sorry if this has already been discussed and I missed
>> it, but the volume of your output is too much for my current git time
>> budget ;-): While this config is for fetching only, do I recall correctly
>> that you have plans to do submodule work tree updates in parallel too?
>> If so, would it make sense to have different settings for fetching and
>> updating?
>
> TL;DR: checkout is serial, network-related stuff only will be using
> submodule.jobs

> In the next series (origin/sb/submodule-parallel-update) this is reused for
> fetches, clones, so only the network stuff. The checkout (as all local
> operations)
> is still done serially, as then you don't run into problems in
> parallel at the same time.
> (checkouts may be parallelized but I haven't done that yet, and postpone that
> until it has settled a bit more)

Makes sense.

Stefan Beller· Nov 11, 2015, 23:34 UTC · re: Jens Lehmann · lore

Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

On Wed, Nov 11, 2015 at 11:55 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:

>>
>> TL;DR: checkout is serial, network-related stuff only will be using
>> submodule.jobs
>
>
> My point being: isn't "jobs" a bit too generic for a config option that
> is only relevant for network-related stuff? Maybe "submodule.fetchJobs"
> or similar would be better, as you are already thinking about adding
> other parallelisms with different constraints later?

Actually I don't think that far ahead.

Jens Lehmann· Nov 13, 2015, 20:47 UTC · re: Stefan Beller · lore

Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

Am 12.11.2015 um 00:34 schrieb Stefan Beller:

> On Wed, Nov 11, 2015 at 11:55 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>>
>>> TL;DR: checkout is serial, network-related stuff only will be using
>>> submodule.jobs
>>
>>
>> My point being: isn't "jobs" a bit too generic for a config option that
>> is only relevant for network-related stuff? Maybe "submodule.fetchJobs"
>> or similar would be better, as you are already thinking about adding
>> other parallelisms with different constraints later?
>
> Actually I don't think that far ahead.

> (I assume network to be the bottleneck for clone/fetch operations)
> All I want is a saturated network all the time, and as the native git protocol
> doesn't provide that (tcp startup takes time until full band witdth is reached,
> local operations both on client and server) I added the parallel stuff
> to 'smear' different submodule network traffics along the timeline,
> such that we have a better approximation of an always fully saturated link
> for the whole operation. So in the long term future, we maybe want to
> reuse an http/ssh session for a different submodule, possibly interleaving
> the different submodules on the wire to make it even faster. Though that
> may not be helping much.
>
> So we're back at bike shedding about the name. submodule.fetchJobs
> sounds like it only applies to fetching, do you think it's sufficient for clone
> as well?

> Once upon a time, Junio used  'submodule.fetchParallel' or  'submodule.paralle'
> in a discussion[1] for the distinction of the local and networked things.
> [1] Discussing "[PATCH] Add fetch.recurseSubmoduleParallelism config option"
>
> How about submodules.parallelNetwork for the networking part and
> submodules.parallelLocal for the local part? (I don't implement parallelLocal in
> the next few weeks I'd estimate).

Stefan Beller· Nov 13, 2015, 21:29 UTC · re: Jens Lehmann · lore

Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

On Fri, Nov 13, 2015 at 12:47 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:

> Am 12.11.2015 um 00:34 schrieb Stefan Beller:
>>
>> On Wed, Nov 11, 2015 at 11:55 AM, Jens Lehmann <Jens.Lehmann@web.de>
>> wrote:
>>>>
>>>>
>>>> TL;DR: checkout is serial, network-related stuff only will be using
>>>> submodule.jobs
>>>
>>>
>>>
>>> My point being: isn't "jobs" a bit too generic for a config option that
>>> is only relevant for network-related stuff? Maybe "submodule.fetchJobs"
>>> or similar would be better, as you are already thinking about adding
>>> other parallelisms with different constraints later?
>>
>>
>> Actually I don't think that far ahead.
>
>
> Maybe I've been bitten once too often by too generic names that became
> a problem later on ... ;-)
>
>> (I assume network to be the bottleneck for clone/fetch operations)
>> All I want is a saturated network all the time, and as the native git
>> protocol
>> doesn't provide that (tcp startup takes time until full band witdth is
>> reached,
>> local operations both on client and server) I added the parallel stuff
>> to 'smear' different submodule network traffics along the timeline,
>> such that we have a better approximation of an always fully saturated link
>> for the whole operation. So in the long term future, we maybe want to
>> reuse an http/ssh session for a different submodule, possibly interleaving
>> the different submodules on the wire to make it even faster. Though that
>> may not be helping much.
>>
>> So we're back at bike shedding about the name. submodule.fetchJobs
>> sounds like it only applies to fetching, do you think it's sufficient for
>> clone
>> as well?
>
>
> Hmm, to me fetching is a part of cloning, so I don't have a problem with
> that. And documenting it accordingly should make it clear to everyone.
>
>> Once upon a time, Junio used  'submodule.fetchParallel' or
>> 'submodule.paralle'
>> in a discussion[1] for the distinction of the local and networked things.
>> [1] Discussing "[PATCH] Add fetch.recurseSubmoduleParallelism config
>> option"
>>
>> How about submodules.parallelNetwork for the networking part and
>> submodules.parallelLocal for the local part? (I don't implement
>> parallelLocal in
>> the next few weeks I'd estimate).
>
>
> If 'submodules.parallelNetwork' will be used for submodule push too as
> soon as that learns parallel operation, I'm ok with that. But if we don't
> have good reason to believe the number of jobs for fetch can simply be
> reused for push, me thinks we should have one config option containing the
> term "fetch" now and another that contains "push" when we need it later,
> just to be on the safe side. Otherwise it might be hard to explain to
> users why 'submodules.parallelNetwork' is only used for fetch and clone
> and why they have to set 'submodules.parallelPush' for pushing ...
>
> So either 'submodule.fetchParallel' or 'submodule.fetchJobs' is fine for
> me, and 'submodules.parallelNetwork' is ok too as long as we have reason
> to believe this value can be used for push later too.

Stefan Beller· Nov 4, 2015, 00:37 UTC · re: Stefan Beller · lore

[PATCHv3 09/11] git submodule update: have a dedicated helper for cloning

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 229 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  45 +++------
 t/t7400-submodule-basic.sh  |   4 +-
 3 files changed, 242 insertions(+), 36 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff..95b45a2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -255,6 +255,234 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static int git_submodule_config(const char *var, const char *value, void *cb)
+{
+	return parse_submodule_config_option(var, value);
+}
+
+struct submodule_update_clone {
+	/* states */
+	int count;
+	int print_unmatched;
+	/* configuration */
+	int quiet;
+	const char *reference;
+	const char *depth;
+	const char *update;
+	const char *recursive_prefix;
+	const char *prefix;
+	struct module_list list;
+	struct string_list projectlines;
+	struct pathspec pathspec;
+};
+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP}
+
+static void fill_clone_command(struct child_process *cp, int quiet,
+			       const char *prefix, const char *path,
+			       const char *name, const char *url,
+			       const char *reference, const char *depth)
+{
+	cp->git_cmd = 1;
+	cp->no_stdin = 1;
+	cp->stdout_to_stderr = 1;
+	cp->err = -1;
+	argv_array_push(&cp->args, "submodule--helper");
+	argv_array_push(&cp->args, "clone");
+	if (quiet)
+		argv_array_push(&cp->args, "--quiet");
+
+	if (prefix)
+		argv_array_pushl(&cp->args, "--prefix", prefix, NULL);
+
+	argv_array_pushl(&cp->args, "--path", path, NULL);
+	argv_array_pushl(&cp->args, "--name", name, NULL);
+	argv_array_pushl(&cp->args, "--url", url, NULL);
+	if (reference)
+		argv_array_push(&cp->args, reference);
+	if (depth)
+		argv_array_push(&cp->args, depth);
+}
+
+static int update_clone_get_next_task(void **pp_task_cb,
+				      struct child_process *cp,
+				      struct strbuf *err,
+				      void *pp_cb)
+{
+	struct submodule_update_clone *pp = pp_cb;
+
+	for (; pp->count < pp->list.nr; pp->count++) {
+		const struct submodule *sub = NULL;
+		const char *displaypath = NULL;
+		const struct cache_entry *ce = pp->list.entries[pp->count];
+		struct strbuf sb = STRBUF_INIT;
+		const char *update_module = NULL;
+		char *url = NULL;
+		int needs_cloning = 0;
+
+		if (ce_stage(ce)) {
+			if (pp->recursive_prefix)
+				strbuf_addf(err, "Skipping unmerged submodule %s/%s\n",
+					pp->recursive_prefix, ce->name);
+			else
+				strbuf_addf(err, "Skipping unmerged submodule %s\n",
+					ce->name);
+			continue;
+		}
+
+		sub = submodule_from_path(null_sha1, ce->name);
+		if (!sub) {
+			strbuf_addf(err, "BUG: internal error managing submodules. "
+				    "The cache could not locate '%s'", ce->name);
+			pp->print_unmatched = 1;
+			continue;
+		}
+
+		if (pp->recursive_prefix)
+			displaypath = relative_path(pp->recursive_prefix, ce->name, &sb);
+		else
+			displaypath = ce->name;
+
+		if (pp->update)
+			update_module = pp->update;
+		if (!update_module)
+			update_module = sub->update;
+		if (!update_module)
+			update_module = "checkout";
+		if (!strcmp(update_module, "none")) {
+			strbuf_addf(err, "Skipping submodule '%s'\n", displaypath);
+			continue;
+		}
+
+		/*
+		 * Looking up the url in .git/config.
+		 * We must not fall back to .gitmodules as we only want to process
+		 * configured submodules.
+		 */
+		strbuf_reset(&sb);
+		strbuf_addf(&sb, "submodule.%s.url", sub->name);
+		git_config_get_string(sb.buf, &url);
+		if (!url) {
+			/*
+			 * Only mention uninitialized submodules when its
+			 * path have been specified
+			 */
+			if (pp->pathspec.nr)
+				strbuf_addf(err, _("Submodule path '%s' not initialized\n"
+					"Maybe you want to use 'update --init'?"), displaypath);
+			continue;
+		}
+
+		strbuf_reset(&sb);
+		strbuf_addf(&sb, "%s/.git", ce->name);
+		needs_cloning = !file_exists(sb.buf);
+
+		strbuf_reset(&sb);
+		strbuf_addf(&sb, "%06o %s %d %d\t%s\n", ce->ce_mode,
+				sha1_to_hex(ce->sha1), ce_stage(ce),
+				needs_cloning, ce->name);
+		string_list_append(&pp->projectlines, sb.buf);
+
+		if (needs_cloning) {
+			fill_clone_command(cp, pp->quiet, pp->prefix, ce->name,
+					   sub->name, url, pp->reference, pp->depth);
+			pp->count++;
+			free(url);
+			return 1;
+		} else
+			free(url);
+	}
+	return 0;
+}
+
+static int update_clone_start_failure(struct child_process *cp,
+				      struct strbuf *err,
+				      void *pp_cb,
+				      void *pp_task_cb)
+{
+	struct submodule_update_clone *pp = pp_cb;
+
+	strbuf_addf(err, "error when starting a child process");
+	pp->print_unmatched = 1;
+
+	return 1;
+}
+
+static int update_clone_task_finished(int result,
+				      struct child_process *cp,
+				      struct strbuf *err,
+				      void *pp_cb,
+				      void *pp_task_cb)
+{
+	struct submodule_update_clone *pp = pp_cb;
+
+	if (!result) {
+		return 0;
+	} else {
+		strbuf_addf(err, "error in one child process");
+		pp->print_unmatched = 1;
+		return 1;
+	}
+}
+
+static int update_clone(int argc, const char **argv, const char *prefix)
+{
+	struct string_list_item *item;
+	struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
+
+	struct option module_list_options[] = {
+		OPT_STRING(0, "prefix", &prefix,
+			   N_("path"),
+			   N_("path into the working tree")),
+		OPT_STRING(0, "recursive_prefix", &pp.recursive_prefix,
+			   N_("path"),
+			   N_("path into the working tree, across nested "
+			      "submodule boundaries")),
+		OPT_STRING(0, "update", &pp.update,
+			   N_("string"),
+			   N_("update command for submodules")),
+		OPT_STRING(0, "reference", &pp.reference, "<repository>",
+			   N_("Use the local reference repository "
+			      "instead of a full clone")),
+		OPT_STRING(0, "depth", &pp.depth, "<depth>",
+			   N_("Create a shallow clone truncated to the "
+			      "specified number of revisions")),
+		OPT__QUIET(&pp.quiet, N_("do't print cloning progress")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper list [--prefix=<path>] [<path>...]"),
+		NULL
+	};
+	pp.prefix = prefix;
+
+	argc = parse_options(argc, argv, prefix, module_list_options,
+			     git_submodule_helper_usage, 0);
+
+	if (module_list_compute(argc, argv, prefix, &pp.pathspec, &pp.list) < 0) {
+		printf("#unmatched\n");
+		return 1;
+	}
+
+	gitmodules_config();
+	/* Overlay the parsed .gitmodules file with .git/config */
+	git_config(git_submodule_config, NULL);
+	run_processes_parallel(1, update_clone_get_next_task,
+				  update_clone_start_failure,
+				  update_clone_task_finished,
+				  &pp);
+
+	if (pp.print_unmatched) {
+		printf("#unmatched\n");
+		return 1;
+	}
+
+	for_each_string_list_item(item, &pp.projectlines) {
+		utf8_fprintf(stdout, "%s", item->string);
+	}
+	return 0;
+}
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -264,6 +492,7 @@ static struct cmd_struct commands[] = {
 	{"list", module_list},
 	{"name", module_name},
 	{"clone", module_clone},
+	{"update-clone", update_clone}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 9bc5c5f..9f554fb 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -664,17 +664,18 @@ cmd_update()
 		cmd_init "--" "$@" || return
 	fi
 
-	cloned_modules=
-	git submodule--helper list --prefix "$wt_prefix" "$@" | {
+	git submodule--helper update-clone ${GIT_QUIET:+--quiet} \
+		${wt_prefix:+--prefix "$wt_prefix"} \
+		${prefix:+--recursive_prefix "$prefix"} \
+		${update:+--update "$update"} \
+		${reference:+--reference "$reference"} \
+		${depth:+--depth "$depth"} \
+		"$@" | {
 	err=
-	while read mode sha1 stage sm_path
+	while read mode sha1 stage just_cloned sm_path
 	do
 		die_if_unmatched "$mode"
-		if test "$stage" = U
-		then
-			echo >&2 "Skipping unmerged submodule $prefix$sm_path"
-			continue
-		fi
+
 		name=$(git submodule--helper name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
 		branch=$(get_submodule_config "$name" branch master)
@@ -691,27 +692,10 @@ cmd_update()
 
 		displaypath=$(relative_path "$prefix$sm_path")
 
-		if test "$update_module" = "none"
-		then
-			echo "Skipping submodule '$displaypath'"
-			continue
-		fi
-
-		if test -z "$url"
-		then
-			# Only mention uninitialized submodules when its
-			# path have been specified
-			test "$#" != "0" &&
-			say "$(eval_gettext "Submodule path '\$displaypath' not initialized
-Maybe you want to use 'update --init'?")"
-			continue
-		fi
-
-		if ! test -d "$sm_path"/.git && ! test -f "$sm_path"/.git
+		if test $just_cloned -eq 1
 		then
-			git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url" "$reference" "$depth" || exit
-			cloned_modules="$cloned_modules;$name"
 			subsha1=
+			update_module=checkout
 		else
 			subsha1=$(clear_local_git_env; cd "$sm_path" &&
 				git rev-parse --verify HEAD) ||
@@ -751,13 +735,6 @@ Maybe you want to use 'update --init'?")"
 				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
 			fi
 
-			# Is this something we just cloned?
-			case ";$cloned_modules;" in
-			*";$name;"*)
-				# then there is no local change to integrate
-				update_module=checkout ;;
-			esac
-
 			must_die_on_failure=
 			case "$update_module" in
 			checkout)
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 540771c..5991e3c 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -462,7 +462,7 @@ test_expect_success 'update --init' '
 	git config --remove-section submodule.example &&
 	test_must_fail git config submodule.example.url &&
 
-	git submodule update init > update.out &&
+	git submodule update init 2> update.out &&
 	cat update.out &&
 	test_i18ngrep "not initialized" update.out &&
 	test_must_fail git rev-parse --resolve-git-dir init/.git &&
@@ -480,7 +480,7 @@ test_expect_success 'update --init from subdirectory' '
 	mkdir -p sub &&
 	(
 		cd sub &&
-		git submodule update ../init >update.out &&
+		git submodule update ../init 2>update.out &&
 		cat update.out &&
 		test_i18ngrep "not initialized" update.out &&
 		test_must_fail git rev-parse --resolve-git-dir ../init/.git &&

-- 
2.6.1.247.ge8f2a41.dirty

Stefan Beller· Nov 4, 2015, 00:37 UTC · re: Stefan Beller · lore

[PATCHv3 10/11] submodule update: expose parallelism to the user

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-submodule.txt |  7 ++++++-
 builtin/submodule--helper.c     | 18 ++++++++++++++----
 git-submodule.sh                |  9 +++++++++
 t/t7406-submodule-update.sh     | 12 ++++++++++++
 4 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index f17687e..c70fafd 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 'git submodule' [--quiet] deinit [-f|--force] [--] <path>...
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
 	      [-f|--force] [--rebase|--merge] [--reference <repository>]
-	      [--depth <depth>] [--recursive] [--] [<path>...]
+	      [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
 	      [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach [--recursive] <command>
@@ -374,6 +374,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.
 	clone with a history truncated to the specified number of revisions.
 	See linkgit:git-clone[1]
 
+-j <n>::
+--jobs <n>::
+	This option is only valid for the update command.
+	Clone new submodules in parallel with as many jobs.
+	Defaults to the `submodule.jobs` option.
 
 <path>...::
 	Paths to submodule(s). When specified this will restrict the command
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 95b45a2..662d329 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -426,6 +426,7 @@ static int update_clone_task_finished(int result,
 
 static int update_clone(int argc, const char **argv, const char *prefix)
 {
+	int max_jobs = -1;
 	struct string_list_item *item;
 	struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
 
@@ -446,6 +447,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "depth", &pp.depth, "<depth>",
 			   N_("Create a shallow clone truncated to the "
 			      "specified number of revisions")),
+		OPT_INTEGER('j', "jobs", &max_jobs,
+			    N_("parallel jobs")),
 		OPT__QUIET(&pp.quiet, N_("do't print cloning progress")),
 		OPT_END()
 	};
@@ -467,10 +470,17 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	gitmodules_config();
 	/* Overlay the parsed .gitmodules file with .git/config */
 	git_config(git_submodule_config, NULL);
-	run_processes_parallel(1, update_clone_get_next_task,
-				  update_clone_start_failure,
-				  update_clone_task_finished,
-				  &pp);
+
+	if (max_jobs < 0)
+		max_jobs = config_parallel_submodules();
+	if (max_jobs < 0)
+		max_jobs = 1;
+
+	run_processes_parallel(max_jobs,
+			       update_clone_get_next_task,
+			       update_clone_start_failure,
+			       update_clone_task_finished,
+			       &pp);
 
 	if (pp.print_unmatched) {
 		printf("#unmatched\n");
diff --git a/git-submodule.sh b/git-submodule.sh
index 9f554fb..10c5af9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -645,6 +645,14 @@ cmd_update()
 		--depth=*)
 			depth=$1
 			;;
+		-j|--jobs)
+			case "$2" in '') usage ;; esac
+			jobs="--jobs=$2"
+			shift
+			;;
+		--jobs=*)
+			jobs=$1
+			;;
 		--)
 			shift
 			break
@@ -670,6 +678,7 @@ cmd_update()
 		${update:+--update "$update"} \
 		${reference:+--reference "$reference"} \
 		${depth:+--depth "$depth"} \
+		${jobs:+$jobs} \
 		"$@" | {
 	err=
 	while read mode sha1 stage just_cloned sm_path
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index dda3929..05ea66f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops module name before recur
 	 test_i18ngrep "Submodule path .deeper/submodule/subsubmodule.: checked out" actual
 	)
 '
+
+test_expect_success 'submodule update can be run in parallel' '
+	(cd super2 &&
+	 GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 7 &&
+	 grep "7 children" trace.out &&
+	 git config submodule.jobs 8 &&
+	 GIT_TRACE=$(pwd)/trace.out git submodule update &&
+	 grep "8 children" trace.out &&
+	 GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 9 &&
+	 grep "9 children" trace.out
+	)
+'
 test_done

-- 
2.6.1.247.ge8f2a41.dirty

Stefan Beller· Nov 4, 2015, 00:37 UTC · re: Stefan Beller · lore

[PATCHv3 11/11] clone: allow an explicit argument for parallel submodule clones

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-clone.txt |  6 +++++-
 builtin/clone.c             | 19 +++++++++++++------
 t/t7406-submodule-update.sh | 15 +++++++++++++++
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index f1f2a3f..01bd6b7 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 	  [-o <name>] [-b <name>] [-u <upload-pack>] [--reference <repository>]
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch]
-	  [--recursive | --recurse-submodules] [--] <repository>
+	  [--recursive | --recurse-submodules] [--jobs <n>] [--] <repository>
 	  [<directory>]
 
 DESCRIPTION
@@ -216,6 +216,10 @@ objects from the source repository into a pack in the cloned repository.
 	The result is Git repository can be separated from working
 	tree.
 
+-j <n>::
+--jobs <n>::
+	The number of submodules fetched at the same time.
+	Defaults to the `submodule.jobs` option.
 
 <repository>::
 	The (possibly remote) repository to clone from.  See the
diff --git a/builtin/clone.c b/builtin/clone.c
index 9eaecd9..ce578d2 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,7 @@ static int option_progress = -1;
 static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
+static int max_jobs = -1;
 
 static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {
 		    N_("initialize submodules in the clone")),
 	OPT_BOOL(0, "recurse-submodules", &option_recursive,
 		    N_("initialize submodules in the clone")),
+	OPT_INTEGER('j', "jobs", &max_jobs,
+		    N_("number of submodules cloned in parallel")),
 	OPT_STRING(0, "template", &option_template, N_("template-directory"),
 		   N_("directory from which templates will be used")),
 	OPT_STRING_LIST(0, "reference", &option_reference, N_("repo"),
@@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {
 	OPT_END()
 };
 
-static const char *argv_submodule[] = {
-	"submodule", "update", "--init", "--recursive", NULL
-};
-
 static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
 {
 	static char *suffix[] = { "/.git", "", ".git/.git", ".git" };
@@ -724,8 +723,16 @@ static int checkout(void)
 	err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
 			   sha1_to_hex(sha1), "1", NULL);
 
-	if (!err && option_recursive)
-		err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
+	if (!err && option_recursive) {
+		struct argv_array args = ARGV_ARRAY_INIT;
+		argv_array_pushl(&args, "submodule", "update", "--init", "--recursive", NULL);
+
+		if (max_jobs != -1)
+			argv_array_pushf(&args, "--jobs=%d", max_jobs);
+
+		err = run_command_v_opt(args.argv, RUN_GIT_CMD);
+		argv_array_clear(&args);
+	}
 
 	return err;
 }
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 05ea66f..ade0524 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -786,4 +786,19 @@ test_expect_success 'submodule update can be run in parallel' '
 	 grep "9 children" trace.out
 	)
 '
+
+test_expect_success 'git clone passes the parallel jobs config on to submodules' '
+	test_when_finished "rm -rf super4" &&
+	GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&
+	grep "7 children" trace.out &&
+	rm -rf super4 &&
+	git config --global submodule.jobs 8 &&
+	GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&
+	grep "8 children" trace.out &&
+	rm -rf super4 &&
+	GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 . super4 &&
+	grep "9 children" trace.out &&
+	rm -rf super4
+'
+
 test_done

-- 
2.6.1.247.ge8f2a41.dirty

Junio C Hamano· Nov 4, 2015, 17:54 UTC · re: Stefan Beller · lore

Re: [PATCHv3 00/11] Expose the submodule parallelism to the user

Stefan Beller <sbeller@google.com> writes:

> Where does it apply?
> ---
> This series applies on top of d075d2604c0f92045caa8d5bb6ab86cf4921a4ae (Merge
> branch 'rs/daemon-plug-child-leak' into sb/submodule-parallel-update) and replaces
> the previous patches in sb/submodule-parallel-update
>
> What does it do?
> ---
> This series should finish the on going efforts of parallelizing
> submodule network traffic. The patches contain tests for clone,
> fetch and submodule update to use the actual parallelism both via
> command line as well as a configured option. I decided to go with
> "submodule.jobs" for all three for now.

Thanks.

Stefan Beller· Nov 4, 2015, 18:08 UTC · re: Junio C Hamano · lore

Re: [PATCHv3 00/11] Expose the submodule parallelism to the user

On Wed, Nov 4, 2015 at 9:54 AM, Junio C Hamano <gitster@pobox.com> wrote:

> Stefan Beller <sbeller@google.com> writes:
>
>> Where does it apply?
>> ---
>> This series applies on top of d075d2604c0f92045caa8d5bb6ab86cf4921a4ae (Merge
>> branch 'rs/daemon-plug-child-leak' into sb/submodule-parallel-update) and replaces
>> the previous patches in sb/submodule-parallel-update
>>
>> What does it do?
>> ---
>> This series should finish the on going efforts of parallelizing
>> submodule network traffic. The patches contain tests for clone,
>> fetch and submodule update to use the actual parallelism both via
>> command line as well as a configured option. I decided to go with
>> "submodule.jobs" for all three for now.
>
> The order of patches and where the series builds makes me suspect
> that I have been expecting too much from the "parallel-fetch" topic.
>
> I've been hoping that it would be useful for the project as a whole
> to polish the other topic and make it available to wider audience
> sooner by itself (both from "end users get improved Git early"
> aspect and from "the core machinery to be reused in follow-up
> improvements are made closer to perfection sooner" perspective).  So
> I've been expecting that "Let's fix it on Windows" change directly
> on top of sb/submodule-parallel-fetch to make that topic usable
> before everything else.

> Other patches in this series may require
> the child_process_cleanup() change, so they may be applied on top of
> the merge between sb/submodule-parallel-fetch (updated for Windows)
> and rs/daemon-plug-child-leak topic.

>
> That does not seem to be what's happening here (note: I am not
> complaining; I am just trying to make sure expectation matches
> reality).  Am I reading you correctly?

>
> I think sb/submodule-parallel-fetch + sb/submodule-parallel-update
> as a single topic would need more time to mature to be in a tagged
> release than we have in the remainder of this cycle.

I agree on that.

>  It is likely
> that the former topic has a chance to get rebased after 2.7 happens.
> And that would allow us to (1) use the child_process_cleanup() from
> get-go instead of _deinit and to (2) get the machinery right both
> for UNIX and Windows from get-go.  Which would make the result
> easier to understand.  As this is one of the more important areas,
> it matters to keep the resulting code and the rationale behind it
> understandable by reading "log --reverse -p".

Junio C Hamano· Nov 4, 2015, 18:17 UTC · re: Stefan Beller · lore

Re: [PATCHv3 00/11] Expose the submodule parallelism to the user

Stefan Beller <sbeller@google.com> writes:

> So you are saying that reading the Windows cleanup patch
> before the s/deinit/clear/ Patch by Rene makes it way easier to understand?

No.