[v2,1/1] clean: show an error message when the path is too long

Commit Message

Linus Arver via GitGitGadget July 18, 2019, 9:30 a.m. UTC

From: Johannes Schindelin <[e-mail]>

Without an error message when `lstat()` failed, `git clean` would
abort without an error message, leaving the user quite puzzled.

In particular on Windows, where the default maximum path length is quite
small (yet there are ways to circumvent that limit in many cases), it is
very important that users be given an indication why their command
failed because of too long paths when it did.

This test case makes sure that a warning is issued that would have
helped the user who reported this issue:

	https://github.com/git-for-windows/git/issues/521

Note that we temporarily set `core.longpaths = false` in the regression
test; This ensures forward-compatibility with the `core.longpaths`
feature that has not yet been upstreamed from Git for Windows.

Helped-by: René Scharfe <[e-mail]>
Helped-by: SZEDER Gábor <[e-mail]>
Helped-by: Junio C Hamano <[e-mail]>
Signed-off-by: Johannes Schindelin <[e-mail]>
---
 builtin/clean.c  |  3 ++-
 t/t7300-clean.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Junio C Hamano July 18, 2019, 4:03 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> Without an error message when `lstat()` failed, `git clean` would
> abort without an error message, leaving the user quite puzzled.

Let's drop the first three words ;-)  Sorry for not catching it
earlier and parrotting the same mistake in my variant yesterday.

> In particular on Windows, where the default maximum path length is quite
> small (yet there are ways to circumvent that limit in many cases), it is
> very important that users be given an indication why their command
> failed because of too long paths when it did.

s/it is very important that users be given/it helps to give users/

Johannes Schindelin July 19, 2019, 12:53 p.m. UTC | #2

Hi Junio,

On Thu, 18 Jul 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > From: Johannes Schindelin <[e-mail]>
> >
> > Without an error message when `lstat()` failed, `git clean` would
> > abort without an error message, leaving the user quite puzzled.
>
> Let's drop the first three words ;-)  Sorry for not catching it
> earlier and parrotting the same mistake in my variant yesterday.

You mean the first four words.

> > In particular on Windows, where the default maximum path length is quite
> > small (yet there are ways to circumvent that limit in many cases), it is
> > very important that users be given an indication why their command
> > failed because of too long paths when it did.
>
> s/it is very important that users be given/it helps to give users/

If you really feel it important to invalidate my personal style of
expression, sure.

Ciao,
Dscho

Junio C Hamano July 19, 2019, 3:10 p.m. UTC | #3

Johannes Schindelin <[e-mail]> writes:

> On Thu, 18 Jul 2019, Junio C Hamano wrote:
>
>> "Johannes Schindelin via GitGitGadget" <[e-mail]>
>> writes:
>>
>> > From: Johannes Schindelin <[e-mail]>
>> >
>> > Without an error message when `lstat()` failed, `git clean` would
>> > abort without an error message, leaving the user quite puzzled.
>>
>> Let's drop the first three words ;-)  Sorry for not catching it
>> earlier and parrotting the same mistake in my variant yesterday.
>
> You mean the first four words.

Eek, yes, I cannot count ;-)

>> > In particular on Windows, where the default maximum path length is quite
>> > small (yet there are ways to circumvent that limit in many cases), it is
>> > very important that users be given an indication why their command
>> > failed because of too long paths when it did.
>>
>> s/it is very important that users be given/it helps to give users/
>
> If you really feel it important to invalidate my personal style of
> expression, sure.

I meant it as a pure improvement---I think the importance of this
change to the affected population is so obvious that it does not
need self promoting exaggeration to convince readers by stating
plainly how it helps.

But if it is very important for you, I'll undo the change before
merging it to 'next'.

Thanks.
t

Patch

diff --git a/builtin/clean.c b/builtin/clean.c
index aaba4af3c2..d5579da716 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -34,6 +34,7 @@  static const char *msg_would_remove = N_("Would remove %s\n");
 static const char *msg_skip_git_dir = N_("Skipping repository %s\n");
 static const char *msg_would_skip_git_dir = N_("Would skip repository %s\n");
 static const char *msg_warn_remove_failed = N_("failed to remove %s");
+static const char *msg_warn_lstat_failed = N_("could not lstat %s\n");
 
 enum color_clean {
 	CLEAN_COLOR_RESET = 0,
@@ -194,7 +195,7 @@  static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		strbuf_setlen(path, len);
 		strbuf_addstr(path, e->d_name);
 		if (lstat(path->buf, &st))
-			; /* fall thru */
+			warning_errno(_(msg_warn_lstat_failed), path->buf);
 		else if (S_ISDIR(st.st_mode)) {
 			if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
 				ret = 1;
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 7b36954d63..a2c45d1902 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -669,4 +669,16 @@  test_expect_success 'git clean -d skips untracked dirs containing ignored files'
 	test_path_is_missing foo/b/bb
 '
 
+test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '
+	test_config core.longpaths false &&
+	a50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
+	mkdir -p $a50$a50/$a50$a50/$a50$a50 &&
+	: >"$a50$a50/test.txt" 2>"$a50$a50/$a50$a50/$a50$a50/test.txt" &&
+	# create a temporary outside the working tree to hide from "git clean"
+	test_must_fail git clean -xdf 2>.git/err &&
+	# grepping for a strerror string is unportable but it is OK here with
+	# MINGW prereq
+	test_i18ngrep "too long" .git/err
+'
+
 test_done