This repository has been archived by the owner. It is now read-only.

Don't use cmd for Git Bash link. #72

Merged
merged 1 commit into from Nov 26, 2012

Conversation

Projects
None yet
2 participants
Contributor

pointoforder commented Nov 12, 2012

Fixes #71. On newer versions of Windows, point directly to sh.exe to avoid affecting other cmd window icons. Note that Windows XP and earlier are not changed, as they need to launch through cmd.exe for other reasons.

Owner

sschuberth commented Nov 12, 2012

Thanks for fixing your own bug :-) A few notes on the commit message style:

  • Please do not use colloquial English, i.e. "don't" should be "do not". Think about commit messages like a serious book that you write.
  • If appropriate, prefix your commit message with a single word describing the overall topic. Here, "Installer: " would be the right thing.
  • Give more details in the commit message as to why this change is necessary, what issue it addresses, and how it is fixed. It should be immediately clear from reading the commit message (without having to consult the issue tracker) why this commit is good. Currently, you do not even mention Windows 8 in the commit message, and the commit message wording is misleading as it sounds as if you do not use cmd at all anymore.
  • The sentence "as they need to launch through cmd.exe for other reasons" is correct, but you should actually explain the reasons (by referring to the SHA1 of the commit that explains them).
  • Use your full name in commits.
  • Sign-off ("git commit -s") your commit.
share/WinGit/install.iss
@@ -83,7 +83,8 @@ Source: ReleaseNotes.rtf; DestDir: {app}; Flags: isreadme replacesameversion; Af
[Icons]
Name: {group}\Git GUI; Filename: {app}\bin\wish.exe; Parameters: """{app}\libexec\git-core\git-gui"""; WorkingDir: %HOMEDRIVE%%HOMEPATH%; IconFilename: {app}\etc\git.ico
-Name: {group}\Git Bash; Filename: {syswow64}\cmd.exe; Parameters: "/c """"{app}\bin\sh.exe"" --login -i"""; WorkingDir: %HOMEDRIVE%%HOMEPATH%; IconFilename: {app}\etc\git.ico
+Name: {group}\Git Bash; Filename: {app}\bin\sh.exe; Parameters: "--login -i"; WorkingDir: %HOMEDRIVE%%HOMEPATH%; IconFilename: {app}\etc\git.ico;MinVersion: 6.0
@sschuberth

sschuberth Nov 12, 2012

Owner

Please add a space (" ") between ";" and "MinVersion" and add this line after the existing one for a slightly easier to read diff.

Owner

sschuberth commented Nov 12, 2012

Also, my understaing from your issue report and the mailing list discussion was that we should do this only starting from Windows 8, not starting with Windows Vista.

Contributor

pointoforder commented Nov 12, 2012

I'll update the formatting as requested. I don't have Vista/7 installed, but my guess is that the same behavior affects them too (since it's the same major version and this kind of behavior appears usually to stay the same across minor versions). I'll leave it up to you.

Contributor

pointoforder commented Nov 12, 2012

Regarding the commit message, thanks for the feedback. Since the commit already exists in pointoforder/msysgit, what's the best thing to do at this point? Should I rewrite history, or will that mess up the pull request mechanism? Or do I leave the commit as-is in pointoforder/msysgit and just update this text in the pull request itself?

Thanks,

David

Owner

sschuberth commented Nov 12, 2012

Just rewrite history and force-push your branch. The pull-request in GitHub will then update automatically.

Contributor

pointoforder commented Nov 21, 2012

Does everything look OK at this point? Is there anything else you'd like me to do?

Thanks,

David

Owner

sschuberth commented Nov 21, 2012

The only formal comments that I have would be:

  1. Please hard-wrap your commit message at 74 characters, so that a standard "git log" in a standard terminal with a width of 80 columns does not wrap the log output (for better readability).
  2. If compatible with 1), please include the hint which issue this fixes into the commit message's subject line, so it's immediately visible.

Other than that I just want to give your patch a try before I merge, which might take some more days, because I rarely work under Windows these days.

Thanks!

Installer: Use sh.exe directly in Git Bash icon if possible (fixes #71).
To avoid side-effects on other cmd.exe windows, point the Git Bash icon
directly to sh.exe on Windows Vista and newer. On 64-bit Windows XP,
launching through 32-bit cmd.exe is required to get sh.exe to launch as
a 32-bit process, but on Windows Vista and newer, this workaround is no
longer required (see commit b6ea386). On
Windows 8, if a start menu link points to cmd.exe but uses a different
icon, that icon is also used by some other cmd.exe windows, even if the
link has command line parameters. On Windows 7, the side-effect is
different; if the Git Bash link uses cmd.exe, Windows groups Git Bash
together with other cmd.exe windows. By avoiding launching through
cmd.exe when it is not necessary, we avoid affecting the icon used by
other cmd.exe windows.

Signed-off-by: David Matson <gitcoder@outlook.com>

sschuberth added a commit that referenced this pull request Nov 26, 2012

Merge pull request #72 from pointoforder/devel
Don't use cmd for Git Bash link.

@sschuberth sschuberth merged commit 73cdc1e into msysgit:devel Nov 26, 2012

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.