mingw: delete call to ShellExecute for opening help.

[boumenot]

The Windows flavor of git calls ShellExecute to view web pages. This call
launches the default handler for .html files. The user is not able to
configure an alternative despite git having full support for doing so on
Windows.

The original request for this change dates back to 12-May-2014, and refers
to dropping commit 4804aab from the msysgit repository. This is exactly
what this commit does.

[dscho]

What about the path translation concerns mentioned in the message of the reverted commit? I do not find any discussion of that in this commit's message, let alone anything that would reassure me that this commit does not break anything...

[boumenot]

Fair question. The path translation was only necessary for executing ShellExecuteA, so I removed it. I have seen other commit messages against master that indicate the code has handling for path issues, so I assumed it was unnecessary.

What is the best way to increase our confidence for this change?

1. Works on My Machine (Windows 10) is not sufficient, but I am happy to validate the change on other platforms. I will not be able to validate all permutations, but I can install the latest major releases of Windows and validate it. (>= Win 7, and >= 2012).
2. I ran the existing test cases for about 2 to 2.5 hours (no failures), and I stopped them. There were no failures, but my machine was dedicating a lot of resources, and I had other work to do. I do not know if there are tests that provide coverage for this feature. I do not know enough about the git test suite to know if it would support an interactive test.
3. I could introduce a flag to switch between the two behaviors, and default to the current experience.
4. Reject the change because there is little support for it. The last time this issue was raised was over two years ago. I scratched my itch because I dislike the opening of a browser for help. It is a little irksome that configurability of the browser does not have parity between platforms, but again, very few seem to be concerned. 😄

minor The code translates up to MAX_PATH, which can now be disabled in recent versions of Windows 10. The issues do not go away, but the code will not handle this new case either.

[dscho]

What is the best way to increase our confidence for this change?

The commit that you revert in this PR claimed that it fixed path conversion issues. That makes me believe that reverting the commit would reintroduce those path conversion issues.

I could imagine that there was a discussion on the msysgit mailing list, leading to the original commit. Maybe in this discussion some concrete path conversion issues were mentioned, and maybe those issues were only issues with MSys but are no longer a problem with MSYS2. There are a lot of "maybe"s here...

[dscho]

Re-reading the commit message of the original commit:

Avoiding MSYS' bash when possible is good because it avoids potential path translation issues.

From this sentence, I imagine that this patch might actually not have been triggered by a reported problem, but simply by some distaste for the MSYS bash. Maybe you can dig into the msysgit mailing list around that time frame?

[boumenot]

Sounds good. I'll dig into this more, and report back my findings.

[dscho]

Thank you so much. You are a big help!

[boumenot]

I looked into this more, and I was not able to find much. I searched the msysgit mailing list, and I cannot find any discussion related to this patch. When I searched across the web for any discussion related to the patch (+/- six months) I get zero results.

The search hits for the msysgit mailing list that I reviewed were all tangential to the original issue, and did not help me to better understand the motivation for the patch.

I agree with you. It appears the that the original motivation was a desire to avoid shelling out as opposed to a reported issue.

I investigated the commits by the original author that happened around the time of the commit in question. The author was implementing an installation that could be moved to different directories without the need to recompile. The author added support for resolving paths relative to exec-dir and htmldir. I speculate that he was in the area, so he implemented the ShellExecute change.

The author's change previous to this was seven months prior, and was unrelated.
The author's changes after the commit in question were mostly related to resolving relative path issues.

I searched commits previous to the one in question for anything related to path translation, and I did not find anything of interest. (I did not dig that deep. I searched commit logs.) The majority of mentions that I found were for git-gui.

[dscho]

I vaguely remember that there was a discussion about not being able to override the web browser, and some people claiming that Windows users would not want to use the config method anyway.

However, I think that your patch is a strict improvement because it does support the config way, while keeping the previous behavior for users who did not configure a different web browser in their config.

Thanks!