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

René Scharfe [off-list ref] writes:

quoted

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.)

quoted

+			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.