Re: [PATCH] sha1_file: use access(), not lstat(), if possible

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 22, 2017, 16:15 UTC
Message-ID: <xmqq1sp8fnxv.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.21.1.1707221309530.4271@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 8 quoted lines

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