From: John Cai <johncai86@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: John Cai via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org,
Christian Couder <christian.couder@gmail.com>
Subject: Re: [PATCH v2 3/3] pack-refs: teach pack-refs --include option
Date: Wed, 10 May 2023 15:52:47 -0400 [thread overview]
Message-ID: <20230510195247.aj2vpzzxzdkvuzff@pop-os> (raw)
In-Reply-To: <xmqqo7mturi1.fsf@gitster.g>
On 23/05/09 02:25PM, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +--include <pattern>::
> > +
> > +Pack refs based on a `glob(7)` pattern. Repetitions of this option
> > +accumulate inclusion patterns. If a ref is both included in `--include` and
> > +`--exclude`, `--exclude` takes precedence. Using `--include` does not preclude
> > +all tags from being included by default. Symbolic refs and broken refs will never
> > +be packed. When used with `--all`, it will be a noop. Use `--no-include` to clear
> > +and reset the list of patterns.
>
> Hmph, that was a bit unexpected. exclude taking precedence over
> include is very much in line with how negative pathspec works and
> the end-users should be familiar with it, but when the user bothers
> to specify with --include what to include, I would have expected
> that the "pack tags by default" would be defeated.
>
> In other words, I would have expected that the program acts as if
> the machinery works this way (iow, the code does not have to exactly
> implement it this way---it just has to behave as if it did):
>
> - it maintains two pattern list, positive and negative,
> both start empty.
> - "--exclude" are accumulated to the negative list.
> - "--include" are accumulated to the positive list.
> - "--all" adds "*" to the positive list.
> - after parsing command line options, if the positive list is
> empty, then "refs/tags/*" is added to the positive list.
> - refs that match positive list but does not match negative list
> are shown.
>
> > +When used with `--include`, it will use what is provided to `--include` as well
> > +as the the default of all tags and already packed refs, minus refs that are
> > +provided to `--exclude`.
>
> IOW, I would expect that the use of "--include" alone is enough to
> defeat the default; the end user does not have to figure out that
> they have to pass "--exclude=refs/tags/*" to do so when they are
> specifying a specific hierarchy to include.
Hm yeah, I think that is a nicer user experience.
>
> > @@ -66,6 +66,7 @@ struct worktree;
> > struct pack_refs_opts {
> > unsigned int flags;
> > struct ref_exclusions *exclusions;
> > + struct string_list *included_refs;
>
> It is unfortunate that the struct is called ref_exclusions to imply
> as if it is only usable for excluding refs from listing. It has
> other members for handling hidden refs, and it would have been very
> natural to extend it to also add included_refs pattern next to
> excluded_refs string list. After all, the struct is used to tweak
> which refs are included and which refs are excluded, and
> historically everything was included unless listed on the excluded
> pattern. We are now allowing the "everything is included" part to
> be customizable with this step. If the struct were named with a
> more neutral term, like ref_visibility to hint that it is about
> setting visibility, then this patch wouldn't have added a separate
> string list to this structure---instead it would have extended the
> ref_exclusions (with a better name) struct and placed included_refs
> string list there.
Thanks for calling this out. I was thinking along very similar lines when
working on this patch, but was too lazy to make the change :)
>
> > };
> >
> > const char *refs_resolve_ref_unsafe(struct ref_store *refs,
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index 6a51267f379..3f8974a4a32 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -1181,6 +1181,17 @@ static int should_pack_ref(const char *refname,
> > REF_WORKTREE_SHARED)
> > return 0;
> >
> > + if (opts->exclusions && ref_excluded(opts->exclusions, refname))
> > + return 0;
> > +
> > + if (opts->included_refs && opts->included_refs->nr) {
> > + struct string_list_item *item;
> > +
> > + for_each_string_list_item(item, opts->included_refs)
> > + if (!wildmatch(item->string, refname, 0))
> > + return 1;
> > + }
>
> We can see why the initial placement of exclusion logic in the
> earlier step was suboptimal here.
>
> > /* Do not pack non-tags unless PACK_REFS_ALL is set: */
> > if (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, "refs/tags/"))
> > return 0;
> > @@ -1193,9 +1204,6 @@ static int should_pack_ref(const char *refname,
> > if (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))
> > return 0;
> >
> > - if (opts->exclusions && ref_excluded(opts->exclusions, refname))
> > - return 0;
> > -
> > return 1;
> > }
>
>
> Other than that, the changes look mostly expected and no surprises.
>
> Thanks.
thanks
John
next prev parent reply other threads:[~2023-05-10 19:52 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-04 15:48 [PATCH] pack-refs: teach --exclude option to exclude refs from being packed John Cai via GitGitGadget
2023-05-04 16:48 ` Junio C Hamano
2023-05-04 21:26 ` John Cai
2023-05-09 19:18 ` [PATCH v2 0/3] pack-refs: Teach " John Cai via GitGitGadget
2023-05-09 19:18 ` [PATCH v2 1/3] docs: clarify git-pack-refs --all will pack all refs John Cai via GitGitGadget
2023-05-09 19:18 ` [PATCH v2 2/3] pack-refs: teach --exclude option to exclude refs from being packed John Cai via GitGitGadget
2023-05-09 21:04 ` Junio C Hamano
2023-05-09 19:18 ` [PATCH v2 3/3] pack-refs: teach pack-refs --include option John Cai via GitGitGadget
2023-05-09 21:25 ` Junio C Hamano
2023-05-10 19:52 ` John Cai [this message]
2023-05-11 18:10 ` [PATCH v3 0/4] pack-refs: allow users control over which refs to pack John Cai via GitGitGadget
2023-05-11 18:10 ` [PATCH v3 1/4] docs: clarify git-pack-refs --all will pack all refs John Cai via GitGitGadget
2023-05-11 23:53 ` Taylor Blau
2023-05-11 18:10 ` [PATCH v3 2/4] pack-refs: teach --exclude option to exclude refs from being packed John Cai via GitGitGadget
2023-05-11 19:34 ` Junio C Hamano
2023-05-12 0:00 ` Taylor Blau
2023-05-12 12:53 ` John Cai
2023-05-12 21:11 ` John Cai
2023-05-11 18:10 ` [PATCH v3 3/4] revision: modify ref_exclusions to handle inclusions John Cai via GitGitGadget
2023-05-11 19:54 ` Junio C Hamano
2023-05-12 14:56 ` John Cai
2023-05-11 18:10 ` [PATCH v3 4/4] pack-refs: teach pack-refs --include option John Cai via GitGitGadget
2023-05-11 20:06 ` Junio C Hamano
2023-05-12 14:48 ` John Cai
2023-05-12 19:03 ` John Cai
2023-05-12 21:34 ` [PATCH v4 0/3] pack-refs: allow users control over which refs to pack John Cai via GitGitGadget
2023-05-12 21:34 ` [PATCH v4 1/3] docs: clarify git-pack-refs --all will pack all refs John Cai via GitGitGadget
2023-05-12 21:34 ` [PATCH v4 2/3] pack-refs: teach --exclude option to exclude refs from being packed John Cai via GitGitGadget
2023-05-12 21:34 ` [PATCH v4 3/3] pack-refs: teach pack-refs --include option John Cai via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230510195247.aj2vpzzxzdkvuzff@pop-os \
--to=johncai86@gmail.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).