Johannes Schindelin [off-list ref] writes:
But this whole thread taps into a gripe I have with parts of Git's code
base: part of the code is not clear at all in its intent by virtue of
calling whatever POSIX function may seem to give the answer for the
intended question, instead of implementing a function whose name says
precisely what question is asked.
In this instance, we do not call a helper get_file_size(). Oh no. That
would make it too obvious. We call lstat() instead.
I agree with you for this case and a case like this in general.
In codepaths at a lot lower level (they tend to be the ancient and
quite fundamental ones) in our codebase, lstat() is often directly
used by the caller because they are interested not only in a single
aspect of a path but many fields in struct stat are of interest.
When the code is interested in existence or size or whatever single
aspect of a path and nothing else, however, the code would become
easier to read if a helper function with a more specific name is
used. And it may even help individual platforms that do not want to
use the full lstat() emulation, by telling them that other fields in
struct stat are not needed.
Of course, then the issue becomes what to do when we are interested
in not just one but a selected few attributes. Perhaps we create a
helper "get_A_B_and_C_attributes_for_path()", which may use lstat()
on POSIX and the most efficient way to get only A, B and C attributes
on non-POSIX platforms. The implementation would be OK, but the naming
becomes a bit hard; we need to give it a good name.
Things gets even more interesting when the set of attributes we are
interested in grows by one and we need to rename the function to
"get_A_B_C_and_D_attributes_for_path()". When it is a lot easier to
fall back to the full lstat() emulation on non-POSIX platforms, the
temptation to just use it even though it would grab attributes that
are not needed in that function grows, which needs to be resisted by
those who are doing the actual implementation for a particular platform.
Hi,
On Sat, 22 Jul 2017, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
But this whole thread taps into a gripe I have with parts of Git's code
base: part of the code is not clear at all in its intent by virtue of
calling whatever POSIX function may seem to give the answer for the
intended question, instead of implementing a function whose name says
precisely what question is asked.
In this instance, we do not call a helper get_file_size(). Oh no. That
would make it too obvious. We call lstat() instead.
I agree with you for this case and a case like this in general.
In codepaths at a lot lower level (they tend to be the ancient and
quite fundamental ones) in our codebase, lstat() is often directly
used by the caller because they are interested not only in a single
aspect of a path but many fields in struct stat are of interest.
When the code is interested in existence or size or whatever single
aspect of a path and nothing else, however, the code would become
easier to read if a helper function with a more specific name is
used. And it may even help individual platforms that do not want to
use the full lstat() emulation, by telling them that other fields in
struct stat are not needed.
Of course, then the issue becomes what to do when we are interested
in not just one but a selected few attributes. Perhaps we create a
helper "get_A_B_and_C_attributes_for_path()", which may use lstat()
on POSIX and the most efficient way to get only A, B and C attributes
on non-POSIX platforms. The implementation would be OK, but the naming
becomes a bit hard; we need to give it a good name.
Things gets even more interesting when the set of attributes we are
interested in grows by one and we need to rename the function to
"get_A_B_C_and_D_attributes_for_path()". When it is a lot easier to
fall back to the full lstat() emulation on non-POSIX platforms, the
temptation to just use it even though it would grab attributes that
are not needed in that function grows, which needs to be resisted by
those who are doing the actual implementation for a particular platform.
It becomes a lot easier to fall back to lstat(), if a lot less readable,
yes.
Until, that is, one realises that the function name does not have to
encode what information is sought. It can be a bit field in a parameter
instead. There are even precendents in Git's own source code for that
rather smart paradigm.
Ciao,
Dscho