Re: [PATCH v3 3/3] ref-filter: allow merged and no-merged filters

From: Junio C Hamano <gitster@pobox.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Aaron Lipman <alipman88@gmail.com>, Git List <git@vger.kernel.org>
Subject: Re: [PATCH v3 3/3] ref-filter: allow merged and no-merged filters
Date: Sun, 13 Sep 2020 14:56:36 -0700	[thread overview]
Message-ID: <xmqqo8m9mjcb.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <CAPig+cTNp84Dm=0n-Bb9o=1nZNDFRE20XDWUPJT8yjdefv15rA@mail.gmail.com> (Eric Sunshine's message of "Sun, 13 Sep 2020 17:36:39 -0400")

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +       struct commit_list *check_reachable_list = reachable ?
>> +               ref_cbdata->filter->reachable_from :
>> +               ref_cbdata->filter->unreachable_from;
>> +
>> +       if (!check_reachable_list)
>> +               return;
>
> Rather than adding a boolean 'reachable' parameter to the function
> signature, you could instead directly pass in the `struct commit_list
> *` upon which to operate, which would allow you to drop the ternary
> operator here, and...
>
>> @@ -2322,8 +2337,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
>> -       if (filter->merge_commit)
>> -               do_merge_filter(&ref_cbdata);
>> +       do_merge_filter(&ref_cbdata, 1);
>> +       do_merge_filter(&ref_cbdata, 0);
>
> ... make the callers a bit less opaque by eliminating the
> less-than-meaningful 0-or-1, and making it obvious which list is being
> consulted:
>
>     do_merge_filter(&ref_cbdata, ref_cbdata->filter->reachable_from);
>     do_merge_filter(&ref_cbdata, ref_cbdata->filter->unreachable_from);

There is this code in do_merge_filter(), though.

		if (is_merged == reachable)
 			array->items[array->nr++] = array->items[i];
 		else
 			free_array_item(item);

This is a body of the loop that runs over (surviving) tips of ref,
after painting the commits from the tips of refs (interesting) and
the [un]reachable_from commits (uninteresting).  The temporary
variable is_merged signals if the tip after revision walk is painted
uninteresting, i.e. some of the [un]reachable_from commits reach the
tip.

If 'reachable' and 'is_merged' are the same, that means either

 (1) the tip is reachable from some commit given as --merged
     <commit>; or

 (2) the tip is NOT reachable from any commit given as --no-merged
     <commit>

which means it survives this round of filtering.

By losing the 'reachable' bit, you make this switch impossible.

I do not mind making the 0/1 a symbolic constant between
do_emreg_filter() and filter_refs() for enhanced readability,
though.

Thanks.