Re: [PATCH v3 4/5] maintenance: add checking logic in `pack_refs_condition()`

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Patrick Steinhardt <ps@pks.im> 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.

"Unless there is a reason to use" sounds like an outright
prohibition to me, though.

By the way, in the on-topic part of the discussion, "required" is a
bool, the helper function that takes &required takes a pointer to a
bool, and the function in question returns a bool.  So I should
update my preference above.  "return required" is the most natural
way to write, and it uses "bool" as it was designed to be used.
When the reader knows that required is a bool already, "return
!!required" is just as pointless as "return required == true".

If required and the helper that takes a pointer to it were "int",
and this function returns a bool, then my original preference would
apply; even if an "int required" has 3 in it, we probably can still
say "return required" and the function would coerce that 3 into
"true", but manually coercing it to 0/1 with !!required is more
explicit and less confusing.

Thanks.