[1/1] clean: show an error message when the path is too long
diff mbox series

Message ID 36677556a26cca9eafd859c88aa9b2c5a6cde309.1563285862.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Show an error if too-long paths are seen by git clean -dfx
Related show

Commit Message

Derrick Stolee via GitGitGadget July 16, 2019, 2:04 p.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

Without an error message when stat() failed, e.g. `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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/clean.c  |  3 ++-
 t/t7300-clean.sh | 11 +++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

Comments

René Scharfe July 16, 2019, 3:01 p.m. UTC | #1
Am 16.07.19 um 16:04 schrieb Johannes Schindelin via GitGitGadget:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Without an error message when stat() failed, e.g. `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
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/clean.c  |  3 ++-
>  t/t7300-clean.sh | 11 +++++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index aaba4af3c2..7be689f480 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -194,7 +194,8 @@ 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 */

I don't understand the "fall thru" comment here.  It only makes sense in
switch statements, doesn't it?  And the code after this if/else-if/else
block is only executed if we pass through here, so why was it placed way
down in the first place?  Perhaps for historical reasons.

dir.c::remove_dir_recurse() has such a comment as well, by the way.

Anyway, I'd keep that strange comment, as I don't see a connection to
your changes.  (Or explain in the commit message why we no longer "fall
thru", whatever that may mean.  Or perhaps I'm just thick.)

> +			warning("Could not stat path '%s': %s",
> +				path->buf, strerror(errno));

The other warnings in that function are issued using warning_errno()
(shorter code, consistency is enforced) and messages are marked for
translation.  That would be nice to have here as well, no?

>  		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..aa08443f6a 100755
> --- a/t/t7300-clean.sh
> +++ b/t/t7300-clean.sh
> @@ -669,4 +669,15 @@ 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' '
> +	git config core.longpaths false &&
> +	test_when_finished git config --unset core.longpaths &&
> +	a50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
> +	mkdir -p $a50$a50/$a50$a50/$a50$a50 &&
> +	touch $a50$a50/test.txt &&
> +	touch $a50$a50/$a50$a50/$a50$a50/test.txt &&
> +	test_must_fail git clean -xdf 2>.git/err &&
> +	grep "too long" .git/err

The pattern "too long" is expected to be supplied by strerror(3), right?
Depending on the locale it might return an message in a different
language, so test_i18ngrep should be used here even if the warning above
is not translated, right?

> +'
> +
>  test_done
>
SZEDER Gábor July 16, 2019, 4:13 p.m. UTC | #2
On Tue, Jul 16, 2019 at 07:04:23AM -0700, Johannes Schindelin via GitGitGadget wrote:
> +test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '
> +	git config core.longpaths false &&
> +	test_when_finished git config --unset core.longpaths &&

'test_config core.longpaths false' could replace the above two lines
with a single one.

> +	a50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
> +	mkdir -p $a50$a50/$a50$a50/$a50$a50 &&
> +	touch $a50$a50/test.txt &&
> +	touch $a50$a50/$a50$a50/$a50$a50/test.txt &&

Is there a reason for using 'touch' to create these files here,
instead of the usual '>"$file"' shell redirections?  Something
Windows/MinGW/long path specific, perhaps?

> +	test_must_fail git clean -xdf 2>.git/err &&

I was puzzled when I saw that '2>.git/err' first, because why put that
file in the .git directory?!  but of course 'git clean' would delete
that file if it were in the worktree.  OK.

> +	grep "too long" .git/err
> +'
> +
>  test_done
> -- 
> gitgitgadget
Junio C Hamano July 16, 2019, 7:56 p.m. UTC | #3
René Scharfe <l.s.r@web.de> writes:

>> diff --git a/builtin/clean.c b/builtin/clean.c
>> index aaba4af3c2..7be689f480 100644
>> --- a/builtin/clean.c
>> +++ b/builtin/clean.c
>> @@ -194,7 +194,8 @@ 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 */
>
> I don't understand the "fall thru" comment here.  It only makes sense in
> switch statements, doesn't it?  And the code after this if/else-if/else
> block is only executed if we pass through here, so why was it placed way
> down in the first place?  Perhaps for historical reasons.

f538a91e ("git-clean: Display more accurate delete messages",
2013-01-11) introduced that line when it first introduced the
function and it is not inherited from anything else.  As the if/else
cascade has a catch-all else that always continues at the end, failing
lstat is the only way for the entire loop to break out early, so as
you hinted above, having the "fail, break and return" right there would
probably be a better organization of this loop.

> Anyway, I'd keep that strange comment, as I don't see a connection to
> your changes.  (Or explain in the commit message why we no longer "fall
> thru", whatever that may mean.  Or perhaps I'm just thick.)
>
>> +			warning("Could not stat path '%s': %s",
>> +				path->buf, strerror(errno));
>
> The other warnings in that function are issued using warning_errno()
> (shorter code, consistency is enforced) and messages are marked for
> translation.  That would be nice to have here as well, no?

Absolutely.  Also, downcase "Could" and perhaps use _() around.

As to the "fall thru" comment, I tend to agree that it does not fall
through to the next "case" in the usual sense and is confusing.
Mentioning that we removed a confusing and pointless comment in the
log message would be nice, but I'd vote for removing it if I was
asked.

Thanks.
Junio C Hamano July 17, 2019, 6:50 p.m. UTC | #4
Junio C Hamano <gitster@pobox.com> writes:

>> The other warnings in that function are issued using warning_errno()
>> (shorter code, consistency is enforced) and messages are marked for
>> translation.  That would be nice to have here as well, no?
>
> Absolutely.  Also, downcase "Could" and perhaps use _() around.


This one is easy enough (not just in the technical sense, but in the
sense that it has little room wasting our time bikeshedding), so
let's tie the loose ends and move on.

I was tempted to fix the proposed log message to excise exaggeration
(I prefer not to see "very", "important", etc.---other things that
is said in the message should be enough to convince readers about
the importance), but didn't.  

What I did do was to not just rephrasing the warning message, but to
give it its own constant and to feed it to warning_errno(), to match
the other warning message.

I also saved one (or perhaps two) fork(s) from the test script ;-)
and added a portability note there.

1:  d93f701a2e ! 1:  b1e100aa6a clean: show an error message when the path is too long
    @@ Metadata
      ## Commit message ##
         clean: show an error message when the path is too long
     
    -    Without an error message when stat() failed, e.g. `git clean` would
    +    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
    @@ Commit message
                 https://github.com/git-for-windows/git/issues/521
     
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    [jc: matched the warning message style to existing ones, fixed test]
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## builtin/clean.c ##
    +@@ builtin/clean.c: 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,
     @@ builtin/clean.c: 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("Could not stat path '%s': %s",
    -+				path->buf, strerror(errno));
    ++			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;
    @@ t/t7300-clean.sh: test_expect_success 'git clean -d skips untracked dirs contain
     +	test_when_finished git config --unset core.longpaths &&
     +	a50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
     +	mkdir -p $a50$a50/$a50$a50/$a50$a50 &&
    -+	touch $a50$a50/test.txt &&
    -+	touch $a50$a50/$a50$a50/$a50$a50/test.txt &&
    ++	: >"$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 &&
    -+	grep "too long" .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



-- >8 --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH] clean: show an error message when the path is too long

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
[jc: matched the warning message style to existing ones, fixed test]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/clean.c  |  3 ++-
 t/t7300-clean.sh | 13 +++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

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..bde55b358c 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -669,4 +669,17 @@ 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' '
+	git config core.longpaths false &&
+	test_when_finished git config --unset core.longpaths &&
+	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
Johannes Schindelin July 18, 2019, 8:49 a.m. UTC | #5
Hi,

On Wed, 17 Jul 2019, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> >> The other warnings in that function are issued using
> >> warning_errno() (shorter code, consistency is enforced) and
> >> messages are marked for translation.  That would be nice to have
> >> here as well, no?
> >
> > Absolutely.  Also, downcase "Could" and perhaps use _() around.
>
>
> This one is easy enough (not just in the technical sense, but in the
> sense that it has little room wasting our time bikeshedding), so let's
> tie the loose ends and move on.
>
> I was tempted to fix the proposed log message to excise exaggeration
> (I prefer not to see "very", "important", etc.---other things that is
> said in the message should be enough to convince readers about the
> importance), but didn't.
>
> What I did do was to not just rephrasing the warning message, but to
> give it its own constant and to feed it to warning_errno(), to match
> the other warning message.
>
> I also saved one (or perhaps two) fork(s) from the test script ;-) and
> added a portability note there.

Thanks!

On top, I integrated Gabór's suggestion to use `test_config` and threw
in a paragraph in the commit message to explain why the `core.longpaths`
variable is touched at all.

v2 incoming,
Dscho

Patch
diff mbox series

diff --git a/builtin/clean.c b/builtin/clean.c
index aaba4af3c2..7be689f480 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -194,7 +194,8 @@  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("Could not stat path '%s': %s",
+				path->buf, strerror(errno));
 		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..aa08443f6a 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -669,4 +669,15 @@  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' '
+	git config core.longpaths false &&
+	test_when_finished git config --unset core.longpaths &&
+	a50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
+	mkdir -p $a50$a50/$a50$a50/$a50$a50 &&
+	touch $a50$a50/test.txt &&
+	touch $a50$a50/$a50$a50/$a50$a50/test.txt &&
+	test_must_fail git clean -xdf 2>.git/err &&
+	grep "too long" .git/err
+'
+
 test_done