Re: [PATCH v2 3/3] ref-filter: populate symref from iterator

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 6, 2024, 20:17 UTC
Message-ID: <xmqqplqls8x8.fsf@gitster.g>
In-Reply-To: <A71B6F98-820D-4692-9712-C6D63F019DCE@gmail.com>

John Cai <johncai86@gmail.com> writes:

Show 20 quoted lines

> Yes that's a good question. I took a look and it seems like in *most* cases by
> the time populate_value() is called, apply_ref_filter() has already been called that
> populates the symref member of ref_array_item.
>
> populate_value() gets called by get_ref_atom_value() which gets called by both
>
> 1. format_ref_array_item()
> 2. cmp_ref_sorting()
>
> In the case of [2], the callchain starts with filter_and_format_refs() which
> calls ref_array_sort() that eventually calls populate_value(). Before
> ref_array_sort() is called, filter_refs() is called which ends up calling
> do_filter_refs() with filter_one(), leading to apply_ref_filter().
>
> In the case of [1] however, there are a couple of code paths that call
> populate_value() without apply_ref_filter() ever being called.
>
> pretty_print_ref() directly calls format_ref_array_item() ->
> get_ref_atom_value() -> populate_value(). However, apply_ref_filter() is not
> called which means the symref will not be populated.

I am perfectly OK to keep the "fallback" code, even if you haven't found a concrete reproduction to trigger it. Removing it is not part of what this series is trying to do, anyway. The theme of the topic has always been "optimize when we can", at least to me.

Having said that, if we somehow can give the analysis you did above as hint to future developers, then they can start from there when they consider if they can remove the "fallback" code. It would help even it just said

    /*
     * NEEDSWORK: this might have become a dead code after
     * optimization to grab symref target in apply_ref_filter().
     */

and nothing else.