Hi Junio, On 11 May 2023, at 16:06, Junio C Hamano wrote: > "John Cai via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
