Jonathan Nieder <jrnieder@xxxxxxxxx> 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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html