Re: [msysGit] Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
Am 13.08.2015 um 09:30 schrieb Johannes Schindelin: Hi Johannes, On 2015-08-12 20:31, Johannes Sixt wrote: Am 12.08.2015 um 13:58 schrieb Erik Faye-Lund: On Wed, Aug 12, 2015 at 1:07 PM, Johannes Schindelin wrote: FWIW Git for Windows has this patch (that I wanted to contribute in due time, what with being busy with all those tickets) to solve the problem mentioned in your patch in a different way: https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45 Yuck. On Windows, it's the extension of a file that dictates what kind of file it is (and if it's executable or not), not the contents. If we get a shell script written with the ".exe"-prefix, it's considered as an invalid executable by the system. We should consider it the same way, otherwise we're on the path to user-experience schizophrenia. I'm not sure I consider this commit a step in the right direction. I, too, think that it is a wrong decision to pessimize git for the sake of a single test case. Oh, you make it sound as if you believe that I had indeed weakened Git *just* for a single test case. Whatever. Since I do not have the time to provide hard numbers that prove my claim that your patch removes an optimization (and, furthermore, I do not want to reply to your arguments that I consider mostly philosophical rather than pragmatic), I bow out. Until this solution or that one is in upstream, I can help myself. Junio, please drop my patch. I do not have the nerves to support it. -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
On Thu, Aug 13, 2015 at 10:37 AM, Johannes Schindelin wrote: > Hi kusma, > > On 2015-08-12 13:58, Erik Faye-Lund wrote: >> On Wed, Aug 12, 2015 at 1:07 PM, Johannes Schindelin >> wrote: >>> >>> On 2015-08-11 22:51, Johannes Sixt wrote: Invoking plink requires special treatment, and we have support and even test cases for the commands 'plink' and 'tortoiseplink'. We also support .exe variants for these two and there is a test for 'plink.exe'. On Windows, however, where support for plink.exe would be relevant, the test case fails because it is not possible to execute a file with a .exe extension that is actually not a binary executable---it is a shell script in our test. We have to disable the test case on Windows. >>> >>> Oh how would I wish you were working on Git for Windows even *just* a bit >>> *with* me. At least I would wish for a more specific description of the >>> development environment, because it sure as hell is not anything anybody >>> can download and install as easily as Git for Windows' SDK. >>> >>> FWIW Git for Windows has this patch (that I wanted to contribute in due >>> time, what with being busy with all those tickets) to solve the problem >>> mentioned in your patch in a different way: >>> >>> https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45 >> >> Yuck. On Windows, it's the extension of a file that dictates what kind >> of file it is (and if it's executable or not), not the contents. > > Careful. If you continue along those lines, interactive rebase, `git add -p` > and all those wonderful scripts Git has will have to stop working. > > Because those scripts completely disagree with what you just said about > Windows if you think about it: *none* of them has an extension. > > I know that you do not mean this, of course, but that is the argument you > were making... ;-) > You should know better than to straw-man like that. I was not arguing to break any current functionality, but to not move further away from Windows' semantics. But if I would have, there's nothing that would stop us from renaming those scrips to *.sh, and let the filename dictate how to execute them. Or provide batch-files to wrap them. >> If we get a shell script written with the ".exe"-prefix, it's considered as >> an invalid executable by the system. > > And if we get a shell script without any `.exe` suffix, it is still > considered as an invalid executable by the system. Nope, it's considered an unknown file, not an executable at all. > And even if we tack on an `.sh` suffix (which is *not* in line with the way > Git works), it is *still* considered as an invalid executable by the system. That's not necessarily true; the Git for Windows installer (optionally, but on by default) registers /bin/sh as a file-handler for .sh files. Windows knows just fine how to execute them, unless the user opts out. But again, I was not arguing to patch git to not parse the shebang. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
Hi kusma, On 2015-08-12 13:58, Erik Faye-Lund wrote: > On Wed, Aug 12, 2015 at 1:07 PM, Johannes Schindelin > wrote: >> >> On 2015-08-11 22:51, Johannes Sixt wrote: >>> Invoking plink requires special treatment, and we have support and even >>> test cases for the commands 'plink' and 'tortoiseplink'. We also support >>> .exe variants for these two and there is a test for 'plink.exe'. >>> >>> On Windows, however, where support for plink.exe would be relevant, the >>> test case fails because it is not possible to execute a file with a .exe >>> extension that is actually not a binary executable---it is a shell >>> script in our test. We have to disable the test case on Windows. >> >> Oh how would I wish you were working on Git for Windows even *just* a bit >> *with* me. At least I would wish for a more specific description of the >> development environment, because it sure as hell is not anything anybody can >> download and install as easily as Git for Windows' SDK. >> >> FWIW Git for Windows has this patch (that I wanted to contribute in due >> time, what with being busy with all those tickets) to solve the problem >> mentioned in your patch in a different way: >> >> https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45 > > Yuck. On Windows, it's the extension of a file that dictates what kind > of file it is (and if it's executable or not), not the contents. Careful. If you continue along those lines, interactive rebase, `git add -p` and all those wonderful scripts Git has will have to stop working. Because those scripts completely disagree with what you just said about Windows if you think about it: *none* of them has an extension. I know that you do not mean this, of course, but that is the argument you were making... ;-) > If we get a shell script written with the ".exe"-prefix, it's considered as > an invalid executable by the system. And if we get a shell script without any `.exe` suffix, it is still considered as an invalid executable by the system. And even if we tack on an `.sh` suffix (which is *not* in line with the way Git works), it is *still* considered as an invalid executable by the system. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
Hi Johannes, On 2015-08-12 20:31, Johannes Sixt wrote: > Am 12.08.2015 um 13:58 schrieb Erik Faye-Lund: >> On Wed, Aug 12, 2015 at 1:07 PM, Johannes Schindelin >> wrote: >>> FWIW Git for Windows has this patch (that I wanted to contribute >>> in due time, what with being busy with all those tickets) to solve the >>> problem mentioned in your patch in a different way: >>> >>> https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45 >> >> Yuck. On Windows, it's the extension of a file that dictates what kind >> of file it is (and if it's executable or not), not the contents. If we >> get a shell script written with the ".exe"-prefix, it's considered as >> an invalid executable by the system. We should consider it the same >> way, otherwise we're on the path to user-experience schizophrenia. >> >> I'm not sure I consider this commit a step in the right direction. > > I, too, think that it is a wrong decision to pessimize git for the > sake of a single test case. Oh, you make it sound as if you believe that I had indeed weakened Git *just* for a single test case. That is quite a strong assumption, and could not be further from the truth, though, I have to point out. It is important to keep in mind that we (actually, IIRC it was you) taught Git to recognize shell scripts when executing external programs *because Windows does not do that for us*. So yes, we are deviating from the standard Windows way of things, and we do that quite intentionally so. Now, let's look at the test case for a moment and let's try to understand the technique it uses (that breaks the test case currently). It puts a script in place of an `.exe`, with the intention to execute the script instead of the original executable. This technique is an age-old technique on Unix, and it just works. There are a range of valid reasons, from debugging to slightly modifying the function of a particular `.exe` (possibly renaming the original `.exe` and calling it from the script) in the easiest way: by scripting on top of it. If we want to allow such a thing -- allowing users to use scripts to modify the behavior of executables -- then we *have* to allow `.exe` suffixes for scripts, because that happens to be the suffix of executables on Windows. I guess you would have had an easier time to understand my thinking if I had replaced the sentence So the assumption that the `.exe` extension implies that the file is *not* a shell script is now wrong. by So this is a strong indicator that it was wrong to assume that `.exe` extensions imply that the file is *not* a shell script. Further, I even looked at the performance impact, but that is at least well documented in the commit message. I also have to point out that the alternative "solution" presented by your patch -- to disable the test case -- is no solution at all: the very platform that is most likely to have plink users is Windows. And to *exclude* that platform from running that unit test is questionable at best. Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
Am 12.08.2015 um 13:58 schrieb Erik Faye-Lund: On Wed, Aug 12, 2015 at 1:07 PM, Johannes Schindelin wrote: FWIW Git for Windows has this patch (that I wanted to contribute in due time, what with being busy with all those tickets) to solve the problem mentioned in your patch in a different way: https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45 Yuck. On Windows, it's the extension of a file that dictates what kind of file it is (and if it's executable or not), not the contents. If we get a shell script written with the ".exe"-prefix, it's considered as an invalid executable by the system. We should consider it the same way, otherwise we're on the path to user-experience schizophrenia. I'm not sure I consider this commit a step in the right direction. I, too, think that it is a wrong decision to pessimize git for the sake of a single test case. -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
On Wed, Aug 12, 2015 at 1:07 PM, Johannes Schindelin wrote: > Hi Johannes, > > On 2015-08-11 22:51, Johannes Sixt wrote: >> Invoking plink requires special treatment, and we have support and even >> test cases for the commands 'plink' and 'tortoiseplink'. We also support >> .exe variants for these two and there is a test for 'plink.exe'. >> >> On Windows, however, where support for plink.exe would be relevant, the >> test case fails because it is not possible to execute a file with a .exe >> extension that is actually not a binary executable---it is a shell >> script in our test. We have to disable the test case on Windows. > > Oh how would I wish you were working on Git for Windows even *just* a bit > *with* me. At least I would wish for a more specific description of the > development environment, because it sure as hell is not anything anybody can > download and install as easily as Git for Windows' SDK. > > FWIW Git for Windows has this patch (that I wanted to contribute in due time, > what with being busy with all those tickets) to solve the problem mentioned > in your patch in a different way: > > https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45 Yuck. On Windows, it's the extension of a file that dictates what kind of file it is (and if it's executable or not), not the contents. If we get a shell script written with the ".exe"-prefix, it's considered as an invalid executable by the system. We should consider it the same way, otherwise we're on the path to user-experience schizophrenia. I'm not sure I consider this commit a step in the right direction. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
