On Thu, Apr 01 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>> - if (get_tree_entry_mode(r, hash2, del_prefix, shifted, &mode))
>> + if (get_tree_entry_path(r, hash2, del_prefix, shifted))
>> die("cannot find path %s in tree %s",
>> del_prefix, oid_to_hex(hash2));
>
> The observation that many "get_tree_entry()" users do not need the
> mode bits and the code can be simplified by taking advantage of
> that fact is good.
>
> It does not automatically follow that it is a good implementation of
> that simplification to introduce a variant that does not take the
> mode pointer. The original function could have just been taught to
> accept NULL in the field the caller is not interested in, as in many
> other functions do.
>
> Besides, get_tree_entry_mode() vs get_tree_entry_path() does not
> make any sense as pair of functions with contrasting names. If it
> were that unlike the former that returns mode, the latter returns
> path instead, it would have made sense, but that is not what is
> going on. The former returns object name and mode, the latter only
> returns object name without mode.
>
> In any case, at this point in the series, it is highly dubious that
> an extra function is an improvement. Teaching get_tree_entry() (do
> not even rename it) to take NULL in *mode pointer would make a lot
> more sense.
Thanks. Yes that makes a lot more sense.
This whole path of introducing multiple functions is an oversight of
mine. I did it because in 7146e66f086 (tree-walk: finally switch over
tree descriptors to contain a pre-parsed entry, 2014-02-06) we removed
canon_mode() from the inline function to make it trivially inline-able.
So I assumed without testing that introducing conditionals in that
function would matter to a compiler, and wanted to not introduce any
performance regressions (however small) in this series.
But looking at the generated *.s code under GCC -O3 it makes no
difference if you've got an "if (ptr) *ptr = xyz" there. The compiler
sees that and produces the same machine code.
So I'll re-roll this to do as you suggested, thanks.
