Re: [RFC 3/3] refspec: add support for negative refspecs
- From
Jacob Keller <jacob.keller@gmail.com>
- Date
- Aug 18, 2020, 00:04 UTC
- Message-ID
- <CA+P7+xpcm51cLPDDW+F1J-XZ2VvwNDWjnZqm54f3DKXxDfBF5Q@mail.gmail.com>
- 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:
Show 25 quoted lines
> > 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.
Show 14 quoted lines
> > @@ -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.
Show 16 quoted lines
> > + 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