git.git as of tonight

git.git as of tonight

[Junio C Hamano]

I've merged a handful of topics to 'next', and hoping that many of
them can be merged to 'master' before I'll go offline for a few
weeks starting on the week #7 of the cycle (starting Nov 9th).

As I hate sending out the whole text back to back (the last one was
issued on the last Friday), here is an abbreviated update for the
"What's cooking" report, highlighting those topics that I want to
see in 'master' by the end of the week #6 (Nov 8th).

Thanks.


* kn/for-each-branch (2015-10-30) 1 commit
  (merged to 'next' on 2015-11-01 at 4249dc9)
 + ref-filter: fallback on alphabetical comparison

 Using the timestamp based criteria in "git branch --sort" did not
 tiebreak branches that point at commits with the same timestamp (or
 the same commit), making the resulting output unstable.

 Will merge to 'master'.


* jc/mailinfo-lib (2015-11-01) 1 commit
  (merged to 'next' on 2015-11-01 at 3ecaa28)
 + mailinfo: fix passing wrong address to git_mailinfo_config

 Hotfix for a topic already in 'master'.

 Will merge to 'master'.


* sb/submodule-parallel-fetch (2015-10-21) 14 commits
  (merged to 'next' on 2015-10-23 at 8f04bbd)
 + run-command: fix missing output from late callbacks
 + test-run-command: increase test coverage
 + test-run-command: test for gracefully aborting
 + run-command: initialize the shutdown flag
 + run-command: clear leftover state from child_process structure
 + run-command: fix early shutdown
  (merged to 'next' on 2015-10-15 at df63590)
 + submodules: allow parallel fetching, add tests and documentation
 + fetch_populated_submodules: use new parallel job processing
 + run-command: add an asynchronous parallel child processor
 + sigchain: add command to pop all common signals
 + strbuf: add strbuf_read_once to read without blocking
 + xread_nonblock: add functionality to read from fds without blocking
 + xread: poll on non blocking fds
 + submodule.c: write "Fetching submodule <foo>" to stderr
 (this branch is used by rs/daemon-leak-fix and sb/submodule-parallel-update.)

 Add a framework to spawn a group of processes in parallel, and use
 it to run "git fetch --recurse-submodules" in parallel.

 Will merge to 'master'.


* rs/daemon-leak-fix (2015-10-31) 3 commits
  (merged to 'next' on 2015-11-01 at 9b6c8f9)
 + daemon: plug memory leak
 + run-command: export child_process_clear()
 + run-command: name the cleanup function child_process_clear()
 (this branch is used by sb/submodule-parallel-update; uses sb/submodule-parallel-fetch.)

 "git daemon" uses "run_command()" without "finish_command()", so it
 needs to release resources itself, which it forgot to do.

 Will merge to 'master'.


* rs/wt-status-detached-branch-fix (2015-11-01) 5 commits
  (merged to 'next' on 2015-11-01 at cb23615)
 + wt-status: use skip_prefix() to get rid of magic string length constants
 + wt-status: don't skip a magical number of characters blindly
 + wt-status: avoid building bogus branch name with detached HEAD
 + wt-status: exit early using goto in wt_shortstatus_print_tracking()
 + t7060: add test for status --branch on a detached HEAD

 "git status --branch --short" accessed beyond the constant string
 "HEAD", which has been corrected.

 Will merge to 'master'.


* da/difftool (2015-10-29) 1 commit
  (merged to 'next' on 2015-11-01 at 4e5ab33)
 + difftool: ignore symbolic links in use_wt_file

 The code to prepare the working tree side of temporary directory
 for the "dir-diff" feature forgot that symbolic links need not be
 copied (or symlinked) to the temporary area, as the code already
 special cases and overwrites them.  Besides, it was wrong to try
 computing the object name of the target of symbolic link, which may
 not even exist or may be a directory.

 Will merge to 'master'.


* jk/initialization-fix-to-add-submodule-odb (2015-10-28) 1 commit
  (merged to 'next' on 2015-11-01 at da94b97)
 + add_submodule_odb: initialize alt_odb list earlier

 We peek objects from submodule's object store by linking it to the
 list of alternate object databases, but the code to do so forgot to
 correctly initialize the list.

 Will merge to 'master'.

Re: git.git as of tonight

[Johannes Sixt]

Am 02.11.2015 um 03:58 schrieb Junio C Hamano:
> * sb/submodule-parallel-fetch (2015-10-21) 14 commits
>    (merged to 'next' on 2015-10-23 at 8f04bbd)
>   + run-command: fix missing output from late callbacks
>   + test-run-command: increase test coverage
>   + test-run-command: test for gracefully aborting
>   + run-command: initialize the shutdown flag
>   + run-command: clear leftover state from child_process structure
>   + run-command: fix early shutdown
>    (merged to 'next' on 2015-10-15 at df63590)
>   + submodules: allow parallel fetching, add tests and documentation
>   + fetch_populated_submodules: use new parallel job processing
>   + run-command: add an asynchronous parallel child processor
>   + sigchain: add command to pop all common signals
>   + strbuf: add strbuf_read_once to read without blocking
>   + xread_nonblock: add functionality to read from fds without blocking
>   + xread: poll on non blocking fds
>   + submodule.c: write "Fetching submodule <foo>" to stderr
>   (this branch is used by rs/daemon-leak-fix and sb/submodule-parallel-update.)
> 
>   Add a framework to spawn a group of processes in parallel, and use
>   it to run "git fetch --recurse-submodules" in parallel.
> 
>   Will merge to 'master'.

Please don't, yet. This series does not build on Windows:

run-command.c: In function 'set_nonblocking':
run-command.c:1011: error: 'F_GETFL' undeclared (first use in this function)
run-command.c:1011: error: (Each undeclared identifier is reported only once
run-command.c:1011: error: for each function it appears in.)
run-command.c:1015: error: 'F_SETFL' undeclared (first use in this function)
run-command.c:1015: error: 'O_NONBLOCK' undeclared (first use in this function)
make: *** [run-command.o] Error 1

I have to investigate whether we can have some sort of Posixy
non-blocking IO on Windows or whether we have to opt-out from this
parallel-process facility. Any help from Windows experts would be
appreciated.

-- Hannes

Re: git.git as of tonight

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 02.11.2015 um 03:58 schrieb Junio C Hamano:
>> * sb/submodule-parallel-fetch (2015-10-21) 14 commits
>>    (merged to 'next' on 2015-10-23 at 8f04bbd)
>>   + run-command: fix missing output from late callbacks
>>...
>>   + submodule.c: write "Fetching submodule <foo>" to stderr
>>   (this branch is used by rs/daemon-leak-fix and sb/submodule-parallel-update.)
>> 
>>   Add a framework to spawn a group of processes in parallel, and use
>>   it to run "git fetch --recurse-submodules" in parallel.
>> 
>>   Will merge to 'master'.
>
> Please don't, yet. This series does not build on Windows:

The only reason the series is listed here is because the cycle is
still young and I was hoping that any fallout will be addressed by
the time we tag -rc0; if the extent of required fixups is too great,
that obviously would not work well.

I'll try to see if I can untangle rs/daemon-leak-fix topic so that
it does not depend on this thing and have it graduate separately.

Thanks for stopping me.

Re: git.git as of tonight

[Stefan Beller]

On Mon, Nov 2, 2015 at 1:15 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 02.11.2015 um 03:58 schrieb Junio C Hamano:
>> * sb/submodule-parallel-fetch (2015-10-21) 14 commits
>>    (merged to 'next' on 2015-10-23 at 8f04bbd)
>>   + run-command: fix missing output from late callbacks
>>   + test-run-command: increase test coverage
>>   + test-run-command: test for gracefully aborting
>>   + run-command: initialize the shutdown flag
>>   + run-command: clear leftover state from child_process structure
>>   + run-command: fix early shutdown
>>    (merged to 'next' on 2015-10-15 at df63590)
>>   + submodules: allow parallel fetching, add tests and documentation
>>   + fetch_populated_submodules: use new parallel job processing
>>   + run-command: add an asynchronous parallel child processor
>>   + sigchain: add command to pop all common signals
>>   + strbuf: add strbuf_read_once to read without blocking
>>   + xread_nonblock: add functionality to read from fds without blocking
>>   + xread: poll on non blocking fds
>>   + submodule.c: write "Fetching submodule <foo>" to stderr
>>   (this branch is used by rs/daemon-leak-fix and sb/submodule-parallel-update.)
>>
>>   Add a framework to spawn a group of processes in parallel, and use
>>   it to run "git fetch --recurse-submodules" in parallel.
>>
>>   Will merge to 'master'.
>
> Please don't, yet. This series does not build on Windows:
>
> run-command.c: In function 'set_nonblocking':
> run-command.c:1011: error: 'F_GETFL' undeclared (first use in this function)
> run-command.c:1011: error: (Each undeclared identifier is reported only once
> run-command.c:1011: error: for each function it appears in.)
> run-command.c:1015: error: 'F_SETFL' undeclared (first use in this function)
> run-command.c:1015: error: 'O_NONBLOCK' undeclared (first use in this function)
> make: *** [run-command.o] Error 1

Going by a quick search http://stackoverflow.com/a/22756664
I'd hope we only need to modify the set_nonblocking function using #ifdefs ?

>
> I have to investigate whether we can have some sort of Posixy
> non-blocking IO on Windows or whether we have to opt-out from this
> parallel-process facility. Any help from Windows experts would be
> appreciated.
>
> -- Hannes
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git.git as of tonight

[Johannes Sixt]

Am 03.11.2015 um 00:06 schrieb Stefan Beller:
> On Mon, Nov 2, 2015 at 1:15 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> run-command.c: In function 'set_nonblocking':
>> run-command.c:1011: error: 'F_GETFL' undeclared (first use in this function)
>> run-command.c:1011: error: (Each undeclared identifier is reported only once
>> run-command.c:1011: error: for each function it appears in.)
>> run-command.c:1015: error: 'F_SETFL' undeclared (first use in this function)
>> run-command.c:1015: error: 'O_NONBLOCK' undeclared (first use in this function)
>> make: *** [run-command.o] Error 1
>
> Going by a quick search http://stackoverflow.com/a/22756664
> I'd hope we only need to modify the set_nonblocking function using #ifdefs ?

Unfortunately, the solutions outlined in that post do not work for us. 
On Windows, the FIONBIO option is only available for sockets. 
"Overlapped IO" can be used only when the file descriptor is set in this 
mode right from the beginning (by open/CreateFile), and if it were so, 
it would have to be used in a totally different way than in Posix code.

My findings so far are negative. The only short-term and mid-term 
solution I see so far is to opt-out from the framework during build-time.

-- Hannes

Re: git.git as of tonight

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> My findings so far are negative. The only short-term and mid-term
> solution I see so far is to opt-out from the framework during
> build-time.

Now, from where I sit, it seems that the way forward would be

 1. Make this an optional feature so that platforms can compile it
    out, if it is not already done.  My preference, even if we go
    that route, would be to see if we can find a way to preserve the
    overall code structure (e.g. instead of spawning multiple
    workers, which is why the code needs NONBLOCK to avoid getting
    stuck on reading from one while others are working, perhaps we
    can spawn only one and not do a nonblock read?).

 2. After that is done, the feature could graduate to 'master'.  As
    this is a bigger framework change than others, however, we do
    not necessarily want to rush it.  On the other hand, because
    this only affects submodules, which means it has fewer users and
    testers that would give us feedback while it is on 'next', we
    may want to push it to 'master' sooner to give it a larger
    exposure.  I dunno, and I do not want to decide this myself the
    week before I'll go offline for a few weeks (i.e. today).

 3. Then we would enlist help from folks who are more familiar with
    Windows platform (like you) to see how the "run parallel workers
    and collect from them" can be (re)done with a nice level of
    abstraction.  I am hoping that we can continue the tradition of
    the evolution of run-command.c API (I am specifically impressed
    by what you did for "async" that allows the callers not to worry
    about threads and processes) aroundt this area.  That is
    obviously a mid- to longer term goal.

Thanks for working together well, you two.

Re: git.git as of tonight

[Stefan Beller]

On Tue, Nov 3, 2015 at 9:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> My findings so far are negative. The only short-term and mid-term
>> solution I see so far is to opt-out from the framework during
>> build-time.

So I started reading up on that[1].
As far as I understand, we don't need to mark a file descriptor
to be non blocking, but rather we could use ReadFileEx[2] with
a flag set for "overlapped" operation.

So that said, we can make set_nonblocking a noop and
provide another implementation for strbuf_read_once
depending on NO_PTHREADS being set.
Maybe not even strbuf_read_once, but rather the underlying
xread_nonblock ?



[1] http://tinyclouds.org/iocp-links.html
[2] https://msdn.microsoft.com/en-us/library/aa365468(v=VS.85).aspx

>
> Now, from where I sit, it seems that the way forward would be
>
>  1. Make this an optional feature so that platforms can compile it
>     out, if it is not already done.  My preference, even if we go
>     that route, would be to see if we can find a way to preserve the
>     overall code structure (e.g. instead of spawning multiple
>     workers, which is why the code needs NONBLOCK to avoid getting
>     stuck on reading from one while others are working, perhaps we
>     can spawn only one and not do a nonblock read?).

Yeah that would be my understanding as well. If we don't come up with
a good solution for parallelism in Windows now, we'd need to make it at
least working in the jobs=1 case as well as it worked before.

>
>  2. After that is done, the feature could graduate to 'master'.  As
>     this is a bigger framework change than others, however, we do
>     not necessarily want to rush it.  On the other hand, because
>     this only affects submodules, which means it has fewer users and
>     testers that would give us feedback while it is on 'next', we
>     may want to push it to 'master' sooner to give it a larger
>     exposure.  I dunno, and I do not want to decide this myself the
>     week before I'll go offline for a few weeks (i.e. today).

Yeah I guess cooking this well done has its benefits.

>
>  3. Then we would enlist help from folks who are more familiar with
>     Windows platform (like you) to see how the "run parallel workers
>     and collect from them" can be (re)done with a nice level of
>     abstraction.  I am hoping that we can continue the tradition of
>     the evolution of run-command.c API (I am specifically impressed
>     by what you did for "async" that allows the callers not to worry
>     about threads and processes) aroundt this area.  That is
>     obviously a mid- to longer term goal.

I just wonder if we can skip step 1) and 2) by having the discussion
now how to change the framework to work well without posix file
descriptors here.

>
> Thanks for working together well, you two.

Re: git.git as of tonight

[Johannes Sixt]

Am 03.11.2015 um 19:18 schrieb Stefan Beller:
> ... ReadFileEx ... "overlapped" operation.

Let's not go there just yet.

>>   1. Make this an optional feature so that platforms can compile it
>>      out, if it is not already done.  My preference, even if we go
>>      that route, would be to see if we can find a way to preserve the
>>      overall code structure (e.g. instead of spawning multiple
>>      workers, which is why the code needs NONBLOCK to avoid getting
>>      stuck on reading from one while others are working, perhaps we
>>      can spawn only one and not do a nonblock read?).
>
> Yeah that would be my understanding as well. If we don't come up with
> a good solution for parallelism in Windows now, we'd need to make it at
> least working in the jobs=1 case as well as it worked before.

That should be possible. I discovered today that we have this function:

static void set_nonblocking(int fd)
{
	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");
}

Notice that it is not a fatal condition if O_NONBLOCK cannot be 
established. (BTW, did you ever test this condition?) If we add two 
lines (which remove the stuff that does not work on Windows) like this:

static void set_nonblocking(int fd)
{
#ifndef GIT_WINDOWS_NATIVE
	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))
#endif
		warning("Could not set file status flags, "
			"output will be degraded");
}

we should get something that works, theoretically. We still need a more 
complete waitpid emulation, but that does not look like rocket science. 
I'll investigate further in this direction.

-- Hannes

Re: git.git as of tonight

[Stefan Beller]

On Tue, Nov 3, 2015 at 1:03 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 03.11.2015 um 19:18 schrieb Stefan Beller:
>>
>> ... ReadFileEx ... "overlapped" operation.
>
>
> Let's not go there just yet.
>
>>>   1. Make this an optional feature so that platforms can compile it
>>>      out, if it is not already done.  My preference, even if we go
>>>      that route, would be to see if we can find a way to preserve the
>>>      overall code structure (e.g. instead of spawning multiple
>>>      workers, which is why the code needs NONBLOCK to avoid getting
>>>      stuck on reading from one while others are working, perhaps we
>>>      can spawn only one and not do a nonblock read?).
>>
>>
>> Yeah that would be my understanding as well. If we don't come up with
>> a good solution for parallelism in Windows now, we'd need to make it at
>> least working in the jobs=1 case as well as it worked before.
>
>
> That should be possible. I discovered today that we have this function:
>
> static void set_nonblocking(int fd)
> {
>         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");
> }
>
> Notice that it is not a fatal condition if O_NONBLOCK cannot be established.
> (BTW, did you ever test this condition?)

No, as I viewed it more like a severe problem, not to be happen in the
near future.
But if it were to happen we would still need to finish the command
instead of giving
up because of degraded output. (I would not know how to test this
system call to fail,
so maybe I am just making up excuses)

I added an #ifdef just as you proposed below and the output itself
doesn't look too bad
except for the warning message themselves. If we'd just remove them it
would look
better to me.

So maybe we could just go with

    static void set_nonblocking(int fd)
    {
    #ifndef GIT_WINDOWS_NATIVE
        int flags = fcntl(fd, F_GETFL);
        if (!(flags < 0))
            fcntl(fd, F_SETFL, flags | O_NONBLOCK)
    #endif
    }

and see how people react to the output then?


> If we add two lines (which remove
> the stuff that does not work on Windows) like this:
>
> static void set_nonblocking(int fd)
> {
> #ifndef GIT_WINDOWS_NATIVE
>         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))
> #endif
>                 warning("Could not set file status flags, "
>                         "output will be degraded");
> }
>
> we should get something that works, theoretically. We still need a more
> complete waitpid emulation, but that does not look like rocket science. I'll
> investigate further in this direction.
>
> -- Hannes
>

Re: O_NONBLOCK under Windows (was: git.git as of tonight)

[Torsten Bögershausen]

On 11/04/2015 12:00 AM, Stefan Beller wrote:
> On Tue, Nov 3, 2015 at 1:03 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>
The #ifdef assumes that Windows never will have O_NONBLOCK/F_GETFL)

Does the following make more sense ?
#if defined (O_NONBLOCK) && defined (F_GETFL)

Or may be:
#ifndef NO_O_NONBLOCK
>> #ifndef GIT_WINDOWS_NATIVE
>>         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))
>> #endif
>>                 warning("Could not set file status flags, "
>>                         "output will be degraded");
>> }
>>
>>
>>
>>   
>>

Re: O_NONBLOCK under Windows (was: git.git as of tonight)

[Stefan Beller]

On Wed, Nov 4, 2015 at 11:59 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 11/04/2015 12:00 AM, Stefan Beller wrote:
>> On Tue, Nov 3, 2015 at 1:03 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>
> The #ifdef assumes that Windows never will have O_NONBLOCK/F_GETFL)
>
> Does the following make more sense ?
> #if defined (O_NONBLOCK) && defined (F_GETFL)
>
> Or may be:
> #ifndef NO_O_NONBLOCK
>>> #ifndef GIT_WINDOWS_NATIVE
>>>         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))
>>> #endif
>>>                 warning("Could not set file status flags, "
>>>                         "output will be degraded");
>>> }
>>>

Reading Junios answer to the resent patch[1], I am currently debating
if this is the right way to go anyway. As Junio points out, this is
not a warning
but rather a critical issue such that we'd maybe rather die(...) than
just warning(...),
which would make the discussion about the correct condition in the #ifdef moot.

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

[PATCH 0/2] Missing O_NONBLOCK under Windows (was: git.git as of tonight)

[Stefan Beller]

The first patch is a general fixup as per discussion,
the second patch will make Git compile in Windows again (hopefully, not tested)

The number of #ifdefs seems acceptable to me, opinions on that?

This has been developed on top of d075d2604c0f9 (Merge branch 'rs/daemon-plug-child-leak'
into sb/submodule-parallel-update), but should also apply on top of origin/sb/submodule-parallel-fetch

Thanks,
Stefan

Stefan Beller (2):
  run-parallel: rename set_nonblocking to set_nonblocking_or_die
  run-parallel: Run sequential if nonblocking I/O is unavailable

 run-command.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

-- 
2.6.1.247.ge8f2a41.dirty

[PATCH 1/2] run-parallel: rename set_nonblocking to set_nonblocking_or_die

[Stefan Beller]

Discussion turned out a warning is not enough, but we need to die in case
of blocking output as we can lockup the child processes.

Junio 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, 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?

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

diff --git a/run-command.c b/run-command.c
index 0a3c24e..86fbe50 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1006,15 +1006,13 @@ static void pp_cleanup(struct parallel_processes *pp)
 	sigchain_pop_common();
 }
 
-static void set_nonblocking(int fd)
+static void set_nonblocking_or_die(int fd)
 {
 	int flags = fcntl(fd, F_GETFL);
 	if (flags < 0)
-		warning("Could not get file status flags, "
-			"output will be degraded");
+		die("Could not get file status flags");
 	else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
-		warning("Could not set file status flags, "
-			"output will be degraded");
+		die("Could not set file status flags");
 }
 
 /* returns
@@ -1052,7 +1050,7 @@ static int pp_start_one(struct parallel_processes *pp)
 		return code ? -1 : 1;
 	}
 
-	set_nonblocking(pp->children[i].process.err);
+	set_nonblocking_or_die(pp->children[i].process.err);
 
 	pp->nr_processes++;
 	pp->children[i].in_use = 1;
-- 
2.6.1.247.ge8f2a41.dirty

[PATCH 2/2] run-parallel: Run sequential if nonblocking I/O is unavailable

[Stefan Beller]

Windows doesn't have O_NONBLOCK nor F_GETFL defined, so we need cannot run
in parallel there. Instead the children will output directly to our stderr
and we run one child at a time.

Bonus: We are setting process.err = -1; which we previously expected the
get_next_task callback to do. It is easy to forget that part in the callback
leading to hard to debug errors.

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

diff --git a/run-command.c b/run-command.c
index 86fbe50..19de253 100644
--- a/run-command.c
+++ b/run-command.c
@@ -958,6 +958,10 @@ static struct parallel_processes *pp_init(int n,
 	if (n < 1)
 		n = online_cpus();
 
+#if !(defined (O_NONBLOCK) && defined (F_GETFL))
+	n = 1;
+#endif
+
 	pp->max_processes = n;
 	pp->data = data;
 	if (!get_next_task)
@@ -1006,6 +1010,7 @@ static void pp_cleanup(struct parallel_processes *pp)
 	sigchain_pop_common();
 }
 
+#if defined (O_NONBLOCK) && defined (F_GETFL)
 static void set_nonblocking_or_die(int fd)
 {
 	int flags = fcntl(fd, F_GETFL);
@@ -1014,6 +1019,7 @@ static void set_nonblocking_or_die(int fd)
 	else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
 		die("Could not set file status flags");
 }
+#endif
 
 /* returns
  *  0 if a new task was started.
@@ -1031,6 +1037,12 @@ static int pp_start_one(struct parallel_processes *pp)
 	if (i == pp->max_processes)
 		die("BUG: bookkeeping is hard");
 
+#if defined (O_NONBLOCK) && defined (F_GETFL)
+	pp->children[i].process.err = -1;
+#else
+	pp->children[i].process.err = 2;
+#endif
+
 	if (!pp->get_next_task(&pp->children[i].data,
 			       &pp->children[i].process,
 			       &pp->children[i].err,
@@ -1049,8 +1061,9 @@ static int pp_start_one(struct parallel_processes *pp)
 		strbuf_reset(&pp->children[i].err);
 		return code ? -1 : 1;
 	}
-
+#if defined (O_NONBLOCK) && defined (F_GETFL)
 	set_nonblocking_or_die(pp->children[i].process.err);
+#endif
 
 	pp->nr_processes++;
 	pp->children[i].in_use = 1;
-- 
2.6.1.247.ge8f2a41.dirty

Re: [PATCH 1/2] run-parallel: rename set_nonblocking to set_nonblocking_or_die

[Torsten Bögershausen]

On 11/04/2015 11:43 PM, Stefan Beller wrote:
> Discussion turned out a warning is not enough, but we need to die in case
> of blocking output as we can lockup the child processes.
>
> Junio 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.
(Jumping into an old discussion, I may be off topic)

When we want to make really sure, not to get stucked, we can do like this:

ssize_t xread_nonblock(int fd, void *buf, size_t len)
{
     ssize_t nr;
     if (len > MAX_IO_SIZE)
         len = MAX_IO_SIZE;
     nr = read(fd, buf, len);
     if (nr < 0) {
        if (errno == EWOULDBLOCK)
             errno = EAGAIN;
      return nr;
}

Once poll() returns POLLIN as set on a fd, we can safely call read() once 
without getting stucked.
"Data can be read without blocking".

And this work regardless if the fd blocking or not, so from that point of view,
the set_nonblocking() is not needed at all.


The major question is, if the poll() works under Windows, (and I haven't found 
time to dig further)

Re: [PATCH 1/2] run-parallel: rename set_nonblocking to set_nonblocking_or_die

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> (Jumping into an old discussion, I may be off topic)

I think this is exactly the latest "I wonder" from Peff, to which I
said "well, perhaps we didn't need nonblock after all from the
beginning".

> And this work regardless if the fd blocking or not, so from that point of view,
> the set_nonblocking() is not needed at all.
>
> The major question is, if the poll() works under Windows, (and I
> haven't found time to dig further)

;-)

Re: [PATCH 1/2] run-parallel: rename set_nonblocking to set_nonblocking_or_die

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Torsten Bögershausen <tboegi@web.de> writes:
>
>> (Jumping into an old discussion, I may be off topic)
>
> I think this is exactly the latest "I wonder" from Peff, to which I
> said "well, perhaps we didn't need nonblock after all from the
> beginning".
>
>> And this work regardless if the fd blocking or not, so from that point of view,
>> the set_nonblocking() is not needed at all.
>>
>> The major question is, if the poll() works under Windows, (and I
>> haven't found time to dig further)
>
> ;-)

Having read your message, I notice these disturbing passages in my
copy of manual pages.

poll(2) has

    BUGS
           See the discussion of spurious readiness notifications under
           the BUGS section of select(2).

and then select(2) has

       Under Linux, select() may report a socket file descriptor as
       "ready for read‐ ing", while nevertheless a subsequent read
       blocks.  This could for example happen when data has arrived
       but upon examination has wrong checksum and is discarded.
       There may be other circumstances in which a file descriptor
       is spuriously reported as ready.  Thus it may be safer to use
       O_NONBLOCK on sock‐ ets that should not block.

The world is not all Linux, so there may be systems that we do not
have to worry about the bug described here, but there still are some
Linux systems in use in the world, so we cannot assume the bug
described above will not matter, either.

So I am not convinced that set_nonblocking() is unnecessary X-<.

Re: [PATCH 1/2] run-parallel: rename set_nonblocking to set_nonblocking_or_die

[Jeff King]

On Wed, Nov 04, 2015 at 10:19:43PM -0800, Junio C Hamano wrote:

> Having read your message, I notice these disturbing passages in my
> copy of manual pages.
> 
> poll(2) has
> 
>     BUGS
>            See the discussion of spurious readiness notifications under
>            the BUGS section of select(2).
> 
> and then select(2) has
> 
>        Under Linux, select() may report a socket file descriptor as
>        "ready for read‐ ing", while nevertheless a subsequent read
>        blocks.  This could for example happen when data has arrived
>        but upon examination has wrong checksum and is discarded.
>        There may be other circumstances in which a file descriptor
>        is spuriously reported as ready.  Thus it may be safer to use
>        O_NONBLOCK on sock‐ ets that should not block.
> 
> The world is not all Linux, so there may be systems that we do not
> have to worry about the bug described here, but there still are some
> Linux systems in use in the world, so we cannot assume the bug
> described above will not matter, either.
> 
> So I am not convinced that set_nonblocking() is unnecessary X-<.

Yes, I've heard of this bug, and I wouldn't be surprised if it is
present on many systems. But notice that it is talking about sockets,
whereas here I think we are dealing only with pipes. Pipes generally
have much simpler and more predictable behavior, and their behaviors are
more closely mandated by POSIX.

Of course that's all hearsay, and I can't quite you chapter and verse of
POSIX (as if that would be enough, anyway) to prove it. But personally,
I would be comfortable dropping the non-blocking bits completely from
Stefan's series and adding them back in later if anybody can actually
find a real-world example that doesn't conform.

Of course, simply omitting the O_NONBLOCK bits on Windows would also
work if we can guarantee the Windows behavior. It means we're carrying
around extra code that may or may not be doing anything useful, but it's
not _that_ much code.

-Peff