From: Junio C Hamano <gitster@pobox•com>
To: Stefan Beller <sbeller@google•com>
Cc: "git@vger•kernel.org" <git@vger•kernel.org>,
"Jeff King" <peff@peff•net>,
"Johannes Schindelin" <johannes.schindelin@gmail•com>,
"Jens Lehmann" <Jens.Lehmann@web•de>,
"Eric Sunshine" <ericsunshine@gmail•com>,
"Torsten Bögershausen" <tboegi@web•de>,
"Johannes Sixt" <j6t@kdbg•org>
Subject: Re: [PATCH 1/2] run-command: Remove set_nonblocking
Date: Thu, 05 Nov 2015 11:37:00 -0800 [thread overview]
Message-ID: <xmqqio5g5jc3.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <CAGZ79kZgEtKoYqxa8+wj0PCJedW140CQAPX6XwEYib1W3fPhXw@mail.gmail.com> (Stefan Beller's message of "Thu, 5 Nov 2015 11:22:29 -0800")
Stefan Beller <sbeller@google•com> writes:
> On Thu, Nov 5, 2015 at 10:45 AM, Junio C Hamano <gitster@pobox•com> wrote:
>> Stefan Beller <sbeller@google•com> writes:
>>
>>> strbuf_read_once can also operate on blocking file descriptors if we are
>>> sure they are ready. The poll (2) command however makes sure this is the
>>> case.
>>>
>>> Reading the manual for poll (2), there may be spurious returns indicating
>>> readiness but that is for network sockets only. Pipes should be unaffected.
>>
>> Given the presence of "for example" in that bug section, I wouldn't
>> say "only" or "should be unaffected".
>
> Reading the documentation we are in agreement, that we expect
> no spurious returns, no?
Given the presence of "for example" in that bug section, I wouldn't
say "only" or "should be unaffected". I cannot say "we expect no
spurious returns".
>> Thanks. Let's apply these fixes on sb/submodule-parallel-fetch,
>> merge the result to 'next' and have people play with it.
>
> Maybe the commit message was weakly crafted. Do you want me to resend?
I somehow feel that it is prudent to let this cook just above 'next'
for a few days (not just for the log message but to verify the
strategy and wait for others to come up with even better ideas), but
then I'll be offline starting next week, so I expect that merging
the final version to 'next' will be done by our interim maintainer,
which means we still have time to polish ;-)
Here is what I queued for now.
-- >8 --
From: Stefan Beller <sbeller@google•com>
Date: Thu, 5 Nov 2015 10:17:18 -0800
Subject: [PATCH] run-command: remove set_nonblocking()
strbuf_read_once can also operate on blocking file descriptors if we
are sure they are ready. And the poll(2) we call before calling
this ensures that this is the case.
Reading the manual for poll(2), there may be spurious returns
indicating readiness but that is for network sockets only and pipes
should be unaffected.
With this change, we rely on
- poll(2) returns only non-empty pipes; and
- read(2) on a non-empty pipe does not block.
This should fix compilation on Windows.
Signed-off-by: Stefan Beller <sbeller@google•com>
Signed-off-by: Junio C Hamano <gitster@pobox•com>
---
run-command.c | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/run-command.c b/run-command.c
index 1fbd286..07424e9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -996,17 +996,6 @@ static void pp_cleanup(struct parallel_processes *pp)
sigchain_pop_common();
}
-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");
-}
-
/* returns
* 0 if a new task was started.
* 1 if no new jobs was started (get_next_task ran out of work, non critical
@@ -1042,8 +1031,6 @@ static int pp_start_one(struct parallel_processes *pp)
return code ? -1 : 1;
}
- set_nonblocking(pp->children[i].process.err);
-
pp->nr_processes++;
pp->children[i].in_use = 1;
pp->pfd[i].fd = pp->children[i].process.err;
--
2.6.2-539-g1c5cd50
next prev parent reply other threads:[~2015-11-05 19:37 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-05 18:17 [PATCH 0/2] Remove non-blocking fds from run-command Stefan Beller
2015-11-05 18:17 ` [PATCH 1/2] run-command: Remove set_nonblocking Stefan Beller
2015-11-05 18:45 ` Junio C Hamano
2015-11-05 19:22 ` Stefan Beller
2015-11-05 19:37 ` Junio C Hamano [this message]
2015-11-05 20:27 ` Johannes Sixt
2015-11-05 20:50 ` Junio C Hamano
2015-11-05 22:20 ` Stefan Beller
2015-11-06 5:51 ` Johannes Sixt
2015-11-06 19:00 ` Stefan Beller
2015-11-06 21:41 ` Johannes Sixt
2015-11-05 18:17 ` [PATCH 2/2] strbuf: Correct documentation for strbuf_read_once Stefan Beller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqio5g5jc3.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=Jens.Lehmann@web$(echo .)de \
--cc=ericsunshine@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=j6t@kdbg$(echo .)org \
--cc=johannes.schindelin@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=sbeller@google$(echo .)com \
--cc=tboegi@web$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox