code review 4969065: hgpatch: workaround for 'hg -q incoming' on windows. (issue 4969065)

[matt...@gmail.com]

Reviewers: golang-dev_googlecode.com, rsc,

Message:
Hello golan...@googlecode.com, r...@golang.org (cc:
golan...@googlegroups.com),

I'd like you to review this change to
http://go.googlecode.com/hg/

Description:
hgpatch: workaround for 'hg -q incoming' on windows.
Mercurial provides binary package and source package for windows.
If using binary package, hg.exe return 1 for exit code correctly.
But hg.bat does not return 1.
This is workaround to get exit code from batch file like following.

cmd /v:on /c hg -q incoming & exit !ERRORLEVEL!

This is working fine for both packages.

Please review this at http://codereview.appspot.com/4969065/

Affected files:
M src/cmd/hgpatch/main.go

Index: src/cmd/hgpatch/main.go
===================================================================
--- a/src/cmd/hgpatch/main.go
+++ b/src/cmd/hgpatch/main.go
@@ -15,6 +15,7 @@
"path/filepath"
"sort"
"strings"
+ "syscall"
)

var checkSync = flag.Bool("checksync", true, "check whether repository is
out of sync")
@@ -269,7 +270,17 @@
// hgIncoming returns true if hg sync will pull in changes.
func hgIncoming() bool {
// hg -q incoming exits 0 when there is nothing incoming, 1 otherwise.
- _, err := run([]string{"hg", "-q", "incoming"}, nil)
+ var err os.Error
+ if syscall.OS == "windows" {
+ // Workaround for mercurial batch file.
+ // Mercurial provides binary package and source package for windows.
+ // If using binary package, hg.exe return 1 for exit code correctly.
+ // But batch file does not return 1. Below is a workarond to get
+ // exit code.
+ _, err = run([]string{"cmd", "/v:on", "/c", "hg -q incoming &
exit !ERRORLEVEL!"}, nil)
+ } else {
+ _, err = run([]string{"hg", "-q", "incoming"}, nil)
+ }
return err == nil
}

[Russ Cox]

Do most people have hg.exe or hg.bat?
Is the buggy hg behavior common enough
to warrant a special case here?

Russ

[mattn]

This is behavior of windows.

If the user install hg from source code, 'setup.py' make 'hg.bat' script to 'c:\python27\scripts\hg.bat'.

And if the user install hg from binary package, 'hg.exe' is installed to 'c:\program files\mercurial\hg.exe'.

So most people have either of them.

GetExitCodeProcess() can't exit code from batch file.

To get it, hg.bat should call following at last line.

    exit /b 1

    or

    exit /b 0

I think, This is not work a bug of mercurial, And this is not a bug of 'script.py'.

[alex.b...@gmail.com]

On 2011/09/08 02:36:06, mattn wrote:

> GetExitCodeProcess() can't exit code from batch file.
> To get it, hg.bat should call following at last line.

> exit /b 1

> or

> exit /b 0

If someone creates hg.bat, that always return exitcode=0, I am happy for
hgpatch to report success all the time. That is what they are asking
for, aren't they?

Alex

http://codereview.appspot.com/4969065/

[mattn]

On Thursday, September 8, 2011 11:58:22 AM UTC+9, brainman wrote:

If someone creates hg.bat, that always return exitcode=0, I am happy for
hgpatch to report success all the time. That is what they are asking
for, aren't they?

No,  As long as that hg is batch file, GetExitCodeProcess() can't get exit code.

That's not the point. :)

 

[alex.b...@gmail.com]

On 2011/09/08 03:15:44, mattn wrote:

> No, As long as that hg is batch file, GetExitCodeProcess() can't get
exit
> code.

I know nothing about batch programming, but didn't you say that batch
should do

exit /b 1

?

> That's not the point. :)

What is the point?

Alex

http://codereview.appspot.com/4969065/

[Russ Cox]

This only matters for people developing
Go on Windows, since hgpatch is only used
by and only exists for the codereview extension.

Can we tell people developing on Windows
that hg.bat is buggy and they should use hg.exe?
There are certainly plenty of other hg bugs we
tell people to work around (usually by upgrading
to a different hg).

Russ

[mattn]

Are you saying mercurial MUST provide hg.bat using "exit /b" ?

Then, that's right :)

However, golang have a lot of problems for windows user.

If user want to use hg.exe, they must do:

    https://groups.google.com/d/msg/golang-dev/rr-7N-UJk-E/paNKZQD1h68J

And If user want to use hg installed from source code, we must wait next release of mercurial.

[alex.b...@gmail.com]

On 2011/09/08 03:34:12, mattn wrote:
> Are you saying mercurial MUST provide hg.bat using "exit /b" ?

All I am saying is, if program (batch or otherwise) wants to report an
error, it must do so in a conventional way = "exit code". If program
always returns exit_code=0, then it, obviously, do not care about
informing its callers about results of execution.

> However, golang have a lot of problems for windows user.
> If user want to use hg.exe, they must do:

https://groups.google.com/d/msg/golang-dev/rr-7N-UJk-E/paNKZQD1h68J

> And If user want to use hg installed from source code, we must wait
next
> release of mercurial.

I have no argument here. Please, fix it, if you know how.

Alex

http://codereview.appspot.com/4969065/

[mattn]

On Thursday, September 8, 2011 12:42:35 PM UTC+9, brainman wrote:

On 2011/09/08 03:34:12, mattn wrote:
> Are you saying mercurial MUST provide hg.bat using "exit /b" ?

I have no argument here. Please, fix it, if you know how.

Ok, I'll report this problem to python dev-team, but the patch will be applied to setuputils or distutils in python module if they accept it.

And I guess, they will say 'Get from %ERRORLEVEL% !'.

Perhaps, it will take many times until that windows user can use hgpatch correctly.

[Russ Cox]

On Wed, Sep 7, 2011 at 23:55, mattn <matt...@gmail.com> wrote:
> windows user

Again, it's not windows users.
It is windows-based developers of Go - people
contributing code to the Go project, not just using Go.
If you are developing Go on Windows you already
have to do special things like install the right mingw.
It doesn't seem too bad that you have to install
the right Mercurial too. We should update the page
that describes a Windows development setup to point
out that you should use the binary Mercurial distribution
and not setup.py, because of a setup.py bug.

Russ

[mattn]

Do you mean 'page' is http://code.google.com/p/go-wiki/ ?

[alex.b...@gmail.com]

On 2011/09/08 03:55:58, mattn wrote:

> ... it will take many times until that windows user can use hgpatch
> correctly.

I do not use codereview on windows. So, perhaps, I am not the right
person to say NO. But I always try to fix the cause, not the
consequences.

Alex

http://codereview.appspot.com/4969065/

[mattn]

Sorry, I just noticed important thing.

I added following line into hg.bat,

   exit /b 1

But hgpatch failed...

Hmm

[mattn]

Hmm,

On the second thought, GetExitCodeProcess() can't get exit code from batch file.

seems impossible if batch file do "exit /b 1"

Try following.

--- foo.c ---

#include <string.h>

#include <stdlib.h>

int main(int argc, char* argv[]) { exit(atoi(argv[1])); }

-------------

--- bar.bat ---

@echo off

exit /b %1

---------------

--- baz.bat ---

@echo off

exit %1

---------------

--- test.go ---

package main

import "exec"

func run(command string, args... string) bool {

_, err := exec.Command(command, args...).CombinedOutput()

return err == nil

}

func main() {

println(run(`.\foo.exe`, "0"))

println(run(`.\foo.exe`, "1"))

println(run(`.\bar.bat`, "0"))

println(run(`.\bar.bat`, "1"))

println(run(`.\baz.bat`, "0"))

println(run(`.\baz.bat`, "1"))

}

---------------

Then, test program output following.

true

false

true

true

true

false

This can get with "exit 1", But it close cmd.exe if run on command prompt.

Thus, we should get exit code from batch file, and pass it to "batch file interpreter(meaning cmd.exe).

[Russ Cox]

It looks to me like hg.bat should say exit, and not exit /b.
exit /b looks like it is only really meant to be used from
within batch files invoked by other batch files.

Russ

[mattn]

Yes, If pass exit code to cmd.exe, we should call as:

cmd /c call hg.bat ...

But this is talking about batch files.

My workaround above is working on both.

[Russ Cox]

If we fix hg.bat to use exit instead of exit /b,
then there is no workaround needed, right?

Russ

[mattn]

No, GetExitCodeProcess() can't get exit code from batch file.

If use "exit" (not "exit /b"), we can get exit code. (as your said)

But this destroy cmd.exe window.

[Russ Cox]

Thanks for that clarification.
It is all slowly coming back to me, like a bad dream.

This problem seems bigger than hgpatch now.
What can we do to fix syscall.StartProcess so that
we can get the exit code from batch files, like bar.bat
in your example? The MSDN docs say that CreateProcess
on a batchfile should say 'cmd /c bar' instead of 'bar'
but I don't see that happening in syscall/exec_windows.go.
Maybe that is related to the problem?

Russ

[mattn]

Now I dont have windwos. I'll look into it later.

Sorry, I must sleep. Zzz...

[mattn]

On Friday, September 9, 2011 3:30:09 AM UTC+9, rsc wrote:

On Thu, Sep 8, 2011 at 14:22, mattn <matt...@gmail.com> wrote:
> No, GetExitCodeProcess() can't get exit code from batch file.
> If use "exit" (not "exit /b"), we can get exit code. (as your said)
> But this destroy cmd.exe window.

Thanks for that clarification.
It is all slowly coming back to me, like a bad dream.

I accustomed that suck^H^H^H^Hsuch behavior of windows.

 

This problem seems bigger than hgpatch now.
What can we do to fix syscall.StartProcess so that
we can get the exit code from batch files, like bar.bat
in your example?  The MSDN docs say that CreateProcess
on a batchfile should say 'cmd /c bar' instead of 'bar'
but I don't see that happening in syscall/exec_windows.go.
Maybe that is related to the problem?

AFAIK, There is no flags to change behavior that make be possible to get exit code from batch files.

Probably, we have better to provide exec.System() aside exec.Command.

And exec.System() kick program specified from arguments with cmd.exe /c or sh -c 

[mattn]

It seems that "call" command accept exe file.

  cmd /c call 8g.exe

Try following.

----

package main

import "exec"

func run(command string, args... string) bool {

_, err := exec.Command(command, args...).CombinedOutput()

return err == nil

}

func main() {

println(run(`cmd`, `/c`, `call 8g.exe`))

println(run(`cmd`, `/c`, `call 8g.exe notfound.go`))

}

-----

This program output as following:

true

false

This maybe become good news. But I think that go have better to provide another function for this, or make be possible to specify option to start with shell(sh -c xxx / cmd.exe /c call xxx)

[brainman]

Mattn,

Please, file an issue to demonstrate your problem with batch files. Something simple. And we will decide what to do about it. Thank you.

Alex

[mattn]

ok, thanks to all.

[Russ Cox]

I would be inclined to have syscall.StartCommand
add the cmd /c call for .bat files (only).
There shouldn't be an option - it should just
do the right thing.

[peterGo]

Russ,

And for .cmd files too?

Peter

[Russ Cox]

On Fri, Sep 9, 2011 at 09:25, peterGo <go.pe...@gmail.com> wrote:
> And for .cmd files too?

Sorry, .cmd files showed up after I left. :-)

Russ

[go.pe...@gmail.com]

Russ,

On 2011/09/08 03:32:39, rsc wrote:
> Can we tell people developing on Windows
> that hg.bat is buggy and they should use hg.exe?

SGTM

Peter

> Russ

http://codereview.appspot.com/4969065/