Fixes “Not a valid executable” error in installer

[aakriti-jain]

This issue was addressed in git-for-windows/git#1951.
Currently, I have only edited the error message as it only showed the first 3 characters if the path was not of a valid .exe file.

I need a suggestion: Should I also make it support CMD commands using %PATH% enviroment? Currently the message is apt for its working.

Signed-off-by: Aakriti Jain [email protected]

[dscho]

I think we can use this opportunity to make the code less confusing... What do you think?

[dscho]

Thank you so much for working on this!

Should I also make it support CMD commands using %PATH% enviroment? Currently the message is apt for its working.

I think it would be wonderful if the EnableNextButtonOnValidExecutablePath() function could try other things if it detects that the file referred by CustomEditorPath does not exist:

• it could see whether CustomEditorPath lacks a file extension, and if it does, append the extensions listed in the environment variable PATHEXT (.com, .exe, .bat, etc) and see whether that exists (adjusting the CustomEditorPath variable accordingly).
• it could see whether there is a dir separator in the path (\ or /), and if there is none, search through all the directories listed in the PATH environment variable.

Furthermore, instead of the hard-coded .exe test, it could extract the file extension and if there is one, verify that it is one of the items listed in PATHEXT (a neat trick is to surround that string with semicolons and then search for ;<ext>;, i.e. Pos(';'+FileExtension+';',';'+PathExt+';')).

This is asking a lot, though... it is kind of the triple serving of fries with home made aioli and the truffle mayonnaise.

[aakriti-jain]

The code is currently such that after every change, it checks weather the variable CustomEditorPath exists and is an .exe file, and enables/disables the Next button accordingly. That means the status of button changes after every hit of keyboard.
I may be wrong, but wont this approach make a lot of overhead?

But still I am working on the solution for it. I will let you know if I manage to get it to work.

[dscho]

The code is currently such that after every change, it checks weather the variable CustomEditorPath exists and is an .exe file, and enables/disables the Next button accordingly. That means the status of button changes after every hit of keyboard.
I may be wrong, but wont this approach make a lot of overhead?

You're right!

I was wrong to mention that function. What I meant was the validation that would bug the user with that error message unless the path exists and ends in .exe.

[dscho]

Please note that Git v2.20.0 has already been released, and therefore I want to publish Git for Windows v2.20.0 as soon as possible.

Unfortunately, this means that this PR won't make it in before that version, but given that the custom editor option is relatively new (and does not seem to be used very often yet), I think we can take our time to hammer out the kinks and get it into v2.20.1 (or v2.20.0(2), or v2.21.0, whichever comes first after we're done with this PR). Does that sound okay, @aakriti-jain?

[aakriti-jain]

Sorry @dscho I had exams in my college and had to pause the work on it. I will surely work on it and complete this task surely before the next release.

[dscho]

@aakriti-jain no worries. Do you want me to pick up some tasks, or do you want me to wait for you to do it all?

[aakriti-jain]

@dscho If we have enough time, I'd love to work on this issue. I want to contribute to the project!

[dscho]

@aakriti-jain excellent. I think we have enough time. Just let me know whenever I can be of assistance.

[aakriti-jain]

@dscho I have written the code as you suggested such that function PathIsValidExecutable now searches for every executable (in %PATHEXT%) in locations in %PATH%. And there is no noticeable lag too 😄
But when I test it with Sublime Text using the command subl -w, it would just get stuck, even after editing the temporary file and closing the editor. I tried this with the released version of Git for Windows (but instead giving complete path), had the same problem. VSCode works smoothly though.

So I guess this was not an error of the new code that I wrote, but with the Sublime Text instead. So should I try to fix this in the same PR or move it to a second?

[dscho]

@aakriti-jain thank you so much! I worked a bit on top of your patches, and I think it should work with Sublime Text now. Could I ask you to test?

[aakriti-jain]

Yes I will test it now! Sorry I was out of station these days and couldn't test it then. I will just test and complete this issue.

[aakriti-jain]

@dscho all the cases have been tested and most of them worked perfectly fine! Thank you so much for the extra patches you made!

Just a thing more: This option is still disabled-

I have Sublime Text 3 properly installed but its not showing so.

[dscho]

I have Sublime Text 3 properly installed but its not showing so.

Hmm. I don't have that installed...

Could you maybe debug by placing strategic "debug print statements" (I usually abuse LogError() for this purpose) and then run ./installer/release.sh -d Editor in /usr/src/build-extra/?

[aakriti-jain]

Can I work on it a bit later in a separate PR? Been a bit busy these days

[dscho]

Can I work on it a bit later in a separate PR? Been a bit busy these days

Of course!