[msysGit] Re: Include a process kill utility in msysgit?
Hi, On Wed, 16 Jul 2008, Alexander Gavrilov wrote: > On Wednesday 16 July 2008 14:45:27 Johannes Schindelin wrote: > > It would have it made much easier to say "yep, that code is correct". > > Now it is not that easy. Are you still sure of your preference? > > I made a version with a separate strtoll_compat() helper function: > > http://repo.or.cz/w/msysgit.git?a=commitdiff;h=81a1ae54e3b05f5c09a48d7e2262114b7dacaa17 Thanks, Dscho
[msysGit] Re: Include a process kill utility in msysgit?
Hello, On Wednesday 16 July 2008 14:45:27 Johannes Schindelin wrote: > It would have it made much easier to say "yep, that code is correct". Now > it is not that easy. Are you still sure of your preference? I made a version with a separate strtoll_compat() helper function: http://repo.or.cz/w/msysgit.git?a=commitdiff;h=81a1ae54e3b05f5c09a48d7e2262114b7dacaa17 Alexander
[msysGit] Re: Include a process kill utility in msysgit?
Hi, On Wed, 16 Jul 2008, Alexander Gavrilov wrote: > On Wed, Jul 16, 2008 at 5:49 AM, Johannes Schindelin > <[EMAIL PROTECTED]> wrote: > > > A better patch, because it is easier verified, would be to implement > > strtoll() in terms of strtoul(), just for this file (where only the > > sign matters, but the mantissa is expected to fit into 32 bits). > > I thought about that, and if there were more than two calls to > strtoll, I would certainly have done so. It would have it made much easier to say "yep, that code is correct". Now it is not that easy. Are you still sure of your preference? > Btw, is it better to simply commit such changes, as I have done, or > amend the existing commit and do a 'push +master:mob'? As long as you try to show your progress, the way you did it is better. Squashing the commits together is easy, splitting them in what you did since you showed us the code last time is not. Thanks, Dscho
[msysGit] Re: Include a process kill utility in msysgit?
On Wed, Jul 16, 2008 at 5:49 AM, Johannes Schindelin <[EMAIL PROTECTED]> wrote: >> http://repo.or.cz/w/msysgit.git?a=commitdiff;h=bde036407e1a29a484138290216737589b888093 > > I suspect that you did not want to force negative values in the first > hunk. The first hunk is called when the option parsing machinery sees a negative number, and treats it as an unknown option. So the minus sign is always present. > A better patch, because it is easier verified, would be to implement > strtoll() in terms of strtoul(), just for this file (where only the sign > matters, but the mantissa is expected to fit into 32 bits). I thought about that, and if there were more than two calls to strtoll, I would certainly have done so. Btw, is it better to simply commit such changes, as I have done, or amend the existing commit and do a 'push +master:mob'? Alexander
[msysGit] Re: Include a process kill utility in msysgit?
Hi, On Tue, 15 Jul 2008, Alexander Gavrilov wrote: > I made a fix for the strtoll workaround, so it should now work correctly > for values in range -MAX_ULONG..MAX_ULONG: > > http://repo.or.cz/w/msysgit.git?a=commitdiff;h=bde036407e1a29a484138290216737589b888093 I suspect that you did not want to force negative values in the first hunk. A better patch, because it is easier verified, would be to implement strtoll() in terms of strtoul(), just for this file (where only the sign matters, but the mantissa is expected to fit into 32 bits). Hth, Dscho
[msysGit] Re: Include a process kill utility in msysgit?
On Tuesday 15 July 2008 15:55:45 Alexander Gavrilov wrote: > On Tue, Jul 15, 2008 at 1:32 PM, Johannes Schindelin > > Well, as you can easily guess, "cygwin_build" is unset in the makefile. > > Therefore the wholle string expands to "/". The effect is the same. For > > the computer. > > > > But not for the human reading it. > > > > Therefore it is a bug, > > Ok, I'll fix it when I return home. Further investigation showed that this variable is not empty (it is set in Makefile.common), so it probably should not be removed. Anyway, it is already used in other places of the Makefile. I made a fix for the strtoll workaround, so it should now work correctly for values in range -MAX_ULONG..MAX_ULONG: http://repo.or.cz/w/msysgit.git?a=commitdiff;h=bde036407e1a29a484138290216737589b888093 Alexander
[msysGit] Re: Include a process kill utility in msysgit?
On Tue, Jul 15, 2008 at 1:32 PM, Johannes Schindelin <[EMAIL PROTECTED]> wrote: > > So the only problem is the sign bit? Use strtoul(). It apparently also tries to support killing process groups by specifying negative process IDs. So it will be necessary to check for '-' and explicitly flip the sign. > Well, as you can easily guess, "cygwin_build" is unset in the makefile. > Therefore the wholle string expands to "/". The effect is the same. For > the computer. > > But not for the human reading it. > > Therefore it is a bug, Ok, I'll fix it when I return home. Alexander
[msysGit] Re: Include a process kill utility in msysgit?
Hi, On Tue, 15 Jul 2008, Alexander Gavrilov wrote: > On Tue, Jul 15, 2008 at 5:12 AM, Johannes Schindelin > <[EMAIL PROTECTED]> wrote: > > I think that the last platform where it breaks is Windows 95. > > strtoll() returns long long, and since the result is a pid, I doubt > > that even long is required for Windows 95. > > On the contrary, while the NT-based systems use kernel handles as > PIDs, which are generally quite small numbers, Windows 95 actually > uses an obfuscated pointer to an internal data structure (see comments > for http://blogs.msdn.com/oldnewthing/archive/2008/02/28/7925962.aspx). > This value can very well be greater than MAX_LONG, and will wrap to > the negative range if put in a signed long. So the only problem is the sign bit? Use strtoul(). > > Looks very good, except for the "-B$(cygwin_build)/", which should be > > a "-B/" AFAICT. > > It is verbatim copy from the original Cygwin Makefile.in, I don't even > know what $(cygwin_build) is used for. Well, as you can easily guess, "cygwin_build" is unset in the makefile. Therefore the wholle string expands to "/". The effect is the same. For the computer. But not for the human reading it. Therefore it is a bug, Dscho
[msysGit] Re: Include a process kill utility in msysgit?
On Tue, Jul 15, 2008 at 5:12 AM, Johannes Schindelin <[EMAIL PROTECTED]> wrote: > I think that the last platform where it breaks is Windows 95. strtoll() > returns long long, and since the result is a pid, I doubt that even long > is required for Windows 95. On the contrary, while the NT-based systems use kernel handles as PIDs, which are generally quite small numbers, Windows 95 actually uses an obfuscated pointer to an internal data structure (see comments for http://blogs.msdn.com/oldnewthing/archive/2008/02/28/7925962.aspx). This value can very well be greater than MAX_LONG, and will wrap to the negative range if put in a signed long. Cygwin kill uses long long for argument parsing so that it can correctly handle values between MIN_LONG (process group IDs) and MAX_ULONG (native Win95 PIDs). > Looks very good, except for the "-B$(cygwin_build)/", which should be a > "-B/" AFAICT. It is verbatim copy from the original Cygwin Makefile.in, I don't even know what $(cygwin_build) is used for. Alexander
[msysGit] Re: Include a process kill utility in msysgit?
Hi, On Tue, 15 Jul 2008, Alexander Gavrilov wrote: > On Monday 14 July 2008 15:31:35 Alexander Gavrilov wrote: > > > So, I recommend you write your own little tool for this process. > > > Shouldn't be that hard. > > > > I think I'll try to build the Cygwin version of kill.exe under MSys first: > > > > I eventually succeded in building it, although I had to replace two > calls to strtoll with strtol, so it will likely break on Windows 95 -- > but who cares. I think that the last platform where it breaks is Windows 95. strtoll() returns long long, and since the result is a pid, I doubt that even long is required for Windows 95. The only thing where it _could_ break is Win64, but we do not support that yet. Besides, as I grew to know the Windows API, Win64 probably still has 16-bit pids. > I added the resulting executable, as well as files necessary to build > it, to the repository, and pushed it to the mob branch: > > http://repo.or.cz/w/msysgit.git?a=commitdiff;h=da2ad26ee006b2013913e425cd520471ac9636f7 > > So far it seems to work. Looks very good, except for the "-B$(cygwin_build)/", which should be a "-B/" AFAICT. Ciao, Dscho
[msysGit] Re: Include a process kill utility in msysgit?
On Monday 14 July 2008 15:31:35 Alexander Gavrilov wrote: > > So, I recommend you write your own little tool for this process. > > Shouldn't be that hard. > > I think I'll try to build the Cygwin version of kill.exe under MSys first: > I eventually succeded in building it, although I had to replace two calls to strtoll with strtol, so it will likely break on Windows 95 -- but who cares. I added the resulting executable, as well as files necessary to build it, to the repository, and pushed it to the mob branch: http://repo.or.cz/w/msysgit.git?a=commitdiff;h=da2ad26ee006b2013913e425cd520471ac9636f7 So far it seems to work. Alexander
[msysGit] Re: Include a process kill utility in msysgit?
Hi,
On Mon, 14 Jul 2008, Marius Storm-Olsen wrote:
> The PsTools from SysInternals have a PsKill command, which takes a -t option,
> which will kill the process and all its descendants.
> Should work for Msys processes and forked off Windows processes, if you can
> get the Windows PID of the MSys process you which to kill..
>
> I guess "PsList git-gui" or "PsList gitk" would give you that..
>
>
> However, (sorry to burst the bubble, but) I don't think you can use these
> tools, since they require the user to accept a M$ EULA before using them.
> ("These license terms are an agreement between Sysinternals (a wholly owned
> subsidiary of Microsoft Corporation) and you.")
Sorry to burst your bubble, but the real reason we do not include them is
that those tools' sources are not available under a free license (I guess,
haven't checked). That is a no-go for msysGit.
Ciao,
Dscho
[msysGit] Re: Include a process kill utility in msysgit?
On Mon, Jul 14, 2008 at 3:18 PM, Marius Storm-Olsen <[EMAIL PROTECTED]> wrote: > However, (sorry to burst the bubble, but) I don't think you can use these > tools, since they require the user to accept a M$ EULA before using them. I already know that. > So, I recommend you write your own little tool for this process. Shouldn't > be that hard. I think I'll try to build the Cygwin version of kill.exe under MSys first: http://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/utils/kill.cc?rev=1.28&content-type=text/x-cvsweb-markup&cvsroot=src It is more portable. Alexander
[msysGit] Re: Include a process kill utility in msysgit?
Alexander Gavrilov said the following on 14.07.2008 12:38:
Git binaries are pure Windows applications, not MSys apps. (Compiled with
MinGW, but not linked to MSys DLLs.) Thus, it will definitely not respond to
the Cygwin/MSys process infrastructure.
Aha!
Killing a native Windows application definitely requires a native kill
utility. Cygwin version of kill.exe supports a special '-f' flag to
kill native apps, but the msys variant that I downloaded (found in
coreutils-5.97-MSYS-1.0.11-snapshot.tar.bz2) doesn't understand it. It
seems that Cygwin uses a separate custom implementation.
The PsTools from SysInternals have a PsKill command, which takes a -t
option, which will kill the process and all its descendants.
Should work for Msys processes and forked off Windows processes, if
you can get the Windows PID of the MSys process you which to kill..
I guess "PsList git-gui" or "PsList gitk" would give you that..
However, (sorry to burst the bubble, but) I don't think you can use
these tools, since they require the user to accept a M$ EULA before
using them. ("These license terms are an agreement between
Sysinternals (a wholly owned subsidiary of Microsoft Corporation) and
you.")
So, I recommend you write your own little tool for this process.
Shouldn't be that hard.
--
.marius
[msysGit] Re: Include a process kill utility in msysgit?
> Git binaries are pure Windows applications, not MSys apps. (Compiled with > MinGW, but not linked to MSys DLLs.) Thus, it will definitely not respond to > the Cygwin/MSys process infrastructure. Aha! Killing a native Windows application definitely requires a native kill utility. Cygwin version of kill.exe supports a special '-f' flag to kill native apps, but the msys variant that I downloaded (found in coreutils-5.97-MSYS-1.0.11-snapshot.tar.bz2) doesn't understand it. It seems that Cygwin uses a separate custom implementation. Alexander
[msysGit] Re: Include a process kill utility in msysgit?
Alexander Gavrilov said the following on 14.07.2008 12:01:
kill.exe (and kill builtin in bash) just calls the 'kill' system
call, which is emulated by msys.
Signal emulation in cygwin and mingw is done on the receiving side
by allocating a shared memory area, named sprintf("cygpid.%x",pid),
plus a named semaphore for communication, and starting a listener
thread. The sender queues the signal and raises the semaphore to
notify the target.
Now, what I can see is that for some reason git binaries don't even
bother initializing their receiving infrastructure. When they are
called from the shell, bash first forks, creating a helper process
(winpid 1008 & 344 in the list above), and then emulates exec by
starting the intended binary as a separate process, changing the
cygwin process name to reflect it, and waiting for the
newly-created process to finish. In this case, the helper process
receives the signal and apparently automatically terminates the
worker process.
Git binaries are pure Windows applications, not MSys apps. (Compiled
with MinGW, but not linked to MSys DLLs.) Thus, it will definitely not
respond to the Cygwin/MSys process infrastructure.
--
.marius
[msysGit] Re: Include a process kill utility in msysgit?
Hello,
I waited for today to recheck my findings on a different system with
'official' msysgit binaries:
On Sun, Jul 13, 2008 at 10:32 PM, Johannes Schindelin
<[EMAIL PROTECTED]> wrote:
> On Sun, 13 Jul 2008, Alexander Gavrilov wrote:
>> OK, so I run 'git gui blame SomeFile' in one window, and look at the list
>> of processes from another one. Here it is, with irrelevant parts removed:
>>
>> $ ps -W
>> PIDPPIDPGID WINPID TTY UIDSTIME COMMAND
>> 904 1 904904 con 500 14:38:44 /bin/bash
>> 1008 9041008 1008 con 500 18:11:20 /bin/git
>> (bash.exe)
>> 976 0 0976?0 18:11:20
>> c:\msysgit\bin\git.exe
>> 344 1 344344 con 500 18:11:20 /mingw/bin/wish
>> (sh.exe)
>> 1104 0 0 1104?0 18:11:20
>> c:\msysgit\mingw\bin\wish.exe
>> 736 0 0736?0 18:11:45
>> C:\msysgit\bin\git-blame.exe
>>
>> (In parentheses I specified what Task Manager thinks about the process)
>>
>> I can kill 1008 and 344, and it cascades to 976 and 1104 respectively.
>> But 736 continues running and hogging the CPU, and when I try to kill it, I
>> get:
>>
>> $ kill 736
>> bash: kill: (736) - No such process
>
> Can you look at the implementation of kill.exe and find out why it does
> that?
kill.exe (and kill builtin in bash) just calls the 'kill' system call,
which is emulated by msys.
Signal emulation in cygwin and mingw is done on the receiving side by
allocating a shared
memory area, named sprintf("cygpid.%x",pid), plus a named semaphore
for communication,
and starting a listener thread. The sender queues the signal and
raises the semaphore to
notify the target.
Now, what I can see is that for some reason git binaries don't even
bother initializing their
receiving infrastructure. When they are called from the shell, bash
first forks, creating a
helper process (winpid 1008 & 344 in the list above), and then
emulates exec by starting
the intended binary as a separate process, changing the cygwin process
name to reflect it,
and waiting for the newly-created process to finish. In this case, the
helper process receives
the signal and apparently automatically terminates the worker process.
On the other hand, when the git binaries are started from native
windows programs (e.g. cmd),
or wish, there is no helper, so the program should receive the signals
itself. Which it does not --
in fact, I cannot see any relevant opened shared object handles in
Process Explorer.
So, kill tries to open the 'cygpid.PID' memory area, fails, and
declares that the process does not exist.
P.S. From the sources I understood that even in the exec case the
worker process should
setup a cygpid area that forwards from its winpid to the real PID of
the impersonating helper
process -- and it doesn't do that. So it seems that something is
fundamentally broken.
Alexander
[msysGit] Re: Include a process kill utility in msysgit?
Hi, On Sun, 13 Jul 2008, Alexander Gavrilov wrote: > On Sunday 13 July 2008 17:40:03 Johannes Schindelin wrote: > > No, last time I checked I could kill a long-running git-rev-list with > > "kill" just fine. Something else must be wrong. > > > > Please investigate, > > OK, so I run 'git gui blame SomeFile' in one window, and look at the list > of processes from another one. Here it is, with irrelevant parts removed: > > $ ps -W > PIDPPIDPGID WINPID TTY UIDSTIME COMMAND > 904 1 904904 con 500 14:38:44 /bin/bash > 1008 9041008 1008 con 500 18:11:20 /bin/git > (bash.exe) > 976 0 0976?0 18:11:20 c:\msysgit\bin\git.exe > 344 1 344344 con 500 18:11:20 /mingw/bin/wish > (sh.exe) > 1104 0 0 1104?0 18:11:20 > c:\msysgit\mingw\bin\wish.exe > 736 0 0736?0 18:11:45 > C:\msysgit\bin\git-blame.exe > > (In parentheses I specified what Task Manager thinks about the process) > > I can kill 1008 and 344, and it cascades to 976 and 1104 respectively. > But 736 continues running and hogging the CPU, and when I try to kill it, I > get: > > $ kill 736 > bash: kill: (736) - No such process Can you look at the implementation of kill.exe and find out why it does that? Ciao, Dscho
[msysGit] Re: Include a process kill utility in msysgit?
On Sunday 13 July 2008 17:40:03 Johannes Schindelin wrote: > No, last time I checked I could kill a long-running git-rev-list with > "kill" just fine. Something else must be wrong. > > Please investigate, OK, so I run 'git gui blame SomeFile' in one window, and look at the list of processes from another one. Here it is, with irrelevant parts removed: $ ps -W PIDPPIDPGID WINPID TTY UIDSTIME COMMAND 904 1 904904 con 500 14:38:44 /bin/bash 1008 9041008 1008 con 500 18:11:20 /bin/git (bash.exe) 976 0 0976?0 18:11:20 c:\msysgit\bin\git.exe 344 1 344344 con 500 18:11:20 /mingw/bin/wish (sh.exe) 1104 0 0 1104?0 18:11:20 c:\msysgit\mingw\bin\wish.exe 736 0 0736?0 18:11:45 C:\msysgit\bin\git-blame.exe (In parentheses I specified what Task Manager thinks about the process) I can kill 1008 and 344, and it cascades to 976 and 1104 respectively. But 736 continues running and hogging the CPU, and when I try to kill it, I get: $ kill 736 bash: kill: (736) - No such process > P.S.: Use Git Bash only is the default, because most of the contributors > to msysGit prefer that option (Hannes being the notable exception). Once > most contributors prefer another option, we will switch. Ok, I'll just build my own installer if it becomes a problem =) > P.P.S.: Why do you ask a different question in a P.S., risking it to be > missed? It seemed too trivial for a separate message. > P.P.P.S.: Why do you put the verb in the question after the subject > instead of before it? :-) Editing artifact, I suppose =) Alexander
[msysGit] Re: Include a process kill utility in msysgit?
Hi, On Sun, 13 Jul 2008, Alexander Gavrilov wrote: > I want to submit upstream a couple of patches to make git-gui blame and > gitk kill their back-end processes upon window close. > > The trouble is, as Tcl does not have a builtin command to send signals to > processes, I have to use an external executable. MSysGit does not contain > a 'kill.exe' binary, and MSys kill does not work anyway -- probably because > the back-end is recognized as a native windows process for some reason, and > does not have a cygwin/msys pid. No, last time I checked I could kill a long-running git-rev-list with "kill" just fine. Something else must be wrong. Please investigate, Dscho > P.S. Why the 'Run Git from the Windows Command Prompt' option 'is > considered safe', but 'Use Git Bash only' is still the default in the > installer? I have to pay attention to switch this option every time I > install msysgit. P.S.: Use Git Bash only is the default, because most of the contributors to msysGit prefer that option (Hannes being the notable exception). Once most contributors prefer another option, we will switch. P.P.S.: Why do you ask a different question in a P.S., risking it to be missed? P.P.P.S.: Why do you put the verb in the question after the subject instead of before it? :-)
