Christian Couder <chriscool@xxxxxxxxxxxxx> writes:
> We will use a doubly linked list to store all information
> about trailers and their configuration.
>
> This way we can easily remove or add trailers to or from
> trailer lists while traversing the lists in either direction.
>
> Signed-off-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> ---
> Makefile | 1 +
> trailer.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 49 insertions(+)
> create mode 100644 trailer.c
>
> diff --git a/Makefile b/Makefile
> index b4af1e2..ec90feb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -871,6 +871,7 @@ LIB_OBJS += submodule.o
> LIB_OBJS += symlinks.o
> LIB_OBJS += tag.o
> LIB_OBJS += trace.o
> +LIB_OBJS += trailer.o
> LIB_OBJS += transport.o
> LIB_OBJS += transport-helper.o
> LIB_OBJS += tree-diff.o
> diff --git a/trailer.c b/trailer.c
> new file mode 100644
> index 0000000..f129b5a
> --- /dev/null
> +++ b/trailer.c
> @@ -0,0 +1,48 @@
> +#include "cache.h"
> +/*
> + * Copyright (c) 2013 Christian Couder <chriscool@xxxxxxxxxxxxx>
> + */
> +
> +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.
> +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.
> +};
> +
> +struct trailer_item {
> + struct trailer_item *previous;
> + struct trailer_item *next;
> + const char *token;
> + const char *value;
> + struct conf_info *conf;
> +};
> +
> +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?
> +/* 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 */
> + 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.
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html