From: Junio C Hamano <gitster@pobox.com>
To: "John Cai via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
Christian Couder <christian.couder@gmail.com>,
John Cai <johncai86@gmail.com>
Subject: Re: [PATCH v3 3/4] revision: modify ref_exclusions to handle inclusions
Date: Thu, 11 May 2023 12:54:23 -0700 [thread overview]
Message-ID: <xmqqfs82ve34.fsf@gitster.g> (raw)
In-Reply-To: <0a0693ad612755e675861dfa5a660baf8d325ed0.1683828635.git.gitgitgadget@gmail.com> (John Cai via GitGitGadget's message of "Thu, 11 May 2023 18:10:33 +0000")
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +static int ref_matched(const char *path,
> + const struct string_list *l,
> + const struct string_list *hidden_refs)
> {
> const char *stripped_path = strip_namespace(path);
> struct string_list_item *item;
>
> - for_each_string_list_item(item, &exclusions->excluded_refs) {
> + for_each_string_list_item(item, l) {
> if (!wildmatch(item->string, path, 0))
> return 1;
> }
>
> - if (ref_is_hidden(stripped_path, path, &exclusions->hidden_refs))
> + if (ref_is_hidden(stripped_path, path, hidden_refs))
> return 1;
>
> return 0;
> }
>
> -void init_ref_exclusions(struct ref_exclusions *exclusions)
> +int ref_excluded(const struct ref_visibility *visibility, const char *path)
> {
> - struct ref_exclusions blank = REF_EXCLUSIONS_INIT;
> - memcpy(exclusions, &blank, sizeof(*exclusions));
> + return ref_matched(path, &visibility->excluded_refs, &visibility->hidden_refs);
> }
>
> -void clear_ref_exclusions(struct ref_exclusions *exclusions)
> +int ref_included(const struct ref_visibility *visibility, const char *path)
> {
> - string_list_clear(&exclusions->excluded_refs, 0);
> - string_list_clear(&exclusions->hidden_refs, 0);
> - exclusions->hidden_refs_configured = 0;
> + return ref_matched(path, &visibility->included_refs, &visibility->hidden_refs);
> }
It is unexpected that we do "hidden" inside ref_matched(). I would
have expected that no matter what exclusion or inclusion patterns
say, hidden things are to be kept hidden. I.e. I expected
- ref_matched(), which takes a path and a list of patterns, checks
if the path matches any of the given patterns;
- ref_excluded(), whcih takes a path and a visibility, asks
ref_matched() if the path matches visibility->excluded_refs and
relays its answer to the caller.
- ref_included(), which takes a path and a visibility, asks
ref_matched() if the path matches visibility->included_refs and
relays its answer to the caller.
- ref_visibility(), which takes a path and a visibility, goes
through the following sequence:
- if ref_is_hidden() says that the path is hidden, it is not
visible;
- if ref_excluded() says the path is excluded, it is not visible;
- if ref_included() says the path is included, it is visible;
- if none of the above triggers, the fate of the path is
determined by some default logic.
or something along that line. That would also allow us to avoid
having to call ref_is_hidden() more than once when we need to check
both inclusion and exclusion list.
But perhaps I am missing some requirements to be taken into
account, so let me read on.
To be honest, I didn't expect the "exclusions can be extended",
which I alluded to as a future, possible, follow-on work, to be
included as a part of this topic. I appreciate your going an extra
mile, but I am not sure if it was a good change in the context of
this series. With this patch, it is not trivial to even validate
that there is no behaviour change to any users of "struct
ref_exclusions" when the included_refs list is empty, which is an
absolute must to avoid regressions.
next prev parent reply other threads:[~2023-05-11 19:54 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
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 [this message]
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=xmqqfs82ve34.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=johncai86@gmail.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).