Subject: Re: [PATCH v5 16/18] tree-walk.h API: add a get_tree_entry_path() function

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 02 Apr 2021 11:41:02 +0200

On Thu, Apr 01 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <[email-masked]> 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.