Re: [PATCH 2/8] Export three functions from diff.c

[Junio C Hamano <gitster@pobox.com>]

Thomas Rast <trast@student.ethz.ch> writes:

> From: Bo Yang <struggleyb.nku@gmail.com>
>
> Use fill_metainfo to fill the line level diff meta data,
> emit_line to print out a line and quote_two to quote
> paths.
>
> Signed-off-by: Bo Yang <struggleyb.nku@gmail.com>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>  diff.c |    6 +++---
>  diff.h |   17 +++++++++++++++++
>  2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 6991ed4..a0ea9e5 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -151,7 +151,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
>  	return git_color_default_config(var, value, cb);
>  }
>  
> -static char *quote_two(const char *one, const char *two)
> +char *quote_two(const char *one, const char *two)
>  {
>  	int need_one = quote_c_style(one, NULL, NULL, 1);
>  	int need_two = quote_c_style(two, NULL, NULL, 1);

This is not specific to "diff" anymore (it is a utility to cquote a path
that happens to be stored as two separate components); wouldn't quote.c
be a better home for it?

> -static void emit_line(struct diff_options *o, const char *set, const char *reset,
> +void emit_line(struct diff_options *o, const char *set, const char *reset,
>  		      const char *line, int len)
>  {
>  	emit_line_0(o, set, reset, line[0], line+1, len-1);

Within the context of "diff", it is clear that a function called
emit_line() will be used to emit a single line of patch output, and within
that context, it is understandable that it takes coloring related arguments
as we do emit diff output in color.

But does it still make sense to give it such a generic sounding name when
exported outside of its original context?  Call it either emit-diff-line
(if "diff-ness" is more important, and the new callers will use the
function to emit diff output) or emit-colored-line (if the new caller
produces output not necessarily related to diff, but is merely borrowing
the coloring infrastructure from this implementation), perhaps?

> @@ -2583,7 +2583,7 @@ static int similarity_index(struct diff_filepair *p)
>  	return p->score * 100 / MAX_SCORE;
>  }
>  
> -static void fill_metainfo(struct strbuf *msg,
> +void fill_metainfo(struct strbuf *msg,
>  			  const char *name,
>  			  const char *other,
>  			  struct diff_filespec *one,

Likewise.  Within the context of "diff", it is clear what kind of metainfo
we mean (that's the one between "diff --git" and the first "@@ ... @@").
Other APIs with different kind of "metainfo" may later want to introduce a
totally different function that fills their metainfo, and calling this
"fill_metainfo" forces them to use some different name, and cause
confusion to readers.  Perhaps fill-diff-metainfo?