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

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

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.

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