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

[Jonathan Nieder <jrnieder@gmail.com>]

(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:

> [Subject: sha1_name: introduce getn_sha1() to take length]

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?

> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1019,12 +1019,11 @@ static char *resolve_relative_path(const char *rel)
>  			   rel);
>  }
>  
> -int get_sha1_with_context_1(const char *name, unsigned char *sha1,
> -			    struct object_context *oc,
> -			    int only_to_die, const char *prefix)
> +static int getn_sha1_with_context_1(const char *name, int namelen,
> +				unsigned char *sha1, struct object_context *oc,
> +				int only_to_die, const char *prefix)

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