Re: [PATCH v2 0/4] refspec: treat 'fetch' as a Boolean value

[Elijah Newren <newren@gmail.com>]

On Tue, Mar 18, 2025 at 3:50 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> Here's a small reroll that cleans up some wording in the first patch,
> and drops the accidental inclusion of a stray 'git-diff-pairs' binary
> that was sitting in my working tree at the time of writing the initial
> commit.
>
> Otherwise the series is unchanged, but a range-diff is attached
> nonetheless. Thanks for reviewing :-).
>
> Taylor Blau (4):
>   refspec: treat 'fetch' as a Boolean value
>   refspec: replace `refspec_init()` with fetch/push variants
>   refspec: remove refspec_item_init_or_die()
>   refspec: replace `refspec_item_init()` with fetch/push variants
>
>  builtin/fetch.c    |  2 +-
>  builtin/pull.c     |  3 ++-
>  refspec.c          | 38 +++++++++++++++++++++++++++-----------
>  refspec.h          | 18 +++++++-----------
>  remote.c           |  4 ++--
>  transport-helper.c |  2 +-
>  6 files changed, 40 insertions(+), 27 deletions(-)
>
> Range-diff against v1:
> 1:  7e662acb5a ! 1:  04e1ab8209 refspec: treat 'fetch' as a Boolean value
>     @@ Commit message
>          refspec: treat 'fetch' as a Boolean value
>
>          Since 6d4c057859 (refspec: introduce struct refspec, 2018-05-16), we
>     -    have constants called REFSPEC_FETCH and REFSPEC_PUSH. This misleadingly
>     -    suggests that we might introduce other modes in the future.
>     +    have macros called REFSPEC_FETCH and REFSPEC_PUSH. This confusingly
>     +    suggests that we might introduce other modes in the future, which, while
>     +    possible, is highly unlikely.
>
>          But these values are treated as a Boolean, and stored in a struct field
>          called 'fetch'. So the following:
>     @@ Commit message
>              if (refspec->fetch) { ... }
>
>          are equivalent. Let's avoid renaming the Boolean values "true" and
>     -    "false" here and remove the two REFSPEC_ constants mentioned above.
>     +    "false" here and remove the two REFSPEC_ macros mentioned above.
>
>          Since this value is truly a Boolean and will only ever take on a value
>          of 0 or 1, we can declare it as a single bit unsigned field. In
>     @@ builtin/pull.c: static const char *get_tracking_branch(const char *remote, const
>         if (!*spec_src || !strcmp(spec_src, "HEAD"))
>                 spec_src = "HEAD";
>
>     - ## git-diff-pairs (new) ##
>     - Binary files /dev/null and git-diff-pairs differ
>     -
>       ## refspec.c ##
>      @@ refspec.c: void refspec_clear(struct refspec *rs)
>       int valid_fetch_refspec(const char *fetch_refspec_str)
> 2:  fd2354dade = 2:  c3021b82ce refspec: replace `refspec_init()` with fetch/push variants
> 3:  49b470de61 = 3:  88f6a91c46 refspec: remove refspec_item_init_or_die()
> 4:  95783265fd = 4:  f0c323988f refspec: replace `refspec_item_init()` with fetch/push variants
>
> base-commit: c702dd48567cfebca3d4a06b691de97da3f8dc4a
> --
> 2.49.0.3.gbb7a4a684c.dirty

I read through the four patches, and it seems like a straightforward
translation of the previous state to essentially a boolean.  I usually
would have picked an enum based on avoiding passing a 0 or 1 to
functions as you have in the intermediate state, but you didn't stop
at that intermediate state and refactored the function names which
makes the end result pretty nice.  Series looks good to me.