| Message ID | b2f3b98cd2461a25ab708adbcd8a95f5e2b18e5e.1683828635.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pack-refs: allow users control over which refs to pack | expand |
"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.
Hi Junio, On 11 May 2023, at 16:06, Junio C Hamano wrote: > "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. Yes, this is the logic. I agree that getting rid of the opts->visibility check would make it more clear. thanks John > > Thanks.
On 11 May 2023, at 14:10, John Cai via GitGitGadget wrote: > From: John Cai <johncai86@gmail.com> > > Allow users to be more selective over which refs to pack by adding an > --include option to git-pack-refs. > > The existing options allow some measure of selectivity. By default > git-pack-refs packs all tags. --all can be used to include all refs, > and the previous commit added the ability to exclude certain refs with > --exclude. > > While these knobs give the user some selection over which refs to pack, > it could be useful to give more control. For instance, a repository may > have a set of branches that are rarely updated and would benefit from > being packed. --include would allow the user to easily include a set of > branches to be packed while leaving everything else unpacked. > > Signed-off-by: John Cai <johncai86@gmail.com> > --- > Documentation/git-pack-refs.txt | 14 +++++++++++++- > builtin/pack-refs.c | 18 ++++++++++++++++-- > refs/files-backend.c | 15 +++++++-------- > t/helper/test-ref-store.c | 8 +++++++- > t/t3210-pack-refs.sh | 21 +++++++++++++++++++++ > 5 files changed, 64 insertions(+), 12 deletions(-) > > diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt > index c0f7426e519..85874a5f5dc 100644 > --- a/Documentation/git-pack-refs.txt > +++ b/Documentation/git-pack-refs.txt > @@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access > SYNOPSIS > -------- > [verse] > -'git pack-refs' [--all] [--no-prune] [--exclude <pattern>] > +'git pack-refs' [--all] [--no-prune] [--include <pattern>] [--exclude <pattern>] > > DESCRIPTION > ----------- > @@ -60,6 +60,15 @@ interests. > The command usually removes loose refs under `$GIT_DIR/refs` > hierarchy after packing them. This option tells it not to. > > +--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` will 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. > + > --exclude <pattern>:: > > Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option > @@ -70,6 +79,9 @@ unpack it. > When used with `--all`, it will use the difference between the set of all refs, > and what is provided to `--exclude`. > > +When used with `--include`, refs provided to `--include`, minus refs that are > +provided to `--exclude` will be packed. > + > > BUGS > ---- > diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c > index 2464575a665..5062206f22e 100644 > --- a/builtin/pack-refs.c > +++ b/builtin/pack-refs.c > @@ -5,9 +5,10 @@ > #include "refs.h" > #include "repository.h" > #include "revision.h" > +#include "trace.h" > > static char const * const pack_refs_usage[] = { > - N_("git pack-refs [--all] [--no-prune] [--exclude <pattern>]"), > + N_("git pack-refs [--all] [--no-prune] [--include <pattern>] [--exclude <pattern>]"), > NULL > }; > > @@ -15,13 +16,17 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix) > { > unsigned int flags = PACK_REFS_PRUNE; > static struct ref_visibility visibility = REF_VISIBILITY_INIT; > - struct pack_refs_opts pack_refs_opts = {.visibility = &visibility, .flags = flags}; > + struct pack_refs_opts pack_refs_opts = { .visibility = &visibility, > + .flags = flags }; > static struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP; > + static struct string_list option_included_refs = STRING_LIST_INIT_NODUP; > struct string_list_item *item; > > struct option opts[] = { > OPT_BIT(0, "all", &pack_refs_opts.flags, N_("pack everything"), PACK_REFS_ALL), > OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE), > + OPT_STRING_LIST(0, "include", &option_included_refs, N_("pattern"), > + N_("references to include")), > OPT_STRING_LIST(0, "exclude", &option_excluded_refs, N_("pattern"), > N_("references to exclude")), > OPT_END(), > @@ -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/*"); > + > return refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts); > } > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 3ef19199788..c669cf8001a 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -1183,13 +1183,6 @@ static int should_pack_ref(const char *refname, > REF_WORKTREE_SHARED) > return 0; > > - if (opts->visibility && ref_excluded(opts->visibility, refname)) > - return 0; > - > - /* Do not pack non-tags unless PACK_REFS_ALL is set: */ > - if (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, "refs/tags/")) > - return 0; > - > /* Do not pack symbolic refs: */ > if (ref_flags & REF_ISSYMREF) > return 0; > @@ -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; > } > > static int files_pack_refs(struct ref_store *ref_store, > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c > index de4197708d9..0dec1223362 100644 > --- a/t/helper/test-ref-store.c > +++ b/t/helper/test-ref-store.c > @@ -5,6 +5,7 @@ > #include "worktree.h" > #include "object-store.h" > #include "repository.h" > +#include "revision.h" > > struct flag_definition { > const char *name; > @@ -116,7 +117,12 @@ static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE), > static int cmd_pack_refs(struct ref_store *refs, const char **argv) > { > unsigned int flags = arg_flags(*argv++, "flags", pack_flags); > - struct pack_refs_opts pack_opts = { .flags = flags }; > + static struct ref_visibility visibility = REF_VISIBILITY_INIT; > + struct pack_refs_opts pack_opts = { .flags = flags, > + .visibility = &visibility }; > + > + if (pack_opts.flags & PACK_REFS_ALL) > + add_ref_inclusion(pack_opts.visibility, "*"); I was wondering about this test function. I had to add this in because now we are no longer checking the PACK_REFS_ALL flag in refs/files-backend.c and instead relying on the inclusions data structure. However, this does not support --exclude and --include options. With the current plumbing code in this file, it doesn't look like it supports options with values as it uses a bitmask. My question is, do we need to add support for every option we add to git-pack-refs here? Or are the changes in this patch sufficient. thanks John > > return refs_pack_refs(refs, &pack_opts); > } > diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh > index ddfc1b6e5f1..9ff6326b646 100755 > --- a/t/t3210-pack-refs.sh > +++ b/t/t3210-pack-refs.sh > @@ -124,6 +124,27 @@ test_expect_success 'test --no-exclude refs clears excluded refs' ' > ! test -f .git/refs/heads/dont_pack3 && > ! test -f .git/refs/heads/dont_pack4' > > +test_expect_success 'test only included refs are packed' ' > + git branch pack_this1 && > + git branch pack_this2 && > + git tag dont_pack5 && > + git pack-refs --include "refs/heads/pack_this*" && > + test -f .git/refs/tags/dont_pack5 && > + ! test -f ./git/refs/heads/pack_this1 && > + ! test -f ./git/refs/heads/pack_this2' > + > +test_expect_success 'test --no-include refs clears included refs' ' > + git branch pack1 && > + git branch pack2 && > + git pack-refs --include "refs/heads/pack*" --no-include && > + test -f .git/refs/heads/pack1 && > + test -f .git/refs/heads/pack2' > + > +test_expect_success 'test --exclude takes precedence over --include' ' > + git branch dont_pack5 && > + git pack-refs --include "refs/heads/pack*" --exclude "refs/heads/pack*" && > + test -f .git/refs/heads/dont_pack5' > + > test_expect_success \ > 'see if up-to-date packed refs are preserved' \ > 'git branch q && > -- > gitgitgadget
diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt index c0f7426e519..85874a5f5dc 100644 --- a/Documentation/git-pack-refs.txt +++ b/Documentation/git-pack-refs.txt @@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access SYNOPSIS -------- [verse] -'git pack-refs' [--all] [--no-prune] [--exclude <pattern>] +'git pack-refs' [--all] [--no-prune] [--include <pattern>] [--exclude <pattern>] DESCRIPTION ----------- @@ -60,6 +60,15 @@ interests. The command usually removes loose refs under `$GIT_DIR/refs` hierarchy after packing them. This option tells it not to. +--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` will 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. + --exclude <pattern>:: Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option @@ -70,6 +79,9 @@ unpack it. When used with `--all`, it will use the difference between the set of all refs, and what is provided to `--exclude`. +When used with `--include`, refs provided to `--include`, minus refs that are +provided to `--exclude` will be packed. + BUGS ---- diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c index 2464575a665..5062206f22e 100644 --- a/builtin/pack-refs.c +++ b/builtin/pack-refs.c @@ -5,9 +5,10 @@ #include "refs.h" #include "repository.h" #include "revision.h" +#include "trace.h" static char const * const pack_refs_usage[] = { - N_("git pack-refs [--all] [--no-prune] [--exclude <pattern>]"), + N_("git pack-refs [--all] [--no-prune] [--include <pattern>] [--exclude <pattern>]"), NULL }; @@ -15,13 +16,17 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix) { unsigned int flags = PACK_REFS_PRUNE; static struct ref_visibility visibility = REF_VISIBILITY_INIT; - struct pack_refs_opts pack_refs_opts = {.visibility = &visibility, .flags = flags}; + struct pack_refs_opts pack_refs_opts = { .visibility = &visibility, + .flags = flags }; static struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP; + static struct string_list option_included_refs = STRING_LIST_INIT_NODUP; struct string_list_item *item; struct option opts[] = { OPT_BIT(0, "all", &pack_refs_opts.flags, N_("pack everything"), PACK_REFS_ALL), OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE), + OPT_STRING_LIST(0, "include", &option_included_refs, N_("pattern"), + N_("references to include")), OPT_STRING_LIST(0, "exclude", &option_excluded_refs, N_("pattern"), N_("references to exclude")), OPT_END(), @@ -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/*"); + return refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts); } diff --git a/refs/files-backend.c b/refs/files-backend.c index 3ef19199788..c669cf8001a 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1183,13 +1183,6 @@ static int should_pack_ref(const char *refname, REF_WORKTREE_SHARED) return 0; - if (opts->visibility && ref_excluded(opts->visibility, refname)) - return 0; - - /* Do not pack non-tags unless PACK_REFS_ALL is set: */ - if (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, "refs/tags/")) - return 0; - /* Do not pack symbolic refs: */ if (ref_flags & REF_ISSYMREF) return 0; @@ -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; } static int files_pack_refs(struct ref_store *ref_store, diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index de4197708d9..0dec1223362 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -5,6 +5,7 @@ #include "worktree.h" #include "object-store.h" #include "repository.h" +#include "revision.h" struct flag_definition { const char *name; @@ -116,7 +117,12 @@ static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE), static int cmd_pack_refs(struct ref_store *refs, const char **argv) { unsigned int flags = arg_flags(*argv++, "flags", pack_flags); - struct pack_refs_opts pack_opts = { .flags = flags }; + static struct ref_visibility visibility = REF_VISIBILITY_INIT; + struct pack_refs_opts pack_opts = { .flags = flags, + .visibility = &visibility }; + + if (pack_opts.flags & PACK_REFS_ALL) + add_ref_inclusion(pack_opts.visibility, "*"); return refs_pack_refs(refs, &pack_opts); } diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh index ddfc1b6e5f1..9ff6326b646 100755 --- a/t/t3210-pack-refs.sh +++ b/t/t3210-pack-refs.sh @@ -124,6 +124,27 @@ test_expect_success 'test --no-exclude refs clears excluded refs' ' ! test -f .git/refs/heads/dont_pack3 && ! test -f .git/refs/heads/dont_pack4' +test_expect_success 'test only included refs are packed' ' + git branch pack_this1 && + git branch pack_this2 && + git tag dont_pack5 && + git pack-refs --include "refs/heads/pack_this*" && + test -f .git/refs/tags/dont_pack5 && + ! test -f ./git/refs/heads/pack_this1 && + ! test -f ./git/refs/heads/pack_this2' + +test_expect_success 'test --no-include refs clears included refs' ' + git branch pack1 && + git branch pack2 && + git pack-refs --include "refs/heads/pack*" --no-include && + test -f .git/refs/heads/pack1 && + test -f .git/refs/heads/pack2' + +test_expect_success 'test --exclude takes precedence over --include' ' + git branch dont_pack5 && + git pack-refs --include "refs/heads/pack*" --exclude "refs/heads/pack*" && + test -f .git/refs/heads/dont_pack5' + test_expect_success \ 'see if up-to-date packed refs are preserved' \ 'git branch q &&