Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 26, 2019, 21:42 UTC
Message-ID: <20191226214245.GA186931@google.com>
In-Reply-To: <nycvar.QRO.7.76.6.1912262221000.46@tvgsbejvaqbjf.bet>

Hi,

Johannes Schindelin wrote:

> On Thu, 26 Dec 2019, Jonathan Nieder wrote:
>> Johannes Schindelin wrote:

>>> Arguably, this is the wrong layer for that error, anyway: As long as the
>>> user never checks out the files whose names contain backslashes, there
>>> should not be any problem in the first place.

[...]

Show 10 quoted lines

>>          between the lines of this commit messages I sense that there
>> *are* repositories in the wild using these kinds of filenames.
>>
>> Can you say more about that?  What repositories are affected?  Do they
>> contain such filenames at HEAD or only in their history?  If someone
>> wants to check out a revision with such filenames, what should happen?
>
> Yes: https://github.com/zephyrproject-rtos/civetweb contains history where
> at some stage, by mistake, a directory was called `\`. It has been fixed a
> long time ago, but users obviously still want to be able to clone it ;-)

Thanks.

What should and does happen if someone tries to check out an offending revision?

[...]

Show 8 quoted lines

> I added this paragraph to the commit message:
>
>     Note: just as before, the check is guarded by `core.protectNTFS` (to
>     allow overriding the check by toggling that config setting), and it
>     is _only_ performed on Windows, as the backslash is not a directory
>     separator elsewhere, even when writing to NTFS-formatted volumes.
>
> Does that clarify the issue enough?

It helps: that tells me why the check is only performed on Windows.

Since this only affects Windows, please only take this review as data about how someone read the patch. The patch doesn't make non Windows specific code any *less* maintainable, and as a general presumption I assume that the Git for Windows maintainer knows best about what is needed for maintainability of Windows-specific code.

But the commit message and docs still don't describe what the desired behavior is. For example, should I be able to check out a revision with a backslash in it (e.g. via Git skipping the offending entry), or is the intended behavior for it to error out and prevent checking out such versions of code?

Is there anything we can or should do to prevent people checking in new examples of paths with backslash in them (on all platforms)?

Thanks, Jonathan