Re: [PATCH 1/4] sha1_name: introduce getn_sha1() to take length

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Apr 4, 2012, 07:35 UTC
Message-ID: <vpqr4w40x1d.fsf@bauges.imag.fr>
In-Reply-To: <20120403220817.GE19858@burratino>

Jonathan Nieder <jrnieder@gmail.com> writes:

> (cc-ing Clément who is one of the last people to change this family of
>  APIs, and Matthieu who knows these codepaths well and may have been a
>  mentor for that project)

I wouldn't say I know them well, but I did touch them in the past.

> Holy cow this function is going crazy.  So let's take a survey of
> the public functions in this family.
>
>  [... nice explanations of different functions ...]

Good job. I think your explanations could actually be added as comments to cache.h to document the corresponding functions.

> My first reaction is that the meaning of the _1 suffix is not going to
> be obvious to newcomers.  Any ideas for addressing that?

It seems I'm the one who introduced get_sha1_with_context_1. I meant "internal variant of the one without _1", which is a convention used in other places of Git's code, but usually as static functions, not in .h files.

> "get_sha1_with_context_1" has no external callers so it could probably
> be made private.

I kept it public as a very small implementation detail: I tried not to introduce performance penalty, hence made get_sha1_with_mode inline (since it is really a trivial wrapper). But we probably wouldn't notice the difference in performance making the _1 version private and losing the "static inline"-ness of the public version.

Another way to say this is: get_sha1_with_context_1() does the real job (perhaps it should be called get_sha1_real()?), and others are convenience wrappers. Since convenience wrappers are convenient, nobody use the actual function directly.

> Or maybe it makes sense to bite the bullet and add the length
> argument to all callers:

I don't think so. Convenience wrappers are meant to be simple to call, and I don't think we want to force everybody to call strlen() before calling them.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/