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

[Howard Chu]

Junio C Hamano  pobox.com> writes:

> 
> David Turner  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).

(LMDB author here, just passing by)

IMO you're making a mistake. You should always typedef the thing itself, not
a pointer to the thing. If you only typedef the pointer, you can only use
the typedef to declare pointers and never the thing itself. If you typedef
the actual thing, you can use the typedef to declare both the thing and
pointer to thing.

It's particularly useful to have function typedefs because later on you can
use the actual typedef to declare instances of that function type in header
files, and guarantee that your function definitions match what they're
intended to match. Otherwise, assigning a bare (*func) to something that
mismatches only generates a warning, instead of an error.

-- 
  -- Howard Chu
  CTO, Symas Corp.   http://www.symas.com
  Director, Highland Sun http://highlandsun.com/hyc/
  Chief Architect, OpenLDAP  http://www.openldap.org/project/ 


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

David Turner  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).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

David Turner  writes:

> diff --git a/cache.h b/cache.h
> index 51c35c3..707455a 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -,6 +,13 @@ extern char *oid_to_hex(const struct object_id *oid);  
> /* same static buffer as s
>  extern int interpret_branch_name(const char *str, int len, struct strbuf *);
>  extern int get_sha1_mb(const char *str, unsigned char *sha1);
>  
> +/*
> + * Return true iff abbrev_name is a possible abbreviation for
> + * full_name according to the rules defined by ref_rev_parse_rules in
> + * refs.c.
> + */
> +extern int refname_match(const char *abbrev_name, const char *full_name);
> +
>  extern int validate_headref(const char *ref);

This hunk is strange.  It duplicates what is in refs.h, risking the
two copies to drift away from each other over time.

As the only callsites are in remote.c that includes refs.h, this
probably should be dropped.

Other changes that give redirection to various operations via vtable
looked all sensible.

>  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;
>  };
>  
>  #endif /* REFS_REFS_INTERNAL_H */
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Turner]

On Fri, 2015-12-11 at 15:33 -0800, Junio C Hamano wrote:
> David Turner  writes:
> 
> > diff --git a/cache.h b/cache.h
> > index 51c35c3..707455a 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -,6 +,13 @@ extern char *oid_to_hex(const struct object_id 
> > *oid);/* same static buffer as s
> >  extern int interpret_branch_name(const char *str, int len, struct strbuf 
> > *);
> >  extern int get_sha1_mb(const char *str, unsigned char *sha1);
> >  
> > +/*
> > + * Return true iff abbrev_name is a possible abbreviation for
> > + * full_name according to the rules defined by ref_rev_parse_rules in
> > + * refs.c.
> > + */
> > +extern int refname_match(const char *abbrev_name, const char *full_name);
> > +
> >  extern int validate_headref(const char *ref);
> 
> This hunk is strange.  It duplicates what is in refs.h, risking the
> two copies to drift away from each other over time.
> 
> As the only callsites are in remote.c that includes refs.h, this
> probably should be dropped.

Will fix, thanks.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Turner]

On Fri, 2015-12-11 at 15:39 -0800, Junio C Hamano wrote:
> David Turner  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?  

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Turner]

On Fri, 2015-12-11 at 16:23 -0800, Junio C Hamano wrote:
> David Turner  writes:
> 
> > 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?  
> 
> Sorry about that. I was agreeing to my mistaken understanding of
> what was pointed out X-<.
> 
> We do prefer fn(args) over (*fn)(args) and somehow I mixed that up
> with the declaration style.

So I should change it back?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

David Turner  writes:

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

Sorry about that. I was agreeing to my mistaken understanding of
what was pointed out X-<.

We do prefer fn(args) over (*fn)(args) and somehow I mixed that up
with the declaration style.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html