* [PATCH v2 4/6] trailer: make args have their own struct
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Improve type safety by making arguments (whether from configuration or
from the command line) have their own "struct arg_item" type, separate
from the "struct trailer_item" type used to represent the trailers in
the buffer being manipulated.
This change also prepares "struct trailer_item" to be further
differentiated from "struct arg_item" in future patches.
---
trailer.c | 135 +++++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 85 insertions(+), 50 deletions(-)
diff --git a/trailer.c b/trailer.c
index 54cc930..a9ed3f8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -29,6 +29,12 @@ struct trailer_item {
struct list_head list;
char *token;
char *value;
+};
+
+struct arg_item {
+ struct list_head list;
+ char *token;
+ char *value;
struct conf_info conf;
};
@@ -62,7 +68,7 @@ static size_t token_len_without_separator(const char *token, size_t len)
return len;
}
-static int same_token(struct trailer_item *a, struct trailer_item *b)
+static int same_token(struct trailer_item *a, struct arg_item *b)
{
size_t a_len = token_len_without_separator(a->token, strlen(a->token));
size_t b_len = token_len_without_separator(b->token, strlen(b->token));
@@ -71,12 +77,12 @@ static int same_token(struct trailer_item *a, struct trailer_item *b)
return !strncasecmp(a->token, b->token, min_len);
}
-static int same_value(struct trailer_item *a, struct trailer_item *b)
+static int same_value(struct trailer_item *a, struct arg_item *b)
{
return !strcasecmp(a->value, b->value);
}
-static int same_trailer(struct trailer_item *a, struct trailer_item *b)
+static int same_trailer(struct trailer_item *a, struct arg_item *b)
{
return same_token(a, b) && same_value(a, b);
}
@@ -98,6 +104,13 @@ static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *
static void free_trailer_item(struct trailer_item *item)
{
+ free(item->token);
+ free(item->value);
+ free(item);
+}
+
+static void free_arg_item(struct arg_item *item)
+{
free(item->conf.name);
free(item->conf.key);
free(item->conf.command);
@@ -137,17 +150,29 @@ static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
}
}
+static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
+{
+ struct trailer_item *new = xcalloc(sizeof(*new), 1);
+ new->token = arg_tok->token;
+ new->value = arg_tok->value;
+ arg_tok->token = arg_tok->value = NULL;
+ free_arg_item(arg_tok);
+ return new;
+}
+
static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct trailer_item *arg_tok)
+ struct arg_item *arg_tok)
{
- if (after_or_end(arg_tok->conf.where))
- list_add(&arg_tok->list, &on_tok->list);
+ int aoe = after_or_end(arg_tok->conf.where);
+ struct trailer_item *to_add = trailer_from_arg(arg_tok);
+ if (aoe)
+ list_add(&to_add->list, &on_tok->list);
else
- list_add_tail(&arg_tok->list, &on_tok->list);
+ list_add_tail(&to_add->list, &on_tok->list);
}
static int check_if_different(struct trailer_item *in_tok,
- struct trailer_item *arg_tok,
+ struct arg_item *arg_tok,
int check_all,
struct list_head *head)
{
@@ -200,7 +225,7 @@ static char *apply_command(const char *command, const char *arg)
return result;
}
-static void apply_item_command(struct trailer_item *in_tok, struct trailer_item *arg_tok)
+static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
{
if (arg_tok->conf.command) {
const char *arg;
@@ -218,13 +243,13 @@ static void apply_item_command(struct trailer_item *in_tok, struct trailer_item
}
static void apply_arg_if_exists(struct trailer_item *in_tok,
- struct trailer_item *arg_tok,
+ struct arg_item *arg_tok,
struct trailer_item *on_tok,
struct list_head *head)
{
switch (arg_tok->conf.if_exists) {
case EXISTS_DO_NOTHING:
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
break;
case EXISTS_REPLACE:
apply_item_command(in_tok, arg_tok);
@@ -241,39 +266,41 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
if (check_if_different(in_tok, arg_tok, 1, head))
add_arg_to_input_list(on_tok, arg_tok);
else
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
break;
case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
apply_item_command(in_tok, arg_tok);
if (check_if_different(on_tok, arg_tok, 0, head))
add_arg_to_input_list(on_tok, arg_tok);
else
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
break;
}
}
static void apply_arg_if_missing(struct list_head *head,
- struct trailer_item *arg_tok)
+ struct arg_item *arg_tok)
{
enum action_where where;
+ struct trailer_item *to_add;
switch (arg_tok->conf.if_missing) {
case MISSING_DO_NOTHING:
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
break;
case MISSING_ADD:
where = arg_tok->conf.where;
apply_item_command(NULL, arg_tok);
+ to_add = trailer_from_arg(arg_tok);
if (after_or_end(where))
- list_add_tail(&arg_tok->list, head);
+ list_add_tail(&to_add->list, head);
else
- list_add(&arg_tok->list, head);
+ list_add(&to_add->list, head);
}
}
static int find_same_and_apply_arg(struct list_head *head,
- struct trailer_item *arg_tok)
+ struct arg_item *arg_tok)
{
struct list_head *pos;
struct trailer_item *in_tok;
@@ -306,11 +333,11 @@ static void process_trailers_lists(struct list_head *head,
struct list_head *arg_head)
{
struct list_head *pos, *p;
- struct trailer_item *arg_tok;
+ struct arg_item *arg_tok;
list_for_each_safe(pos, p, arg_head) {
int applied = 0;
- arg_tok = list_entry(pos, struct trailer_item, list);
+ arg_tok = list_entry(pos, struct arg_item, list);
list_del(pos);
@@ -375,20 +402,20 @@ static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
dst->command = xstrdup(src->command);
}
-static struct trailer_item *get_conf_item(const char *name)
+static struct arg_item *get_conf_item(const char *name)
{
struct list_head *pos;
- struct trailer_item *item;
+ struct arg_item *item;
/* Look up item with same name */
list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct trailer_item, list);
+ item = list_entry(pos, struct arg_item, list);
if (!strcasecmp(item->conf.name, name))
return item;
}
/* Item does not already exists, create it */
- item = xcalloc(sizeof(struct trailer_item), 1);
+ item = xcalloc(sizeof(*item), 1);
duplicate_conf(&item->conf, &default_conf_info);
item->conf.name = xstrdup(name);
@@ -442,7 +469,7 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v
static int git_trailer_config(const char *conf_key, const char *value, void *cb)
{
const char *trailer_item, *variable_name;
- struct trailer_item *item;
+ struct arg_item *item;
struct conf_info *conf;
char *name = NULL;
enum trailer_info_type type;
@@ -500,7 +527,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
return 0;
}
-static const char *token_from_item(struct trailer_item *item, char *tok)
+static const char *token_from_item(struct arg_item *item, char *tok)
{
if (item->conf.key)
return item->conf.key;
@@ -509,7 +536,7 @@ static const char *token_from_item(struct trailer_item *item, char *tok)
return item->conf.name;
}
-static int token_matches_item(const char *tok, struct trailer_item *item, int tok_len)
+static int token_matches_item(const char *tok, struct arg_item *item, int tok_len)
{
if (!strncasecmp(tok, item->conf.name, tok_len))
return 1;
@@ -521,7 +548,7 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
{
size_t len;
struct strbuf seps = STRBUF_INIT;
- struct trailer_item *item;
+ struct arg_item *item;
int tok_len;
struct list_head *pos;
@@ -547,12 +574,14 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
/* Lookup if the token matches something in the config */
tok_len = token_len_without_separator(tok->buf, tok->len);
- *conf = &default_conf_info;
+ if (conf)
+ *conf = &default_conf_info;
list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct trailer_item, list);
+ item = list_entry(pos, struct arg_item, list);
if (token_matches_item(tok->buf, item, tok_len)) {
char *tok_buf = strbuf_detach(tok, NULL);
- *conf = &item->conf;
+ if (conf)
+ *conf = &item->conf;
strbuf_addstr(tok, token_from_item(item, tok_buf));
free(tok_buf);
break;
@@ -562,43 +591,51 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
return 0;
}
-static void add_trailer_item(struct list_head *head, char *tok, char *val,
- const struct conf_info *conf)
+static void add_trailer_item(struct list_head *head, char *tok, char *val)
{
struct trailer_item *new = xcalloc(sizeof(*new), 1);
new->token = tok;
new->value = val;
- duplicate_conf(&new->conf, conf);
list_add_tail(&new->list, head);
}
+static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
+ const struct conf_info *conf)
+{
+ struct arg_item *new = xcalloc(sizeof(*new), 1);
+ new->token = tok;
+ new->value = val;
+ duplicate_conf(&new->conf, conf);
+ list_add_tail(&new->list, arg_head);
+}
+
static void process_command_line_args(struct list_head *arg_head,
struct string_list *trailers)
{
struct string_list_item *tr;
- struct trailer_item *item;
+ struct arg_item *item;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
const struct conf_info *conf;
struct list_head *pos;
- /* Add a trailer item for each configured trailer with a command */
+ /* Add an arg item for each configured trailer with a command */
list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct trailer_item, list);
+ item = list_entry(pos, struct arg_item, list);
if (item->conf.command)
- add_trailer_item(arg_head,
- xstrdup(token_from_item(item, NULL)),
- xstrdup(""),
- &item->conf);
+ add_arg_item(arg_head,
+ xstrdup(token_from_item(item, NULL)),
+ xstrdup(""),
+ &item->conf);
}
- /* Add a trailer item for each trailer on the command line */
+ /* Add an arg item for each trailer on the command line */
for_each_string_list_item(tr, trailers) {
if (!parse_trailer(&tok, &val, &conf, tr->string))
- add_trailer_item(arg_head,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
- conf);
+ add_arg_item(arg_head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf);
}
}
@@ -721,7 +758,6 @@ static int process_input_file(FILE *outfile,
int patch_start, trailer_start, trailer_end, i;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
- const struct conf_info *conf;
/* Get the line count */
while (lines[count])
@@ -740,11 +776,10 @@ static int process_input_file(FILE *outfile,
/* Parse trailer lines */
for (i = trailer_start; i < trailer_end; i++) {
if (lines[i]->buf[0] != comment_line_char &&
- !parse_trailer(&tok, &val, &conf, lines[i]->buf))
+ !parse_trailer(&tok, &val, NULL, lines[i]->buf))
add_trailer_item(head,
strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
- conf);
+ strbuf_detach(&val, NULL));
}
return trailer_end;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 3/6] trailer: streamline trailer item create and add
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Currently, creation and addition (to a list) of trailer items are spread
across multiple functions. Streamline this by only having 2 functions:
one to parse the user-supplied string, and one to add the parsed
information to a list.
---
trailer.c | 130 +++++++++++++++++++++++++++++---------------------------------
1 file changed, 60 insertions(+), 70 deletions(-)
diff --git a/trailer.c b/trailer.c
index 0afa240..54cc930 100644
--- a/trailer.c
+++ b/trailer.c
@@ -500,10 +500,31 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
return 0;
}
-static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *trailer)
+static const char *token_from_item(struct trailer_item *item, char *tok)
+{
+ if (item->conf.key)
+ return item->conf.key;
+ if (tok)
+ return tok;
+ return item->conf.name;
+}
+
+static int token_matches_item(const char *tok, struct trailer_item *item, int tok_len)
+{
+ if (!strncasecmp(tok, item->conf.name, tok_len))
+ return 1;
+ return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
+}
+
+static int parse_trailer(struct strbuf *tok, struct strbuf *val,
+ const struct conf_info **conf, const char *trailer)
{
size_t len;
struct strbuf seps = STRBUF_INIT;
+ struct trailer_item *item;
+ int tok_len;
+ struct list_head *pos;
+
strbuf_addstr(&seps, separators);
strbuf_addch(&seps, '=');
len = strcspn(trailer, seps.buf);
@@ -523,74 +544,31 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *tra
strbuf_addstr(tok, trailer);
strbuf_trim(tok);
}
- return 0;
-}
-
-static const char *token_from_item(struct trailer_item *item, char *tok)
-{
- if (item->conf.key)
- return item->conf.key;
- if (tok)
- return tok;
- return item->conf.name;
-}
-
-static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
- char *tok, char *val)
-{
- struct trailer_item *new = xcalloc(sizeof(*new), 1);
- new->value = val ? val : xstrdup("");
-
- if (conf_item) {
- duplicate_conf(&new->conf, &conf_item->conf);
- new->token = xstrdup(token_from_item(conf_item, tok));
- free(tok);
- } else {
- duplicate_conf(&new->conf, &default_conf_info);
- new->token = tok;
- }
-
- return new;
-}
-
-static int token_matches_item(const char *tok, struct trailer_item *item, int tok_len)
-{
- if (!strncasecmp(tok, item->conf.name, tok_len))
- return 1;
- return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
-}
-
-static struct trailer_item *create_trailer_item(const char *string)
-{
- struct strbuf tok = STRBUF_INIT;
- struct strbuf val = STRBUF_INIT;
- struct trailer_item *item;
- int tok_len;
- struct list_head *pos;
-
- if (parse_trailer(&tok, &val, string))
- return NULL;
-
- tok_len = token_len_without_separator(tok.buf, tok.len);
/* Lookup if the token matches something in the config */
+ tok_len = token_len_without_separator(tok->buf, tok->len);
+ *conf = &default_conf_info;
list_for_each(pos, &conf_head) {
item = list_entry(pos, struct trailer_item, list);
- if (token_matches_item(tok.buf, item, tok_len))
- return new_trailer_item(item,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL));
+ if (token_matches_item(tok->buf, item, tok_len)) {
+ char *tok_buf = strbuf_detach(tok, NULL);
+ *conf = &item->conf;
+ strbuf_addstr(tok, token_from_item(item, tok_buf));
+ free(tok_buf);
+ break;
+ }
}
- return new_trailer_item(NULL,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL));
+ return 0;
}
-static void add_trailer_item(struct list_head *head, struct trailer_item *new)
+static void add_trailer_item(struct list_head *head, char *tok, char *val,
+ const struct conf_info *conf)
{
- if (!new)
- return;
+ struct trailer_item *new = xcalloc(sizeof(*new), 1);
+ new->token = tok;
+ new->value = val;
+ duplicate_conf(&new->conf, conf);
list_add_tail(&new->list, head);
}
@@ -599,21 +577,28 @@ static void process_command_line_args(struct list_head *arg_head,
{
struct string_list_item *tr;
struct trailer_item *item;
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ const struct conf_info *conf;
struct list_head *pos;
/* Add a trailer item for each configured trailer with a command */
list_for_each(pos, &conf_head) {
item = list_entry(pos, struct trailer_item, list);
- if (item->conf.command) {
- struct trailer_item *new = new_trailer_item(item, NULL, NULL);
- add_trailer_item(arg_head, new);
- }
+ if (item->conf.command)
+ add_trailer_item(arg_head,
+ xstrdup(token_from_item(item, NULL)),
+ xstrdup(""),
+ &item->conf);
}
/* Add a trailer item for each trailer on the command line */
for_each_string_list_item(tr, trailers) {
- struct trailer_item *new = create_trailer_item(tr->string);
- add_trailer_item(arg_head, new);
+ if (!parse_trailer(&tok, &val, &conf, tr->string))
+ add_trailer_item(arg_head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf);
}
}
@@ -734,6 +719,9 @@ static int process_input_file(FILE *outfile,
{
int count = 0;
int patch_start, trailer_start, trailer_end, i;
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ const struct conf_info *conf;
/* Get the line count */
while (lines[count])
@@ -751,10 +739,12 @@ static int process_input_file(FILE *outfile,
/* Parse trailer lines */
for (i = trailer_start; i < trailer_end; i++) {
- if (lines[i]->buf[0] != comment_line_char) {
- struct trailer_item *new = create_trailer_item(lines[i]->buf);
- add_trailer_item(head, new);
- }
+ if (lines[i]->buf[0] != comment_line_char &&
+ !parse_trailer(&tok, &val, &conf, lines[i]->buf))
+ add_trailer_item(head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf);
}
return trailer_end;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 2/6] trailer: use list.h for doubly-linked list
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Replace the existing handwritten implementation of a doubly-linked list
in trailer.c with the functions and macros from list.h. This
significantly simplifies the code.
---
trailer.c | 258 ++++++++++++++++++++++----------------------------------------
1 file changed, 91 insertions(+), 167 deletions(-)
diff --git a/trailer.c b/trailer.c
index 1f191b2..0afa240 100644
--- a/trailer.c
+++ b/trailer.c
@@ -4,6 +4,7 @@
#include "commit.h"
#include "tempfile.h"
#include "trailer.h"
+#include "list.h"
/*
* Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
*/
@@ -25,19 +26,24 @@ struct conf_info {
static struct conf_info default_conf_info;
struct trailer_item {
- struct trailer_item *previous;
- struct trailer_item *next;
+ struct list_head list;
char *token;
char *value;
struct conf_info conf;
};
-static struct trailer_item *first_conf_item;
+LIST_HEAD(conf_head);
static char *separators = ":";
#define TRAILER_ARG_STRING "$ARG"
+/* Iterate over the elements of the list. */
+#define list_for_each_dir(pos, head, is_reverse) \
+ for (pos = is_reverse ? (head)->prev : (head)->next; \
+ pos != (head); \
+ pos = is_reverse ? pos->prev : pos->next)
+
static int after_or_end(enum action_where where)
{
return (where == WHERE_AFTER) || (where == WHERE_END);
@@ -120,101 +126,49 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-static void print_all(FILE *outfile, struct trailer_item *first, int trim_empty)
+static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
{
+ struct list_head *pos;
struct trailer_item *item;
- for (item = first; item; item = item->next) {
+ list_for_each(pos, head) {
+ item = list_entry(pos, struct trailer_item, list);
if (!trim_empty || strlen(item->value) > 0)
print_tok_val(outfile, item->token, item->value);
}
}
-static void update_last(struct trailer_item **last)
-{
- if (*last)
- while ((*last)->next != NULL)
- *last = (*last)->next;
-}
-
-static void update_first(struct trailer_item **first)
-{
- if (*first)
- while ((*first)->previous != NULL)
- *first = (*first)->previous;
-}
-
static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct trailer_item *arg_tok,
- struct trailer_item **first,
- struct trailer_item **last)
-{
- if (after_or_end(arg_tok->conf.where)) {
- arg_tok->next = on_tok->next;
- on_tok->next = arg_tok;
- arg_tok->previous = on_tok;
- if (arg_tok->next)
- arg_tok->next->previous = arg_tok;
- update_last(last);
- } else {
- arg_tok->previous = on_tok->previous;
- on_tok->previous = arg_tok;
- arg_tok->next = on_tok;
- if (arg_tok->previous)
- arg_tok->previous->next = arg_tok;
- update_first(first);
- }
+ struct trailer_item *arg_tok)
+{
+ if (after_or_end(arg_tok->conf.where))
+ list_add(&arg_tok->list, &on_tok->list);
+ else
+ list_add_tail(&arg_tok->list, &on_tok->list);
}
static int check_if_different(struct trailer_item *in_tok,
struct trailer_item *arg_tok,
- int check_all)
+ int check_all,
+ struct list_head *head)
{
enum action_where where = arg_tok->conf.where;
+ struct list_head *next_head;
do {
- if (!in_tok)
- return 1;
if (same_trailer(in_tok, arg_tok))
return 0;
/*
* if we want to add a trailer after another one,
* we have to check those before this one
*/
- in_tok = after_or_end(where) ? in_tok->previous : in_tok->next;
+ next_head = after_or_end(where) ? in_tok->list.prev
+ : in_tok->list.next;
+ if (next_head == head)
+ break;
+ in_tok = list_entry(next_head, struct trailer_item, list);
} while (check_all);
return 1;
}
-static void remove_from_list(struct trailer_item *item,
- struct trailer_item **first,
- struct trailer_item **last)
-{
- struct trailer_item *next = item->next;
- struct trailer_item *previous = item->previous;
-
- if (next) {
- item->next->previous = previous;
- item->next = NULL;
- } else if (last)
- *last = previous;
-
- if (previous) {
- item->previous->next = next;
- item->previous = NULL;
- } else if (first)
- *first = next;
-}
-
-static struct trailer_item *remove_first(struct trailer_item **first)
-{
- struct trailer_item *item = *first;
- *first = item->next;
- if (item->next) {
- item->next->previous = NULL;
- item->next = NULL;
- }
- return item;
-}
-
static char *apply_command(const char *command, const char *arg)
{
struct strbuf cmd = STRBUF_INIT;
@@ -266,8 +220,7 @@ static void apply_item_command(struct trailer_item *in_tok, struct trailer_item
static void apply_arg_if_exists(struct trailer_item *in_tok,
struct trailer_item *arg_tok,
struct trailer_item *on_tok,
- struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last)
+ struct list_head *head)
{
switch (arg_tok->conf.if_exists) {
case EXISTS_DO_NOTHING:
@@ -275,40 +228,34 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
break;
case EXISTS_REPLACE:
apply_item_command(in_tok, arg_tok);
- add_arg_to_input_list(on_tok, arg_tok,
- in_tok_first, in_tok_last);
- remove_from_list(in_tok, in_tok_first, in_tok_last);
+ add_arg_to_input_list(on_tok, arg_tok);
+ list_del(&in_tok->list);
free_trailer_item(in_tok);
break;
case EXISTS_ADD:
apply_item_command(in_tok, arg_tok);
- add_arg_to_input_list(on_tok, arg_tok,
- in_tok_first, in_tok_last);
+ add_arg_to_input_list(on_tok, arg_tok);
break;
case EXISTS_ADD_IF_DIFFERENT:
apply_item_command(in_tok, arg_tok);
- if (check_if_different(in_tok, arg_tok, 1))
- add_arg_to_input_list(on_tok, arg_tok,
- in_tok_first, in_tok_last);
+ if (check_if_different(in_tok, arg_tok, 1, head))
+ add_arg_to_input_list(on_tok, arg_tok);
else
free_trailer_item(arg_tok);
break;
case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
apply_item_command(in_tok, arg_tok);
- if (check_if_different(on_tok, arg_tok, 0))
- add_arg_to_input_list(on_tok, arg_tok,
- in_tok_first, in_tok_last);
+ if (check_if_different(on_tok, arg_tok, 0, head))
+ add_arg_to_input_list(on_tok, arg_tok);
else
free_trailer_item(arg_tok);
break;
}
}
-static void apply_arg_if_missing(struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last,
+static void apply_arg_if_missing(struct list_head *head,
struct trailer_item *arg_tok)
{
- struct trailer_item **in_tok;
enum action_where where;
switch (arg_tok->conf.if_missing) {
@@ -317,68 +264,60 @@ static void apply_arg_if_missing(struct trailer_item **in_tok_first,
break;
case MISSING_ADD:
where = arg_tok->conf.where;
- in_tok = after_or_end(where) ? in_tok_last : in_tok_first;
apply_item_command(NULL, arg_tok);
- if (*in_tok) {
- add_arg_to_input_list(*in_tok, arg_tok,
- in_tok_first, in_tok_last);
- } else {
- *in_tok_first = arg_tok;
- *in_tok_last = arg_tok;
- }
- break;
+ if (after_or_end(where))
+ list_add_tail(&arg_tok->list, head);
+ else
+ list_add(&arg_tok->list, head);
}
}
-static int find_same_and_apply_arg(struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last,
+static int find_same_and_apply_arg(struct list_head *head,
struct trailer_item *arg_tok)
{
+ struct list_head *pos;
struct trailer_item *in_tok;
struct trailer_item *on_tok;
- struct trailer_item *following_tok;
enum action_where where = arg_tok->conf.where;
int middle = (where == WHERE_AFTER) || (where == WHERE_BEFORE);
int backwards = after_or_end(where);
- struct trailer_item *start_tok = backwards ? *in_tok_last : *in_tok_first;
+ struct trailer_item *start_tok;
- for (in_tok = start_tok; in_tok; in_tok = following_tok) {
- following_tok = backwards ? in_tok->previous : in_tok->next;
+ if (list_empty(head))
+ return 0;
+
+ start_tok = list_entry(backwards ? head->prev : head->next,
+ struct trailer_item,
+ list);
+
+ list_for_each_dir(pos, head, backwards) {
+ in_tok = list_entry(pos, struct trailer_item, list);
if (!same_token(in_tok, arg_tok))
continue;
on_tok = middle ? in_tok : start_tok;
- apply_arg_if_exists(in_tok, arg_tok, on_tok,
- in_tok_first, in_tok_last);
+ apply_arg_if_exists(in_tok, arg_tok, on_tok, head);
return 1;
}
return 0;
}
-static void process_trailers_lists(struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last,
- struct trailer_item **arg_tok_first)
+static void process_trailers_lists(struct list_head *head,
+ struct list_head *arg_head)
{
+ struct list_head *pos, *p;
struct trailer_item *arg_tok;
- struct trailer_item *next_arg;
-
- if (!*arg_tok_first)
- return;
- for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
+ list_for_each_safe(pos, p, arg_head) {
int applied = 0;
+ arg_tok = list_entry(pos, struct trailer_item, list);
- next_arg = arg_tok->next;
- remove_from_list(arg_tok, arg_tok_first, NULL);
+ list_del(pos);
- applied = find_same_and_apply_arg(in_tok_first,
- in_tok_last,
- arg_tok);
+ applied = find_same_and_apply_arg(head, arg_tok);
if (!applied)
- apply_arg_if_missing(in_tok_first,
- in_tok_last,
- arg_tok);
+ apply_arg_if_missing(head, arg_tok);
}
}
@@ -438,13 +377,12 @@ static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
static struct trailer_item *get_conf_item(const char *name)
{
+ struct list_head *pos;
struct trailer_item *item;
- struct trailer_item *previous;
/* Look up item with same name */
- for (previous = NULL, item = first_conf_item;
- item;
- previous = item, item = item->next) {
+ list_for_each(pos, &conf_head) {
+ item = list_entry(pos, struct trailer_item, list);
if (!strcasecmp(item->conf.name, name))
return item;
}
@@ -454,12 +392,7 @@ static struct trailer_item *get_conf_item(const char *name)
duplicate_conf(&item->conf, &default_conf_info);
item->conf.name = xstrdup(name);
- if (!previous)
- first_conf_item = item;
- else {
- previous->next = item;
- item->previous = previous;
- }
+ list_add_tail(&item->list, &conf_head);
return item;
}
@@ -633,6 +566,7 @@ static struct trailer_item *create_trailer_item(const char *string)
struct strbuf val = STRBUF_INIT;
struct trailer_item *item;
int tok_len;
+ struct list_head *pos;
if (parse_trailer(&tok, &val, string))
return NULL;
@@ -640,7 +574,8 @@ static struct trailer_item *create_trailer_item(const char *string)
tok_len = token_len_without_separator(tok.buf, tok.len);
/* Lookup if the token matches something in the config */
- for (item = first_conf_item; item; item = item->next) {
+ list_for_each(pos, &conf_head) {
+ item = list_entry(pos, struct trailer_item, list);
if (token_matches_item(tok.buf, item, tok_len))
return new_trailer_item(item,
strbuf_detach(&tok, NULL),
@@ -652,44 +587,34 @@ static struct trailer_item *create_trailer_item(const char *string)
strbuf_detach(&val, NULL));
}
-static void add_trailer_item(struct trailer_item **first,
- struct trailer_item **last,
- struct trailer_item *new)
+static void add_trailer_item(struct list_head *head, struct trailer_item *new)
{
if (!new)
return;
- if (!*last) {
- *first = new;
- *last = new;
- } else {
- (*last)->next = new;
- new->previous = *last;
- *last = new;
- }
+ list_add_tail(&new->list, head);
}
-static struct trailer_item *process_command_line_args(struct string_list *trailers)
+static void process_command_line_args(struct list_head *arg_head,
+ struct string_list *trailers)
{
- struct trailer_item *arg_tok_first = NULL;
- struct trailer_item *arg_tok_last = NULL;
struct string_list_item *tr;
struct trailer_item *item;
+ struct list_head *pos;
/* Add a trailer item for each configured trailer with a command */
- for (item = first_conf_item; item; item = item->next) {
+ list_for_each(pos, &conf_head) {
+ item = list_entry(pos, struct trailer_item, list);
if (item->conf.command) {
struct trailer_item *new = new_trailer_item(item, NULL, NULL);
- add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+ add_trailer_item(arg_head, new);
}
}
/* Add a trailer item for each trailer on the command line */
for_each_string_list_item(tr, trailers) {
struct trailer_item *new = create_trailer_item(tr->string);
- add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+ add_trailer_item(arg_head, new);
}
-
- return arg_tok_first;
}
static struct strbuf **read_input_file(const char *file)
@@ -805,8 +730,7 @@ static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end
static int process_input_file(FILE *outfile,
struct strbuf **lines,
- struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last)
+ struct list_head *head)
{
int count = 0;
int patch_start, trailer_start, trailer_end, i;
@@ -829,18 +753,19 @@ static int process_input_file(FILE *outfile,
for (i = trailer_start; i < trailer_end; i++) {
if (lines[i]->buf[0] != comment_line_char) {
struct trailer_item *new = create_trailer_item(lines[i]->buf);
- add_trailer_item(in_tok_first, in_tok_last, new);
+ add_trailer_item(head, new);
}
}
return trailer_end;
}
-static void free_all(struct trailer_item **first)
+static void free_all(struct list_head *head)
{
- while (*first) {
- struct trailer_item *item = remove_first(first);
- free_trailer_item(item);
+ struct list_head *pos, *p;
+ list_for_each_safe(pos, p, head) {
+ list_del(pos);
+ free_trailer_item(list_entry(pos, struct trailer_item, list));
}
}
@@ -877,9 +802,8 @@ static FILE *create_in_place_tempfile(const char *file)
void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)
{
- struct trailer_item *in_tok_first = NULL;
- struct trailer_item *in_tok_last = NULL;
- struct trailer_item *arg_tok_first;
+ LIST_HEAD(head);
+ LIST_HEAD(arg_head);
struct strbuf **lines;
int trailer_end;
FILE *outfile = stdout;
@@ -894,15 +818,15 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
outfile = create_in_place_tempfile(file);
/* Print the lines before the trailers */
- trailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);
+ trailer_end = process_input_file(outfile, lines, &head);
- arg_tok_first = process_command_line_args(trailers);
+ process_command_line_args(&arg_head, trailers);
- process_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);
+ process_trailers_lists(&head, &arg_head);
- print_all(outfile, in_tok_first, trim_empty);
+ print_all(outfile, &head, trim_empty);
- free_all(&in_tok_first);
+ free_all(&head);
/* Print the lines after the trailers as is */
print_lines(outfile, lines, trailer_end, INT_MAX);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 1/6] trailer: improve const correctness
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Change "const char *" to "char *" in struct trailer_item and in the
return value of apply_command (since those strings are owned strings).
Change "struct conf_info *" to "const struct conf_info *" (since that
struct is not modified).
---
trailer.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/trailer.c b/trailer.c
index c6ea9ac..1f191b2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -27,8 +27,8 @@ static struct conf_info default_conf_info;
struct trailer_item {
struct trailer_item *previous;
struct trailer_item *next;
- const char *token;
- const char *value;
+ char *token;
+ char *value;
struct conf_info conf;
};
@@ -95,8 +95,8 @@ static void free_trailer_item(struct trailer_item *item)
free(item->conf.name);
free(item->conf.key);
free(item->conf.command);
- free((char *)item->token);
- free((char *)item->value);
+ free(item->token);
+ free(item->value);
free(item);
}
@@ -215,13 +215,13 @@ static struct trailer_item *remove_first(struct trailer_item **first)
return item;
}
-static const char *apply_command(const char *command, const char *arg)
+static char *apply_command(const char *command, const char *arg)
{
struct strbuf cmd = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
struct child_process cp = CHILD_PROCESS_INIT;
const char *argv[] = {NULL, NULL};
- const char *result;
+ char *result;
strbuf_addstr(&cmd, command);
if (arg)
@@ -425,7 +425,7 @@ static int set_if_missing(struct conf_info *item, const char *value)
return 0;
}
-static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
+static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
{
*dst = *src;
if (src->name)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 0/6] allow non-trailers and multiple-line trailers
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476232683.git.jonathantanmy@google.com>
Thanks, Peff, for the pointer to list.h. Using list.h does simplify the
code by a similar amount to switching it to a singly-linked list, so I
have done that (replacing my earlier "trailer: use singly-linked list,
not doubly" patch). Another advantage is that I no longer need to change
the algorithm, making for a smaller patch.
(There are some quirks resulting from list.h implementing a circular
list, like needing to pass "head" as a sentinel when iterating from the
middle of the list, but those are minor, and my original singly-linked
list implementation had quirks too anyway like needing to pass a pointer
to the next pointer.)
Updates:
(-> 1/6)
- Added separate patch for const correctness changes
(1/5 -> 2/6)
- Dropped singly-linked list patch, instead replacing existing
doubly-linked list implementation with list.h
(5/5 -> 6/6)
- Used "char *" instead of "struct strbuf"
- Modified test slightly to test whitespace at beginning of line
Jonathan Tan (6):
trailer: improve const correctness
trailer: use list.h for doubly-linked list
trailer: streamline trailer item create and add
trailer: make args have their own struct
trailer: allow non-trailers in trailer block
trailer: support values folded to multiple lines
Documentation/git-interpret-trailers.txt | 10 +-
t/t7513-interpret-trailers.sh | 174 ++++++++++
trailer.c | 538 +++++++++++++++----------------
3 files changed, 444 insertions(+), 278 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* Re: [PATCHv3] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 23:33 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, git, j6t, jacob.keller
In-Reply-To: <20161012224109.23410-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> @@ -89,15 +114,20 @@ static void setup_check(void)
>
> ------------
> const char *path;
> + struct git_attr_result *result;
>
> setup_check();
> - git_check_attr(path, check);
> + result = git_check_attr(path, check);
This looks stale by a few revisions of the other parts of the patch?
> diff --git a/archive.c b/archive.c
> index 11e3951..15849a8 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -107,10 +107,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
> void *context)
> {
> static struct strbuf path = STRBUF_INIT;
> + static struct git_attr_check *check;
> +
> struct archiver_context *c = context;
> struct archiver_args *args = c->args;
> write_archive_entry_fn_t write_entry = c->write_entry;
> - static struct git_attr_check *check;
> + struct git_attr_result result = GIT_ATTR_RESULT_INIT;
> const char *path_without_prefix;
> int err;
>
> @@ -124,12 +126,16 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
> strbuf_addch(&path, '/');
> path_without_prefix = path.buf + args->baselen;
>
> - if (!check)
> - check = git_attr_check_initl("export-ignore", "export-subst", NULL);
> - if (!git_check_attr(path_without_prefix, check)) {
> - if (ATTR_TRUE(check->check[0].value))
> + git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
> + git_attr_result_init(&result, check);
> +
> + if (!git_check_attr(path_without_prefix, check, &result)) {
> + if (ATTR_TRUE(result.value[0])) {
> + git_attr_result_clear(&result);
> return 0;
> - args->convert = ATTR_TRUE(check->check[1].value);
> + }
> + args->convert = ATTR_TRUE(result.value[1]);
> + git_attr_result_clear(&result);
> }
This is exactly what I meant by "can we avoid alloc/free of result
in leaf function when we _know_ how many attributes we are
interested in already, which is the majority of the case?".
Starting with a simple but unoptimized internal implementation of
the attr subsystem is one thing (which is good). Exposing an API that
cannot be optimally implemented later without changing the callers
is another (which is bad).
By encapsulating each element into "struct git_attr_result", we can
extend the API without changing the API user [*1*].
But I do not think of a way to allow an efficient implementation
later unless the source of the API user somehow says "this user is
only interested in this many attributes", like having this
struct git_attr_result result[2];
(because this caller only wants "ignore" and "subst") on the API
user's side [*2*]. Without such a clue (like the patch above, that
only says "there is a structure called 'result'"), I do not think of
a way to avoid dynamic allocation on the API implementation side.
All the other callers in the patch (pack-objects, convert, ll-merge,
etc.) seem to share the exact same pattern. Each of the leaf
functions knows a fixed set of attributes it is interested in, the
caller iterates over many paths and makes calls to these leaf
functions, and it is a waste to pay alloc/free overhead for each and
every iteration when we know how many elements result needs to
store.
[Footnote]
*1* Would we need a wrapping struct around the array of results? If
that is the case, we may need something ugly like this on the
API user side:
GIT_ATTR_RESULT_TYPE(2) result = {2,};
with something like the following on the API implementation
side:
#define GIT_ATTR_RESULT_TYPE(n) \
struct { \
int num_slots; \
const char *value[n]; \
}
struct git_attr_result {
int num_slots;
const char *value[FLEX_ARRAY];
};
git_attr_result_init(void *result_, struct git_attr_check *check)
{
struct git_attr_result *result = result_;
assert(result->num_slots, check->num_attrs);
...
}
git_check_attr(const char *path,
struct git_attr_check *check,
void *result_)
{
struct git_attr_result *result = result_;
assert(result->num_slots, check->num_attrs);
for (i = 0; i < check_num_attrs; i++)
result->value[i] = ... found value ...;
}
*2* Or the uglier
GIT_ATTR_RESULT_TYPE(2) result = {2,};
I can see why the "check" side would benefit from a structure
that contains an array, but I do not see why "result" side would
want to, so I am hoping that we won't have to do this uglier
variant and just go with the simple "array of resulting values".
^ permalink raw reply
* Re: [PATCH] worktree: allow the main brach of a bare repository to be checked out
From: Dennis Kaarsemaker @ 2016-10-12 19:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, pclouds, mtutty, rappazzo
In-Reply-To: <xmqqfuo14dnr.fsf@gitster.mtv.corp.google.com>
On Wed, 2016-10-12 at 11:37 -0700, Junio C Hamano wrote:
> > ++test_expect_success '"add" default branch of a bare repo' '
>
> Huh?
Copy paste error. And I missed
ok 17 - checkout from a bare repo without "add"
./t2025-worktree-add.sh: 141: ./t2025-worktree-add.sh: +test_expect_success: not found
in the output of 'make test'. Thanks for fixing up!
D.
^ permalink raw reply
* Re: Huge performance bottleneck reading packs
From: Jeff King @ 2016-10-12 23:18 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <20161012230143.5kxcmtityaasra5j@sigill.intra.peff.net>
On Wed, Oct 12, 2016 at 07:01:43PM -0400, Jeff King wrote:
> On Thu, Oct 13, 2016 at 12:30:52AM +0200, Vegard Nossum wrote:
>
> > However, the commit found by 'git blame' above appears just fine to me,
> > I haven't been able to spot a bug in it.
> >
> > A closer inspection reveals the problem to really be that this is an
> > extremely hot path with more than -- holy cow -- 4,106,756,451
> > iterations on the 'packed_git' list for a single 'git fetch' on my
> > repository. I'm guessing the patch above just made the inner loop
> > ever so slightly slower.
> >
> > My .git/objects/pack/ has ~2088 files (1042 idx files, 1042 pack files,
> > and 4 tmp_pack_* files).
>
> Yeah. I agree that the commit you found makes the check a little more
> expensive, but I think the root of the problem is calling
> prepare_packed_git_one many times. This _should_ happen once for each
> pack at program startup, and possibly again if we need to re-scan the
> pack directory to account for racing with a simultaneous repack.
>
> The latter is generally triggered when we fail to look up an object we
> expect to exist. So I'd suspect 45e8a74 (has_sha1_file: re-check pack
> directory before giving up, 2013-08-30) is playing a part. We dealt with
> that to some degree in 0eeb077 (index-pack: avoid excessive re-reading
> of pack directory, 2015-06-09), but it would not surprise me if there is
> another spot that needs similar treatment.
>
> Does the patch below help?
Also, is it possible to make the repository in question available? I
might be able to reproduce based on your description, but it would save
time if I could directly run gdb on your example.
Specifically, I'm interested in the call graph that leads to calling
reprepare_packed_git().
-Peff
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Jeff King @ 2016-10-12 23:13 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Larry Finger, Mathieu Lienard--Mayor, Remi Lespinet,
git
In-Reply-To: <xmqqtwch2srj.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 12, 2016 at 01:53:52PM -0700, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
> >>> If it's not in the body of the message, then where is it?
> >>
> >> This point is clarified in the thread
> >> http://marc.info/?l=linux-wireless&m=147625930203434&w=2, which is
> >> with my upstream maintainer.
> >
> > Which explicitly states that the syntax is not [$number], but # $number,
> > right?
>
> But I do not think that works, either. Let's step back.
>
> People write things like these
>
> Cc: Stable <stable@vger.kernel.org> # 4.8
> Cc: Stable <stable@vger.kernel.org> [4.8+]
>
> in the trailer part in the body of the message. Are these lines
> meant to be usable if they appear as Cc: headers of an outgoing
> piece of e-mail as-is?
I think the answer is pretty clearly no. It's just that historically we
have auto-munged it into something useful. I think the viable options
are basically:
1. Tell people not to do that, and to do something RFC compliant like
"Stable [4.8+]" <stable@vger.kernel.org>. This is a little funny
for git because we otherwise do not require things like
rfc-compliant quoting for our name/email pairs. But it Just Works
without anybody having to write extra code, or worry about corner
cases in parsing.
2. Drop everything after the trailing ">". This gives a valid rfc2822
cc, and people can pick the "# 4.8" from the cc line in the body.
3. Rewrite
A <B@C> D
into
A D <B@C>
regardless of what is in "D". This retains the information in the
rfc2822 cc.
Starting from scratch, I'd say that (2) seems like a good combination of
simplicity and friendliness. But (3) matches what we have done
historically (and still do at least for some values of "D", and
depending on the presence of Mail::Address).
Once we decide on a behavior, it seems like we should be able to apply
it consistently with or without Mail::Address by grabbing the bits after
the final ">".
Larry seems to be against (2), but I'm not sure I understand why pulling
the value from the in-body cc (which gets copied into the commit message
by git-am, too) would be a problem.
-Peff
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 22:57 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kaBhXHLDEK0XMLjm3QofmtaGZspA3EEx5x4-qCYY--wZA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> But many callers do not follow that; rather they do
>>
>> loop to iterate over paths {
>> call a helper func to learn attr X for path
>> use the value of attr X
>> }
>>
>> using a callchain that embeds a helper function deep inside, and
>> "check" is kept in the helper, check-attr function is called from
>> there, and "result" is not passed from the caller to the helper
>> (obviously, because it does not exist in the current API). See the
>> callchain that leads down to convert.c::convert_attrs() for a
>> typical example. When converted to the new API, it needs to have a
>> new "result" structure every time it is called, and cannot reuse the
>> one that was used in its previous call.
>
> Why would that be? i.e. I do not understand the reasoning/motivation
> as well as what you propose to change here.
The leaf function may look like
check_eol(const char *path)
{
static struct git_attr_check *check;
initl(&check, "eol", NULL);
git_check_attr(&check, path, result);
return nth_element_in_result(result, 0);
}
and we want "result" to be thread-ready.
A naive and undesired way to put it on stack is like so:
const char *check_eol(const char *path)
{
static struct git_attr_check *check;
struct git_attr_result result = RESULT_INIT;
const char *eol;
initl(&check, "eol", NULL);
init_result(&check, &result);
git_check_attr(&check, path, &result);
eol = nth_element_in_result(&result, 0);
clear_result(&result);
return eol;
}
where your "struct git_attr_result" has a fixed size, and the actual
result array is allocated via ALLOC_GROW() etc. That's overhead
that we do not want. Instead can't we do this?
const char *check_eol(const char *path)
{
static struct git_attr_check *check;
/* we know we only want one */
struct git_attr_result result[1];
const char *eol;
initl(&check, "eol", NULL);
git_check_attr(&check, path, result);
return result[0];
}
That way, we won't be doing ALLOC_GROW() in init_result() or free in
clear_result().
If you use a structure that has pointer to an array (i.e. the "naive
and undesired way" above), you cannot amortize the alloc/free by
making result "static" if you want to be thread-ready.
^ permalink raw reply
* Re: Huge performance bottleneck reading packs
From: Jeff King @ 2016-10-12 23:01 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <ea8db41f-2ea4-b37b-e6f8-1f1d428aea5d@oracle.com>
On Thu, Oct 13, 2016 at 12:30:52AM +0200, Vegard Nossum wrote:
> However, the commit found by 'git blame' above appears just fine to me,
> I haven't been able to spot a bug in it.
>
> A closer inspection reveals the problem to really be that this is an
> extremely hot path with more than -- holy cow -- 4,106,756,451
> iterations on the 'packed_git' list for a single 'git fetch' on my
> repository. I'm guessing the patch above just made the inner loop
> ever so slightly slower.
>
> My .git/objects/pack/ has ~2088 files (1042 idx files, 1042 pack files,
> and 4 tmp_pack_* files).
Yeah. I agree that the commit you found makes the check a little more
expensive, but I think the root of the problem is calling
prepare_packed_git_one many times. This _should_ happen once for each
pack at program startup, and possibly again if we need to re-scan the
pack directory to account for racing with a simultaneous repack.
The latter is generally triggered when we fail to look up an object we
expect to exist. So I'd suspect 45e8a74 (has_sha1_file: re-check pack
directory before giving up, 2013-08-30) is playing a part. We dealt with
that to some degree in 0eeb077 (index-pack: avoid excessive re-reading
of pack directory, 2015-06-09), but it would not surprise me if there is
another spot that needs similar treatment.
Does the patch below help?
diff --git a/builtin/fetch.c b/builtin/fetch.c
index d5329f9..c0f3c2c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -243,7 +243,7 @@ static void find_non_local_tags(struct transport *transport,
if (ends_with(ref->name, "^{}")) {
if (item && !has_object_file(&ref->old_oid) &&
!will_fetch(head, ref->old_oid.hash) &&
- !has_sha1_file(item->util) &&
+ !has_sha1_file_with_flags(item->util, HAS_SHA1_QUICK) &&
!will_fetch(head, item->util))
item->util = NULL;
item = NULL;
@@ -256,7 +256,8 @@ static void find_non_local_tags(struct transport *transport,
* to check if it is a lightweight tag that we want to
* fetch.
*/
- if (item && !has_sha1_file(item->util) &&
+ if (item &&
+ !has_sha1_file_with_flags(item->util, HAS_SHA1_QUICK) &&
!will_fetch(head, item->util))
item->util = NULL;
@@ -276,7 +277,8 @@ static void find_non_local_tags(struct transport *transport,
* We may have a final lightweight tag that needs to be
* checked to see if it needs fetching.
*/
- if (item && !has_sha1_file(item->util) &&
+ if (item &&
+ !has_sha1_file_with_flags(item->util, HAS_SHA1_QUICK) &&
!will_fetch(head, item->util))
item->util = NULL;
-Peff
^ permalink raw reply related
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-12 21:47 UTC (permalink / raw)
To: Jacob Keller
Cc: Johannes Sixt, Junio C Hamano, Git mailing list, Brandon Williams
In-Reply-To: <CA+P7+xqBUT3jUsxciVydO+nRoR+iJygWG=y_ARpiQSs+-kcH2A@mail.gmail.com>
On Wed, Oct 12, 2016 at 2:45 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Wed, Oct 12, 2016 at 1:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>
>> Sigh. DCLP, the Double Checked Locking Pattern. These days, it should be
>> common knowledge among professionals that this naïve version _does_not_work_
>> [1]!
>>
>> I suggest you go without it, then measure, and only *then* optimize if it is
>> a bottleneck. Did I read "we do not expect much contention" somewhere?
>>
>> [1] http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf C++ centric,
>> but applies to C just as well
>>
>> -- Hannes
>>
>
>
> You know, I always wondered why Linux Kernel code needed memory
> barriers but userspace programs didn't seem to use them.. turns out
> they actually *do* need them for the same exact types of problems...
>
> Thanks,
> Jake
In a former job I made use of them, too. So I am kinda embarrassed.
(I cannot claim I did not know about these patterns and memory
fencing, it just escaped my consciousness).
^ permalink raw reply
* Re: Huge performance bottleneck reading packs
From: Junio C Hamano @ 2016-10-12 22:45 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce, Jeff King,
Nguyễn Thái Ngọc Duy
In-Reply-To: <ea8db41f-2ea4-b37b-e6f8-1f1d428aea5d@oracle.com>
Vegard Nossum <vegard.nossum@oracle.com> writes:
> A closer inspection reveals the problem to really be that this is an
> extremely hot path with more than -- holy cow -- 4,106,756,451
> iterations on the 'packed_git' list for a single 'git fetch' on my
> repository. I'm guessing the patch above just made the inner loop
> ever so slightly slower.
Very plausible, and this ...
> My .git/objects/pack/ has ~2088 files (1042 idx files, 1042 pack files,
> and 4 tmp_pack_* files).
... may explain why nobody else has seen a difference.
Is there a reason why your repository has that many pack files? Is
automatic GC not working for some reason?
"gc" would try to make sure that you have reasonably low number of
packs, as having too many packs is detrimental for performance for
multiple reasons, including:
* All objects in a single pack expressed in delta format (i.e. only
the difference from another object is stored) must eventually
have another object that its difference is based on recorded in
the full format in the same packfile.
* A single packfile records a single object only once, but it is
normal (and often required because of the point above) that the
same object appears in multiple packfiles.
* Locating of objects from a single packfile uses its .idx file by
binary search of sorted list of object names, which is efficient,
but this cost is multiplied linearly as the number of packs you
have in your repository.
^ permalink raw reply
* [PATCHv3] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-12 22:41 UTC (permalink / raw)
To: gitster, bmwill; +Cc: git, j6t, jacob.keller, Stefan Beller
This revamps the API of the attr subsystem to be thread safe.
Before we had the question and its results in one struct type.
The typical usage of the API was
static struct git_attr_check *check;
if (!check)
check = git_attr_check_initl("text", NULL);
git_check_attr(path, check);
act_on(check->value[0]);
This has a couple of issues when it comes to thread safety:
* the initialization is racy in this implementation. To make it
thread safe, we need to acquire a mutex, such that only one
thread is executing the code in git_attr_check_initl.
As we do not want to introduce a mutex at each call site,
this is best done in the attr code. However to do so, we need
to have access to the `check` variable, i.e. the API has to
look like
git_attr_check_initl(struct git_attr_check*, ...);
Then one of the threads calling git_attr_check_initl will
acquire the mutex and init the `check`, while all other threads
will wait on the mutex just to realize they're late to the
party and they'll return with no work done.
* While the check for attributes to be questioned only need to
be initalized once as that part will be read only after its
initialisation, the answer may be different for each path.
Because of that we need to decouple the check and the answer,
such that each thread can obtain an answer for the path it
is currently processing.
This commit changes the API and adds locking in
git_attr_check_initl that provides the thread safety.
The usage of the new API will be:
/*
* The initl call will thread-safely check whether the
* struct git_attr_check has been initialized. We only
* want to do the initialization work once, hence we do
* that work inside a thread safe environment.
*/
static struct git_attr_check *check;
git_attr_check_initl(&check, "text", NULL);
/*
* Resize internal data structures in the result to match
* the size of `check`:
*/
struct git_attr_result *result = GIT_ATTR_RESULT_INIT;
git_attr_result_init(&result, check);
/* Perform the check and act on it: */
git_check_attr(path, check, &result);
act_on(check->value[0]);
/*
* Free the result.
* The check is static, so doesn't require free'ing
*/
git_attr_result_clear(&result);
Signed-off-by: Stefan Beller <sbeller@google.com>
---
* Do not use the Double Check Locking Pattern
* The callers are all thread safe now (i.e. no static result)
* So this is roughly what we want for the first version of the conversion?
Thanks,
Stefan
Documentation/technical/api-gitattributes.txt | 93 +++++++++++++------
archive.c | 18 ++--
attr.c | 125 +++++++++++++++++---------
attr.h | 80 +++++++++++------
builtin/check-attr.c | 31 ++++---
builtin/pack-objects.c | 20 +++--
convert.c | 43 +++++----
ll-merge.c | 28 +++---
userdiff.c | 24 +++--
ws.c | 17 ++--
10 files changed, 314 insertions(+), 165 deletions(-)
diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
index 92fc32a..ac97244 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -8,6 +8,18 @@ attributes to set of paths.
Data Structure
--------------
+extern struct git_attr *git_attr(const char *);
+
+/*
+ * Return the name of the attribute represented by the argument. The
+ * return value is a pointer to a null-delimited string that is part
+ * of the internal data structure; it should not be modified or freed.
+ */
+extern const char *git_attr_name(const struct git_attr *);
+
+extern int attr_name_valid(const char *name, size_t namelen);
+extern void invalid_attr_name_message(struct strbuf *, const char *, int);
+
`struct git_attr`::
An attribute is an opaque object that is identified by its name.
@@ -16,15 +28,17 @@ Data Structure
of no interest to the calling programs. The name of the
attribute can be retrieved by calling `git_attr_name()`.
-`struct git_attr_check_elem`::
-
- This structure represents one attribute and its value.
-
`struct git_attr_check`::
- This structure represents a collection of `git_attr_check_elem`.
+ This structure represents a collection of `struct git_attrs`.
It is passed to `git_check_attr()` function, specifying the
- attributes to check, and receives their values.
+ attributes to check, and receives their values into a corresponding
+ `struct git_attr_result`.
+
+`struct git_attr_result`::
+
+ This structure represents a collection of results to its
+ corresponding `struct git_attr_check`, that has the same order.
Attribute Values
@@ -53,19 +67,30 @@ value of the attribute for the path.
Querying Specific Attributes
----------------------------
-* Prepare `struct git_attr_check` using git_attr_check_initl()
+* Prepare a `struct git_attr_check` using `git_attr_check_initl()`
function, enumerating the names of attributes whose values you are
interested in, terminated with a NULL pointer. Alternatively, an
- empty `struct git_attr_check` can be prepared by calling
- `git_attr_check_alloc()` function and then attributes you want to
- ask about can be added to it with `git_attr_check_append()`
- function.
-
-* Call `git_check_attr()` to check the attributes for the path.
-
-* Inspect `git_attr_check` structure to see how each of the
- attribute in the array is defined for the path.
-
+ empty `struct git_attr_check` as allocated by git_attr_check_alloc()
+ can be prepared by calling `git_attr_check_alloc()` function and
+ then attributes you want to ask about can be added to it with
+ `git_attr_check_append()` function.
+ `git_attr_check_initl()` is thread safe, i.e. you can call it
+ from different threads at the same time; when check determines
+ the initialzisation is still needed, the threads will use a
+ single global mutex to perform the initialization just once, the
+ others will wait on the the thread to actually perform the
+ initialization.
+
+* Prepare a `struct git_attr_result` using `git_attr_result_init()`
+ for the check struct. The call to initialize the result is not thread
+ safe, because different threads need their own thread local result
+ anyway.
+
+* Call `git_check_attr()` to check the attributes for the path,
+ the returned `git_attr_result` contains the result.
+
+* Inspect the returned `git_attr_result` structure to see how
+ each of the attribute in the array is defined for the path.
Example
-------
@@ -89,15 +114,20 @@ static void setup_check(void)
------------
const char *path;
+ struct git_attr_result *result;
setup_check();
- git_check_attr(path, check);
+ result = git_check_attr(path, check);
------------
-. Act on `.value` member of the result, left in `check->check[]`:
+The `result` must not be free'd as it is owned by the attr subsystem.
+It is reused by the same thread, so a subsequent call to git_check_attr
+in the same thread will overwrite the result.
+
+. Act on `result->value[]`:
------------
- const char *value = check->check[0].value;
+ const char *value = result->value[0];
if (ATTR_TRUE(value)) {
The attribute is Set, by listing only the name of the
@@ -123,6 +153,8 @@ the first step in the above would be different.
static struct git_attr_check *check;
static void setup_check(const char **argv)
{
+ if (check)
+ return; /* already done */
check = git_attr_check_alloc();
while (*argv) {
struct git_attr *attr = git_attr(*argv);
@@ -138,17 +170,20 @@ Querying All Attributes
To get the values of all attributes associated with a file:
-* Prepare an empty `git_attr_check` structure by calling
- `git_attr_check_alloc()`.
+* Setup a local variables on the stack for both the question
+ `struct git_attr_check` as well as the result `struct git_attr_result`.
+ Zero them out via their respective _INIT macro.
-* Call `git_all_attrs()`, which populates the `git_attr_check`
- with the attributes attached to the path.
+* Call `git_all_attrs()`
-* Iterate over the `git_attr_check.check[]` array to examine
- the attribute names and values. The name of the attribute
- described by a `git_attr_check.check[]` object can be retrieved via
- `git_attr_name(check->check[i].attr)`. (Please note that no items
+* Iterate over the `git_attr_check.attr[]` array to examine the
+ attribute names. The name of the attribute described by a
+ `git_attr_check.attr[]` object can be retrieved via
+ `git_attr_name(check->attr[i])`. (Please note that no items
will be returned for unset attributes, so `ATTR_UNSET()` will return
false for all returned `git_array_check` objects.)
+ The respective value for an attribute can be found in the same
+ index position in of `git_attr_result`.
-* Free the `git_array_check` by calling `git_attr_check_free()`.
+* Clear the local variables by calling `git_attr_check_clear()` and
+ `git_attr_result_clear()`.
diff --git a/archive.c b/archive.c
index 11e3951..15849a8 100644
--- a/archive.c
+++ b/archive.c
@@ -107,10 +107,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
void *context)
{
static struct strbuf path = STRBUF_INIT;
+ static struct git_attr_check *check;
+
struct archiver_context *c = context;
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
- static struct git_attr_check *check;
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
const char *path_without_prefix;
int err;
@@ -124,12 +126,16 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
strbuf_addch(&path, '/');
path_without_prefix = path.buf + args->baselen;
- if (!check)
- check = git_attr_check_initl("export-ignore", "export-subst", NULL);
- if (!git_check_attr(path_without_prefix, check)) {
- if (ATTR_TRUE(check->check[0].value))
+ git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
+ git_attr_result_init(&result, check);
+
+ if (!git_check_attr(path_without_prefix, check, &result)) {
+ if (ATTR_TRUE(result.value[0])) {
+ git_attr_result_clear(&result);
return 0;
- args->convert = ATTR_TRUE(check->check[1].value);
+ }
+ args->convert = ATTR_TRUE(result.value[1]);
+ git_attr_result_clear(&result);
}
if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
diff --git a/attr.c b/attr.c
index 0faa69f..648c4f9 100644
--- a/attr.c
+++ b/attr.c
@@ -14,6 +14,7 @@
#include "dir.h"
#include "utf8.h"
#include "quote.h"
+#include "thread-utils.h"
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
@@ -46,6 +47,19 @@ struct git_attr {
static int attr_nr;
static struct git_attr *(git_attr_hash[HASHSIZE]);
+#ifndef NO_PTHREADS
+
+static pthread_mutex_t attr_mutex;
+#define attr_lock() pthread_mutex_lock(&attr_mutex)
+#define attr_unlock() pthread_mutex_unlock(&attr_mutex)
+
+#else
+
+#define attr_lock() (void)0
+#define attr_unlock() (void)0
+
+#endif /* NO_PTHREADS */
+
/*
* NEEDSWORK: maybe-real, maybe-macro are not property of
* an attribute, as it depends on what .gitattributes are
@@ -55,6 +69,16 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);
*/
static int cannot_trust_maybe_real;
+/*
+ * Send one or more git_attr_check to git_check_attrs(), and
+ * each 'value' member tells what its value is.
+ * Unset one is returned as NULL.
+ */
+struct git_attr_check_elem {
+ const struct git_attr *attr;
+ const char *value;
+};
+
/* NEEDSWORK: This will become per git_attr_check */
static struct git_attr_check_elem *check_all_attr;
@@ -781,7 +805,7 @@ static int macroexpand_one(int nr, int rem)
static int attr_check_is_dynamic(const struct git_attr_check *check)
{
- return (void *)(check->check) != (void *)(check + 1);
+ return (void *)(check->attr) != (void *)(check + 1);
}
static void empty_attr_check_elems(struct git_attr_check *check)
@@ -799,7 +823,8 @@ static void empty_attr_check_elems(struct git_attr_check *check)
* any and all attributes that are visible are collected in it.
*/
static void collect_some_attrs(const char *path, int pathlen,
- struct git_attr_check *check, int collect_all)
+ struct git_attr_check *check,
+ struct git_attr_result *result, int collect_all)
{
struct attr_stack *stk;
@@ -825,13 +850,11 @@ static void collect_some_attrs(const char *path, int pathlen,
check_all_attr[i].value = ATTR__UNKNOWN;
if (!collect_all && !cannot_trust_maybe_real) {
- struct git_attr_check_elem *celem = check->check;
-
rem = 0;
for (i = 0; i < check->check_nr; i++) {
- if (!celem[i].attr->maybe_real) {
+ if (!check->attr[i]->maybe_real) {
struct git_attr_check_elem *c;
- c = check_all_attr + celem[i].attr->attr_nr;
+ c = check_all_attr + check->attr[i]->attr_nr;
c->value = ATTR__UNSET;
rem++;
}
@@ -845,38 +868,45 @@ static void collect_some_attrs(const char *path, int pathlen,
rem = fill(path, pathlen, basename_offset, stk, rem);
if (collect_all) {
- empty_attr_check_elems(check);
for (i = 0; i < attr_nr; i++) {
const struct git_attr *attr = check_all_attr[i].attr;
const char *value = check_all_attr[i].value;
if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
continue;
- git_attr_check_append(check, attr)->value = value;
+
+ git_attr_check_append(check, attr);
+
+ ALLOC_GROW(result->value,
+ result->check_nr + 1,
+ result->check_alloc);
+ result->value[result->check_nr++] = value;
}
}
}
static int git_check_attrs(const char *path, int pathlen,
- struct git_attr_check *check)
+ struct git_attr_check *check,
+ struct git_attr_result *result)
{
int i;
- struct git_attr_check_elem *celem = check->check;
- collect_some_attrs(path, pathlen, check, 0);
+ collect_some_attrs(path, pathlen, check, result, 0);
for (i = 0; i < check->check_nr; i++) {
- const char *value = check_all_attr[celem[i].attr->attr_nr].value;
+ const char *value = check_all_attr[check->attr[i]->attr_nr].value;
if (value == ATTR__UNKNOWN)
value = ATTR__UNSET;
- celem[i].value = value;
+ result->value[i] = value;
}
return 0;
}
-void git_all_attrs(const char *path, struct git_attr_check *check)
+void git_all_attrs(const char *path,
+ struct git_attr_check *check,
+ struct git_attr_result *result)
{
- collect_some_attrs(path, strlen(path), check, 1);
+ collect_some_attrs(path, strlen(path), check, result, 1);
}
void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
@@ -892,36 +922,42 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
use_index = istate;
}
-static int git_check_attr_counted(const char *path, int pathlen,
- struct git_attr_check *check)
+int git_check_attr(const char *path,
+ struct git_attr_check *check,
+ struct git_attr_result *result)
{
+ if (result->check_nr != check->check_nr)
+ die("BUG: git_attr_result is not prepared correctly");
check->finalized = 1;
- return git_check_attrs(path, pathlen, check);
+ return git_check_attrs(path, strlen(path), check, result);
}
-int git_check_attr(const char *path, struct git_attr_check *check)
+void git_attr_check_initl(struct git_attr_check **check_,
+ const char *one, ...)
{
- return git_check_attr_counted(path, strlen(path), check);
-}
-
-struct git_attr_check *git_attr_check_initl(const char *one, ...)
-{
- struct git_attr_check *check;
int cnt;
va_list params;
const char *param;
+ struct git_attr_check *check;
+
+ attr_lock();
+ if (*check_) {
+ attr_unlock();
+ return;
+ }
va_start(params, one);
for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
;
va_end(params);
+
check = xcalloc(1,
- sizeof(*check) + cnt * sizeof(*(check->check)));
+ sizeof(*check) + cnt * sizeof(*(check->attr)));
check->check_nr = cnt;
check->finalized = 1;
- check->check = (struct git_attr_check_elem *)(check + 1);
+ check->attr = (const struct git_attr **)(check + 1);
- check->check[0].attr = git_attr(one);
+ check->attr[0] = git_attr(one);
va_start(params, one);
for (cnt = 1; cnt < check->check_nr; cnt++) {
struct git_attr *attr;
@@ -932,10 +968,11 @@ struct git_attr_check *git_attr_check_initl(const char *one, ...)
attr = git_attr(param);
if (!attr)
die("BUG: %s: not a valid attribute name", param);
- check->check[cnt].attr = attr;
+ check->attr[cnt] = attr;
}
va_end(params);
- return check;
+ *check_ = check;
+ attr_unlock();
}
struct git_attr_check *git_attr_check_alloc(void)
@@ -943,18 +980,23 @@ struct git_attr_check *git_attr_check_alloc(void)
return xcalloc(1, sizeof(struct git_attr_check));
}
-struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *check,
- const struct git_attr *attr)
+void git_attr_result_init(struct git_attr_result *result,
+ struct git_attr_check *check)
+{
+ ALLOC_GROW(result->value, check->check_nr, result->check_alloc);
+ result->check_nr = check->check_nr;
+}
+
+
+void git_attr_check_append(struct git_attr_check *check,
+ const struct git_attr *attr)
{
- struct git_attr_check_elem *elem;
if (check->finalized)
die("BUG: append after git_attr_check structure is finalized");
if (!attr_check_is_dynamic(check))
die("BUG: appending to a statically initialized git_attr_check");
- ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);
- elem = &check->check[check->check_nr++];
- elem->attr = attr;
- return elem;
+ ALLOC_GROW(check->attr, check->check_nr + 1, check->check_alloc);
+ check->attr[check->check_nr++] = attr;
}
void git_attr_check_clear(struct git_attr_check *check)
@@ -962,12 +1004,13 @@ void git_attr_check_clear(struct git_attr_check *check)
empty_attr_check_elems(check);
if (!attr_check_is_dynamic(check))
die("BUG: clearing a statically initialized git_attr_check");
- free(check->check);
+ free(check->attr);
check->check_alloc = 0;
}
-void git_attr_check_free(struct git_attr_check *check)
+void git_attr_result_clear(struct git_attr_result *result)
{
- git_attr_check_clear(check);
- free(check);
+ free(result->value);
+ result->check_nr = 0;
+ result->check_alloc = 0;
}
diff --git a/attr.h b/attr.h
index 163fcd6..620f6f8 100644
--- a/attr.h
+++ b/attr.h
@@ -10,6 +10,13 @@ struct git_attr;
*/
extern struct git_attr *git_attr(const char *);
+/*
+ * Return the name of the attribute represented by the argument. The
+ * return value is a pointer to a null-delimited string that is part
+ * of the internal data structure; it should not be modified or freed.
+ */
+extern const char *git_attr_name(const struct git_attr *);
+
extern int attr_name_valid(const char *name, size_t namelen);
extern void invalid_attr_name_message(struct strbuf *, const char *, int);
@@ -22,44 +29,65 @@ extern const char git_attr__false[];
#define ATTR_FALSE(v) ((v) == git_attr__false)
#define ATTR_UNSET(v) ((v) == NULL)
-/*
- * Send one or more git_attr_check to git_check_attrs(), and
- * each 'value' member tells what its value is.
- * Unset one is returned as NULL.
- */
-struct git_attr_check_elem {
- const struct git_attr *attr;
- const char *value;
-};
-
struct git_attr_check {
int finalized;
int check_nr;
int check_alloc;
- struct git_attr_check_elem *check;
+ const struct git_attr **attr;
};
+#define GIT_ATTR_CHECK_INIT {0, 0, 0, NULL}
-extern struct git_attr_check *git_attr_check_initl(const char *, ...);
-extern int git_check_attr(const char *path, struct git_attr_check *);
+struct git_attr_result {
+ int check_nr;
+ int check_alloc;
+ const char **value;
+};
+#define GIT_ATTR_RESULT_INIT {0, 0, NULL}
+/*
+ * Initialize the `git_attr_check` via one of the following three functions:
+ *
+ * git_attr_check_alloc allocates an empty check,
+ * git_attr_check_append add an attribute to the given git_attr_check
+ * git_attr_result_init prepare the result struct for a given check struct
+ *
+ * git_all_attrs allocates a check and fills in all attributes that
+ * are set for the given path.
+ * git_attr_check_initl takes a pointer to where the check will be initialized,
+ * followed by all attributes that are to be checked.
+ * This makes it potentially thread safe as it could
+ * internally have a mutex for that memory location.
+ * Currently it is not thread safe!
+ */
extern struct git_attr_check *git_attr_check_alloc(void);
-extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *, const struct git_attr *);
+extern void git_attr_check_append(struct git_attr_check *,
+ const struct git_attr *);
+extern void git_attr_check_initl(struct git_attr_check **,
+ const char *, ...);
-extern void git_attr_check_clear(struct git_attr_check *);
-extern void git_attr_check_free(struct git_attr_check *);
+extern void git_attr_result_init(struct git_attr_result *,
+ struct git_attr_check *);
-/*
- * Return the name of the attribute represented by the argument. The
- * return value is a pointer to a null-delimited string that is part
- * of the internal data structure; it should not be modified or freed.
- */
-extern const char *git_attr_name(const struct git_attr *);
+extern void git_all_attrs(const char *path,
+ struct git_attr_check *,
+ struct git_attr_result *);
-/*
- * Retrieve all attributes that apply to the specified path.
- * check holds the attributes and their values.
+/* Query a path for its attributes */
+extern int git_check_attr(const char *path,
+ struct git_attr_check *,
+ struct git_attr_result *);
+
+
+
+/**
+ * Free or clear the result struct. The underlying strings are not free'd
+ * as they are globally known.
*/
-void git_all_attrs(const char *path, struct git_attr_check *check);
+extern void git_attr_result_free(struct git_attr_result *);
+extern void git_attr_result_clear(struct git_attr_result *);
+
+extern void git_attr_check_clear(struct git_attr_check *);
+
enum git_attr_direction {
GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index ec61476..5e78d5d 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -24,13 +24,17 @@ static const struct option check_attr_options[] = {
OPT_END()
};
-static void output_attr(struct git_attr_check *check, const char *file)
+static void output_attr(struct git_attr_check *check,
+ struct git_attr_result *result, const char *file)
{
int j;
int cnt = check->check_nr;
+ if (check->check_nr != result->check_nr)
+ die("BUG: confused check and result internally");
+
for (j = 0; j < cnt; j++) {
- const char *value = check->check[j].value;
+ const char *value = result->value[j];
if (ATTR_TRUE(value))
value = "set";
@@ -44,11 +48,11 @@ static void output_attr(struct git_attr_check *check, const char *file)
"%s%c" /* attrname */
"%s%c" /* attrvalue */,
file, 0,
- git_attr_name(check->check[j].attr), 0, value, 0);
+ git_attr_name(check->attr[j]), 0, value, 0);
} else {
quote_c_style(file, NULL, stdout, 0);
printf(": %s: %s\n",
- git_attr_name(check->check[j].attr), value);
+ git_attr_name(check->attr[j]), value);
}
}
}
@@ -59,16 +63,21 @@ static void check_attr(const char *prefix,
{
char *full_path =
prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+ struct git_attr_check local_check = GIT_ATTR_CHECK_INIT;
+
if (check != NULL) {
- if (git_check_attr(full_path, check))
- die("git_check_attr died");
- output_attr(check, file);
+ git_attr_result_init(&result, check);
+ git_check_attr(full_path, check, &result);
} else {
- check = git_attr_check_alloc();
- git_all_attrs(full_path, check);
- output_attr(check, file);
- git_attr_check_free(check);
+ git_all_attrs(full_path, &local_check, &result);
+ check = &local_check;
}
+
+ output_attr(check, &result, file);
+
+ git_attr_result_clear(&result);
+ git_attr_check_clear(&local_check);
free(full_path);
}
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3918c07..14b764a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -900,13 +900,19 @@ static int no_try_delta(const char *path)
{
static struct git_attr_check *check;
- if (!check)
- check = git_attr_check_initl("delta", NULL);
- if (git_check_attr(path, check))
- return 0;
- if (ATTR_FALSE(check->check[0].value))
- return 1;
- return 0;
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+ int ret = 0;
+
+ git_attr_check_initl(&check, "delta", NULL);
+ git_attr_result_init(&result, check);
+
+ if (!git_check_attr(path, check, &result)) {
+ if (ATTR_FALSE(result.value[0]))
+ ret = 1;
+ }
+
+ git_attr_result_clear(&result);
+ return ret;
}
/*
diff --git a/convert.c b/convert.c
index bb2435a..01de8ef 100644
--- a/convert.c
+++ b/convert.c
@@ -718,10 +718,8 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
return 1;
}
-static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)
+static enum crlf_action git_path_check_crlf(const char *value)
{
- const char *value = check->value;
-
if (ATTR_TRUE(value))
return CRLF_TEXT;
else if (ATTR_FALSE(value))
@@ -735,10 +733,8 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)
return CRLF_UNDEFINED;
}
-static enum eol git_path_check_eol(struct git_attr_check_elem *check)
+static enum eol git_path_check_eol(const char *value)
{
- const char *value = check->value;
-
if (ATTR_UNSET(value))
;
else if (!strcmp(value, "lf"))
@@ -748,9 +744,8 @@ static enum eol git_path_check_eol(struct git_attr_check_elem *check)
return EOL_UNSET;
}
-static struct convert_driver *git_path_check_convert(struct git_attr_check_elem *check)
+static struct convert_driver *git_path_check_convert(const char *value)
{
- const char *value = check->value;
struct convert_driver *drv;
if (ATTR_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value))
@@ -761,10 +756,8 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check_elem
return NULL;
}
-static int git_path_check_ident(struct git_attr_check_elem *check)
+static int git_path_check_ident(const char *value)
{
- const char *value = check->value;
-
return !!ATTR_TRUE(value);
}
@@ -778,25 +771,29 @@ struct conv_attrs {
static void convert_attrs(struct conv_attrs *ca, const char *path)
{
static struct git_attr_check *check;
+ static int init_user_convert_tail;
- if (!check) {
- check = git_attr_check_initl("crlf", "ident",
- "filter", "eol", "text",
- NULL);
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+
+ git_attr_check_initl(&check, "crlf", "ident", "filter",
+ "eol", "text", NULL);
+ git_attr_result_init(&result, check);
+
+ if (!init_user_convert_tail) {
user_convert_tail = &user_convert;
git_config(read_convert_config, NULL);
+ init_user_convert_tail = 1;
}
- if (!git_check_attr(path, check)) {
- struct git_attr_check_elem *ccheck = check->check;
- ca->crlf_action = git_path_check_crlf(ccheck + 4);
+ if (!git_check_attr(path, check, &result)) {
+ ca->crlf_action = git_path_check_crlf(result.value[4]);
if (ca->crlf_action == CRLF_UNDEFINED)
- ca->crlf_action = git_path_check_crlf(ccheck + 0);
+ ca->crlf_action = git_path_check_crlf(result.value[0]);
ca->attr_action = ca->crlf_action;
- ca->ident = git_path_check_ident(ccheck + 1);
- ca->drv = git_path_check_convert(ccheck + 2);
+ ca->ident = git_path_check_ident(result.value[1]);
+ ca->drv = git_path_check_convert(result.value[2]);
if (ca->crlf_action != CRLF_BINARY) {
- enum eol eol_attr = git_path_check_eol(ccheck + 3);
+ enum eol eol_attr = git_path_check_eol(result.value[3]);
if (ca->crlf_action == CRLF_AUTO && eol_attr == EOL_LF)
ca->crlf_action = CRLF_AUTO_INPUT;
else if (ca->crlf_action == CRLF_AUTO && eol_attr == EOL_CRLF)
@@ -820,6 +817,8 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
ca->crlf_action = CRLF_AUTO_CRLF;
if (ca->crlf_action == CRLF_UNDEFINED && auto_crlf == AUTO_CRLF_INPUT)
ca->crlf_action = CRLF_AUTO_INPUT;
+
+ git_attr_result_clear(&result);
}
int would_convert_to_git_filter_fd(const char *path)
diff --git a/ll-merge.c b/ll-merge.c
index bc6479c..3abc77a 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -355,6 +355,7 @@ int ll_merge(mmbuffer_t *result_buf,
{
static struct git_attr_check *check;
static const struct ll_merge_options default_opts;
+ static struct git_attr_result result;
const char *ll_driver_name = NULL;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
const struct ll_merge_driver *driver;
@@ -368,13 +369,15 @@ int ll_merge(mmbuffer_t *result_buf,
normalize_file(theirs, path);
}
- if (!check)
- check = git_attr_check_initl("merge", "conflict-marker-size", NULL);
+ if (!check) {
+ git_attr_check_initl(&check, "merge", "conflict-marker-size", NULL);
+ git_attr_result_init(&result, check);
+ }
- if (!git_check_attr(path, check)) {
- ll_driver_name = check->check[0].value;
- if (check->check[1].value) {
- marker_size = atoi(check->check[1].value);
+ if (!git_check_attr(path, check, &result)) {
+ ll_driver_name = result.value[0];
+ if (result.value[1]) {
+ marker_size = atoi(result.value[1]);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
}
@@ -394,14 +397,19 @@ int ll_merge(mmbuffer_t *result_buf,
int ll_merge_marker_size(const char *path)
{
static struct git_attr_check *check;
+
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
- if (!check)
- check = git_attr_check_initl("conflict-marker-size", NULL);
- if (!git_check_attr(path, check) && check->check[0].value) {
- marker_size = atoi(check->check[0].value);
+ git_attr_check_initl(&check, "conflict-marker-size", NULL);
+ git_attr_result_init(&result, check);
+
+ if (!git_check_attr(path, check, &result) && result.value[0]) {
+ marker_size = atoi(result.value[0]);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
}
+ git_attr_result_clear(&result);
+
return marker_size;
}
diff --git a/userdiff.c b/userdiff.c
index 46dfd32..62b1ed6 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -264,20 +264,30 @@ struct userdiff_driver *userdiff_find_by_path(const char *path)
{
static struct git_attr_check *check;
- if (!check)
- check = git_attr_check_initl("diff", NULL);
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+ const char *value;
+
if (!path)
return NULL;
- if (git_check_attr(path, check))
+
+ git_attr_check_initl(&check, "diff", NULL);
+ git_attr_result_init(&result, check);
+
+ if (git_check_attr(path, check, &result)) {
+ git_attr_result_clear(&result);
return NULL;
+ }
+
+ value = result.value[0];
+ git_attr_result_clear(&result);
- if (ATTR_TRUE(check->check[0].value))
+ if (ATTR_TRUE(value))
return &driver_true;
- if (ATTR_FALSE(check->check[0].value))
+ if (ATTR_FALSE(value))
return &driver_false;
- if (ATTR_UNSET(check->check[0].value))
+ if (ATTR_UNSET(value))
return NULL;
- return userdiff_find_by_name(check->check[0].value);
+ return userdiff_find_by_name(value);
}
struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver)
diff --git a/ws.c b/ws.c
index bb3270c..5e30fef 100644
--- a/ws.c
+++ b/ws.c
@@ -73,15 +73,20 @@ unsigned parse_whitespace_rule(const char *string)
unsigned whitespace_rule(const char *pathname)
{
- static struct git_attr_check *attr_whitespace_rule;
+ static struct git_attr_check *check;
- if (!attr_whitespace_rule)
- attr_whitespace_rule = git_attr_check_initl("whitespace", NULL);
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+ const char *value;
+ int r;
- if (!git_check_attr(pathname, attr_whitespace_rule)) {
- const char *value;
+ git_attr_check_initl(&check, "whitespace", NULL);
+ git_attr_result_init(&result, check);
- value = attr_whitespace_rule->check[0].value;
+ r = git_check_attr(pathname, check, &result);
+ value = result.value[0];
+ git_attr_result_clear(&result);
+
+ if (!r) {
if (ATTR_TRUE(value)) {
/* true (whitespace) */
unsigned all_rule = ws_tab_width(whitespace_rule_cfg);
--
2.10.1.432.g8a36cd8.dirty
^ permalink raw reply related
* Huge performance bottleneck reading packs
From: Vegard Nossum @ 2016-10-12 22:30 UTC (permalink / raw)
To: git
Cc: Quentin Casasnovas, Shawn Pearce, Jeff King,
Nguyễn Thái Ngọc Duy, Junio C Hamano
Hi all,
I've bisected a performance regression (noticed by Quentin and myself)
which caused a 'git fetch' to go from ~1m30s to ~2m40s:
commit 47bf4b0fc52f3ad5823185a85f5f82325787c84b
Author: Jeff King <peff@peff.net>
Date: Mon Jun 30 13:04:03 2014 -0400
prepare_packed_git_one: refactor duplicate-pack check
Reverting this commit from a recent mainline master brings the time back
down from ~2m24s to ~1m19s.
The bisect log:
v2.8.1 -- 2m41s, 2m50s (bad)
v1.9.0 -- 1m39s, 1m46s (good)
2.3.4.312.gea1fd48 -- 2m40s
2.1.0.18.gc285171 -- 2m42s
2.0.0.140.g6753d8a -- 1m27s
2.0.1.480.g60e2f5a -- 1m34s
2.0.2.631.gad25da0 -- 2m39s
2.0.1.565.ge0a064a -- 1m30s
2.0.1.622.g2e42338 -- 2m29s
2.0.0.rc1.32.g5165dd5 -- 1m30s
2.0.1.607.g5418212 -- 1m32s
2.0.1.7.g6dda4e6 -- 1m28s
2.0.1.619.g6e40947 -- 2m25s
2.0.1.9.g47bf4b0 -- 2m18s
2.0.1.8.gd6cd00c -- 1m36.542s
However, the commit found by 'git blame' above appears just fine to me,
I haven't been able to spot a bug in it.
A closer inspection reveals the problem to really be that this is an
extremely hot path with more than -- holy cow -- 4,106,756,451
iterations on the 'packed_git' list for a single 'git fetch' on my
repository. I'm guessing the patch above just made the inner loop
ever so slightly slower.
My .git/objects/pack/ has ~2088 files (1042 idx files, 1042 pack files,
and 4 tmp_pack_* files).
I am convinced that it is not necessary to rescan the entire pack
directory 11,348 times or do all 4 _BILLION_ memcmp() calls for a single
'git fetch', even for a large repository like mine.
I could try to write a patch to reduce the number of times we rescan the
pack directory. However, I've never even looked at the file before
today, so any hints regarding what would need to be done would be
appreciated.
Thanks,
(Cced some people with changes in the area.)
Vegard
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 21:50 UTC (permalink / raw)
To: Stefan Beller; +Cc: Johannes Sixt, git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kb-xKDNG-LC56i8Y-FAaYZwr8zzXuC9snj1PavnQ6cdCA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Wed, Oct 12, 2016 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Sigh. DCLP, the Double Checked Locking Pattern. ...
>>> I suggest you go without it, then measure, and only *then* optimize if
>>> it is a bottleneck.
>>
>> That comes from me in earlier discussion before the patch, namely in
>> <xmqqeg3m8y6y.fsf@gitster.mtv.corp.google.com>, where I wondered if
>> a cheap check outside the lock may be a possible optimization
>> opportunity, as this is a classic singleton that will not be
>> deinitialized, and once the codepath gets exercised, we would be
>> taking the "nothing to do" route 100% of the time.
>
> Having followed that advice (and internally having a DCLP), I think
> we have Triple Checked Locking Pattern in this patch. Nobody wrote
> a paper on how that would not work, yet. ;)
>
> In the reroll I plan to reduce it to a Single Checked (inside a mutex)
> Locking Pattern, though I would expect that performance (or lack thereof)
> will show then. But let's postpone measuring until we have a working patch.
Oh, that wasn't even an "advice" (read it again). I fully agree
that starting simple would be the way to go.
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 20:59 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kaBhXHLDEK0XMLjm3QofmtaGZspA3EEx5x4-qCYY--wZA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>>> Well all of the hunks in the patch are not threaded, so they
>>> don't follow a threading pattern, but the static pattern to not be
>>> more expensive than needed.
>>
>> Is it too invasive a change to make them as if they are thread-ready
>> users of API that happen to know their callers are not threading?
>> It would be ideal if we can prepare them so that the way they
>> interact with the attr subsystem will not have to change after this
>> step.
>
> As far as I see the future, we do not need to change those in the future,
> unless we add the threading to the current callers, which is usually a very
> invasive thing?
It does not matter how invasive the thread set-up and teardown that
happens in the callers.
I am talking about the part of _THIS_ code that you are updating,
that interacts with attr API. The way they prepare "check" and
"result", the way they ask questions by calling git_check_attr()
function.
Think of a thread-safe library function (like malloc()). If you
write
func (...) {
buf = malloc(20);
...
free(buf);
}
in a function that happens to be only called in a non-threaded
program today, you do not have to update these calls to malloc(3)
and free(3) when you update the callchain to threadable, right?
That kind of thread-preparedness is what I am trying to see if we
can achieve with this update.
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Jacob Keller @ 2016-10-12 21:45 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Stefan Beller, Junio C Hamano, Git mailing list, bmwill
In-Reply-To: <44c554b8-7ac1-047d-59f0-b4d5331ed496@kdbg.org>
On Wed, Oct 12, 2016 at 1:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>
> Sigh. DCLP, the Double Checked Locking Pattern. These days, it should be
> common knowledge among professionals that this naïve version _does_not_work_
> [1]!
>
> I suggest you go without it, then measure, and only *then* optimize if it is
> a bottleneck. Did I read "we do not expect much contention" somewhere?
>
> [1] http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf C++ centric,
> but applies to C just as well
>
> -- Hannes
>
You know, I always wondered why Linux Kernel code needed memory
barriers but userspace programs didn't seem to use them.. turns out
they actually *do* need them for the same exact types of problems...
Thanks,
Jake
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 21:44 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kaQGs_3dhd-yEzaq=CRXGQhbMuN_7FdFY=FeKZ4scmMeQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Wed, Oct 12, 2016 at 2:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> This version adds the actual thread safety,
>>> that is promised in the documentation, however it doesn't add any optimization,
>>> i.e. it's a single global lock. But as we do not expect contention, that is fine.
>>
>> Because we have to start _somewhere_, I agree it is a good approach
>> to first try the simplest implementation and then optimize later,
>> but is it an agreed consensus that we do not expect contention?
>
> I agree on that. Did you mean this is obvious to the reader?
I meant to say that "But as we do not expect" sounded like a
justification for the approach based on an unwarranted assumption
that is not even the list concensus. I do not think it is obvious
to the reader that there is no need to worry about contention.
It all is outside the log message, so as long as readers understand
what we meant from this discussion, that is OK.
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 21:38 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Stefan Beller, git, bmwill
In-Reply-To: <44c554b8-7ac1-047d-59f0-b4d5331ed496@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Sigh. DCLP, the Double Checked Locking Pattern. ...
> I suggest you go without it, then measure, and only *then* optimize if
> it is a bottleneck.
That comes from me in earlier discussion before the patch, namely in
<xmqqeg3m8y6y.fsf@gitster.mtv.corp.google.com>, where I wondered if
a cheap check outside the lock may be a possible optimization
opportunity, as this is a classic singleton that will not be
deinitialized, and once the codepath gets exercised, we would be
taking the "nothing to do" route 100% of the time.
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-12 21:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git@vger.kernel.org, Brandon Williams
In-Reply-To: <xmqq8ttt2qpp.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 12, 2016 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Sigh. DCLP, the Double Checked Locking Pattern. ...
>> I suggest you go without it, then measure, and only *then* optimize if
>> it is a bottleneck.
>
> That comes from me in earlier discussion before the patch, namely in
> <xmqqeg3m8y6y.fsf@gitster.mtv.corp.google.com>, where I wondered if
> a cheap check outside the lock may be a possible optimization
> opportunity, as this is a classic singleton that will not be
> deinitialized, and once the codepath gets exercised, we would be
> taking the "nothing to do" route 100% of the time.
>
Having followed that advice (and internally having a DCLP), I think
we have Triple Checked Locking Pattern in this patch. Nobody wrote
a paper on how that would not work, yet. ;)
In the reroll I plan to reduce it to a Single Checked (inside a mutex)
Locking Pattern, though I would expect that performance (or lack thereof)
will show then. But let's postpone measuring until we have a working patch.
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-12 21:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <xmqqh98h2r5t.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 12, 2016 at 2:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> This version adds the actual thread safety,
>> that is promised in the documentation, however it doesn't add any optimization,
>> i.e. it's a single global lock. But as we do not expect contention, that is fine.
>
> Because we have to start _somewhere_, I agree it is a good approach
> to first try the simplest implementation and then optimize later,
> but is it an agreed consensus that we do not expect contention?
>
I agree on that. Did you mean this is obvious to the reader? I just wanted
to state how I'll do it, such that if in the future someone digs up
this code, they'll
know I did not look at performance in the first run.
^ permalink raw reply
* Re: [PATCH] merge-base: handle --fork-point without reflog
From: Junio C Hamano @ 2016-10-12 21:29 UTC (permalink / raw)
To: Jeff King; +Cc: Stepan Kasal, John Keeping, git
In-Reply-To: <20161012201040.pyrp6bktz3fgmqzn@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Subject: merge-base: handle --fork-point without reflog
>
> The --fork-point option looks in the reflog to try to find
> where a derived branch forked from a base branch. However,
> if the reflog for the base branch is totally empty (as it
> commonly is right after cloning, which does not write a
> reflog entry), then our for_each_reflog call will not find
> any entries, and we will come up with no merge base, even
> though there may be one with the current tip of the base.
>
> We can fix this by just adding the current tip to
> our list of collected entries.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> It would actually be correct to just unconditionally add the ref tip, as
> add_one_commit already drops duplicates. But it would only be necessary
> in other cases if you have a broken reflog which is missing the entry
> that moved us to the current tip.
Makes sense. And doing conditionally is not much more work ;-)
Thanks.
>
> builtin/merge-base.c | 3 +++
> t/t6010-merge-base.sh | 6 ++++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/builtin/merge-base.c b/builtin/merge-base.c
> index c0d1822..b572a37 100644
> --- a/builtin/merge-base.c
> +++ b/builtin/merge-base.c
> @@ -173,6 +173,9 @@ static int handle_fork_point(int argc, const char **argv)
> revs.initial = 1;
> for_each_reflog_ent(refname, collect_one_reflog_ent, &revs);
>
> + if (!revs.nr && !get_sha1(refname, sha1))
> + add_one_commit(sha1, &revs);
> +
> for (i = 0; i < revs.nr; i++)
> revs.commit[i]->object.flags &= ~TMP_MARK;
>
> diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
> index e0c5f44..31db7b5 100755
> --- a/t/t6010-merge-base.sh
> +++ b/t/t6010-merge-base.sh
> @@ -260,6 +260,12 @@ test_expect_success 'using reflog to find the fork point' '
> test_cmp expect3 actual
> '
>
> +test_expect_success '--fork-point works with empty reflog' '
> + git -c core.logallrefupdates=false branch no-reflog base &&
> + git merge-base --fork-point no-reflog derived &&
> + test_cmp expect3 actual
> +'
> +
> test_expect_success 'merge-base --octopus --all for complex tree' '
> # Best common ancestor for JE, JAA and JDD is JC
> # JE
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 21:28 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill
In-Reply-To: <20161011235951.8358-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> This version adds the actual thread safety,
> that is promised in the documentation, however it doesn't add any optimization,
> i.e. it's a single global lock. But as we do not expect contention, that is fine.
Because we have to start _somewhere_, I agree it is a good approach
to first try the simplest implementation and then optimize later,
but is it an agreed consensus that we do not expect contention?
^ permalink raw reply
* Re: [PATCH v3 25/25] sequencer: mark all error messages for translation
From: Junio C Hamano @ 2016-10-12 21:24 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Jakub Narębski
In-Reply-To: <d24a3823-1ed0-ad97-f02d-febab7a97590@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Can we please have the following change instead? I think it makes sense
> to deviate from the usual conventions in a case like this.
You have at least two independent changes relative to Dscho's
version.
(1) Show line breaks more prominently by avoiding "\n\n" and
breaking the string at "\n"; this matches the way how the
result would be displayed more closely to how the source looks
like.
(2) Ignore the usual indentation rule and have messages start at
the left end of the source.
Which one are you saying "makes sense" to? Both?
I guess both can be grouped together into one theme: match the way
the final output and the source code look like.
If that is the motivation behind "makes sense", I'd prefer to see
the change explained explicitly with that rationale in the log
message.
Thanks. I personally agree with that motivation (if the one I
guessed above is your motivation, that is).
> Note that this is an error() text, hence, there should not be a
> fullstop on the first line. That's now a good excuse to start the next
> sentence on a new line; hence, this is not a faithful conversion to _()
> anymore (a will happily take authorship and all blame if you don't
> want to for this reason). Also note that _( is not moved to the
> beginning of the line because it would be picked up as hunk header by
> git diff.
>
> ---- 8< ----
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Subject: [PATCH] sequencer: mark all error messages for translation
>
> There was actually only one error message that was not yet marked for
> translation.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> sequencer.c | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 95a382e..79f7aa4 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -515,16 +515,20 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> if (!env) {
> const char *gpg_opt = gpg_sign_opt_quoted(opts);
>
> - return error("you have staged changes in your working "
> - "tree. If these changes are meant to be\n"
> - "squashed into the previous commit, run:\n\n"
> - " git commit --amend %s\n\n"
> - "If they are meant to go into a new commit, "
> - "run:\n\n"
> - " git commit %s\n\n"
> - "In both cases, once you're done, continue "
> - "with:\n\n"
> - " git rebase --continue\n", gpg_opt, gpg_opt);
> + return error(_(
> +"you have staged changes in your working tree\n"
> +"If these changes are meant to be squashed into the previous commit, run:\n"
> +"\n"
> +" git commit --amend %s\n"
> +"\n"
> +"If they are meant to go into a new commit, run:\n"
> +"\n"
> +" git commit %s\n"
> +"\n"
> +"In both cases, once you're done, continue with:\n"
> +"\n"
> +" git rebase --continue\n"),
> + gpg_opt, gpg_opt);
> }
> }
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox