Re: [PATCH 5/9] refspec_ref_prefixes(): clean up refspec_item logic

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2025, 23:25 UTC
Message-ID: <xmqqplif5jfw.fsf@gitster.g>
In-Reply-To: <Z9ibhJxjlc2DxKdX@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

> OK... I agree that these are at least named confusingly ;-). We could do
> something like:

"Type" is so overly generic a word that it is only marginally better than .fetch that is not a Boolean. And in a sense it is worse. At least .fetch hints us that it wants to choose between fetch and something else, most likely push, but .type does not tell us what kind of type it is about.

Do we anticipate that we would acquire a new "type" other than FETCH and PUSH? If not, .fetch = yes/no might be a better choice and if the meaning of the member is so obvious, we may even be able to lose REFSPEC_{FETCH,PUSH} symbols for its value.

On the other hand, if we were to add new kind of refspec used when doing something other than fetch and push (perhaps when bundling? I dunno), then the member specifies how the transfer goes, perhaps, so .transfer = REFSPEC_{FETCH,PUSH,BUNDLE} might be a better choice. But the refspecs are per remote thing (whether the remote is configured or the refspec is used as a one-shot basis fetching from or pushing to a remote) and while you can fetch from a bundle, you cannot push to it, and even if the system is updated to allow pushing into a bundle (which I do not think is such a bad idea), the refspec used in such a case would still be either fetch refspec or push refspec, so perhaps a new, third kind of refspec would not fit well within the design after all.

So, if we can reasonably expect that the choice will stay between fetch and push and we wouldn't be adding a new kind, I think reverting the meaning of .fetch to yes/no and getting rid of REFSPEC_{FETCH,PUSH} may be a better approach. If we stil want to keep the descriptive CPP macro, then perhaps .transfer (or .direction) that lets us choose between fetch or push? I dunno.

Show 12 quoted lines

> , which gives us the "default" case in the switch statement. But this
> really is a boolean. I wonder if we should just use 0/1 constants and
> leave the field name alone. That would turn something like:
>
>     if (rs->fetch == REFSPEC_FETCH) { ... }
>
> into:
>
>     if (rs->fetch) { ... }
>
> , which I think is cleaner. There's no reason to rename true/false to
> FETCH and PUSH if the field name itself is already 'fetch'.

Yup, that makes two of us.