{"thread":{"id":"51484","subject":"[PATCH 0/1] Show an error if too-long paths are seen by git clean -dfx","startedAt":"2019-07-16T14:04:26Z","lastAt":"2019-07-19T15:10:45Z","messageCount":12,"participants":["Johannes Schindelin via GitGitGadget","René Scharfe","SZEDER Gábor","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"378961","messageId":"pull.219.git.gitgitgadget@gmail.com","threadId":"51484","inReplyTo":null,"subject":"[PATCH 0/1] Show an error if too-long paths are seen by git clean -dfx","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-16T14:04:23Z","receivedAt":"2019-07-16T14:04:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is particularly important on Windows, where PATH_MAX is very small\ncompared to Unix/Linux.\n\nJohannes Schindelin (1):\n clean: show an error message when the path is too long\n\n builtin/clean.c | 3 ++-\n t/t7300-clean.sh | 11 +++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\n\nbase-commit: aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-219%2Fdscho%2Fclean-long-paths-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-219/dscho/clean-long-paths-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/219\n-- \ngitgitgadget\n"},{"id":"378962","messageId":"36677556a26cca9eafd859c88aa9b2c5a6cde309.1563285862.git.gitgitgadget@gmail.com","threadId":"51484","inReplyTo":"pull.219.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] clean: show an error message when the path is too long","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-16T14:04:23Z","receivedAt":"2019-07-16T14:04:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWithout an error message when stat() failed, e.g. `git clean` would\nabort without an error message, leaving the user quite puzzled.\n\nIn particular on Windows, where the default maximum path length is quite\nsmall (yet there are ways to circumvent that limit in many cases), it is\nvery important that users be given an indication why their command\nfailed because of too long paths when it did.\n\nThis test case makes sure that a warning is issued that would have\nhelped the user who reported this issue:\n\n\thttps://github.com/git-for-windows/git/issues/521\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/clean.c | 3 ++-\n t/t7300-clean.sh | 11 +++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex aaba4af3c2..7be689f480 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -194,7 +194,8 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \t\tstrbuf_setlen(path, len);\n \t\tstrbuf_addstr(path, e->d_name);\n \t\tif (lstat(path->buf, &st))\n-\t\t\t; /* fall thru */\n+\t\t\twarning(\"Could not stat path '%s': %s\",\n+\t\t\t\tpath->buf, strerror(errno));\n \t\telse if (S_ISDIR(st.st_mode)) {\n \t\t\tif (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))\n \t\t\t\tret = 1;\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 7b36954d63..aa08443f6a 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -669,4 +669,15 @@ test_expect_success 'git clean -d skips untracked dirs containing ignored files'\n \ttest_path_is_missing foo/b/bb\n '\n \n+test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n+\tgit config core.longpaths false &&\n+\ttest_when_finished git config --unset core.longpaths &&\n+\ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n+\tmkdir -p $a50$a50/$a50$a50/$a50$a50 &&\n+\ttouch $a50$a50/test.txt &&\n+\ttouch $a50$a50/$a50$a50/$a50$a50/test.txt &&\n+\ttest_must_fail git clean -xdf 2>.git/err &&\n+\tgrep \"too long\" .git/err\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"378976","messageId":"a7fee3c7-8fd5-11ef-8b0d-ff8053987b0c@web.de","threadId":"51484","inReplyTo":"36677556a26cca9eafd859c88aa9b2c5a6cde309.1563285862.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] clean: show an error message when the path is too long","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-07-16T15:01:27Z","receivedAt":"2019-07-16T15:01:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.07.19 um 16:04 schrieb Johannes Schindelin via GitGitGadget:\n> From: Johannes Schindelin \n>\n> Without an error message when stat() failed, e.g. `git clean` would\n> abort without an error message, leaving the user quite puzzled.\n>\n> In particular on Windows, where the default maximum path length is quite\n> small (yet there are ways to circumvent that limit in many cases), it is\n> very important that users be given an indication why their command\n> failed because of too long paths when it did.\n>\n> This test case makes sure that a warning is issued that would have\n> helped the user who reported this issue:\n>\n> \thttps://github.com/git-for-windows/git/issues/521\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/clean.c | 3 ++-\n> t/t7300-clean.sh | 11 +++++++++++\n> 2 files changed, 13 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index aaba4af3c2..7be689f480 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -194,7 +194,8 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n> \t\tstrbuf_setlen(path, len);\n> \t\tstrbuf_addstr(path, e->d_name);\n> \t\tif (lstat(path->buf, &st))\n> -\t\t\t; /* fall thru */\n\nI don't understand the \"fall thru\" comment here. It only makes sense in\nswitch statements, doesn't it? And the code after this if/else-if/else\nblock is only executed if we pass through here, so why was it placed way\ndown in the first place? Perhaps for historical reasons.\n\ndir.c::remove_dir_recurse() has such a comment as well, by the way.\n\nAnyway, I'd keep that strange comment, as I don't see a connection to\nyour changes. (Or explain in the commit message why we no longer \"fall\nthru\", whatever that may mean. Or perhaps I'm just thick.)\n\n> +\t\t\twarning(\"Could not stat path '%s': %s\",\n> +\t\t\t\tpath->buf, strerror(errno));\n\nThe other warnings in that function are issued using warning_errno()\n(shorter code, consistency is enforced) and messages are marked for\ntranslation. That would be nice to have here as well, no?\n\n> \t\telse if (S_ISDIR(st.st_mode)) {\n> \t\t\tif (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))\n> \t\t\t\tret = 1;\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> index 7b36954d63..aa08443f6a 100755\n> --- a/t/t7300-clean.sh\n> +++ b/t/t7300-clean.sh\n> @@ -669,4 +669,15 @@ test_expect_success 'git clean -d skips untracked dirs containing ignored files'\n> \ttest_path_is_missing foo/b/bb\n> '\n>\n> +test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n> +\tgit config core.longpaths false &&\n> +\ttest_when_finished git config --unset core.longpaths &&\n> +\ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n> +\tmkdir -p $a50$a50/$a50$a50/$a50$a50 &&\n> +\ttouch $a50$a50/test.txt &&\n> +\ttouch $a50$a50/$a50$a50/$a50$a50/test.txt &&\n> +\ttest_must_fail git clean -xdf 2>.git/err &&\n> +\tgrep \"too long\" .git/err\n\nThe pattern \"too long\" is expected to be supplied by strerror(3), right?\nDepending on the locale it might return an message in a different\nlanguage, so test_i18ngrep should be used here even if the warning above\nis not translated, right?\n\n> +'\n> +\n> test_done\n>\n\n"},{"id":"378977","messageId":"20190716161351.GH20404@szeder.dev","threadId":"51484","inReplyTo":"36677556a26cca9eafd859c88aa9b2c5a6cde309.1563285862.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] clean: show an error message when the path is too long","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-07-16T16:13:51Z","receivedAt":"2019-07-16T16:13:59Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jul 16, 2019 at 07:04:23AM -0700, Johannes Schindelin via GitGitGadget wrote:\n> +test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n> +\tgit config core.longpaths false &&\n> +\ttest_when_finished git config --unset core.longpaths &&\n\n'test_config core.longpaths false' could replace the above two lines\nwith a single one.\n\n> +\ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n> +\tmkdir -p $a50$a50/$a50$a50/$a50$a50 &&\n> +\ttouch $a50$a50/test.txt &&\n> +\ttouch $a50$a50/$a50$a50/$a50$a50/test.txt &&\n\nIs there a reason for using 'touch' to create these files here,\ninstead of the usual '>\"$file\"' shell redirections? Something\nWindows/MinGW/long path specific, perhaps?\n\n> +\ttest_must_fail git clean -xdf 2>.git/err &&\n\nI was puzzled when I saw that '2>.git/err' first, because why put that\nfile in the .git directory?! but of course 'git clean' would delete\nthat file if it were in the worktree. OK.\n\n> +\tgrep \"too long\" .git/err\n> +'\n> +\n> test_done\n> -- \n> gitgitgadget\n"},{"id":"378990","messageId":"xmqqftn53g8z.fsf@gitster-ct.c.googlers.com","threadId":"51484","inReplyTo":"a7fee3c7-8fd5-11ef-8b0d-ff8053987b0c@web.de","subject":"Re: [PATCH 1/1] clean: show an error message when the path is too long","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-16T19:56:44Z","receivedAt":"2019-07-16T19:56:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> diff --git a/builtin/clean.c b/builtin/clean.c\n>> index aaba4af3c2..7be689f480 100644\n>> --- a/builtin/clean.c\n>> +++ b/builtin/clean.c\n>> @@ -194,7 +194,8 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n>> \t\tstrbuf_setlen(path, len);\n>> \t\tstrbuf_addstr(path, e->d_name);\n>> \t\tif (lstat(path->buf, &st))\n>> -\t\t\t; /* fall thru */\n>\n> I don't understand the \"fall thru\" comment here. It only makes sense in\n> switch statements, doesn't it? And the code after this if/else-if/else\n> block is only executed if we pass through here, so why was it placed way\n> down in the first place? Perhaps for historical reasons.\n\nf538a91e (\"git-clean: Display more accurate delete messages\",\n2013-01-11) introduced that line when it first introduced the\nfunction and it is not inherited from anything else. As the if/else\ncascade has a catch-all else that always continues at the end, failing\nlstat is the only way for the entire loop to break out early, so as\nyou hinted above, having the \"fail, break and return\" right there would\nprobably be a better organization of this loop.\n\n> Anyway, I'd keep that strange comment, as I don't see a connection to\n> your changes. (Or explain in the commit message why we no longer \"fall\n> thru\", whatever that may mean. Or perhaps I'm just thick.)\n>\n>> +\t\t\twarning(\"Could not stat path '%s': %s\",\n>> +\t\t\t\tpath->buf, strerror(errno));\n>\n> The other warnings in that function are issued using warning_errno()\n> (shorter code, consistency is enforced) and messages are marked for\n> translation. That would be nice to have here as well, no?\n\nAbsolutely. Also, downcase \"Could\" and perhaps use _() around.\n\nAs to the \"fall thru\" comment, I tend to agree that it does not fall\nthrough to the next \"case\" in the usual sense and is confusing.\nMentioning that we removed a confusing and pointless comment in the\nlog message would be nice, but I'd vote for removing it if I was\nasked.\n\nThanks.\n\n\n\n\n\n"},{"id":"379027","messageId":"xmqqsgr4zea9.fsf@gitster-ct.c.googlers.com","threadId":"51484","inReplyTo":"xmqqftn53g8z.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] clean: show an error message when the path is too long","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-17T18:50:22Z","receivedAt":"2019-07-17T18:50:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> The other warnings in that function are issued using warning_errno()\n>> (shorter code, consistency is enforced) and messages are marked for\n>> translation. That would be nice to have here as well, no?\n>\n> Absolutely. Also, downcase \"Could\" and perhaps use _() around.\n\n\nThis one is easy enough (not just in the technical sense, but in the\nsense that it has little room wasting our time bikeshedding), so\nlet's tie the loose ends and move on.\n\nI was tempted to fix the proposed log message to excise exaggeration\n(I prefer not to see \"very\", \"important\", etc.---other things that\nis said in the message should be enough to convince readers about\nthe importance), but didn't. \n\nWhat I did do was to not just rephrasing the warning message, but to\ngive it its own constant and to feed it to warning_errno(), to match\nthe other warning message.\n\nI also saved one (or perhaps two) fork(s) from the test script ;-)\nand added a portability note there.\n\n1: d93f701a2e ! 1: b1e100aa6a clean: show an error message when the path is too long\n @@ Metadata\n ## Commit message ##\n clean: show an error message when the path is too long\n \n - Without an error message when stat() failed, e.g. `git clean` would\n + Without an error message when lstat() failed, `git clean` would\n abort without an error message, leaving the user quite puzzled.\n \n In particular on Windows, where the default maximum path length is quite\n @@ Commit message\n https://github.com/git-for-windows/git/issues/521\n \n Signed-off-by: Johannes Schindelin \n + [jc: matched the warning message style to existing ones, fixed test]\n Signed-off-by: Junio C Hamano \n \n ## builtin/clean.c ##\n +@@ builtin/clean.c: static const char *msg_would_remove = N_(\"Would remove %s\\n\");\n + static const char *msg_skip_git_dir = N_(\"Skipping repository %s\\n\");\n + static const char *msg_would_skip_git_dir = N_(\"Would skip repository %s\\n\");\n + static const char *msg_warn_remove_failed = N_(\"failed to remove %s\");\n ++static const char *msg_warn_lstat_failed = N_(\"could not lstat %s\\n\");\n + \n + enum color_clean {\n + \tCLEAN_COLOR_RESET = 0,\n @@ builtin/clean.c: static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \t\tstrbuf_setlen(path, len);\n \t\tstrbuf_addstr(path, e->d_name);\n \t\tif (lstat(path->buf, &st))\n -\t\t\t; /* fall thru */\n -+\t\t\twarning(\"Could not stat path '%s': %s\",\n -+\t\t\t\tpath->buf, strerror(errno));\n ++\t\t\twarning_errno(_(msg_warn_lstat_failed), path->buf);\n \t\telse if (S_ISDIR(st.st_mode)) {\n \t\t\tif (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))\n \t\t\t\tret = 1;\n @@ t/t7300-clean.sh: test_expect_success 'git clean -d skips untracked dirs contain\n +\ttest_when_finished git config --unset core.longpaths &&\n +\ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n +\tmkdir -p $a50$a50/$a50$a50/$a50$a50 &&\n -+\ttouch $a50$a50/test.txt &&\n -+\ttouch $a50$a50/$a50$a50/$a50$a50/test.txt &&\n ++\t: >\"$a50$a50/test.txt\" 2>\"$a50$a50/$a50$a50/$a50$a50/test.txt\" &&\n ++\t# create a temporary outside the working tree to hide from \"git clean\"\n +\ttest_must_fail git clean -xdf 2>.git/err &&\n -+\tgrep \"too long\" .git/err\n ++\t# grepping for a strerror string is unportable but it is OK here with\n ++\t# MINGW prereq\n ++\ttest_i18ngrep \"too long\" .git/err\n +'\n +\n test_done\n\n\n\n-- >8 --\nFrom: Johannes Schindelin \nSubject: [PATCH] clean: show an error message when the path is too long\n\nWithout an error message when lstat() failed, `git clean` would\nabort without an error message, leaving the user quite puzzled.\n\nIn particular on Windows, where the default maximum path length is quite\nsmall (yet there are ways to circumvent that limit in many cases), it is\nvery important that users be given an indication why their command\nfailed because of too long paths when it did.\n\nThis test case makes sure that a warning is issued that would have\nhelped the user who reported this issue:\n\n\thttps://github.com/git-for-windows/git/issues/521\n\nSigned-off-by: Johannes Schindelin \n[jc: matched the warning message style to existing ones, fixed test]\nSigned-off-by: Junio C Hamano \n---\n builtin/clean.c | 3 ++-\n t/t7300-clean.sh | 13 +++++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex aaba4af3c2..d5579da716 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -34,6 +34,7 @@ static const char *msg_would_remove = N_(\"Would remove %s\\n\");\n static const char *msg_skip_git_dir = N_(\"Skipping repository %s\\n\");\n static const char *msg_would_skip_git_dir = N_(\"Would skip repository %s\\n\");\n static const char *msg_warn_remove_failed = N_(\"failed to remove %s\");\n+static const char *msg_warn_lstat_failed = N_(\"could not lstat %s\\n\");\n \n enum color_clean {\n \tCLEAN_COLOR_RESET = 0,\n@@ -194,7 +195,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \t\tstrbuf_setlen(path, len);\n \t\tstrbuf_addstr(path, e->d_name);\n \t\tif (lstat(path->buf, &st))\n-\t\t\t; /* fall thru */\n+\t\t\twarning_errno(_(msg_warn_lstat_failed), path->buf);\n \t\telse if (S_ISDIR(st.st_mode)) {\n \t\t\tif (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))\n \t\t\t\tret = 1;\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 7b36954d63..bde55b358c 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -669,4 +669,17 @@ test_expect_success 'git clean -d skips untracked dirs containing ignored files'\n \ttest_path_is_missing foo/b/bb\n '\n \n+test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n+\tgit config core.longpaths false &&\n+\ttest_when_finished git config --unset core.longpaths &&\n+\ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n+\tmkdir -p $a50$a50/$a50$a50/$a50$a50 &&\n+\t: >\"$a50$a50/test.txt\" 2>\"$a50$a50/$a50$a50/$a50$a50/test.txt\" &&\n+\t# create a temporary outside the working tree to hide from \"git clean\"\n+\ttest_must_fail git clean -xdf 2>.git/err &&\n+\t# grepping for a strerror string is unportable but it is OK here with\n+\t# MINGW prereq\n+\ttest_i18ngrep \"too long\" .git/err\n+'\n+\n test_done\n-- \n2.22.0-653-g37fc7794bc\n\n\n\n\n\n\n"},{"id":"379033","messageId":"nycvar.QRO.7.76.6.1907181048280.47@tvgsbejvaqbjf.bet","threadId":"51484","inReplyTo":"xmqqsgr4zea9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] clean: show an error message when the path is too long","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-18T08:49:53Z","receivedAt":"2019-07-18T08:50:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 17 Jul 2019, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n> >> The other warnings in that function are issued using\n> >> warning_errno() (shorter code, consistency is enforced) and\n> >> messages are marked for translation. That would be nice to have\n> >> here as well, no?\n> >\n> > Absolutely. Also, downcase \"Could\" and perhaps use _() around.\n>\n>\n> This one is easy enough (not just in the technical sense, but in the\n> sense that it has little room wasting our time bikeshedding), so let's\n> tie the loose ends and move on.\n>\n> I was tempted to fix the proposed log message to excise exaggeration\n> (I prefer not to see \"very\", \"important\", etc.---other things that is\n> said in the message should be enough to convince readers about the\n> importance), but didn't.\n>\n> What I did do was to not just rephrasing the warning message, but to\n> give it its own constant and to feed it to warning_errno(), to match\n> the other warning message.\n>\n> I also saved one (or perhaps two) fork(s) from the test script ;-) and\n> added a portability note there.\n\nThanks!\n\nOn top, I integrated Gabór's suggestion to use `test_config` and threw\nin a paragraph in the commit message to explain why the `core.longpaths`\nvariable is touched at all.\n\nv2 incoming,\nDscho\n"},{"id":"379034","messageId":"pull.219.v2.git.gitgitgadget@gmail.com","threadId":"51484","inReplyTo":"pull.219.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] Show an error if too-long paths are seen by git clean -dfx","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-18T09:30:32Z","receivedAt":"2019-07-18T09:30:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is particularly important on Windows, where PATH_MAX is very small\ncompared to Unix/Linux.\n\nChanges since v1:\n\n * Matched the warning message style to existing ones,\n * Fixed test in multiple ways: * Avoiding touch in favor of : >.\n * Using test_config.\n * Using test_i18ngrep instead of grep to avoid localization problems.\n * Add helpful comments.\n \n \n * The commit message now talks about lstat() instead of stat().\n * The commit message also explains where that core.longpaths comes from.\n\nJohannes Schindelin (1):\n clean: show an error message when the path is too long\n\n builtin/clean.c | 3 ++-\n t/t7300-clean.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\n\nbase-commit: aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-219%2Fdscho%2Fclean-long-paths-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-219/dscho/clean-long-paths-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/219\n\nRange-diff vs v1:\n\n 1: 36677556a2 ! 1: c7b11fe410 clean: show an error message when the path is too long\n @@ -2,7 +2,7 @@\n \n clean: show an error message when the path is too long\n \n - Without an error message when stat() failed, e.g. `git clean` would\n + Without an error message when `lstat()` failed, `git clean` would\n abort without an error message, leaving the user quite puzzled.\n \n In particular on Windows, where the default maximum path length is quite\n @@ -15,18 +15,32 @@\n \n https://github.com/git-for-windows/git/issues/521\n \n + Note that we temporarily set `core.longpaths = false` in the regression\n + test; This ensures forward-compatibility with the `core.longpaths`\n + feature that has not yet been upstreamed from Git for Windows.\n +\n + Helped-by: René Scharfe \n + Helped-by: SZEDER Gábor \n + Helped-by: Junio C Hamano \n Signed-off-by: Johannes Schindelin \n \n diff --git a/builtin/clean.c b/builtin/clean.c\n --- a/builtin/clean.c\n +++ b/builtin/clean.c\n +@@\n + static const char *msg_skip_git_dir = N_(\"Skipping repository %s\\n\");\n + static const char *msg_would_skip_git_dir = N_(\"Would skip repository %s\\n\");\n + static const char *msg_warn_remove_failed = N_(\"failed to remove %s\");\n ++static const char *msg_warn_lstat_failed = N_(\"could not lstat %s\\n\");\n + \n + enum color_clean {\n + \tCLEAN_COLOR_RESET = 0,\n @@\n \t\tstrbuf_setlen(path, len);\n \t\tstrbuf_addstr(path, e->d_name);\n \t\tif (lstat(path->buf, &st))\n -\t\t\t; /* fall thru */\n -+\t\t\twarning(\"Could not stat path '%s': %s\",\n -+\t\t\t\tpath->buf, strerror(errno));\n ++\t\t\twarning_errno(_(msg_warn_lstat_failed), path->buf);\n \t\telse if (S_ISDIR(st.st_mode)) {\n \t\t\tif (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))\n \t\t\t\tret = 1;\n @@ -39,14 +53,15 @@\n '\n \n +test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n -+\tgit config core.longpaths false &&\n -+\ttest_when_finished git config --unset core.longpaths &&\n ++\ttest_config core.longpaths false &&\n +\ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n +\tmkdir -p $a50$a50/$a50$a50/$a50$a50 &&\n -+\ttouch $a50$a50/test.txt &&\n -+\ttouch $a50$a50/$a50$a50/$a50$a50/test.txt &&\n ++\t: >\"$a50$a50/test.txt\" 2>\"$a50$a50/$a50$a50/$a50$a50/test.txt\" &&\n ++\t# create a temporary outside the working tree to hide from \"git clean\"\n +\ttest_must_fail git clean -xdf 2>.git/err &&\n -+\tgrep \"too long\" .git/err\n ++\t# grepping for a strerror string is unportable but it is OK here with\n ++\t# MINGW prereq\n ++\ttest_i18ngrep \"too long\" .git/err\n +'\n +\n test_done\n\n-- \ngitgitgadget\n"},{"id":"379035","messageId":"c7b11fe410196c14e142756a036e2bdae5d4bcab.1563442231.git.gitgitgadget@gmail.com","threadId":"51484","inReplyTo":"pull.219.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] clean: show an error message when the path is too long","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-18T09:30:33Z","receivedAt":"2019-07-18T09:30:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWithout an error message when `lstat()` failed, `git clean` would\nabort without an error message, leaving the user quite puzzled.\n\nIn particular on Windows, where the default maximum path length is quite\nsmall (yet there are ways to circumvent that limit in many cases), it is\nvery important that users be given an indication why their command\nfailed because of too long paths when it did.\n\nThis test case makes sure that a warning is issued that would have\nhelped the user who reported this issue:\n\n\thttps://github.com/git-for-windows/git/issues/521\n\nNote that we temporarily set `core.longpaths = false` in the regression\ntest; This ensures forward-compatibility with the `core.longpaths`\nfeature that has not yet been upstreamed from Git for Windows.\n\nHelped-by: René Scharfe \nHelped-by: SZEDER Gábor \nHelped-by: Junio C Hamano \nSigned-off-by: Johannes Schindelin \n---\n builtin/clean.c | 3 ++-\n t/t7300-clean.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex aaba4af3c2..d5579da716 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -34,6 +34,7 @@ static const char *msg_would_remove = N_(\"Would remove %s\\n\");\n static const char *msg_skip_git_dir = N_(\"Skipping repository %s\\n\");\n static const char *msg_would_skip_git_dir = N_(\"Would skip repository %s\\n\");\n static const char *msg_warn_remove_failed = N_(\"failed to remove %s\");\n+static const char *msg_warn_lstat_failed = N_(\"could not lstat %s\\n\");\n \n enum color_clean {\n \tCLEAN_COLOR_RESET = 0,\n@@ -194,7 +195,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \t\tstrbuf_setlen(path, len);\n \t\tstrbuf_addstr(path, e->d_name);\n \t\tif (lstat(path->buf, &st))\n-\t\t\t; /* fall thru */\n+\t\t\twarning_errno(_(msg_warn_lstat_failed), path->buf);\n \t\telse if (S_ISDIR(st.st_mode)) {\n \t\t\tif (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))\n \t\t\t\tret = 1;\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 7b36954d63..a2c45d1902 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -669,4 +669,16 @@ test_expect_success 'git clean -d skips untracked dirs containing ignored files'\n \ttest_path_is_missing foo/b/bb\n '\n \n+test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n+\ttest_config core.longpaths false &&\n+\ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n+\tmkdir -p $a50$a50/$a50$a50/$a50$a50 &&\n+\t: >\"$a50$a50/test.txt\" 2>\"$a50$a50/$a50$a50/$a50$a50/test.txt\" &&\n+\t# create a temporary outside the working tree to hide from \"git clean\"\n+\ttest_must_fail git clean -xdf 2>.git/err &&\n+\t# grepping for a strerror string is unportable but it is OK here with\n+\t# MINGW prereq\n+\ttest_i18ngrep \"too long\" .git/err\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"379062","messageId":"xmqqef2nz5xe.fsf@gitster-ct.c.googlers.com","threadId":"51484","inReplyTo":"c7b11fe410196c14e142756a036e2bdae5d4bcab.1563442231.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] clean: show an error message when the path is too long","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-18T16:03:09Z","receivedAt":"2019-07-18T16:03:18Z","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> Without an error message when `lstat()` failed, `git clean` would\n> abort without an error message, leaving the user quite puzzled.\n\nLet's drop the first three words ;-) Sorry for not catching it\nearlier and parrotting the same mistake in my variant yesterday.\n\n> In particular on Windows, where the default maximum path length is quite\n> small (yet there are ways to circumvent that limit in many cases), it is\n> very important that users be given an indication why their command\n> failed because of too long paths when it did.\n\ns/it is very important that users be given/it helps to give users/\n\n"},{"id":"379096","messageId":"nycvar.QRO.7.76.6.1907191452270.47@tvgsbejvaqbjf.bet","threadId":"51484","inReplyTo":"xmqqef2nz5xe.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] clean: show an error message when the path is too long","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-19T12:53:33Z","receivedAt":"2019-07-19T12:53:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 18 Jul 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > Without an error message when `lstat()` failed, `git clean` would\n> > abort without an error message, leaving the user quite puzzled.\n>\n> Let's drop the first three words ;-) Sorry for not catching it\n> earlier and parrotting the same mistake in my variant yesterday.\n\nYou mean the first four words.\n\n> > In particular on Windows, where the default maximum path length is quite\n> > small (yet there are ways to circumvent that limit in many cases), it is\n> > very important that users be given an indication why their command\n> > failed because of too long paths when it did.\n>\n> s/it is very important that users be given/it helps to give users/\n\nIf you really feel it important to invalidate my personal style of\nexpression, sure.\n\nCiao,\nDscho\n"},{"id":"379102","messageId":"xmqqlfwuvz4i.fsf@gitster-ct.c.googlers.com","threadId":"51484","inReplyTo":"nycvar.QRO.7.76.6.1907191452270.47@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/1] clean: show an error message when the path is too long","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-19T15:10:37Z","receivedAt":"2019-07-19T15:10:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Thu, 18 Jul 2019, Junio C Hamano wrote:\n>\n>> \"Johannes Schindelin via GitGitGadget\" \n>> writes:\n>>\n>> > From: Johannes Schindelin \n>> >\n>> > Without an error message when `lstat()` failed, `git clean` would\n>> > abort without an error message, leaving the user quite puzzled.\n>>\n>> Let's drop the first three words ;-) Sorry for not catching it\n>> earlier and parrotting the same mistake in my variant yesterday.\n>\n> You mean the first four words.\n\nEek, yes, I cannot count ;-)\n\n>> > In particular on Windows, where the default maximum path length is quite\n>> > small (yet there are ways to circumvent that limit in many cases), it is\n>> > very important that users be given an indication why their command\n>> > failed because of too long paths when it did.\n>>\n>> s/it is very important that users be given/it helps to give users/\n>\n> If you really feel it important to invalidate my personal style of\n> expression, sure.\n\nI meant it as a pure improvement---I think the importance of this\nchange to the affected population is so obvious that it does not\nneed self promoting exaggeration to convince readers by stating\nplainly how it helps.\n\nBut if it is very important for you, I'll undo the change before\nmerging it to 'next'.\n\nThanks.\nt\n"}]}