[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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  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");
>> }
>>
>>
>>
>>   
>>

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  wrote:
> On 11/04/2015 12:00 AM, Stefan Beller wrote:
>> On Tue, Nov 3, 2015 at 1:03 PM, Johannes Sixt  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git.git as of tonight

[Junio C Hamano]

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

Re: git.git as of tonight

[Stefan Beller]

On Tue, Nov 3, 2015 at 9:05 AM, Junio C Hamano  wrote:
> Johannes Sixt  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git.git as of tonight

[Stefan Beller]

On Tue, Nov 3, 2015 at 1:03 PM, Johannes Sixt  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
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@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 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@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  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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git.git as of tonight

[Junio C Hamano]

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

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 " 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git.git as of tonight

[Stefan Beller]

On Mon, Nov 2, 2015 at 1:15 PM, Johannes Sixt  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 " 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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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