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 4/4] pack-refs: teach pack-refs --include option
Date: Thu, 11 May 2023 13:06:27 -0700 [thread overview]
Message-ID: <xmqq8rduvdj0.fsf@gitster.g> (raw)
In-Reply-To: <b2f3b98cd2461a25ab708adbcd8a95f5e2b18e5e.1683828635.git.gitgitgadget@gmail.com> (John Cai via GitGitGadget's message of "Thu, 11 May 2023 18:10:34 +0000")
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -1198,7 +1191,13 @@ static int should_pack_ref(const char *refname,
> if (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))
> return 0;
>
> - return 1;
> + if (opts->visibility && ref_excluded(opts->visibility, refname))
> + return 0;
> +
> + if (opts->visibility && ref_included(opts->visibility, refname))
> + return 1;
> +
> + return 0;
> }
When the user did not say --exclude or --include, can we not have
opts->visibility? IOW, can opts->visiblity be NULL?
Even if it can be NULL, shouldn't we be defaulting to "pack", as we
rejected refs to be excluded already?
> @@ -33,5 +38,14 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
> for_each_string_list_item(item, &option_excluded_refs)
> add_ref_exclusion(pack_refs_opts.visibility, item->string);
>
> + for_each_string_list_item(item, &option_included_refs)
> + add_ref_inclusion(pack_refs_opts.visibility, item->string);
> +
> + if (pack_refs_opts.flags & PACK_REFS_ALL)
> + add_ref_inclusion(pack_refs_opts.visibility, "*");
> +
> + if (!pack_refs_opts.visibility->included_refs.nr)
> + add_ref_inclusion(pack_refs_opts.visibility, "refs/tags/*");
Given the above code, I think visibility is always non-NULL, and the
inclusion list has at least one element. So the above "what should
be the default?" question is theoretical. But it would be nicer if
we do not check opts->visibility there. By writing
if (opts->visibility && ref_included(opts->visibility, refname))
return 1;
you are saying "if visibility is defined and it is included, say
YES", and logically it follows that, if we did not return true from
here, we do not know if the end-user supplied inclusion list did not
match (i.e. ref_included() said no), or we skipped the check because
the end-user did not supply the visibility rule. It is easy to
mistake that the next statement after this, i.e. "return 0;", is the
default action when the end-user did not give any rule.
But that is not what is going on. Because visibility is always
given,
The last part would become
if (ref_included(opts->visibility, refname))
return 1;
return 0;
and the "return 0" is no longer confusing. If inclusion check says
yes, the result is "to include", otherwise the result is "not to
include". Even shorter, we could say
return !ref_excluded(opts->visibility, refname) &&
ref_included(opts->visibility, refname) &&
which we can trivially read the design decision: exclude list has
priority, and include list is consulted only after exclude list does
not ban it.
Thanks.
next prev parent reply other threads:[~2023-05-11 20:06 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
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 [this message]
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=xmqq8rduvdj0.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).