Ævar Arnfjörð Bjarmason [off-list ref] 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.
On Thu, Apr 01 2021, Junio C Hamano wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
- 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.