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

[Junio C Hamano <gitster@pobox.com>]

John Cai <johncai86@gmail.com> writes:

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