René Scharfe <l.s.r@xxxxxx> 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.
