| Message ID | pull.1515.git.git.1687376112.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | revision: refactor ref_excludes to ref_visibility | expand |
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > The ref_excludes API is used to tell which refs should be excluded. However, > there are times when we would want to add refs to explicitly include as > well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12) > taught pack-refs how to include certain refs, but did it in a more manual > way by keeping the ref patterns in a separate string list. Instead, we can > easily extend the ref_excludes API to include refs as well, since this use > case fits into the API nicely. Hmph, how would this interact with the other topic in flight that touch the ref exclusion logic tb/refs-exclusion-and-packed-refs? Thanks.
On Wed, Jun 21, 2023 at 07:35:09PM +0000, John Cai via GitGitGadget wrote: > The ref_excludes API is used to tell which refs should be excluded. However, > there are times when we would want to add refs to explicitly include as > well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12) > taught pack-refs how to include certain refs, but did it in a more manual > way by keeping the ref patterns in a separate string list. Instead, we can > easily extend the ref_excludes API to include refs as well, since this use > case fits into the API nicely. After reading this description, I am not sure why you can't "include" a reference that would otherwise be excluded by passing the rules: - refs/heads/exclude/* - !refs/heads/exclude/but/include/me (where the '!' prefix in the last rule is what brings back the included reference). But let's read on and see if there is something that I'm missing. > Refactor the API by renaming it to ref_visibility, and add a ref_visible() > helper that takes into account ref inclusion. Hmm. Is this a replacement for ref_is_hidden(), which is already public? Thanks, Taylor
On Thu, Jun 22, 2023 at 08:42:24AM -0400, Taylor Blau wrote: > On Wed, Jun 21, 2023 at 07:35:09PM +0000, John Cai via GitGitGadget wrote: > > The ref_excludes API is used to tell which refs should be excluded. However, > > there are times when we would want to add refs to explicitly include as > > well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12) > > taught pack-refs how to include certain refs, but did it in a more manual > > way by keeping the ref patterns in a separate string list. Instead, we can > > easily extend the ref_excludes API to include refs as well, since this use > > case fits into the API nicely. > > After reading this description, I am not sure why you can't "include" a > reference that would otherwise be excluded by passing the rules: > > - refs/heads/exclude/* > - !refs/heads/exclude/but/include/me > > (where the '!' prefix in the last rule is what brings back the included > reference). > > But let's read on and see if there is something that I'm missing. Having read this series in detail, I am puzzled. I don't think that there is any limitation of the existing reference hiding rules that wouldn't permit what you're trying to do by adding the list of references you want to include at the end of the exclude list, so long as they are each prefixed with the magic "!" sentinel. I think splitting the list of excluded references into individual excluded and non-excluded references creates some awkwardness. For one: excluded references already can cause us to include a reference, so splitting that behavior across two lists seems difficult to reason about. For example, if your excluded list contains: - refs/heads/foo - refs/heads/bar - !refs/heads/foo/baz and your included lists contains: - refs/heads/bar/baz/quux I am left wondering: why doesn't the rule pertaining to refs/heads/foo/baz show up in the included list? Likewise, what happens with refs/heads/bar/baz/quux? It is a child of an excluded rule, so the question is which list takes priority. Mostly, I am wondering if I am missing something that would explain why you couldn't modify the above example's excluded list to contain something like "!refs/heads/bar/baz/quux", eliminating the need for the include list entirely. Thanks, Taylor
On Wed, Jun 21, 2023 at 01:56:13PM -0700, Junio C Hamano wrote: > "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > The ref_excludes API is used to tell which refs should be excluded. However, > > there are times when we would want to add refs to explicitly include as > > well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12) > > taught pack-refs how to include certain refs, but did it in a more manual > > way by keeping the ref patterns in a separate string list. Instead, we can > > easily extend the ref_excludes API to include refs as well, since this use > > case fits into the API nicely. > > Hmph, how would this interact with the other topic in flight that > touch the ref exclusion logic tb/refs-exclusion-and-packed-refs? Good question. Besides trivial conflicts from John's patches to rename this API, I think the sensible thing to do with my tb/refs-exclusion-and-packed-refs topic would be to also refuse to use the jump list if there are any non-trivial exclusion *or* inclusion entries. But I have some more general concerns about the approach taken by this topic, namely that I do not understand a reference "foo" cannot be included by adding a "!foo" entry to the excluded list. Thanks, Taylor
On Thu, Jun 22, 2023 at 08:49:43AM -0400, Taylor Blau wrote: > I am left wondering: why doesn't the rule pertaining to > refs/heads/foo/baz show up in the included list? Likewise, what happens > with refs/heads/bar/baz/quux? It is a child of an excluded rule, so the > question is which list takes priority. > > Mostly, I am wondering if I am missing something that would explain why > you couldn't modify the above example's excluded list to contain > something like "!refs/heads/bar/baz/quux", eliminating the need for the > include list entirely. Another potential quirk that I just now thought of: what are the rules for what can go in the include list? Fully qualified references only? Or can we have patterns (e.g. refs/foo/bar/*). Presumably you'd want to have the namespace-stripping operator ^, but not !, since negating an include rule seems to imply that it should be in the exclude list. Thanks, Taylor
On Thu, Jun 22, 2023 at 08:53:53AM -0400, Taylor Blau wrote: > On Thu, Jun 22, 2023 at 08:49:43AM -0400, Taylor Blau wrote: > > I am left wondering: why doesn't the rule pertaining to > > refs/heads/foo/baz show up in the included list? Likewise, what happens > > with refs/heads/bar/baz/quux? It is a child of an excluded rule, so the > > question is which list takes priority. > > > > Mostly, I am wondering if I am missing something that would explain why > > you couldn't modify the above example's excluded list to contain > > something like "!refs/heads/bar/baz/quux", eliminating the need for the > > include list entirely. > > Another potential quirk that I just now thought of: what are the rules > for what can go in the include list? Fully qualified references only? Or > can we have patterns (e.g. refs/foo/bar/*). Presumably you'd want to > have the namespace-stripping operator ^, but not !, since negating an > include rule seems to imply that it should be in the exclude list. Sorry for the long chain of self-replies. I think one clarifying point that I am not sure of yet is whether or not the exclude rules you're talking about are interpreted as patterns (as in transfer.hideRefs) or wildmatch patterns. If they are wildmatch patterns, would it suffice to add the references you *do* want to enumerate to the traversal ahead of time? There is also the hidden_refs member of that struct, so perhaps adding negated entries there would work. Either way, I think emphasizing the difference between ref exclusions as it pertains to traversal, and ref exclusions as it pertains to hiding would greatly help other reviewers of this series. Thanks, Taylor
Hi Taylor, On 22 Jun 2023, at 8:49, Taylor Blau wrote: > On Thu, Jun 22, 2023 at 08:42:24AM -0400, Taylor Blau wrote: >> On Wed, Jun 21, 2023 at 07:35:09PM +0000, John Cai via GitGitGadget wrote: >>> The ref_excludes API is used to tell which refs should be excluded. However, >>> there are times when we would want to add refs to explicitly include as >>> well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12) >>> taught pack-refs how to include certain refs, but did it in a more manual >>> way by keeping the ref patterns in a separate string list. Instead, we can >>> easily extend the ref_excludes API to include refs as well, since this use >>> case fits into the API nicely. >> >> After reading this description, I am not sure why you can't "include" a >> reference that would otherwise be excluded by passing the rules: >> >> - refs/heads/exclude/* >> - !refs/heads/exclude/but/include/me >> >> (where the '!' prefix in the last rule is what brings back the included >> reference). >> >> But let's read on and see if there is something that I'm missing. > > Having read this series in detail, I am puzzled. I don't think that > there is any limitation of the existing reference hiding rules that > wouldn't permit what you're trying to do by adding the list of > references you want to include at the end of the exclude list, so long > as they are each prefixed with the magic "!" sentinel. To be honest, I had no idea "!" would have this effect--so thanks for bringing it to my attention. > > I think splitting the list of excluded references into individual > excluded and non-excluded references creates some awkwardness. For one: > excluded references already can cause us to include a reference, so > splitting that behavior across two lists seems difficult to reason > about. > > For example, if your excluded list contains: > > - refs/heads/foo > - refs/heads/bar > - !refs/heads/foo/baz > > and your included lists contains: > > - refs/heads/bar/baz/quux > > I am left wondering: why doesn't the rule pertaining to > refs/heads/foo/baz show up in the included list? Likewise, what happens > with refs/heads/bar/baz/quux? It is a child of an excluded rule, so the > question is which list takes priority. Now knowing the effect of "!", I understand the concerns about having a separate list for included references. However, I do think it would be odd if there is a ref_included() function to also use ref_excluded() to include a ref. Also, even with the current API, I think it can be confusing to reason about what takes precedence if there is a mix of inclusions and exclusions. For example, if the excluded list contains: - refs/heads/foo/baz - !refs/heads/foo would the inclusion take precedence, or the exclusion? > > Mostly, I am wondering if I am missing something that would explain why > you couldn't modify the above example's excluded list to contain > something like "!refs/heads/bar/baz/quux", eliminating the need for the > include list entirely. I think my one reservation however, is with usability of the current API. It's not very intuitive to include references by adding them with ref_excluded(&exclusions, "!ref/to/be/included"). I do think it would be easier to reason about if we kept two separate lists, one for inclusion and one for exclusion. The existence of the "!" magic sentinel does make things much more confusing however. I almost want to remove support for the magic sentinel in favor of keeping two distinct lists that cannot be mixed. Wondering your thoughts on that approach? > > Thanks, > Taylor thanks! JOhn
John Cai <johncai86@gmail.com> writes: >>> After reading this description, I am not sure why you can't "include" a >>> reference that would otherwise be excluded by passing the rules: >>> >>> - refs/heads/exclude/* >>> - !refs/heads/exclude/but/include/me >>> >>> (where the '!' prefix in the last rule is what brings back the included >>> reference). >>> >>> But let's read on and see if there is something that I'm missing. >> >> Having read this series in detail, I am puzzled. I don't think that >> there is any limitation of the existing reference hiding rules that >> wouldn't permit what you're trying to do by adding the list of >> references you want to include at the end of the exclude list, so long >> as they are each prefixed with the magic "!" sentinel. > > To be honest, I had no idea "!" would have this effect--so thanks for bringing > it to my attention. FWIW, "--exclude=!" gets zero hits in t/ directory. ref_excluded() merely calls wildmatch() like so: int ref_excluded(const struct ref_exclusions *exclusions, const char *path) { const char *stripped_path = strip_namespace(path); struct string_list_item *item; for_each_string_list_item(item, &exclusions->excluded_refs) { if (!wildmatch(item->string, path, 0)) return 1; } if (ref_is_hidden(stripped_path, path, &exclusions->hidden_refs)) return 1; return 0; } so I do not know what to think about it. This is called from inside callback of things like "log --exclude=A --exclude=B ... --all" when we are trying to add all refs in response to "--all", and it appears to me that the first match would already determine the ref's fate without even looking at the later patterns (prefixed with bang '!' or not). Taylor, am I looking at a wrong code? Puzzled...