From: Karthik Nayak Date: Fri, 07 Nov 2025 15:58:21 GMT Subject: Re: [PATCH v3 4/5] maintenance: add checking logic in `pack_refs_condition()` Message-ID: In-Reply-To: Junio C Hamano writes: > Patrick Steinhardt writes: > >>> + /* Check for all refs, similar to 'git refs optimize --all'. */ >>> + string_list_append(optimize_opts.includes, "*"); >>> + >>> + if (refs_optimize_required(get_main_ref_store(the_repository), >>> + &optimize_opts, &required)) >>> + return 0; >>> + >>> + clear_ref_exclusions(&excludes); >>> + string_list_clear(&included_refs, 0); >>> + >>> + return required == true; >> >> Tiny nit: I think in our codebase this can be written in a more >> idiomatic way by saying `!!required`. > > Comparing for equality with Boolean in general is stupid, as > Booleans are designed to be usable as-is. If it is "true", it is > true, and you do not have to compare it with "true" to ascertain > that it is true. > > I do 100% prefer "!!required" over "required == true" or "required > != false" all the time, since it is more idiomatic, but I vaguely > recall we had something that contradicts it in the CodingGuidelines > document. Perhaps we'd want to fix that. > I could only find - Some clever tricks, like using the !! operator with arithmetic constructs, can be extremely confusing to others. Avoid them, unless there is a compelling reason to use them. I think its okay? This is more of a suggestion than a rule. > Thanks. > > > [Footnote] > > But doesn't your suggested rewrite potentially change the meaning? > > The original allows required to be "true" and nothing else, while > "!!required" allows it to be any form of true (and in C, things that > are not zero, even a pointer that is not NULL, are all true). I get what you mean, but with the context that required is of type 'bool', this would mean that we simply convert it to '0'/'1' here. With all this, perhaps `return required` as used in the v1 was the best approach. I'm happy to go either ways.