Re: [RFC PATCH 02/10] Move textconv_object to be with other textconv methods

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

Jeff Smith <whydoubt@gmail.com> writes:

> Signed-off-by: Jeff Smith <whydoubt@gmail.com>
> ---
>  builtin.h          |  2 --
>  builtin/blame.c    | 28 ----------------------------
>  builtin/cat-file.c |  1 +
>  diff.c             | 23 +++++++++++++++++++++++
>  diff.h             |  7 +++++++
>  5 files changed, 31 insertions(+), 30 deletions(-)

This change makes sense regardless of your primary goal of the
series.  It was not good that one builtin borrowing a helper from
another.  The common helper should live outside builtin/ as a common
code, and in this case, diff.[ch] may be an OK place for it.


>
> diff --git a/builtin.h b/builtin.h
> index 9e4a898..498ac80 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -25,8 +25,6 @@ struct fmt_merge_msg_opts {
>  extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
>  			 struct fmt_merge_msg_opts *);
>  
> -extern int textconv_object(const char *path, unsigned mode, const struct object_id *oid, int oid_valid, char **buf, unsigned long *buf_size);
> -
>  extern int is_builtin(const char *s);
>  
>  extern int cmd_add(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 42c56eb..c419981 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -147,34 +147,6 @@ static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b,
>  }
>  
>  /*
> - * Prepare diff_filespec and convert it using diff textconv API
> - * if the textconv driver exists.
> - * Return 1 if the conversion succeeds, 0 otherwise.
> - */
> -int textconv_object(const char *path,
> -		    unsigned mode,
> -		    const struct object_id *oid,
> -		    int oid_valid,
> -		    char **buf,
> -		    unsigned long *buf_size)
> -{
> -	struct diff_filespec *df;
> -	struct userdiff_driver *textconv;
> -
> -	df = alloc_filespec(path);
> -	fill_filespec(df, oid->hash, oid_valid, mode);
> -	textconv = get_textconv(df);
> -	if (!textconv) {
> -		free_filespec(df);
> -		return 0;
> -	}
> -
> -	*buf_size = fill_textconv(textconv, df, buf);
> -	free_filespec(df);
> -	return 1;
> -}
> -
> -/*
>   * Given an origin, prepare mmfile_t structure to be used by the
>   * diff machinery
>   */
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 1890d7a..79a2c82 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -5,6 +5,7 @@
>   */
>  #include "cache.h"
>  #include "builtin.h"
> +#include "diff.h"
>  #include "parse-options.h"
>  #include "userdiff.h"
>  #include "streaming.h"
> diff --git a/diff.c b/diff.c
> index 11eef1c..a62e989 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5270,6 +5270,29 @@ size_t fill_textconv(struct userdiff_driver *driver,
>  	return size;
>  }
>  
> +int textconv_object(const char *path,
> +		    unsigned mode,
> +		    const struct object_id *oid,
> +		    int oid_valid,
> +		    char **buf,
> +		    unsigned long *buf_size)
> +{
> +	struct diff_filespec *df;
> +	struct userdiff_driver *textconv;
> +
> +	df = alloc_filespec(path);
> +	fill_filespec(df, oid->hash, oid_valid, mode);
> +	textconv = get_textconv(df);
> +	if (!textconv) {
> +		free_filespec(df);
> +		return 0;
> +	}
> +
> +	*buf_size = fill_textconv(textconv, df, buf);
> +	free_filespec(df);
> +	return 1;
> +}
> +
>  void setup_diff_pager(struct diff_options *opt)
>  {
>  	/*
> diff --git a/diff.h b/diff.h
> index 5be1ee7..52ebd54 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -385,6 +385,13 @@ extern size_t fill_textconv(struct userdiff_driver *driver,
>   */
>  extern struct userdiff_driver *get_textconv(struct diff_filespec *one);
>  
> +/*
> + * Prepare diff_filespec and convert it using diff textconv API
> + * if the textconv driver exists.
> + * Return 1 if the conversion succeeds, 0 otherwise.
> + */
> +extern int textconv_object(const char *path, unsigned mode, const struct object_id *oid, int oid_valid, char **buf, unsigned long *buf_size);
> +
>  extern int parse_rename_score(const char **cp_p);
>  
>  extern long parse_algorithm_value(const char *value);