Re: [PATCH v4 3/4] count-objects: report garbage files in pack directory too

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 13, 2013, 16:54 UTC
Message-ID: <7vpq049msu.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7vehgkb43v.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 8 quoted lines

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