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

[Taylor Blau <me@ttaylorr.com>]

On Mon, Mar 17, 2025 at 08:24:36PM -0400, Jeff King wrote:
> On Mon, Mar 17, 2025 at 06:24:24PM -0400, Taylor Blau wrote:
>
> > 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.
>
> I don't know that I'd call it misleading. We _could_ introduce new modes
> if we had new operations. But I do agree it's unlikely (even if we had
> other operations like git-archive, cat-file, etc, they would probably
> not have refspecs).
>
> So it seems like a reasonable direction to me.

Fair, I think a more accurate statement might be to swap "misleadingly"
for "confusingly". I'll swap that and amend the paragraph to say "[...],
which is possible, but highly unlikely"

> The one thing I don't like is:
>
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index 9c4a00620a..8bbfcce729 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -738,7 +738,7 @@ static const char *get_tracking_branch(const char *remote, const char *refspec)
> >  	const char *spec_src;
> >  	const char *merge_branch;
> >
> > -	refspec_item_init_or_die(&spec, refspec, REFSPEC_FETCH);
> > +	refspec_item_init_or_die(&spec, refspec, 1);
>
> The third argument here (and elsewhere) becomes much more mysterious to
> the reader.  Maybe not a big deal, though.

Hmm. I see later on in the thread that the final patch resolves this
awkwardness. I figured that readers might have a similar thought here,
which is why I included "Note that this introduces some awkwardness like
[...]", so perhaps there is a way to clarify that. But...

> > diff --git a/git-diff-pairs b/git-diff-pairs
>
> Hmm.... :)

...I could forgive you for not noticing given this ugliness ;-).

Thanks,
Taylor