Re: [PATCH 02/16] refs: add methods for misc ref operations

[David Turner <dturner@twopensource.com>]

On Fri, 2015-12-11 at 15:39 -0800, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> >  struct ref_be {
> >  	struct ref_be *next;
> >  	const char *name;
> >  	ref_transaction_commit_fn *transaction_commit;
> > +
> > +	pack_refs_fn *pack_refs;
> > +	peel_ref_fn *peel_ref;
> > +	create_symref_fn *create_symref;
> > +
> > +	resolve_ref_unsafe_fn *resolve_ref_unsafe;
> > +	verify_refname_available_fn *verify_refname_available;
> > +	resolve_gitlink_ref_fn *resolve_gitlink_ref;
> >  };
> 
> This may have been pointed out in the previous reviews by somebody
> else, but I think it is more customary to declare a struct member
> that is a pointer to a customization function without leading '*',
> i.e.
> 
> 	typedef TYPE (*customize_fn)(ARGS);
> 
>         struct vtable {
> 		...
>         	cutomize_fn fn;
> 		...
> 	};
> 
> in our codebase (cf. string_list::cmp, prio_queue::compare).

The previous review was here:
http://permalink.gmane.org/gmane.comp.version-control.git/279062

Michael wrote:
> Hmmm, I thought our convention was to define typedefs for functions
> themselves, not for the pointer-to-function; e.g.,
>
>     typedef struct ref_transaction *ref_transaction_begin_fn(struct
> strbuf *err);
>
> (which would require `struct ref_be` to be changed to
>
>         ref_transaction_begin_fn *transaction_begin;

And you agreed.  So I changed it.  Do you want me to change it back?