{"thread":{"id":"52522","subject":"[PATCH 0/1] Disallow writing, but not fetching commits with file names containing backslashes","startedAt":"2019-12-26T17:42:36Z","lastAt":"2020-01-04T21:29:52Z","messageCount":19,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Jonathan Nieder","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"388926","messageId":"pull.682.git.git.1577382151.gitgitgadget@gmail.com","threadId":"52522","inReplyTo":null,"subject":"[PATCH 0/1] Disallow writing, but not fetching commits with file names containing backslashes","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-26T17:42:30Z","receivedAt":"2019-12-26T17:42:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"As of Git for Windows, v2.24.1(2). cloning a repository that contained a\nfile with a backslash in its name some time in the past, the command will \nsucceed but at the same time print errors like this:\n\n error: filename in tree entry contains backslash: '\\'\n\nA corresponding git fetch will also show those errors, but fail.\n\nThe reason is that v2.24.1 is much more strict about backslashes in tree\nentries than earlier versions. The intention was actually to prevent \nchecking out such files, though: if there was a mistake in a repository long \nago that has been fixed long since, there is actually no reason why we\nshould require the history to be rewritten.\n\nThis fixes https://github.com/git-for-windows/git/issues/2435.\n\nThe idea of this patch is to stop complaining about tree entries, and focus\ninstead on the index: whenever a file is added to the index, we do not want\nany backslashes in the file name on Windows.\n\nAs before, this check is only performed on Windows, and only under \ncore.protectNTFS. On other platforms, even if core.protectNTFS is turned on,\nthe backslash is not a directory separator, therefore the Operating System's\nsyscalls will (should?) refuse to create files on NTFS with backslashes in\ntheir file name.\n\nI would appreciate reviews with a particular eye on keeping users safe: I am\nnot 100% certain that all relevant file writes go through the index (I think \nthat they all go through the index, but I might have well missed a corner\ncase).\n\nJohannes Schindelin (1):\n mingw: only test index entries for backslashes, not tree entries\n\n read-cache.c | 5 +++++\n t/t7415-submodule-names.sh | 7 ++++---\n tree-walk.c | 6 ------\n 3 files changed, 9 insertions(+), 9 deletions(-)\n\n\nbase-commit: 12029dc57db23baef008e77db1909367599210ee\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-682%2Fdscho%2Fmingw-only-error-on-backslash-in-index-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-682/dscho/mingw-only-error-on-backslash-in-index-v1\nPull-Request: https://github.com/git/git/pull/682\n-- \ngitgitgadget\n"},{"id":"388927","messageId":"4a120fd0b32d2d6492eac6b0494ad6b1bc2ba500.1577382151.git.gitgitgadget@gmail.com","threadId":"52522","inReplyTo":"pull.682.git.git.1577382151.gitgitgadget@gmail.com","subject":"[PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-26T17:42:31Z","receivedAt":"2019-12-26T17:42:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nDuring a clone of a repository that contained a file with a backslash in\nits name in the past, as of v2.24.1(2), Git for Windows prints errors\nlike this:\n\n\terror: filename in tree entry contains backslash: '\\'\n\nWhile the clone still succeeds, a similar error prevents the equivalent\n`git fetch` operation, which is inconsistent.\n\nArguably, this is the wrong layer for that error, anyway: As long as the\nuser never checks out the files whose names contain backslashes, there\nshould not be any problem in the first place.\n\nSo let's instead prevent such files to be added to the index.\n\nThis addresses https://github.com/git-for-windows/git/issues/2435\n\nSigned-off-by: Johannes Schindelin \n---\n read-cache.c | 5 +++++\n t/t7415-submodule-names.sh | 7 ++++---\n tree-walk.c | 6 ------\n 3 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex ad0b48c84d..737916ebd9 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1278,6 +1278,11 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \tint skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;\n \tint new_only = option & ADD_CACHE_NEW_ONLY;\n \n+#ifdef GIT_WINDOWS_NATIVE\n+\tif (protect_ntfs && strchr(ce->name, '\\\\'))\n+\t\treturn error(_(\"filename in tree entry contains backslash: '%s'\"), ce->name);\n+#endif\n+\n \tif (!(option & ADD_CACHE_KEEP_CACHE_TREE))\n \t\tcache_tree_invalidate_path(istate, ce->name);\n \ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex 905a557585..7ae0dc8ff4 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -207,6 +207,9 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '\n \t\t\tgit hash-object -w --stdin)\" &&\n \t\trev=\"$(git rev-parse --verify HEAD)\" &&\n \t\thash=\"$(echo x | git hash-object -w --stdin)\" &&\n+\t\ttest_must_fail git update-index --add \\\n+\t\t\t--cacheinfo 160000,$rev,d\\\\a 2>err &&\n+\t\ttest_i18ngrep backslash err &&\n \t\tgit -c core.protectNTFS=false update-index --add \\\n \t\t\t--cacheinfo 100644,$modules,.gitmodules \\\n \t\t\t--cacheinfo 160000,$rev,c \\\n@@ -214,9 +217,7 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '\n \t\t\t--cacheinfo 100644,$hash,d./a/x \\\n \t\t\t--cacheinfo 100644,$hash,d./a/..git &&\n \t\ttest_tick &&\n-\t\tgit -c core.protectNTFS=false commit -m \"module\" &&\n-\t\ttest_must_fail git show HEAD: 2>err &&\n-\t\ttest_i18ngrep backslash err\n+\t\tgit -c core.protectNTFS=false commit -m \"module\"\n \t) &&\n \ttest_must_fail git -c core.protectNTFS=false \\\n \t\tclone --recurse-submodules squatting squatting-clone 2>err &&\ndiff --git a/tree-walk.c b/tree-walk.c\nindex b3d162051f..d5a8e096a6 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -43,12 +43,6 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l\n \t\tstrbuf_addstr(err, _(\"empty filename in tree entry\"));\n \t\treturn -1;\n \t}\n-#ifdef GIT_WINDOWS_NATIVE\n-\tif (protect_ntfs && strchr(path, '\\\\')) {\n-\t\tstrbuf_addf(err, _(\"filename in tree entry contains backslash: '%s'\"), path);\n-\t\treturn -1;\n-\t}\n-#endif\n \tlen = strlen(path) + 1;\n \n \t/* Initialize the descriptor entry */\n-- \ngitgitgadget\n"},{"id":"388936","messageId":"xmqqr20qlxtz.fsf@gitster-ct.c.googlers.com","threadId":"52522","inReplyTo":"4a120fd0b32d2d6492eac6b0494ad6b1bc2ba500.1577382151.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-26T18:56:56Z","receivedAt":"2019-12-26T18:57:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> During a clone of a repository that contained a file with a backslash in\n> its name in the past, as of v2.24.1(2), Git for Windows prints errors\n> like this:\n>\n> \terror: filename in tree entry contains backslash: '\\'\n>\n> While the clone still succeeds, a similar error prevents the equivalent\n> `git fetch` operation, which is inconsistent.\n\nYes, inconsistent is bad and it is puzzling. I would have expected,\nif this gate on the transport layer is desirable, such a check would\nbe implemented as a part of transfer.fsckObjects and covered equally\nby fetch and clone codepaths. What went wrong to allow \"clone\" to\ngo through while stopping \"fetch\"? Can you describe the root cause\nof the difference in the log message?\n\n> Arguably, this is the wrong layer for that error, anyway: As long as the\n> user never checks out the files whose names contain backslashes, there\n> should not be any problem in the first place.\n\nI do agree that rejecting these tree objects that has a slash in its\npath component is probably wrong. A GIT_WINDOWS_NATIVE box should\nbe able to host a bare repository on it, and users on machines that\nare OK with paths that Windows may not like should be able to\ninteract with it, by pushing to it, fetching from it, and updating\nthe repository on that Windows box by going there and fetching from\nelsewhere. Rejecting these names at the object validity level means\nGit on Windows would be incompatible with Git elsewhere.\n\nAnd It hink the same logic apply to those names like prn, con, nul,\netc. How are the users protected from them? We should prevent\nthese names from entering the index the same way, shouldn't we?\n\n> So let's instead prevent such files to be added to the index.\n\n... and loosen the check that (incorrectly) gets triggered from what\ncodepaths in \"git fetch\" (but not from \"git clone\")?\n\n> This addresses https://github.com/git-for-windows/git/issues/2435\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> read-cache.c | 5 +++++\n> t/t7415-submodule-names.sh | 7 ++++---\n> tree-walk.c | 6 ------\n> 3 files changed, 9 insertions(+), 9 deletions(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index ad0b48c84d..737916ebd9 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1278,6 +1278,11 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n> \tint skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;\n> \tint new_only = option & ADD_CACHE_NEW_ONLY;\n> \n> +#ifdef GIT_WINDOWS_NATIVE\n> +\tif (protect_ntfs && strchr(ce->name, '\\\\'))\n\nAs I wondered above, names that must not enter the index may not be\nlimited to those with backslashes in them. Perhaps you'd want a\nseparate helper function so that you can extend the logic more\neasily, i.e.\n\n\tif (protect_ntfs && invalid_name_on_windows(ce->name))\n\nor something like that.\n\n> diff --git a/tree-walk.c b/tree-walk.c\n> index b3d162051f..d5a8e096a6 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -43,12 +43,6 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l\n> \t\tstrbuf_addstr(err, _(\"empty filename in tree entry\"));\n> \t\treturn -1;\n> \t}\n> -#ifdef GIT_WINDOWS_NATIVE\n> -\tif (protect_ntfs && strchr(path, '\\\\')) {\n> -\t\tstrbuf_addf(err, _(\"filename in tree entry contains backslash: '%s'\"), path);\n> -\t\treturn -1;\n> -\t}\n> -#endif\n> \tlen = strlen(path) + 1;\n> \n> \t/* Initialize the descriptor entry */\n"},{"id":"388940","messageId":"xmqqk16ilwnw.fsf@gitster-ct.c.googlers.com","threadId":"52522","inReplyTo":"pull.682.git.git.1577382151.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/1] Disallow writing, but not fetching commits with file names containing backslashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-26T19:22:11Z","receivedAt":"2019-12-26T19:22:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> I would appreciate reviews with a particular eye on keeping users safe: I am\n> not 100% certain that all relevant file writes go through the index (I think \n> that they all go through the index, but I might have well missed a corner\n> case).\n\nThere are peripheral commands that do not use the index at all, such\nas \"archive\"; piping \"git archive\" output to unarchiver that writes\ninto the filesystem would be a way. But I do not think that\nqualifies as an attack vector you are looking for.\n"},{"id":"388945","messageId":"20191226200316.GD170890@google.com","threadId":"52522","inReplyTo":"4a120fd0b32d2d6492eac6b0494ad6b1bc2ba500.1577382151.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-26T20:03:16Z","receivedAt":"2019-12-26T20:06:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJohannes Schindelin wrote:\n\n> During a clone of a repository that contained a file with a backslash in\n> its name in the past, as of v2.24.1(2), Git for Windows prints errors\n> like this:\n>\n> \terror: filename in tree entry contains backslash: '\\'\n>\n> While the clone still succeeds, a similar error prevents the equivalent\n> `git fetch` operation, which is inconsistent.\n>\n> Arguably, this is the wrong layer for that error, anyway: As long as the\n> user never checks out the files whose names contain backslashes, there\n> should not be any problem in the first place.\n\nHm. The choice of right layer depends on what repositories in the wild\ncontain. If there are none containing filenames with '\\', then fsck et\nal would be an appropriate layer for this. With hindsight, it was not\na good idea to support this kind of filename.\n\nHowever, between the lines of this commit messages I sense that there\n*are* repositories in the wild using these kinds of filenames.\n\nCan you say more about that? What repositories are affected? Do they\ncontain such filenames at HEAD or only in their history? If someone\nwants to check out a revision with such filenames, what should happen?\n\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1278,6 +1278,11 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n> \tint skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;\n> \tint new_only = option & ADD_CACHE_NEW_ONLY;\n> \n> +#ifdef GIT_WINDOWS_NATIVE\n> +\tif (protect_ntfs && strchr(ce->name, '\\\\'))\n> +\t\treturn error(_(\"filename in tree entry contains backslash: '%s'\"), ce->name);\n> +#endif\n> +\n\nWhy is this specific to the GIT_WINDOWS_NATIVE case? Wouldn't it affect\nntfs usage on other platforms as well?\n\n[...]\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -43,12 +43,6 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l\n> \t\tstrbuf_addstr(err, _(\"empty filename in tree entry\"));\n> \t\treturn -1;\n> \t}\n> -#ifdef GIT_WINDOWS_NATIVE\n> -\tif (protect_ntfs && strchr(path, '\\\\')) {\n> -\t\tstrbuf_addf(err, _(\"filename in tree entry contains backslash: '%s'\"), path);\n> -\t\treturn -1;\n> -\t}\n> -#endif\n\nAh, it's inherited from there, so orthogonal to this patch.\n\nTo summarize: I think the commit message and docs could use some work\nto describe what invariants we're trying to maintain and what\nreal-world usage motivates them.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"388954","messageId":"nycvar.QRO.7.76.6.1912262209190.46@tvgsbejvaqbjf.bet","threadId":"52522","inReplyTo":"xmqqr20qlxtz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-26T21:16:15Z","receivedAt":"2019-12-26T21:16:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 26 Dec 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > During a clone of a repository that contained a file with a backslash in\n> > its name in the past, as of v2.24.1(2), Git for Windows prints errors\n> > like this:\n> >\n> > \terror: filename in tree entry contains backslash: '\\'\n> >\n> > While the clone still succeeds, a similar error prevents the equivalent\n> > `git fetch` operation, which is inconsistent.\n>\n> Yes, inconsistent is bad and it is puzzling. I would have expected,\n> if this gate on the transport layer is desirable, such a check would\n> be implemented as a part of transfer.fsckObjects and covered equally\n> by fetch and clone codepaths. What went wrong to allow \"clone\" to\n> go through while stopping \"fetch\"? Can you describe the root cause\n> of the difference in the log message?\n\nMy bad, I should have root-caused this better.\n\nTurns out that this inconsistency is only in Git for Windows v2.24.1(2)\nbut not in current `master` of Git, so I simply struck that part from the\ncommit message.\n\n> > Arguably, this is the wrong layer for that error, anyway: As long as\n> > the user never checks out the files whose names contain backslashes,\n> > there should not be any problem in the first place.\n>\n> I do agree that rejecting these tree objects that has a slash in its\n> path component is probably wrong. A GIT_WINDOWS_NATIVE box should\n> be able to host a bare repository on it, and users on machines that\n> are OK with paths that Windows may not like should be able to\n> interact with it, by pushing to it, fetching from it, and updating\n> the repository on that Windows box by going there and fetching from\n> elsewhere. Rejecting these names at the object validity level means\n> Git on Windows would be incompatible with Git elsewhere.\n>\n> And It hink the same logic apply to those names like prn, con, nul,\n> etc. How are the users protected from them? We should prevent\n> these names from entering the index the same way, shouldn't we?\n>\n> > So let's instead prevent such files to be added to the index.\n>\n> ... and loosen the check that (incorrectly) gets triggered from what\n> codepaths in \"git fetch\" (but not from \"git clone\")?\n\nI rephrased it to:\n\n So let's loosen the requirements: we now leave tree entries with\n backslashes in their file names alone, but we do require any entries\n that are added to the Git index to contain no backslashes on Windows.\n\n> > This addresses https://github.com/git-for-windows/git/issues/2435\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > read-cache.c | 5 +++++\n> > t/t7415-submodule-names.sh | 7 ++++---\n> > tree-walk.c | 6 ------\n> > 3 files changed, 9 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/read-cache.c b/read-cache.c\n> > index ad0b48c84d..737916ebd9 100644\n> > --- a/read-cache.c\n> > +++ b/read-cache.c\n> > @@ -1278,6 +1278,11 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n> > \tint skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;\n> > \tint new_only = option & ADD_CACHE_NEW_ONLY;\n> >\n> > +#ifdef GIT_WINDOWS_NATIVE\n> > +\tif (protect_ntfs && strchr(ce->name, '\\\\'))\n>\n> As I wondered above, names that must not enter the index may not be\n> limited to those with backslashes in them. Perhaps you'd want a\n> separate helper function so that you can extend the logic more\n> easily, i.e.\n>\n> \tif (protect_ntfs && invalid_name_on_windows(ce->name))\n>\n> or something like that.\n\nI decided to perform those checks at yet another layer: when trying to\ncreate new files. My idea was that I would want to catch even things like\n`git config -f LPT1 ...` (`LPT1` is a reserved name on Windows, you cannot\ncreate a file with that name).\n\nObviously, I cannot handle the backslash in the same code path, as e.g.\n`git config -f C:\\Users\\me\\.gitconfig ...` is totally valid.\n\nCiao,\nDscho\n\n> > diff --git a/tree-walk.c b/tree-walk.c\n> > index b3d162051f..d5a8e096a6 100644\n> > --- a/tree-walk.c\n> > +++ b/tree-walk.c\n> > @@ -43,12 +43,6 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l\n> > \t\tstrbuf_addstr(err, _(\"empty filename in tree entry\"));\n> > \t\treturn -1;\n> > \t}\n> > -#ifdef GIT_WINDOWS_NATIVE\n> > -\tif (protect_ntfs && strchr(path, '\\\\')) {\n> > -\t\tstrbuf_addf(err, _(\"filename in tree entry contains backslash: '%s'\"), path);\n> > -\t\treturn -1;\n> > -\t}\n> > -#endif\n> > \tlen = strlen(path) + 1;\n> >\n> > \t/* Initialize the descriptor entry */\n>\n>\n"},{"id":"388956","messageId":"nycvar.QRO.7.76.6.1912262216540.46@tvgsbejvaqbjf.bet","threadId":"52522","inReplyTo":"xmqqk16ilwnw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/1] Disallow writing, but not fetching commits with file names containing backslashes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-26T21:19:53Z","receivedAt":"2019-12-26T21:20:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 26 Dec 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > I would appreciate reviews with a particular eye on keeping users safe: I am\n> > not 100% certain that all relevant file writes go through the index (I think\n> > that they all go through the index, but I might have well missed a corner\n> > case).\n>\n> There are peripheral commands that do not use the index at all, such\n> as \"archive\"; piping \"git archive\" output to unarchiver that writes\n> into the filesystem would be a way. But I do not think that\n> qualifies as an attack vector you are looking for.\n\nYes, I thought about `git archive`, too. The thing is, I could imagine\nlegitimate use cases where a user wants to generate e.g. a `.zip` on\nWindows, with the intention to unpack it on Linux. In such a case, we\nwould not want to prevent said `.zip` from being generated.\n\nI also briefly considered the scripts that write some trees into a\ntemporary location, but those scripts typically use `git read-tree -u -m`\nwith a temporary index, i.e. they _do_ go through the index, still.\n\nThank you for your thoughts about this issue,\nDscho\n"},{"id":"388957","messageId":"nycvar.QRO.7.76.6.1912262221000.46@tvgsbejvaqbjf.bet","threadId":"52522","inReplyTo":"20191226200316.GD170890@google.com","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-26T21:23:26Z","receivedAt":"2019-12-26T21:23:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Thu, 26 Dec 2019, Jonathan Nieder wrote:\n\n> Johannes Schindelin wrote:\n>\n> > During a clone of a repository that contained a file with a backslash in\n> > its name in the past, as of v2.24.1(2), Git for Windows prints errors\n> > like this:\n> >\n> > \terror: filename in tree entry contains backslash: '\\'\n> >\n> > While the clone still succeeds, a similar error prevents the equivalent\n> > `git fetch` operation, which is inconsistent.\n> >\n> > Arguably, this is the wrong layer for that error, anyway: As long as the\n> > user never checks out the files whose names contain backslashes, there\n> > should not be any problem in the first place.\n>\n> Hm. The choice of right layer depends on what repositories in the wild\n> contain. If there are none containing filenames with '\\', then fsck et\n> al would be an appropriate layer for this. With hindsight, it was not\n> a good idea to support this kind of filename.\n>\n> However, between the lines of this commit messages I sense that there\n> *are* repositories in the wild using these kinds of filenames.\n>\n> Can you say more about that? What repositories are affected? Do they\n> contain such filenames at HEAD or only in their history? If someone\n> wants to check out a revision with such filenames, what should happen?\n\nYes: https://github.com/zephyrproject-rtos/civetweb contains history where\nat some stage, by mistake, a directory was called `\\`. It has been fixed a\nlong time ago, but users obviously still want to be able to clone it ;-)\n\n> > --- a/read-cache.c\n> > +++ b/read-cache.c\n> > @@ -1278,6 +1278,11 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n> > \tint skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;\n> > \tint new_only = option & ADD_CACHE_NEW_ONLY;\n> >\n> > +#ifdef GIT_WINDOWS_NATIVE\n> > +\tif (protect_ntfs && strchr(ce->name, '\\\\'))\n> > +\t\treturn error(_(\"filename in tree entry contains backslash: '%s'\"), ce->name);\n> > +#endif\n> > +\n>\n> Why is this specific to the GIT_WINDOWS_NATIVE case? Wouldn't it affect\n> ntfs usage on other platforms as well?\n>\n> [...]\n> > --- a/tree-walk.c\n> > +++ b/tree-walk.c\n> > @@ -43,12 +43,6 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l\n> > \t\tstrbuf_addstr(err, _(\"empty filename in tree entry\"));\n> > \t\treturn -1;\n> > \t}\n> > -#ifdef GIT_WINDOWS_NATIVE\n> > -\tif (protect_ntfs && strchr(path, '\\\\')) {\n> > -\t\tstrbuf_addf(err, _(\"filename in tree entry contains backslash: '%s'\"), path);\n> > -\t\treturn -1;\n> > -\t}\n> > -#endif\n>\n> Ah, it's inherited from there, so orthogonal to this patch.\n>\n> To summarize: I think the commit message and docs could use some work\n> to describe what invariants we're trying to maintain and what\n> real-world usage motivates them.\n\nI added this paragraph to the commit message:\n\n Note: just as before, the check is guarded by `core.protectNTFS` (to\n allow overriding the check by toggling that config setting), and it\n is _only_ performed on Windows, as the backslash is not a directory\n separator elsewhere, even when writing to NTFS-formatted volumes.\n\nDoes that clarify the issue enough?\nDscho\n\n>\n> Thanks and hope that helps,\n> Jonathan\n>\n>\n"},{"id":"388959","messageId":"20191226214245.GA186931@google.com","threadId":"52522","inReplyTo":"nycvar.QRO.7.76.6.1912262221000.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-26T21:42:45Z","receivedAt":"2019-12-26T21:42:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJohannes Schindelin wrote:\n> On Thu, 26 Dec 2019, Jonathan Nieder wrote:\n>> Johannes Schindelin wrote:\n\n>>> Arguably, this is the wrong layer for that error, anyway: As long as the\n>>> user never checks out the files whose names contain backslashes, there\n>>> should not be any problem in the first place.\n[...]\n>> between the lines of this commit messages I sense that there\n>> *are* repositories in the wild using these kinds of filenames.\n>>\n>> Can you say more about that? What repositories are affected? Do they\n>> contain such filenames at HEAD or only in their history? If someone\n>> wants to check out a revision with such filenames, what should happen?\n>\n> Yes: https://github.com/zephyrproject-rtos/civetweb contains history where\n> at some stage, by mistake, a directory was called `\\`. It has been fixed a\n> long time ago, but users obviously still want to be able to clone it ;-)\n\nThanks.\n\nWhat should and does happen if someone tries to check out an offending\nrevision?\n\n[...]\n> I added this paragraph to the commit message:\n>\n> Note: just as before, the check is guarded by `core.protectNTFS` (to\n> allow overriding the check by toggling that config setting), and it\n> is _only_ performed on Windows, as the backslash is not a directory\n> separator elsewhere, even when writing to NTFS-formatted volumes.\n>\n> Does that clarify the issue enough?\n\nIt helps: that tells me why the check is only performed on Windows.\n\nSince this only affects Windows, please only take this review as data\nabout how someone read the patch. The patch doesn't make non Windows\nspecific code any *less* maintainable, and as a general presumption I\nassume that the Git for Windows maintainer knows best about what is\nneeded for maintainability of Windows-specific code.\n\nBut the commit message and docs still don't describe what the desired\nbehavior is. For example, should I be able to check out a revision\nwith a backslash in it (e.g. via Git skipping the offending entry), or\nis the intended behavior for it to error out and prevent checking out\nsuch versions of code?\n\nIs there anything we can or should do to prevent people checking in\nnew examples of paths with backslash in them (on all platforms)?\n\nThanks,\nJonathan\n"},{"id":"388960","messageId":"xmqqfth6kaqi.fsf@gitster-ct.c.googlers.com","threadId":"52522","inReplyTo":"20191226214245.GA186931@google.com","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-26T22:01:09Z","receivedAt":"2019-12-26T22:01:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Is there anything we can or should do to prevent people checking in\n> new examples of paths with backslash in them (on all platforms)?\n\nI obviously won't dictate what should happen on Windows, but I think\nthe overall principle for paths recorded in a tree object that can\nbe problematic on some of the platforms ought to be:\n\n * fsck and transfer.fsckobjects should be taught to notice\n offending characteristics (e.g. has a backslash in it, is one of\n the \"reserved names\" on some platform like LPT1).\n\n * if paths with the offending characteristics are *so* obviously\n useless in real life and are possible only in a crafted path that\n is only useful to attack users, the check in fsck should default\n to \"reject\" to help the disease spread via hosting sites.\n\n * otherwise, the check should be to \"warn\" but not \"reject\", so\n that projects can keep using paths that may problematic on\n platforms that do not matter to them.\n\nI think LPT1 and friends fall into the \"warning is fine\" category,\nand a path component that contains a backslash would fall into the\n\"this is an attack, just reject\" category.\n\n"},{"id":"388961","messageId":"xmqqblruk9lz.fsf@gitster-ct.c.googlers.com","threadId":"52522","inReplyTo":"xmqqfth6kaqi.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-26T22:25:28Z","receivedAt":"2019-12-26T22:25:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Nieder writes:\n>\n>> Is there anything we can or should do to prevent people checking in\n>> new examples of paths with backslash in them (on all platforms)?\n>\n> I obviously won't dictate what should happen on Windows, but I think\n> the overall principle for paths recorded in a tree object that can\n> be problematic on some of the platforms ought to be:\n>\n> * fsck and transfer.fsckobjects should be taught to notice\n> offending characteristics (e.g. has a backslash in it, is one of\n> the \"reserved names\" on some platform like LPT1).\n>\n> * if paths with the offending characteristics are *so* obviously\n> useless in real life and are possible only in a crafted path that\n> is only useful to attack users, the check in fsck should default\n> to \"reject\" to help the disease spread via hosting sites.\n>\n> * otherwise, the check should be to \"warn\" but not \"reject\", so\n> that projects can keep using paths that may problematic on\n> platforms that do not matter to them.\n>\n> I think LPT1 and friends fall into the \"warning is fine\" category,\n> and a path component that contains a backslash would fall into the\n> \"this is an attack, just reject\" category.\n\nI guess I should have stepped back a bit.\n\nIn the message I am responding to, I focused solely on how tree\nobjects that originate elsewhere should be treated, but there are\ntwo more data paths we need to worry about:\n\n * A new path gets introduced to the system, via \"update-index\",\n \"add\", etc., to the index and then \"write-tree\" to form tree\n objects in the local repository.\n\n * A path in the index, either created locally via \"update-index\",\n \"add\", etc., or read from a tree object, gets written to the\n local filesystem, resulting in an attempt to create a path that\n the local filesystem cannot represent (or worse---behaves badly,\n like \"sending random garbage to the printer\").\n\nI think we should apply the same principle as the one I outlined for\nthe tree objects. The fsckobjects mechanism may not be reusable to\ncatch violations in add_index_entry_with_check() as-is, but we need\nto aim for as much reuse of logic and code as possible so that our\nchecks at various layers all behave consistently.\n\nThanks.\n\n\n\n"},{"id":"389109","messageId":"xmqqk16dfpcy.fsf@gitster-ct.c.googlers.com","threadId":"52522","inReplyTo":"nycvar.QRO.7.76.6.1912262209190.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-30T21:57:49Z","receivedAt":"2019-12-30T21:57:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Turns out that this inconsistency is only in Git for Windows v2.24.1(2)\n> but not in current `master` of Git, so I simply struck that part from the\n> commit message.\n> ...\n> I rephrased it to:\n>\n> So let's loosen the requirements: we now leave tree entries with\n> backslashes in their file names alone, but we do require any entries\n> that are added to the Git index to contain no backslashes on Windows.\n> ...\n\nWe are in -rc so there is no real rush, but I take these to mean\nthat I should just leave this loose end hanging untied, and wait\nfor an updated version to replace it sometime early next year.\n\nThanks and happy new year.\n"},{"id":"389138","messageId":"nycvar.QRO.7.76.6.1912312329480.46@tvgsbejvaqbjf.bet","threadId":"52522","inReplyTo":"xmqqblruk9lz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-31T22:51:35Z","receivedAt":"2019-12-31T22:51:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio & Jonathan,\n\nOn Thu, 26 Dec 2019, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n> > Jonathan Nieder writes:\n> >\n> >> Is there anything we can or should do to prevent people checking in\n> >> new examples of paths with backslash in them (on all platforms)?\n> >\n> > I obviously won't dictate what should happen on Windows, but I think\n> > the overall principle for paths recorded in a tree object that can\n> > be problematic on some of the platforms ought to be:\n> >\n> > * fsck and transfer.fsckobjects should be taught to notice\n> > offending characteristics (e.g. has a backslash in it, is one of\n> > the \"reserved names\" on some platform like LPT1).\n\nAgree. This is on my radar, but so far not too-high priority, as the\n`fsck` checks are not (yet?) standard practice in PR builds (and warnings\non the server side are prone to be ignored).\n\n> > * if paths with the offending characteristics are *so* obviously\n> > useless in real life and are possible only in a crafted path that\n> > is only useful to attack users, the check in fsck should default\n> > to \"reject\" to help the disease spread via hosting sites.\n\nI don't think that reserved names such as `aux`, nor names containing\nbackslashes should be rejected _always_. While I cannot think of _any_\ninstance where I would want to have a backslash in a file name, I am sure\nthat just like `aux.c`, there _must_ be somebody out there who thought of\na file name that contains a backslash and makes at least some sort of\nsense.\n\n> > * otherwise, the check should be to \"warn\" but not \"reject\", so\n> > that projects can keep using paths that may problematic on\n> > platforms that do not matter to them.\n\nYes, it should be \"warn\".\n\n> > I think LPT1 and friends fall into the \"warning is fine\" category,\n> > and a path component that contains a backslash would fall into the\n> > \"this is an attack, just reject\" category.\n>\n> I guess I should have stepped back a bit.\n>\n> In the message I am responding to, I focused solely on how tree\n> objects that originate elsewhere should be treated, but there are\n> two more data paths we need to worry about:\n>\n> * A new path gets introduced to the system, via \"update-index\",\n> \"add\", etc., to the index and then \"write-tree\" to form tree\n> objects in the local repository.\n\nRight, that's what I had in mind when I wrote this patch. The path gets\nadded to the index, we detect a backslash, and on Windows (under\n`core.protectNTFS`) fail with an error.\n\n> * A path in the index, either created locally via \"update-index\",\n> \"add\", etc., or read from a tree object, gets written to the\n> local filesystem, resulting in an attempt to create a path that\n> the local filesystem cannot represent (or worse---behaves badly,\n> like \"sending random garbage to the printer\").\n\nHappily, my patch seems to catch this code path, too: when reading from a\n`tree` object into the index, we use `add_index_entry()` (called via\nvarious code paths in the `unpack_trees()` machinery). That's exactly the\npatched function.\n\nOr maybe you know of a code path in the `unpack_trees()` machinery that\ndoes _not_ go through `add_index_entry()`? I would be very interested to\nlearn about such code paths.\n\n> I think we should apply the same principle as the one I outlined for\n> the tree objects. The fsckobjects mechanism may not be reusable to\n> catch violations in add_index_entry_with_check() as-is, but we need\n> to aim for as much reuse of logic and code as possible so that our\n> checks at various layers all behave consistently.\n\nI am afraid that we won't be able to reuse code paths for checking the\nbackslash here, but for reserved names I am planning on refactoring the\ncode accordingly.\n\nThanks,\nDscho\n"},{"id":"389139","messageId":"pull.682.v2.git.git.1577832830.gitgitgadget@gmail.com","threadId":"52522","inReplyTo":"pull.682.git.git.1577382151.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] Disallow writing, but not fetching commits with file names containing backslashes","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-31T22:53:49Z","receivedAt":"2019-12-31T22:53:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"As of Git for Windows, v2.24.1(2). cloning a repository that contained a\nfile with a backslash in its name some time in the past, the command will \nsucceed but at the same time print errors like this:\n\n error: filename in tree entry contains backslash: '\\'\n\nA corresponding git fetch will also show those errors, but fail.\n\nThe reason is that v2.24.1 is much more strict about backslashes in tree\nentries than earlier versions. The intention was actually to prevent \nchecking out such files, though: if there was a mistake in a repository long \nago that has been fixed long since, there is actually no reason why we\nshould require the history to be rewritten.\n\nThis fixes https://github.com/git-for-windows/git/issues/2435.\n\nThe idea of this patch is to stop complaining about tree entries, and focus\ninstead on the index: whenever a file is added to the index, we do not want\nany backslashes in the file name on Windows.\n\nAs before, this check is only performed on Windows, and only under \ncore.protectNTFS. On other platforms, even if core.protectNTFS is turned on,\nthe backslash is not a directory separator, therefore the Operating System's\nsyscalls will (should?) refuse to create files on NTFS with backslashes in\ntheir file name.\n\nI would appreciate reviews with a particular eye on keeping users safe: I am\nnot 100% certain that all relevant file writes go through the index (I think \nthat they all go through the index, but I might have well missed a corner\ncase).\n\nChanges since v1:\n\n * Clarified the commit message (what is the goal, explain that the\n requirement is now loosened, explain why the code is still \n GIT_WINDOWS_NATIVE-only, etc).\n\nJohannes Schindelin (1):\n mingw: only test index entries for backslashes, not tree entries\n\n read-cache.c | 5 +++++\n t/t7415-submodule-names.sh | 7 ++++---\n tree-walk.c | 6 ------\n 3 files changed, 9 insertions(+), 9 deletions(-)\n\n\nbase-commit: 12029dc57db23baef008e77db1909367599210ee\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-682%2Fdscho%2Fmingw-only-error-on-backslash-in-index-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-682/dscho/mingw-only-error-on-backslash-in-index-v2\nPull-Request: https://github.com/git/git/pull/682\n\nRange-diff vs v1:\n\n 1: 4a120fd0b3 ! 1: d6da8315d3 mingw: only test index entries for backslashes, not tree entries\n @@ -8,14 +8,31 @@\n \n error: filename in tree entry contains backslash: '\\'\n \n - While the clone still succeeds, a similar error prevents the equivalent\n - `git fetch` operation, which is inconsistent.\n + The idea is to prevent Git from even trying to write files with\n + backslashes in their file names: while these characters are valid in\n + file names on other platforms, on Windows it is interpreted as directory\n + separator (which would obviously lead to ambiguities, e.g. when there is\n + a file `a\\b` and there is also a file `a/b`).\n \n - Arguably, this is the wrong layer for that error, anyway: As long as the\n - user never checks out the files whose names contain backslashes, there\n - should not be any problem in the first place.\n + Arguably, this is the wrong layer for that error: As long as the user\n + never checks out the files whose names contain backslashes, there should\n + not be any problem in the first place.\n \n - So let's instead prevent such files to be added to the index.\n + So let's loosen the requirements: we now leave tree entries with\n + backslashes in their file names alone, but we do require any entries\n + that are added to the Git index to contain no backslashes on Windows.\n +\n + Note: just as before, the check is guarded by `core.protectNTFS` (to\n + allow overriding the check by toggling that config setting), and it\n + is _only_ performed on Windows, as the backslash is not a directory\n + separator elsewhere, even when writing to NTFS-formatted volumes.\n +\n + An alternative approach would be to try to prevent creating files with\n + backslashes in their file names. However, that comes with its own set of\n + problems. For example, `git config -f C:\\ProgramData\\Git\\config ...` is\n + a very valid way to specify a custom config location, and we obviously\n + do _not_ want to prevent that. Therefore, the approach chosen in this\n + patch would appear to be better.\n \n This addresses https://github.com/git-for-windows/git/issues/2435\n \n\n-- \ngitgitgadget\n"},{"id":"389140","messageId":"d6da8315d37c6264dfd777f487b19ba200dd31fd.1577832830.git.gitgitgadget@gmail.com","threadId":"52522","inReplyTo":"pull.682.v2.git.git.1577832830.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-31T22:53:50Z","receivedAt":"2019-12-31T22:53:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nDuring a clone of a repository that contained a file with a backslash in\nits name in the past, as of v2.24.1(2), Git for Windows prints errors\nlike this:\n\n\terror: filename in tree entry contains backslash: '\\'\n\nThe idea is to prevent Git from even trying to write files with\nbackslashes in their file names: while these characters are valid in\nfile names on other platforms, on Windows it is interpreted as directory\nseparator (which would obviously lead to ambiguities, e.g. when there is\na file `a\\b` and there is also a file `a/b`).\n\nArguably, this is the wrong layer for that error: As long as the user\nnever checks out the files whose names contain backslashes, there should\nnot be any problem in the first place.\n\nSo let's loosen the requirements: we now leave tree entries with\nbackslashes in their file names alone, but we do require any entries\nthat are added to the Git index to contain no backslashes on Windows.\n\nNote: just as before, the check is guarded by `core.protectNTFS` (to\nallow overriding the check by toggling that config setting), and it\nis _only_ performed on Windows, as the backslash is not a directory\nseparator elsewhere, even when writing to NTFS-formatted volumes.\n\nAn alternative approach would be to try to prevent creating files with\nbackslashes in their file names. However, that comes with its own set of\nproblems. For example, `git config -f C:\\ProgramData\\Git\\config ...` is\na very valid way to specify a custom config location, and we obviously\ndo _not_ want to prevent that. Therefore, the approach chosen in this\npatch would appear to be better.\n\nThis addresses https://github.com/git-for-windows/git/issues/2435\n\nSigned-off-by: Johannes Schindelin \n---\n read-cache.c | 5 +++++\n t/t7415-submodule-names.sh | 7 ++++---\n tree-walk.c | 6 ------\n 3 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex ad0b48c84d..737916ebd9 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1278,6 +1278,11 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \tint skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;\n \tint new_only = option & ADD_CACHE_NEW_ONLY;\n \n+#ifdef GIT_WINDOWS_NATIVE\n+\tif (protect_ntfs && strchr(ce->name, '\\\\'))\n+\t\treturn error(_(\"filename in tree entry contains backslash: '%s'\"), ce->name);\n+#endif\n+\n \tif (!(option & ADD_CACHE_KEEP_CACHE_TREE))\n \t\tcache_tree_invalidate_path(istate, ce->name);\n \ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex 905a557585..7ae0dc8ff4 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -207,6 +207,9 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '\n \t\t\tgit hash-object -w --stdin)\" &&\n \t\trev=\"$(git rev-parse --verify HEAD)\" &&\n \t\thash=\"$(echo x | git hash-object -w --stdin)\" &&\n+\t\ttest_must_fail git update-index --add \\\n+\t\t\t--cacheinfo 160000,$rev,d\\\\a 2>err &&\n+\t\ttest_i18ngrep backslash err &&\n \t\tgit -c core.protectNTFS=false update-index --add \\\n \t\t\t--cacheinfo 100644,$modules,.gitmodules \\\n \t\t\t--cacheinfo 160000,$rev,c \\\n@@ -214,9 +217,7 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '\n \t\t\t--cacheinfo 100644,$hash,d./a/x \\\n \t\t\t--cacheinfo 100644,$hash,d./a/..git &&\n \t\ttest_tick &&\n-\t\tgit -c core.protectNTFS=false commit -m \"module\" &&\n-\t\ttest_must_fail git show HEAD: 2>err &&\n-\t\ttest_i18ngrep backslash err\n+\t\tgit -c core.protectNTFS=false commit -m \"module\"\n \t) &&\n \ttest_must_fail git -c core.protectNTFS=false \\\n \t\tclone --recurse-submodules squatting squatting-clone 2>err &&\ndiff --git a/tree-walk.c b/tree-walk.c\nindex b3d162051f..d5a8e096a6 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -43,12 +43,6 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l\n \t\tstrbuf_addstr(err, _(\"empty filename in tree entry\"));\n \t\treturn -1;\n \t}\n-#ifdef GIT_WINDOWS_NATIVE\n-\tif (protect_ntfs && strchr(path, '\\\\')) {\n-\t\tstrbuf_addf(err, _(\"filename in tree entry contains backslash: '%s'\"), path);\n-\t\treturn -1;\n-\t}\n-#endif\n \tlen = strlen(path) + 1;\n \n \t/* Initialize the descriptor entry */\n-- \ngitgitgadget\n"},{"id":"389170","messageId":"nycvar.QRO.7.76.6.2001022051410.46@tvgsbejvaqbjf.bet","threadId":"52522","inReplyTo":"xmqqk16dfpcy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-02T19:53:16Z","receivedAt":"2020-01-02T19:53:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 30 Dec 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > Turns out that this inconsistency is only in Git for Windows v2.24.1(2)\n> > but not in current `master` of Git, so I simply struck that part from the\n> > commit message.\n> > ...\n> > I rephrased it to:\n> >\n> > So let's loosen the requirements: we now leave tree entries with\n> > backslashes in their file names alone, but we do require any entries\n> > that are added to the Git index to contain no backslashes on Windows.\n> > ...\n>\n> We are in -rc so there is no real rush,\n\nMy intention was actually to integrate this patch into Git for Windows\nv2.25.0-rc1 already, that's why I sent it out for review even this late in\nthe -rc phase.\n\n> but I take these to mean that I should just leave this loose end hanging\n> untied, and wait for an updated version to replace it sometime early\n> next year.\n\nI sent out a new iteration of the patch *just* before the new year arrived\nover here ;-)\n\n> Thanks and happy new year.\n\nThe same to you!\nDscho\n"},{"id":"389173","messageId":"nycvar.QRO.7.76.6.2001022053530.46@tvgsbejvaqbjf.bet","threadId":"52522","inReplyTo":"20191226214245.GA186931@google.com","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-02T19:58:13Z","receivedAt":"2020-01-02T19:58:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Thu, 26 Dec 2019, Jonathan Nieder wrote:\n\n> Johannes Schindelin wrote:\n> > On Thu, 26 Dec 2019, Jonathan Nieder wrote:\n> >> Johannes Schindelin wrote:\n>\n> >>> Arguably, this is the wrong layer for that error, anyway: As long as the\n> >>> user never checks out the files whose names contain backslashes, there\n> >>> should not be any problem in the first place.\n> [...]\n> >> between the lines of this commit messages I sense that there\n> >> *are* repositories in the wild using these kinds of filenames.\n> >>\n> >> Can you say more about that? What repositories are affected? Do they\n> >> contain such filenames at HEAD or only in their history? If someone\n> >> wants to check out a revision with such filenames, what should happen?\n> >\n> > Yes: https://github.com/zephyrproject-rtos/civetweb contains history where\n> > at some stage, by mistake, a directory was called `\\`. It has been fixed a\n> > long time ago, but users obviously still want to be able to clone it ;-)\n>\n> Thanks.\n>\n> What should and does happen if someone tries to check out an offending\n> revision?\n\nTo answer this question, I added this paragraph to the commit message:\n\n The idea is to prevent Git from even trying to write files with\n backslashes in their file names: while these characters are valid in\n file names on other platforms, on Windows it is interpreted as directory\n separator (which would obviously lead to ambiguities, e.g. when there is\n a file `a\\b` and there is also a file `a/b`).\n\n> [...]\n> > I added this paragraph to the commit message:\n> >\n> > Note: just as before, the check is guarded by `core.protectNTFS` (to\n> > allow overriding the check by toggling that config setting), and it\n> > is _only_ performed on Windows, as the backslash is not a directory\n> > separator elsewhere, even when writing to NTFS-formatted volumes.\n> >\n> > Does that clarify the issue enough?\n>\n> It helps: that tells me why the check is only performed on Windows.\n>\n> Since this only affects Windows, please only take this review as data\n> about how someone read the patch. The patch doesn't make non Windows\n> specific code any *less* maintainable, and as a general presumption I\n> assume that the Git for Windows maintainer knows best about what is\n> needed for maintainability of Windows-specific code.\n>\n> But the commit message and docs still don't describe what the desired\n> behavior is. For example, should I be able to check out a revision\n> with a backslash in it (e.g. via Git skipping the offending entry), or\n> is the intended behavior for it to error out and prevent checking out\n> such versions of code?\n\nAs mentioned above, the idea is to prevent Git from attempting to create\nfiles with illegal file name characters.\n\n> Is there anything we can or should do to prevent people checking in\n> new examples of paths with backslash in them (on all platforms)?\n\nAs mentioned in my reply to Junio, I don't think that it would be wise to\neven try to warn about backslashes in the file names. There are _so_ many\nGit users out there, I am convinced that at least some of them have valid\nuse cases for file names with backslashes in them.\n\nCiao,\nDscho\n"},{"id":"389208","messageId":"20200104015749.GE130883@google.com","threadId":"52522","inReplyTo":"nycvar.QRO.7.76.6.2001022053530.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-01-04T01:57:49Z","receivedAt":"2020-01-04T01:57:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Schindelin wrote:\n\n> As mentioned above, the idea is to prevent Git from attempting to create\n> files with illegal file name characters.\n[...]\n> On Thu, 26 Dec 2019, Jonathan Nieder wrote:\n\n>> Is there anything we can or should do to prevent people checking in\n>> new examples of paths with backslash in them (on all platforms)?\n>\n> As mentioned in my reply to Junio, I don't think that it would be wise to\n> even try to warn about backslashes in the file names. There are _so_ many\n> Git users out there, I am convinced that at least some of them have valid\n> use cases for file names with backslashes in them.\n\nThanks for the quick answers. It helps.\n\nI think allowing people to clone\nhttps://github.com/zephyrproject-rtos/civetweb but not to check out\nthe problematic historic revision is a reasonable choice, especially\nsince it's still possible to get the data from there using\n\n\tgit checkout -- . ':!bad-paths'\n\n[...]\n> Or maybe you know of a code path in the `unpack_trees()` machinery that\n> does _not_ go through `add_index_entry()`? I would be very interested to\n> learn about such code paths.\n\nEvery once in a while someone (e.g., in #git) has wanted \"git checkout\n--skip-index -- \", and that would be the natural way to\nimplement such a thing. But no one has done it yet. :)\n\nWe'll just have to keep a watchful eye as people make new\ncontributions.\n\nSincerely,\nJonathan\n"},{"id":"389226","messageId":"nycvar.QRO.7.76.6.2001042225080.46@tvgsbejvaqbjf.bet","threadId":"52522","inReplyTo":"20200104015749.GE130883@google.com","subject":"Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-04T21:29:31Z","receivedAt":"2020-01-04T21:29:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Fri, 3 Jan 2020, Jonathan Nieder wrote:\n\n> Johannes Schindelin wrote:\n>\n> > As mentioned above, the idea is to prevent Git from attempting to create\n> > files with illegal file name characters.\n> [...]\n> > On Thu, 26 Dec 2019, Jonathan Nieder wrote:\n>\n> >> Is there anything we can or should do to prevent people checking in\n> >> new examples of paths with backslash in them (on all platforms)?\n> >\n> > As mentioned in my reply to Junio, I don't think that it would be wise to\n> > even try to warn about backslashes in the file names. There are _so_ many\n> > Git users out there, I am convinced that at least some of them have valid\n> > use cases for file names with backslashes in them.\n>\n> Thanks for the quick answers. It helps.\n>\n> I think allowing people to clone\n> https://github.com/zephyrproject-rtos/civetweb but not to check out\n> the problematic historic revision is a reasonable choice, especially\n> since it's still possible to get the data from there using\n>\n> \tgit checkout -- . ':!bad-paths'\n>\n> [...]\n> > Or maybe you know of a code path in the `unpack_trees()` machinery that\n> > does _not_ go through `add_index_entry()`? I would be very interested to\n> > learn about such code paths.\n>\n> Every once in a while someone (e.g., in #git) has wanted \"git checkout\n> --skip-index -- \", and that would be the natural way to\n> implement such a thing. But no one has done it yet. :)\n>\n> We'll just have to keep a watchful eye as people make new\n> contributions.\n\nRight.\n\nAs to my patch, in the meantime I wondered whether I should simply extend\n`verify_path()` to take a `flags` parameter where I can specify whether\nthis is intended for checkout, and handle the backslashes _there_. That\nwould cover e.g. uses of `make_transient_cache_entry()`, too.\n\nThe only reason why I would want to introduce `flags` is that `git apply`\nhas this `--unsafe-paths` option where `verify_paths()` is called on the\nfile names in the diff, and I was unsure whether there are legitimate use\ncases with diffs containing DOS-style paths (I suspect that there are\nlegitimate scenarios where this is desirable).\n\nWhat do you think of that?\n\nCiao,\nDscho\n"}]}