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

From: Junio C Hamano <redacted>

Christian Couder [off-list ref] writes:

quoted

+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.
 

quoted

+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.

quoted

+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.

quoted

+/* 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.

quoted

+	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.