Comments (44)
So the problem must be with the pipes/redirected output since it works pre PR, albeit reporting spurious error messages. Maybe it is waiting on a duplicated handle that should have been closed? Perhaps it combines stdout and stderr.
It would be real helpful if you could investigate further, i.e. compile Git-Cheetah (using the msysGit net installer) with debug messages that show where it hangs.
Incidentally, this will not actually compile in Visual Studio because the
*debug
variable is not declared at the beginning of theexec_program_v
function which MSVC requires for .c files.
Patches welcome.
from git-cheetah.
It would be real helpful if you could investigate further
Of course. I just need to get the next release of ComposerSetup out and am waiting on a response on the msysGit mailing list to finish off: https://groups.google.com/forum/#!topic/msysgit/P7a2MVF0ok8
Patches welcome.
No problem, but I couldn't see the point of patching something that doesn't work at this stage.
from git-cheetah.
Thanks for the bump, I responded to that mail.
BTW I did not realize that you are a contributor because I completely forgot to add you to the GitHub team! Sorry! This is fixed now.
from git-cheetah.
@johnstevenson is this addressed by #14?
from git-cheetah.
No, it still hangs. Sorry, I haven't had a chance to look at this yet.
from git-cheetah.
@johnstevenson even after pulling master
? That would be bad.
from git-cheetah.
Sadly, yes. Tested with master at da97444
from git-cheetah.
Bummer. I actually got a chance to test this myself and I can reproduce. Darn. I'm out of Git time budget this week (thanks to some non-fun administrativa yesterday).
from git-cheetah.
Could you please give #15 a try?
from git-cheetah.
Sorry, but it still hangs - I haven't had the chance to debug it yet.
More worryingly, it crashes Explorer/Far Manager when you click on the Git Bash entry. Note that I have built the dll with Visual Studio and it crashes calling dup2
.
from git-cheetah.
Whoa. It does not do that here. (I built with gcc.) Do you have any idea what parameters dup2()
gets?
To be precise, for me it only fixed things in Explorer if I killed the Explorer and restarted it. The usual make uninstall uninstall-user && make install-user
did not fix it.
from git-cheetah.
To be clear, how does "killing Explorer and restarting it" fix anything? If Explorer is hanging, blocking on some call or another expecting some I/O or something, I believe that's the whole point of the issue. Those of us who do use the Explorer context menu find that unacceptable. For me, it's a pain, but there's a workaround. Would be nice if it worked through context menu as well, but can be dealt with other ways. Thank you...
from git-cheetah.
To be clear, how does "killing Explorer and restarting it" fix anything?
It fixes Explorer's failure to pick up an updated Cheetah.
from git-cheetah.
Okay. Well, I am not a Git Cheetah much less shell extension Subject Matter Expert. But something tells me as an extension, Git Cheetah is to blame, not Explorer.
from git-cheetah.
@mwpowellhtx I take it you did not build the code from this pull request.
from git-cheetah.
I'm just reporting the bug. It sounds like it's already been fixed. You should discuss that issue with the authors of the fix.
from git-cheetah.
@mwpowellhtx you claimed that you wanted to be an official contributor. Contributors contribute code, test fixes and help others with their issues.
BTW I am in contact with the author of the fix.
The author is yours truly.
from git-cheetah.
I was under the impression there was not already a fix available. What I proposed was counting the cost, not committing whether I would. But since there already is fix, I expect you are testing your own code. That is all.
from git-cheetah.
Calling dup2(-1, 1)
is the problem: https://github.com/msysgit/Git-Cheetah/blob/far-manager/common/exec.c#L69
The function has been passed the DETACHMODE
flag, so devnull never gets changed from an invalid handle value:
if ((!output || !error_output) && !(DETACHMODE & flags))
devnull = open("/dev/null", O_WRONLY);
from git-cheetah.
@mwpowellhtx will you be contributing something else than words, too? Of course you must test the fix on your side, otherwise you risk that your problem is not fixed. It is a bit sad that this does not go without saying.
from git-cheetah.
@johnstevenson whoops. you're right, devnull
is set to -1 by default, not 0, therefore my beautiful if (devnull)
did not catch it.
I just force-pushed a new version; could you please give this one a try, too?
from git-cheetah.
Okay, we are not crashing Explorer now by calling Git Bash. We are still causing Far Manager to hang forever, if you try and switch branches for example.
This all seems extraordinarily complicated - if we want to invoke Git Bash from the context-menu why can't we use a simple ShellExecute?
from git-cheetah.
@johnstevenson in the short run, it would help fix things, I agree, but in the long run we have an incredible technical debt: Git-Cheetah is supposed to be cross-platform.
I will have a look at the switching branches problem next week if nobody beats me to it.
from git-cheetah.
@johnstevenson I invested half of today's Git time budget to try to reproduce the failure you described, but I failed! FarManager does not hang here if I switch branches. It does exactly what I want it to do, and it is actually very fast doing it.
To recapitulate what I did:
- quit Far Manager (otherwise the final linking step will fail because Far Manager uses the
.dll
, still) - rebuild Git-Cheetah from scratch, on the
far-manager
branch:make uninstall uninstall-user clean install-user
Since I have a 64-bit VM and a 32-bit Far Manager running inside, I did not pass the W64=1
setting (otherwise I would generate a 64-bit shell extension that the 32-bit Far Manager does not use).
To make sure everything is still groovy here, I also ran make W64=1 uninstall uninstall-user clean
, killed the explorer via the Task Manager, then ran make W64=1 install-user
and restarted the explorer (in the Task Manager, on the Applications
tab, via the New Task...
button).
If this still does not work for you, I really need to ask you to look deeper into the issue because I have no way to reproduce the problem here anymore (at least not after my most recent update to the far-manager
branch yesterday).
from git-cheetah.
I am in the process of setting up a new dev box, so could you zip and email
me the 32 and 64-bit dlls (john-stevenson at blueyonder.co.uk) and I'll
test those, in case this is a VS compilation issue.
On 25 March 2014 13:19, dscho [email protected] wrote:
@johnstevenson https://github.com/johnstevenson I invested half of
today's Git time budget to try to reproduce the failure you described, but
I failed! FarManager does not hang here if I switch branches. It does
exactly what I want it to do, and it is actually very fast doing it.To recapitulate what I did:
- quit Far Manager (otherwise the final linking step will fail
because Far Manager uses the .dll, still)- rebuild Git-Cheetah from scratch, on the far-manager branch: make
uninstall uninstall-user clean install-userSince I have a 64-bit VM and a 32-bit Far Manager running inside, I did
not pass the W64=1 setting (otherwise I would generate a 64-bit shell
extension that the 32-bit Far Manager does not use).To make sure everything is still groovy here, I also ran make W64=1
uninstall uninstall-user clean, killed the explorer via the Task Manager,
then ran make W64=1 install-user and restarted the explorer (in the Task
Manager, on the Applications tab, via the New Task... button).If this still does not work for you, I really need to ask you to look
deeper into the issue because I have no way to reproduce the problem here
anymore (at least not after my most recent update to the far-managerbranch yesterday).Reply to this email directly or view it on GitHubhttps://github.com//issues/13#issuecomment-38562676
.
from git-cheetah.
@johnstevenson here you are: https://github.com/msysgit/Git-Cheetah/releases/tag/for-johnstevenson
from git-cheetah.
Thanks. You will be pleased to know that this was a vs compile issue. Your
32 and 64-bit dlls worked fine in Vista 32 and Win 8.1 64 (in both
environments). That is they did not hang their appropriate Far Manager.
Strangely, my vs 32 and 64-bit dlls also work fine on Win 8.1 64 (in both
environments), but the 32-bit causes Far Manager to hang on Vista 32.
Sorry to have used up your time on this (though I have spent quite a bit
myself). Incidentally, I am baffled that you were testing a 32-bit Far
Manager on Win 64 when this is not a possible configuration.
from git-cheetah.
Thank you so much @johnstevenson! I do appreciate all the help you provided, and I would like to ask for a little more: do you think you can dig deeper into the MSVC problem? I am really, really curious what goes wrong there (even if I am unlikely to compile Cheetah with anything but gcc anytime soon).
from git-cheetah.
Don't worry, I do not need appreciation! I just couldn't resist a little banter regarding your, understandably, precious git time. I am part way through an output-redirection implementation in a Win32 stylee, just to prove to myself that it is/is not a case of Far manager waiting for an unclosed/unreadable pipe. So we will see what that will bring. Otherwise I am at a bit of a loss as to why this happens.
from git-cheetah.
As I had half expected it is an output redirection issue (possibly due to a bad CRT implementation). Using Win32 handles rather than C file-descriptors cures the problem with an MSVC build - ie Far Manager does not hang on Vista 32.
https://github.com/johnstevenson/Git-Cheetah/tree/winexec
I can only assume that the reason the normal (ie using C file-descriptors) MSVC builds work on Windows 8.1 is that the OS must intervene in some way and tidy up the handles. Of course, this is only a wild guess.
I have also tested building on VS2012 and VS2013 in case the CRT had changed, but to no avail. What a shame, because using VS is a quick way to get up and running to test and debug the extension. At least, that is how I got involved.
from git-cheetah.
@johnstevenson this is really good stuff! And I think we can get this into a real nice shape and include it in Git-Cheetah.
As you know, I am interested in maintaining the cross-platform nature of Git-Cheetah, but that is easy! All we need to do is modify your code slightly to offer a drop-in replacement for the function exec_program_v
declared in exec.h
. For that, I actually do not think that we need a winexec.h
at all (because the definitions are actually only used inside winexec.c
; we can define the constants and functions in the appropriate order in winexec.c
and make them static
), and I'd also like to move winexec.c
to common/
, with the usual #ifdef _WIN32
guards. We also do not really need a new name for the function, but simply put the current definition of exec_program()
between ifndef _WIN32
guards and rename the win_exec()
function to exec_program_v()
.
What do you think?
from git-cheetah.
Just in case you're okay with that plan, I started working on it: master...winexec (but got interrupted; will continue later).
from git-cheetah.
Yes I am fine with it - thanks for setting up and re-formatting. Have you got any code guidelines - tabs or spaces, function names etc?
How is this going to work? I want to make a slight improvement to the code, so I guess a PR is the best way?
from git-cheetah.
Forget the above (a plan to use SetStdHandles
didn't work). Incidentally it will not compile on VS 2010 without a #pragma once
top line in winexec.c
, due to this bug:
http://connect.microsoft.com/VisualStudio/feedback/details/651405/pch-warning-after-installing-sp1
This does not happen on VS2012.
from git-cheetah.
Good point you remind me of: I wanted to ask whether you can make sure it compiles on VS, and works, still...
Regarding the #pragma once
statement, how does
Line 1 in fb6e62f
If you're okay with it, I would push this branch (with just one addition I just made to make it link with gcc
) to master
and take it from there, PRs welcome everytime you have something!
from git-cheetah.
Oh, as to coding style, I tried to follow https://github.com/msysgit/git/blob/master/Documentation/CodingGuidelines, but not too strictly. I do care about tabs instead of spaces because having mixed indentation makes rebasing and merging a major PITA. Likewise, I think it makes sense to keep with the convention of putting the opening curly brackets on the end of the same line as the if
statement. But I'm not a stickler to stripping curly brackets from single-line code blocks, for example, because all I really care about is making working with the code easy. Well, and it helps if it looks nice. ;-)
from git-cheetah.
It looks lovely! Actually it did subsequently build without the #pragma line (huh!) but it would be good to keep to avoid the warning (and make the Intelli bit work).
Compiles on VS 2010 and works with Far Manager on Vista 32. Haven't tested 64-bit. Incidentally, you can drop the #include <stdint.h>
line - it was a leftover from something or other.
I changed the code in path_lookup_prog
a little from the orginal lookup_prog
function in mingw.c. You might want to check that I have got this right.
from git-cheetah.
Merged! Your path_lookup_prog
looked okay, I would probably have done it with strbuf
s now... But it works. Probably ;-)
from git-cheetah.
Oh yeah, I should close this ticket, too ;-)
from git-cheetah.
Eeek! I am not freeing orig
, created here:
Line 43 in fb6e62f
Thanks for the code guidelines. I did look there but was once told off by one of the msysgit guys for using spaces in a file that had 80% spaces and 20% tabs! So I was not sure.
You can happily remove my braces from single-line code-blocks if you wish.
from git-cheetah.
I am not freeing
orig
[...]
Yeah, for a long-running shell extension, this can cause problems. Would you care for fixing this in a pull request, please? I ran out of Git time today...
I [...] was once told off by one of the msysgit guys for using spaces
I hope it was not me. If it was, I apologize and hope that you will be happy to learn that I am much nicer now.
You can happily remove my braces from single-line code-blocks if you wish.
As I said, I am not a stickler for that because it does not cause merge conflicts. Most of the time ;-)
from git-cheetah.
No it wasn't you (and it was only a minor, if somewhat baffling, reprimand). You have always been very pleasant - although you do have an annoying knack of getting me to do stuff !! I have actually learned a lot, so thanks.
from git-cheetah.
Awesome work, thanks!
from git-cheetah.
@johnstevenson if that is the only thing you find annoying with me, I can live with that :-)
from git-cheetah.
Related Issues (10)
- git clone does not set tracking branch HOT 1
- Suggestion: respect default terminal emulator HOT 5
- Git Cheetah causes explorer to crash when ".git/packed-refs" file is long enough HOT 1
- git_shell_ext64.dll makes Far Manager run git before running batch files HOT 20
- Put all Msysgit's right-click context menu options in a sub-menu in Windows Exporer? HOT 4
- Context menu modifications HOT 7
- WM_SETTINGCHANGE not reflected HOT 20
- Duplicated menu items in Folder pane HOT 2
- configurabe context menu? HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from git-cheetah.