> On 03 Aug 2016, at 23:43, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > On Wed, Aug 3, 2016 at 2:37 PM, Lars Schneider <larsxschneider@xxxxxxxxx> wrote: >>> >>> I think this was already pointed out in the previous review by Peff, >>> but a variable "ret" that says "0 is bad" somehow makes it hard to >>> follow the code. Perhaps rename it to "int error", flip the meaning, >>> and if the caller wants this function to return non-zero on success >>> flip the polarity in the return statement itself, i.e. "return !errors", >>> may make it easier to follow? >> >> This follows the existing filter function. Please see Peff's later >> reply here: > > Which I did before mentioning "pointed out in his review". > >> That's why I kept it the way it is. If you prefer the "!errors" approach >> then I will change that. > > I am not suggesting to change the RETURN VALUE from this function. > That is why I mentioned "return !errors" to flip the polarity at the end. > Inside the function, "ret" variable _forces_ the readers to think "this > function unlike the others signal an error with 0" constantly while > reading it, and one possible approach to reduce the mental burden > is to replace "ret" variable with "errors" variable, which is clear to > anybody that it would be non-zero when we saw error(s). > > Oh, I am not suggesting to _count_ the number of errors by > mentioning a possible variable name "errors"; the only reason > why I mentioned that name is because "error" is already > taken, and "seen_error" is a bit too long. Agreed. I got that you didn't suggest to change the return value :-) In order to be consistent I would also adjust the error handling in the existing apply_filter() function that I renamed to apply_single_file_filter() in 11/12. OK? Thanks, Lars -- 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