Re: [PATCH v5 01/14] Add data structures and basic functions for commit trailers
From: Junio C Hamano gits...@pobox.com
Christian Couder chrisc...@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.
--
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 v5 01/14] Add data structures and basic functions for commit trailers
From: Junio C Hamano gits...@pobox.com
Christian Couder chrisc...@tuxfamily.org writes:
+enum action_if_exist if_exist;
+enum action_if_missing if_missing;
Probably if_exists is more gramatically correct.
if (x-if_exists) {
... do this ...
}
would read well, but not x-if_exist.
Ok, I will use if_exists instead of if_exist and also:
enum action_if_exists { EXISTS_ADD_IF_DIFFERENT,
EXISTS_ADD_IF_DIFFERENT_NEIGHBOR,
EXISTS_ADD, EXISTS_OVERWRITE, EXISTS_DO_NOTHING };
instead of:
enum action_if_exist { EXIST_ADD_IF_DIFFERENT, EXIST_ADD_IF_DIFFERENT_NEIGHBOR,
EXIST_ADD, EXIST_OVERWRITE, EXIST_DO_NOTHING };
to be consistent.
Thanks,
Christian.
--
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 v5 01/14] Add data structures and basic functions for commit trailers
On Sun, Feb 9, 2014 at 8:48 AM, Christian Couder
chrisc...@tuxfamily.org wrote:
From: Junio C Hamano gits...@pobox.com
Christian Couder chrisc...@tuxfamily.org writes:
+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.
+/* 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?
--
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
[PATCH v5 01/14] Add data structures and basic functions for commit trailers
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 chrisc...@tuxfamily.org
---
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 000..f129b5a
--- /dev/null
+++ b/trailer.c
@@ -0,0 +1,48 @@
+#include cache.h
+/*
+ * Copyright (c) 2013 Christian Couder chrisc...@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 };
+
+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;
+};
+
+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);
+}
+
+/* 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;
+}
--
1.8.5.2.206.g98f5689.dirty
--
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 v5 01/14] Add data structures and basic functions for commit trailers
Christian Couder chrisc...@tuxfamily.org 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 chrisc...@tuxfamily.org
---
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 000..f129b5a
--- /dev/null
+++ b/trailer.c
@@ -0,0 +1,48 @@
+#include cache.h
+/*
+ * Copyright (c) 2013 Christian Couder chrisc...@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.
+}
--
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 v5 01/14] Add data structures and basic functions for commit trailers
Christian Couder chrisc...@tuxfamily.org writes:
+ enum action_if_exist if_exist;
+ enum action_if_missing if_missing;
Probably if_exists is more gramatically correct.
if (x-if_exists) {
... do this ...
}
would read well, but not x-if_exist.
--
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
