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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Nov 7, 2025, 15:58 UTC
Message-ID: <CAOLa=ZT6CnTRz5bX+Vv7pb_3oqV0XNSMEzh=57sF6O5bFYxWhQ@mail.gmail.com>
In-Reply-To: <xmqqpl9vjiaj.fsf@gitster.g>

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

Show 27 quoted lines

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

Show 10 quoted lines

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