Re: [RFC 3/3] refspec: add support for negative refspecs

From: Jacob Keller <jacob.keller@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>,
	Git mailing list <git@vger.kernel.org>, Jeff King <peff@peff.net>
Subject: Re: [RFC 3/3] refspec: add support for negative refspecs
Date: Mon, 17 Aug 2020 17:04:00 -0700	[thread overview]
Message-ID: <CA+P7+xpcm51cLPDDW+F1J-XZ2VvwNDWjnZqm54f3DKXxDfBF5Q@mail.gmail.com> (raw)
In-Reply-To: <xmqq5z9gzvmb.fsf@gitster.c.googlers.com>

On Mon, Aug 17, 2020 at 4:43 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
> > Refspecs today are commutative, meaning that order doesn't expressly
> > matter. Rather than forcing an implied order, negative refspecs will
> > always be applied last. That is, in order to match, a ref must match at
> > least one positive refspec, and match none of the negative refspecs.
> > This is similar to how negative pathspecs work.
>
> Yes, enumerate what positive ones match and then exclude what
> negative ones match from the result is a time-tested pattern our
> users know how things work.
>
> > @@ -530,6 +530,9 @@ static struct ref *get_ref_map(struct remote *remote,
> >               tail = &rm->next;
> >       }
> >
> > +     /* apply any negative refspecs now to prune the list of refs */
> > +     ref_map = apply_negative_refspecs(ref_map, rs);
> > +
> >       ref_map = ref_remove_duplicates(ref_map);
>
> How was the ordering here decided?  Should it result the same set if
> negative ones are excluded after duplicates are removed?
>

Good question. This was what was done in peff's original patch. I need
to understand a bit more about what ref_remove_duplicates does to
really figure this out.

> > @@ -1441,6 +1445,8 @@ int match_push_refs(struct ref *src, struct ref **dst,
> >               string_list_clear(&src_ref_index, 0);
> >       }
> >
> > +     *dst = apply_negative_refspecs(*dst, rs);
> > +
>
> The block of code whose tail is shown in the pre-context has
> prepared "delete these refs because we no longer have them" to the
> other side under MATCH_REFS_PRUNE but that was done based on the
> *dst list before we applied the negative refspec.  Is the ordering
> of these two correct, or should we filter the dst list with negative
> ones and use the resulting one in pruning operation?
>

I think we need to swap the order here. I'll take a closer look.

> > +     if (item->negative) {
> > +             struct object_id unused;
> > +
> > +             /*
> > +              * Negative refspecs only have a LHS, which indicates a ref
> > +              * (or pattern of refs) to exclude from other matches. This
> > +              * can either be a simple ref, a glob pattern, or even an
> > +              * exact sha1 match.
> > +              */
>
> "a ref (or pattern of refs)" is clarified with the next sentence
> anyway, so let's not say it, e.g.
>
>         ... only have a LHS, which indicates what to exclude from
>         other matches.
>

Sure. There's also a slight bug here because in "fetch" mode,
standalone LHS-only refs cannot be globs, and I need to fix that too.

Thanks,
Jake