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

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