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

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2023, 20:06 UTC
Message-ID: <xmqq8rduvdj0.fsf@gitster.g>
In-Reply-To: <b2f3b98cd2461a25ab708adbcd8a95f5e2b18e5e.1683828635.git.gitgitgadget@gmail.com>

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 13 quoted lines

> @@ -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?

Show 12 quoted lines

> @@ -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.