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

(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)
Hi,

Ramkumar Ramachandra wrote:

You're probably going to hate this, but oh well:

I love the new function. I really dislike its name. Do we have any other (function that takes a C-style string/function that takes a buffer with length) pairs in the spirit of strchr/memchr to draw inspiration from?

Holy cow this function is going crazy. So let's take a survey of the public functions in this family.

 get_sha1(name, sha1)::

	Looks up a sha1 expression like xyz^ and returns the
	corresponding object name in the 20-byte buffer sha1.
	Returns -1 on failure.

 get_sha1_with_mode(name, sha1, mode)::

	Like get_sha1, but when name refers to a path within
	a tree, also returns the mode in *mode.

 get_sha1_with_context(name, sha1, context)::

	Likewise, but also returns the parsed <tree, path>
	pair. 

 get_sha1_with_mode_1(name, sha1, mode, flag, prefix)::

	Like get_sha1_with_mode, but with some extra features:

	 - flag indicates whether we are in the "detailed diagnosis"
	   codepath and only calling this function for the
	   side-effect of die()-ing with a meaningful message.

	 - prefix indicates the cwd prefix for use in the "did you
	   mean?" diagnostic.

 get_sha1_with_context_1(name, sah1, context, flag, prefix)::

	Variant in the same vein for get_sha1_with_context.

Plus the corresponding variants with "name, namelen" pairs after your patch.

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

"get_sha1_with_context_1" has no external callers so it could probably be made private. So there could be a sequence of functions with increasing detail:

	get_sha1(name, sha1)
	get_sha1_with_mode(name, sha1, mode)
	get_sha1_with_context(name, sha1, context)
	die_sha1_not_in_index(name, sha1, context, prefix)

The "len" argument could be introduced at some convenient place in this list to avoid having to change too many callers, for example:

	get_sha1(name, sha1)
	get_sha1_with_mode(name, sha1, mode)
	get_sha1_with_context(name, namelen, sha1, context)
	die_sha1_not_in_index(name, namelen, sha1, context, prefix)

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

	get_sha1(name, namelen, sha1)
	get_sha1_with_mode(name, namelen, sha1, mode)
	get_sha1_with_context(name, namelen, sha1, context)
	die_sha1_not_in_index(name, namelen, sha1, context, prefix)

If I were doing it, I might just ask the maintainer to make the decision for me, by making a somewhat funny patch series:

 1. add "how to use the get_sha1 family" to Documentation/technical.
 2. make get_sha1_with_context_1 static.
 3. replace get_sha1_with_mode_1 with simpler die_sha1_not_in_index
    function.
 4. add namelen argument to die_sha1_not_... and
    get_sha1_with_context, adjust callers, and make use of
    get_sha1_with_context with this arg in sequencer code.
 5. add namelen arg to get_sha1_with_mode, adjust callers, and
    make sequencer code use this instead.
 6. add namelen arg to get_sha1, adjust callers, and make sequener
    code use this instead.

That way, the maintainer could take patches 1 - 3 to get the basic API cleanup, patch 4 to get the sequencer enhancement and make a judgement call about whether to take patches 5 and 6 depending on how much of a pain the churn would be given other patches in flight.

What do you think? Jonathan