Subject: Re: [PATCH v3 4/4] pack-refs: teach pack-refs --include option

From: Junio C Hamano <[email-masked]>
Date: Thu, 11 May 2023 13:06:27 -0700

"John Cai via GitGitGadget" <[email-masked]> 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.