-
Notifications
You must be signed in to change notification settings - Fork 2.7k
git-wrapper: support MSys2 #42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@sschuberth could you have a quick look? It's really a tiny patch, and my recent patches really benefited from your review. |
Oh, I just got another idea: We could replace the |
Ok, I'm fine with the PR, but I do not merge yet as you said:
And I'm unclear whether you want to do this as part of this PR. |
Have a look at 18f6085 and 70bfdb7 (part of this PR) :-) |
@sschuberth |
But it must. Just like we encode the knowledge about "if you are called as a builtin, reconstruct the command line in the following manner and call the
I thought about that. From a purely technical point of view, you are right. However, from the users' point of view, it would be a terrible idea: how many users know what A better idea would be to handle command-line options. But since we are essentially reusing the Git wrapper executable for all the builtins, for
Just as we simply copy The only purpose of 857b1a5 is to have the Git wrapper compiled as a GUI program (in addition to the console program that is compiled to the name |
Another couple of thoughts:
Please tell me what you think about this plan. |
Sorry, I still do not get why that should be the case. IMO the only thing a If someone wants to use mintty with git-bash.exe, that someone would nee to create a shortcut / minttyrc that runs |
But then you have two terminal windows: the real terminal you wanted, and a Win32 console that waits until the other terminal window is closed. Not what we want. |
What? No, why? 857b1a5c178001c578a0433cec8bcd763b4a4f6a builds git-bash.exe as a windows program, so it will not allocate a console for itself, but the spawned Bash will, resulting in only a single console window, no? |
Because of what you asked for:
|
That referred to the regular Windows console window that Bash opens for us. I was just meaning to point out the regular, i.e. not mintty or any tabbed console wrapper. |
Oh, and you reminded me of something I should change, still, no matter what: if |
Right. And I think we have to separate out the For example, if you double-click the And if we are already going that route, we might just as well add that configuration file handling you were implicitly asking for. |
No, I was not asking for that. When I was referring to configuration files, I meant those of the specific console wrapper program to use. I'm not a fan of configuration files in this case, but rather command line arguments. |
We are trying to make it easy for users, not difficult. Forcing them to type into some command-line window
every single time they want a Git Bash is kinda, uhm, not what I want. Like, at all. The whole, entire point of having a So either it is a configuration file (easy to edit) or it is a resource embedded in the |
Code like
should simply be but into a .bat / .cmd / .vbs file (that is easy to edit) that we ship. |
The problem is of course that every |
Yeah, scratch .bat / .cmd, that was wrong, I meant only .vbs. Just double-click the "Git Bash.vbs" in the MSYS1-based SDK to see how it opens only a single console window. |
But we cannot change the icon of the |
The idea I am following right now is to add conditional code (switched on when the cp git-bash.exe res-edit.exe
./res-edit icon git-bash.exe git-for-windows.ico
./res-edit command git-bash.exe 'usr\\bin\\mintty.exe -i /usr/share/git/git-for-windows.ico /usr/bin/bash --login -i' '@@COMSPEC@@ /C "@@EXEPATH@@\\usr\\bin\\bash.exe --login -i"' |
So here is the code for the updated Git wrapper. I decided to use the |
Prepare to prefix the command-line with non-builtins. We are now using the Git wrapper to call non-Git programs, too. Signed-off-by: Johannes Schindelin <[email protected]>
Okay, this is it. Now I am happy. |
@sschuberth @t-b unless you have strong objections, I will merge this tonight, along with git-for-windows/MINGW-packages#4. |
Ok, sarcasm of my previous comments aside. Again, I'm sorry I have to say this, but I do think this series does some very wrong things. But this is just my personal opinion, and I currently do not have the time to come up with a better solution, which probably is what you would be asking me for if I "strongly object" to merge. So it's basically your opinion against mine as I guess @t-b will not have time for a review. Since you're pushing for the new release, I will not "strongly object", but just "object", hoping that I'll never have to deal with this code once it's merged. |
Seeing that we exhausted all available constructive suggestions, at least for now, I am going to merge. We can always revisit the issue should somebody come up with a clearly superior solution. |
It did indicate what I'd preferably do like to see this replaced with: not hardcoding anything, and also not configuring anything, by not putting any terminal program related code into git-wrapper in the first place.
I'm sure you understand that I won't endorse a solution that I regard as sub-optimal just because it took you a long time to implement. I was your choice to come up with a resource-based approach (and yes, I did propose another alternative to configuration files: command line arguments), so you have to deal with the consequences of your choice.
I believe my linked comments were constructive. Maybe they did not help to make you solve your problem, or solve in a way that you like it so be solved, but saying that a problem is not worth solving in this context (i.e. the problem of configuring a terminal program) is also constructive criticism IMO. |
And I am sure that you will eventually understand that I won't endorse objections that come without constructive suggestions how to do things instead.
I already pointed out why command-line arguments are not an option: they cannot be passed via double-clicking.
It would be constructive, had you not known the reasons for abandoning the In any case, I am bowing out of this discussion until a time when I see a compelling idea how to address the list of bullet points (that I just edited) above. |
And in response to that I was saying that we should just have a .vbs file anyway, but you again objected as a .vbs cannot have a custom icon.
Please correct me if I'm wrong: As I see it, the only reason not to use a simple .vbs is that it cannot have a custom icon, right? If so, that's again where I would say: Solving the problem of the missing icon by implementing the logic into the git-wrapper is not worth the effort. To recap, my proposal is:
Did I miss to cover any of the requirements? |
@dscho It leaves me kindof speechless that you merged this in mid-discussion, while there are objections by no less than three contributors, including constructive suggestions such as using shell links.
You've added more than 300 lines of code without a single line of comment, which is completely un-maintainable. A very seasoned Windows developer may suspect that your icon parsing code is based on the Therefore, the term "ugly hack" seemed appropriate to me. It was not my intention to sound emotional or to hurt your feelings. Btw., the icon parsing code in particular seems to be copied verbatim from [3], but without including copyright and licensing information from the source file / project. [1] https://msdn.microsoft.com/en-us/library/ms997538.aspx |
I've never seen an application that is configured by modifying its own string resources. This simply doesn't work for end users without write access to the installation directory.
These comments don't mention shell links, which can have arbitrary icons and can execute arbitrary commands or scripts without opening a console window.
Users typically don't start applications by browsing the C:/Program Files tree, so IMO having an icon is not a strict requirement. Its not even an issue as shell links can have arbitrary icons. Supporting mintty or other terminal emulators is a completely separate issue from supporting MSys2. Mintty reportedly has serious problems with native console applications [1], so we cannot use it - at least not by default. Besides, most Windows users will be more familiar / comfortable with a 'real' console window. |
minor bike shed:
While it isn't common to browse to the C:/Program Files tree, I do find I need to do it moderately often on my work PC because (exploring & editing) the start-menu is hidden to me (corporate rule), such that say Notepad++ (which I have installed) doesn't show on the regular Start menu. That means I need to navigate the tree as discussed - for Notepad++, I've added for myself a desktop short cut and a pinned task bar shortcut, so I don't need to drill for that, - but there are other programs I use less frequently where I keep needing to drill down to 'cos I know I have them but the PC's start menu won't show them :-( As long as it's possible to navigate to the tree, it's not really a problem that it doesn't have the right icon, so it's only a cosmetic issue (aren't they the worst sort of customer issue ! ;-). |
The word "constructive" means something different in my book. Also: in the meantime, I reverted those changes (two days ago, no less). As to the shell links, their targets are absolute directories. That does not work for portable applications.
This has been discussed already. The point you are making is that not every interactive console program works in mintty. But that is something completely different from what Git for Windows is trying to offer!
Even if there are legitimate reasons to browse Program Files and to rightfully expect the executables to be non-amateurish, I feel compelled to repeat myself -- again -- that a major use case is the portable application. Therefore, while certainly intended to be helpful, I once again recall your attention to the non-exhaustive list of needs of Git for Windows. |
Hi @dscho, |
@PhilipOakley no apologies necessary! |
Relative shell links can be created by specifying
Where? All such discussions that I could find in the ML seem to conclude that we cannot use mintty.
I always thought that Git for Windows was about porting git to the native Windows platform. IMO this includes working seamlessly with native Windows applications. But I guess I must have been wrong. I also thought that the MSys(2) dependency was just because we don't have a working mingw-bash and mingw-perl (yet?). IMO one of the biggest issues we're facing ATM is the necessity to rebase dlls, potentially after each Windows patch day, and even without guarantee that it'll work afterwards. |
Exactly. Git. Not a development environment where you can run an unmodified interactive Python console. Just Git. |
Several times: https://github.com/git-for-windows/git/issues?utf8=%E2%9C%93&q=mintty+terminal |
So in order to fix a flaw such a kludge should be introduced? Seriously, I really want a clean solution. Batch files are okay for prototyping, but they come with plenty of downsides compared to real executables. There are enough reasons listed in the commit message that replaced It is a nice hack to use |
There was no discussion. Instead, there was completely unnecessary incendiary language. And quite a bit of criticism without suggesting any better alternatives. To protect my time and my enthusiasm for this project, I therefore cut this whatever-you-want-to-call-it short. |
@dscho I'm questioning that the portable application is such a major use case. It just happens so that you like it and GitHub wants one. Unfortunately our download stats from Google Code seem to be gone, but IIRC portable downloads were just a small fraction of full installer downloads.
@kblees Same here. And I'd like to take the change of the Git Merge conference in Paris to discuss what "Git for Windows" really should be. To me, it means to bring the best Git experience to Windows, in general. It does not necessarily mean to me to port the existing main Git implementation to Windows, although that's we have always been doing. It could also mean to come up with a new Git client based on libgit2, for example.
@kblees There will never be a mingw-bash. I had been looking into existing native Windows implementations of bash in the past to get rid of MSYS altogether, but nothing I found was promising / usable. Once you realize that there will be no execution of Git shell scripts without bash, and bash requires MSYS, but we actually do not want to depend on MSYS, you come to the conclusion that we need to get rid of the shell scripts. Now, that could mean many things, from re-implementing them as C code (which is why I like the GSoC proposal to make pull and am builtins so much) over building upon another Git implementation that does not depend on shell scripts, to starting a new implementation of Git from scratch (maybe in C++, finally).
@kblees We don't have that issue with 64-bit MSYS2 / Git due to the bigger address space anymore, which is why I think we should focus on that one currently.
@dscho While strictly speaking you're correct, this just won't work IMO. Once you ship and promote mintty as the shell for Git, people will start using that shell for any of their daily development work on Git hosted projects. I guess no command line user would want to switch shells during their work on e.g. a Ruby project depending on whether they want to run Git or the interactive Ruby interpreter. I'm repeating myself saying that if we ship mintty, we must be prepared to support it. And when @dscho agreed to support it, I guess he only meant to support it for use with Git. But that won't be enough. You'd also have to deal with all the issue reports about interactive Ruby (or similar) not working in mintty. |
I agree that its a kludge, nevertheless one that Windows users are familiar with (just google "relative shortcut"). The big advantage is that shell links can be configured via normal Windows mechanisms (right click - properties...).
Icon, manifest and version info can all be supplied at compile time. My main issue is with configuring the command to launch via string resources. E.g. to change the terminal from mintty to native console or vice versa, you would neet to:
IMO this is far from a clean solution, and no Windows user would remotely expect it to work that way... |
I fear that you completely misunderstood the purpose of That is incorrect. The intended use case was for our very own installer, so that there can be a page asking the user "Which terminal do you want to use?" and then adjusting At this point, the horse has been dead for five days already, though, half an eternity in terms of the current development speed of Git for Windows). A more constructive thing to do than to argue against a change that has already been taken out (so there is really nothing any longer to argue against) might be to install the development environment and get cracking with some performance tests of the msys2-runtime to determine where we can improve the speed. |
@sschuberth Do you actually need full-fledged bash, or would a simpler posix shell be sufficient? For Debian or other distributions where |
Not "by now". Git's shell scripts always have been POSIX shell scripts (except when there were bugs, and those bugs have always been fixed swiftly AFAIR). I actually looked at BusyBox a couple of years ago and it was not ready back then. But even if you solve the POSIX shell problem (and note that the shell scripts still assume POSIX semantics, such as: POSIX paths, POSIX-type signal handling, POSIX process control), we still need a Perl with Subversion bindings. You are certainly welcome to try your hand at getting that beast to compile against a MinGW Perl... |
Great, I was under that impression last time I grepped git's source code for
I don't know perl modules all that well, but which beast are you referring to exactly? Do the subversion bindings get built as part of subversion itself, or are they a separate cpan module? |
To qualify that statement, you would have to demonstrate that it works with, say, BusyBox' dash.
That file does not really say anything about the question how Perl bindings for Subversion need to be provided, nor the technical details how to build them. The |
@tkelman As @dscho has already said there never were supposed to be bashisms in Git shell scripts. In that context a simpler shell, like dash, should indeed be sufficient, but again as already mentioned that yet needs to be proven. I was just saying bash initially as that's the shell we ship. I was not meaning to say that Git shell scripts depend on bash. |
True. I just tried it - in busybox's dash, with
Right, I only meant the last bit of my question, that the MSYS2-based git for windows currently looks like it's using an msys2 perl rather than a mingw one. Should a separate tracking issue be opened for centralizing information on what would need to be done to switch to a mingw perl? |
I'd rather have a preliminary PR with some initial work done on that front instead of a ticket that is most likely going nowhere ;-) |
The original purpose of the Git wrapper is to run from inside Git for
Windows' /cmd/ directory, to allow setting up some environment variables
before Git is allowed to take over.
Due to differences in the file system layout, MSys2 requires some
changes for that to work.
Signed-off-by: Johannes Schindelin [email protected]