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 v3 3/4] revision: modify ref_exclusions to handle inclusions
Date: Fri, 12 May 2023 10:56:03 -0400 [thread overview]
Message-ID: <866E98A8-E622-43A3-BCEC-7B6E46D49D14@gmail.com> (raw)
In-Reply-To: <xmqqfs82ve34.fsf@gitster.g>
Hi Junio,
On 11 May 2023, at 15:54, Junio C Hamano wrote:
> "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
Oops. Yes this was an oversight.
>
> - 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.
That's a good point. I didn't think about adding a ref_visibility() function
that conslidates inclusion and exclusion.
>
> 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.
Okay, maybe we can include this refactor in a subsequent series then.
thanks
John
next prev parent reply other threads:[~2023-05-12 14:56 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
2023-05-12 14:56 ` John Cai [this message]
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=866E98A8-E622-43A3-BCEC-7B6E46D49D14@gmail.com \
--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).