git.git as of tonight

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'.

Am 02.11.2015 um 03:58 schrieb Junio C Hamano:

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

Johannes Sixt <j6t@kdbg.org> writes:

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.

On Mon, Nov 2, 2015 at 1:15 PM, Johannes Sixt <j6t@kdbg.org> wrote:

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

Am 03.11.2015 um 00:06 schrieb Stefan Beller:

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

Johannes Sixt <j6t@kdbg.org> writes:

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.

On Tue, Nov 3, 2015 at 9:05 AM, Junio C Hamano <gitster@pobox.com> wrote:

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

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.

Yeah I guess cooking this well done has its benefits.

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.

Am 03.11.2015 um 19:18 schrieb Stefan Beller:

Let's not go there just yet.

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

On Tue, Nov 3, 2015 at 1:03 PM, Johannes Sixt <j6t@kdbg.org> wrote:

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?

On 11/04/2015 12:00 AM, Stefan Beller 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

On Wed, Nov 4, 2015 at 11:59 AM, Torsten Bögershausen <tboegi@web.de> wrote:

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

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(-)

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:

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

On 11/04/2015 11:43 PM, Stefan Beller wrote:

(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)

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

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

;-)

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

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

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

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

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(-)