Re: [PATCH v5 01/14] Add data structures and basic functions for commit trailers

[Christian Couder <chriscool@tuxfamily.org>]

From: Junio C Hamano <gitster@pobox.com>
>
> Christian Couder <chriscool@tuxfamily.org> writes:
> 
>> +enum action_where { WHERE_AFTER, WHERE_BEFORE };
>> +enum action_if_exist { EXIST_ADD_IF_DIFFERENT, EXIST_ADD_IF_DIFFERENT_NEIGHBOR,
>> +		       EXIST_ADD, EXIST_OVERWRITE, EXIST_DO_NOTHING };
>> +enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };
> 
> All these names and "conf_info" below are not named to be specific
> to this little tool.  Can I assume that these will never be exposed
> to the rest of the system?  If so, they are fine.

Yeah, I don't plan them to be exposed to other files.
 
>> +struct conf_info {
>> +	char *name;
>> +	char *key;
>> +	char *command;
>> +	enum action_where where;
>> +	enum action_if_exist if_exist;
>> +	enum action_if_missing if_missing;
> 
> It still feels somewhat strange.  It is true that an item can be
> either "exist" or "missing" and it is understandable that it tempts
> you to split that into two, but EXIST_OVERWRITE will not trigger
> either WHERE_AFTER or WHERE_BEFORE action.

Yeah, it's true that WHERE_AFTER/WHERE_BEFORE does not make sense for
EXIST_OVERWRITE, EXIST_DO_NOTHING and MISSING_DO_NOTHING, but it's a
fact of life that sometimes some options do not make sense with
others.

>> +static inline int same_token(struct trailer_item *a, struct trailer_item *b, int alnum_len)
>> +{
>> +	return !strncasecmp(a->token, b->token, alnum_len);
>> +}
>> +
>> +static inline int same_value(struct trailer_item *a, struct trailer_item *b)
>> +{
>> +	return !strcasecmp(a->value, b->value);
>> +}
>> +
>> +static inline int same_trailer(struct trailer_item *a, struct trailer_item *b, int alnum_len)
>> +{
>> +	return same_token(a, b, alnum_len) && same_value(a, b);
>> +}
> 
> All these "inlines" look premature optimization that can be
> delegated to any decent compiler, don't they?

Yeah, but as Eric suggested to add them like in header files and you
did not reply to him, I thought you agreed with him.
I will remove them.

>> +/* Get the length of buf from its beginning until its last alphanumeric character */
>> +static inline size_t alnum_len(const char *buf, int len)
>> +{
>> +	while (--len >= 0 && !isalnum(buf[len]));
> 
> Style:
> 
> 	while (--len >= 0 && !isalnum(buf[len]))
>         	;
> 
> You may add a comment on the empty statement to make it stand out
> even more, i.e.
> 
> 		; /* nothing */

Ok, I will do that.

>> +	return (size_t) len + 1;
> 
> This is somewhat unfortunate.  if the caller wants to receive
> size_t, perhaps it should be passing in size_t (or ssize_t) to the
> function?  Hard to guess without an actual caller, though.

Ok, I will make it return an int.

Thanks,
Christian.