From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 22 Sep 2016 20:50:54 GMT
Subject: Re: [PATCH 3/6] ref-filter: Expose wrappers for ref_item functions
Message-ID: <xmqqvaxn3bgx.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160922185317.349-4-santiago@nyu.edu>

santiago@nyu.edu writes:

> From: Lukas P <luk.puehringer@gmail.com>
>
> Ref-filter functions are useful for printing git object information
> without a format specifier. However, some functions may not want to use
> a complete ref-array, and just a single item instead. Expose
> create/show/free functions for ref_array_items through wrappers around
> the original functions.
>
> Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
> ---
>  ref-filter.c | 20 ++++++++++++++++++++
>  ref-filter.h | 10 ++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 9adbb8a..b013799 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1329,6 +1329,14 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
>  	return ref;
>  }
>  
> +/* Wrapper: Create ref_array_item w/o referencing container in function name */
> +struct ref_array_item *new_ref_item(const char *refname,
> +						 const unsigned char *objectname,
> +						 int flag)
> +{
> +	return new_ref_array_item(refname, objectname, flag);
> +}

Why?  As a public function name, new_ref_item() is a horrible one,
as there are other structures about "ref" elsewhere in the system.
If a new caller needs to be able to get a new ref_array_item, you
are better off just exposing it, not an ill-named wrapper.

>  static int filter_ref_kind(struct ref_filter *filter, const char *refname)
>  {
>  	unsigned int i;
> @@ -1426,6 +1434,12 @@ static void free_array_item(struct ref_array_item *item)
>  	free(item);
>  }
>  
> +/* Wrapper: Free ref_array_item w/o referencing container in function name */
> +void free_ref_item(struct ref_array_item *ref_item)
> +{
> +	free_array_item(ref_item);
> +}

Again, why?  free_array_item() is a horrible name for a public
function, and it is OK to rename it to free_ref_array_item() while
giving external callers an access to it, so that their names are
descriptive enough to convey that they are about ref_array_item
structure used in ref-filter API while at the same time making it
clear to readers that the two functions with related names are
indeed related.

> @@ -1637,6 +1651,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>  	putchar('\n');
>  }
>  
> +/* Wrapper: Show ref_array_item w/o referencing container in function name */
> +void show_ref_item(struct ref_array_item *ref_item, const char *format, int quote_style)
> +{
> +	show_ref_array_item(ref_item, format, quote_style);
> +}

Ditto.