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

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

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.

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