Christian Couder [off-list ref] writes:
quoted hunk
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 <redacted>
---
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@tuxfamily.org>
+ */
+
+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.
+}
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.
On Sun, Feb 9, 2014 at 8:48 AM, Christian Couder
[off-list ref] wrote:
From: Junio C Hamano <redacted>
quoted
Christian Couder [off-list ref] writes:
quoted
+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.
If these functions are used only by trailer.c, then it would make
sense to move them from trailer.h to trailer.c so that they don't get
defined unnecessarily by each .c file which includes trailer.h.
quoted
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]));
+ 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.
Why not adjust the loop condition slightly instead so that you can
continue to accept and return size_t?