code review 3989059: codereviwe: make hgpatch working on win32

codereviwe: make hgpatch working on win32
don't fork.

[rsc, 2011-02-02 04:35:47]

I don't think this is the right way to write this:

> -       if os.fork() == 0:
> +
> +       if sys.platform == "win32":
>                cmd.stdin.write(patch)
> -               os._exit(0)
> +               cmd.stdin.flush()
> +       else:
> +               if os.fork() == 0:
> +                       cmd.stdin.write(patch)
> +                       os._exit(0)
>        cmd.stdin.close()

What if the patch is large enough to fill the input pipe buffer
and at the same time hgpatch fills its output pipe buffer?
Then everything hangs.  That's why the fork is there.

It is possible that the fork is unnecessary, because hgpatch
never generates very much output.  I think that might be the
case.  If so, then we'd want to get rid of the fork on all systems
and not have special-purpose windows code.  And if not - that
is, if the fork is really needed - then the special-purpose windows
code is wrong.  Either way, it shouldn't be different.

Russ

[rsc, 2011-02-02 04:50:10]

>> It is possible that the fork is unnecessary, because hgpatch
>> never generates very much output.  I think that might be the
>> case.  If so, then we'd want to get rid of the fork on all systems
>> and not have special-purpose windows code.  And if not - that
>> is, if the fork is really needed - then the special-purpose windows
>> code is wrong.  Either way, it shouldn't be different.
> Yes, I don't think so that this is best solution.

I don't think hgpatch generates much output.
I'd say remove the fork entirely.

Russ

[mattn, 2011-02-03 00:34:41]

Hello golang-dev@googlegroups.com, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc, 2011-02-03 03:04:50]

please delete the line below.
otherwise looks good
thanks.

http://codereview.appspot.com/3989059/diff/4002/lib/codereview/codereview.py
File lib/codereview/codereview.py (right):

http://codereview.appspot.com/3989059/diff/4002/lib/codereview/codereview.py#...
lib/codereview/codereview.py:1132: cmd.stdin.flush()
This should be unnecessary given the close on the next line.

[mattn, 2011-02-03 03:33:19]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[mattn, 2011-02-03 03:33:37]

http://codereview.appspot.com/3989059/diff/4002/lib/codereview/codereview.py
File lib/codereview/codereview.py (right):

http://codereview.appspot.com/3989059/diff/4002/lib/codereview/codereview.py#...
lib/codereview/codereview.py:1132: cmd.stdin.flush()
On 2011/02/03 03:04:50, rsc wrote:
> This should be unnecessary given the close on the next line.

Done.

[rsc, 2011-02-03 03:43:38]

LGTM

[rsc, 2011-02-03 03:43:41]

*** Submitted as http://code.google.com/p/go/source/detail?r=765d120d38f1 ***

codereview: fix hgpatch on windows

R=rsc
CC=golang-dev
http://codereview.appspot.com/3989059

Committer: Russ Cox <rsc@golang.org>

[Joe Poirier, 2011-02-06 22:36:00]

On Tue, Feb 1, 2011 at 10:50 PM, Russ Cox <rsc@golang.org> wrote:
>>> It is possible that the fork is unnecessary, because hgpatch
>>> never generates very much output.  I think that might be the
>>> case.  If so, then we'd want to get rid of the fork on all systems
>>> and not have special-purpose windows code.  And if not - that
>>> is, if the fork is really needed - then the special-purpose windows
>>> code is wrong.  Either way, it shouldn't be different.
>> Yes, I don't think so that this is best solution.
>
> I don't think hgpatch generates much output.
> I'd say remove the fork entirely.
>
> Russ
>

Sorry for chiming in so late...

This looks like another instance where the use of popen's
communicate method can be used as it specifically avoids
the piped output deadlock.

Eg:

try:
  # fully buffered: -1, could be set to some know size, eg 4096, etc.
  cmd = subprocess.Popen(argv, buffsize=-1, shell=False,
                         stdin=subprocess.PIPE, stdout=subprocess.PIPE,
                         stderr=None, close_fds=sys.platform != "win32")
except:	
  return "hgpatch: " + ExceptionDetail()

out = cmd.communicate(patch)[0]

if cmd.poll() != 0 and not opts["ignore_hgpatch_failure"]:	
  return "hgpatch failed"

-joe

[rsc, 2011-02-08 20:43:47]

please use cmd.communicate then.