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

Jonathan Nieder [off-list ref] 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/