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