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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 16, 2019, 19:56 UTC
Message-ID: <xmqqftn53g8z.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <a7fee3c7-8fd5-11ef-8b0d-ff8053987b0c@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 14 quoted lines

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

Show 10 quoted lines

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