Re: [msysGit] Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>> <johannes.schindelin@xxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]