Junio C Hamano <gitster@xxxxxxxxx> writes: >> + if (has_extension(de->d_name, ".idx") || >> + has_extension(de->d_name, ".pack") || >> + has_extension(de->d_name, ".keep")) >> + string_list_append(&garbage, path); > > It might be OK to put .pack and .keep in the same "if (A || B)" as > it may happen to be that they do not need any special treatment > right now, but I do not think this is a good idea in general. Actually I take this part back. I can see that the condition part grow over time but I do not think the body should. That is the whole point of collecting paths that cannot be judged as garbage by themselves into a list; we shouldn't be doing anything else by definition in the body. Everything else I said in the review still stands, though.. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html