* [PATCH 4/5] trailer: allow non-trailers in trailer block
From: Jonathan Tan @ 2016-10-12 1:23 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, christian.couder, gitster
In-Reply-To: <cover.1476232683.git.jonathantanmy@google.com>
Currently, interpret-trailers requires all lines of a trailer block to
be trailers (or comments) - if not it would not identify that block as a
trailer block, and thus create its own trailer block, inserting a blank
line. For example:
echo -e "\na: b\nnot trailer" |
git interpret-trailers --trailer "c: d"
would result in:
a: b
not trailer
c: d
Relax the definition of a trailer block to only require 1 trailer, so
that trailers can be directly added to such blocks, resulting in:
a: b
not trailer
c: d
This allows arbitrary lines to be included in trailer blocks, like those
in [1], and still allow interpret-trailers to be used.
This change also makes comments in the trailer block be treated as any
other non-trailer line, preserving them in the output of
interpret-trailers.
[1]
https://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/e7d316a02f683864a12389f8808570e37fb90aa3
---
There are some possible inaccuracies in the master branch's
find_trailer_start (in particular, handling of lines that *start* with a
separator, which should not be considered a trailer line) - this patch
preserves the existing behavior.
Documentation/git-interpret-trailers.txt | 3 +-
t/t7513-interpret-trailers.sh | 35 +++++++++++++++
trailer.c | 77 ++++++++++++++++++++++----------
3 files changed, 90 insertions(+), 25 deletions(-)
diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 93d1db6..c480da6 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -48,7 +48,8 @@ with only spaces at the end of the commit message part, one blank line
will be added before the new trailer.
Existing trailers are extracted from the input message by looking for
-a group of one or more lines that contain a colon (by default), where
+a group of one or more lines in which at least one line contains a
+colon (by default), where
the group is preceded by one or more empty (or whitespace-only) lines.
The group must either be at the end of the message or be the last
non-whitespace lines before a line that starts with '---'. Such three
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index aee785c..7f5cd2a 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -126,6 +126,37 @@ test_expect_success 'with multiline title in the message' '
test_cmp expected actual
'
+test_expect_success 'with non-trailer lines mixed with trailer lines' '
+ cat >patch <<-\EOF &&
+
+ this: is a trailer
+ this is not a trailer
+ EOF
+ cat >expected <<-\EOF &&
+
+ this: is a trailer
+ this is not a trailer
+ token: value
+ EOF
+ git interpret-trailers --trailer "token: value" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'with non-trailer lines only' '
+ cat >patch <<-\EOF &&
+
+ this is not a trailer
+ EOF
+ cat >expected <<-\EOF &&
+
+ this is not a trailer
+
+ token: value
+ EOF
+ git interpret-trailers --trailer "token: value" patch >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'with config setup' '
git config trailer.ack.key "Acked-by: " &&
cat >expected <<-\EOF &&
@@ -257,6 +288,8 @@ test_expect_success 'with message that has comments' '
cat >>expected <<-\EOF &&
# comment
+ # other comment
+ # yet another comment
Reviewed-by: Johan
Cc: Peff
# last comment
@@ -286,6 +319,8 @@ test_expect_success 'with message that has an old style conflict block' '
cat >>expected <<-\EOF &&
# comment
+ # other comment
+ # yet another comment
Reviewed-by: Johan
Cc: Peff
# last comment
diff --git a/trailer.c b/trailer.c
index 167b2fd..97e96a9 100644
--- a/trailer.c
+++ b/trailer.c
@@ -26,6 +26,10 @@ static struct conf_info default_conf_info;
struct trailer_item {
struct trailer_item *next;
+ /*
+ * If this is not a trailer line, the line is stored in value
+ * (excluding the terminating newline) and token is NULL.
+ */
char *token;
char *value;
};
@@ -63,9 +67,14 @@ static size_t token_len_without_separator(const char *token, size_t len)
static int same_token(const struct trailer_item *a, const 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));
- size_t min_len = (a_len > b_len) ? b_len : a_len;
+ size_t a_len, b_len, min_len;
+
+ if (!a->token)
+ return 0;
+
+ a_len = token_len_without_separator(a->token, strlen(a->token));
+ b_len = token_len_without_separator(b->token, strlen(b->token));
+ min_len = (a_len > b_len) ? b_len : a_len;
return !strncasecmp(a->token, b->token, min_len);
}
@@ -123,7 +132,14 @@ static char last_non_space_char(const char *s)
static void print_tok_val(FILE *outfile, const char *tok, const char *val)
{
- char c = last_non_space_char(tok);
+ char c;
+
+ if (!tok) {
+ fprintf(outfile, "%s\n", val);
+ return;
+ }
+
+ c = last_non_space_char(tok);
if (!c)
return;
if (strchr(separators, c))
@@ -510,8 +526,16 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le
return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
}
+/*
+ * Parse the given trailer into token and value parts.
+ *
+ * If the given trailer does not have a separator (and thus cannot be separated
+ * into token and value parts), it is treated as a token (if parse_as_arg) or
+ * as a non-trailer line (if not parse_as_arg).
+ */
static int parse_trailer(struct strbuf *tok, struct strbuf *val,
- const struct conf_info **conf, const char *trailer)
+ const struct conf_info **conf, const char *trailer,
+ int parse_as_arg)
{
size_t len;
struct strbuf seps = STRBUF_INIT;
@@ -523,11 +547,18 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
len = strcspn(trailer, seps.buf);
strbuf_release(&seps);
if (len == 0) {
- int l = strlen(trailer);
+ int l;
+ if (!parse_as_arg)
+ return -1;
+
+ l = strlen(trailer);
while (l > 0 && isspace(trailer[l - 1]))
l--;
return error(_("empty trailer token in trailer '%.*s'"), l, trailer);
}
+ if (!parse_as_arg && len == strlen(trailer))
+ return -1;
+
if (len < strlen(trailer)) {
strbuf_add(tok, trailer, len);
strbuf_trim(tok);
@@ -598,7 +629,7 @@ static struct arg_item *process_command_line_args(struct string_list *trailers)
/* 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))
+ if (!parse_trailer(&tok, &val, &conf, tr->string, 1))
add_arg_item(&arg_tok_tail,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL),
@@ -652,7 +683,7 @@ static int find_patch_start(struct strbuf **lines, int count)
*/
static int find_trailer_start(struct strbuf **lines, int count)
{
- int start, end_of_title, only_spaces = 1;
+ int start, end_of_title, only_spaces = 1, trailer_found = 0;
/* The first paragraph is the title and cannot be trailers */
for (start = 0; start < count; start++) {
@@ -668,22 +699,17 @@ static int find_trailer_start(struct strbuf **lines, int count)
* for a line with only spaces before lines with one separator.
*/
for (start = count - 1; start >= end_of_title; start--) {
- if (lines[start]->buf[0] == comment_line_char)
- continue;
if (contains_only_spaces(lines[start]->buf)) {
if (only_spaces)
continue;
- return start + 1;
+ return trailer_found ? start + 1 : count;
}
- if (strcspn(lines[start]->buf, separators) < lines[start]->len) {
- if (only_spaces)
- only_spaces = 0;
- continue;
- }
- return count;
+ only_spaces = 0;
+ if (strcspn(lines[start]->buf, separators) < lines[start]->len)
+ trailer_found = 1;
}
- return only_spaces ? count : 0;
+ return count;
}
/* Get the index of the end of the trailers */
@@ -704,11 +730,8 @@ static int find_trailer_end(struct strbuf **lines, int patch_start)
static int has_blank_line_before(struct strbuf **lines, int start)
{
- for (;start >= 0; start--) {
- if (lines[start]->buf[0] == comment_line_char)
- continue;
+ if (start >= 0)
return contains_only_spaces(lines[start]->buf);
- }
return 0;
}
@@ -744,11 +767,17 @@ 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, NULL, lines[i]->buf))
+ if (!parse_trailer(&tok, &val, NULL, lines[i]->buf, 0))
add_trailer_item(in_tok_tail,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL));
+ else {
+ strbuf_addbuf(&val, lines[i]);
+ strbuf_strip_suffix(&val, "\n");
+ add_trailer_item(in_tok_tail,
+ NULL,
+ strbuf_detach(&val, NULL));
+ }
}
return trailer_end;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 2/5] trailer: streamline trailer item create and add
From: Jonathan Tan @ 2016-10-12 1:23 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, christian.couder, gitster
In-Reply-To: <cover.1476232683.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 | 135 +++++++++++++++++++++++++++++---------------------------------
1 file changed, 62 insertions(+), 73 deletions(-)
diff --git a/trailer.c b/trailer.c
index bf3d7d0..e8b1bfb 100644
--- a/trailer.c
+++ b/trailer.c
@@ -335,7 +335,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)
@@ -467,10 +467,30 @@ 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;
+
strbuf_addstr(&seps, separators);
strbuf_addch(&seps, '=');
len = strcspn(trailer, seps.buf);
@@ -490,73 +510,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;
-
- 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;
for (item = first_conf_item; item; item = item->next) {
- 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 trailer_item ***tail,
- struct trailer_item *new)
+static void add_trailer_item(struct trailer_item ***tail, 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);
+
**tail = new;
*tail = &new->next;
}
@@ -567,19 +545,25 @@ static struct trailer_item *process_command_line_args(struct string_list *traile
struct trailer_item **arg_tok_tail = &arg_tok_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;
/* Add a trailer item for each configured trailer with a command */
- for (item = first_conf_item; item; item = item->next) {
- if (item->conf.command) {
- struct trailer_item *new = new_trailer_item(item, NULL, NULL);
- add_trailer_item(&arg_tok_tail, new);
- }
- }
+ for (item = first_conf_item; item; item = item->next)
+ if (item->conf.command)
+ add_trailer_item(&arg_tok_tail,
+ 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_tok_tail, new);
+ if (!parse_trailer(&tok, &val, &conf, tr->string))
+ add_trailer_item(&arg_tok_tail,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf);
}
return arg_tok_head;
@@ -702,6 +686,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])
@@ -719,10 +706,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(in_tok_tail, new);
- }
+ if (lines[i]->buf[0] != comment_line_char &&
+ !parse_trailer(&tok, &val, &conf, lines[i]->buf))
+ add_trailer_item(in_tok_tail,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf);
}
return trailer_end;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 5/5] trailer: support values folded to multiple lines
From: Jonathan Tan @ 2016-10-12 1:23 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, christian.couder, gitster
In-Reply-To: <cover.1476232683.git.jonathantanmy@google.com>
Currently, interpret-trailers requires that a trailer be only on 1 line.
For example:
a: first line
second line
would be interpreted as one trailer line followed by one non-trailer line.
Make interpret-trailers support RFC 822-style folding, treating those
lines as one logical trailer.
---
Documentation/git-interpret-trailers.txt | 7 +-
t/t7513-interpret-trailers.sh | 139 +++++++++++++++++++++++++++++++
trailer.c | 40 ++++++---
3 files changed, 170 insertions(+), 16 deletions(-)
diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index c480da6..cfec636 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -57,11 +57,12 @@ minus signs start the patch part of the message.
When reading trailers, there can be whitespaces before and after the
token, the separator and the value. There can also be whitespaces
-inside the token and the value.
+inside the token and the value. The value may be split over multiple lines with
+each subsequent line starting with whitespace, like the "folding" in RFC 822.
Note that 'trailers' do not follow and are not intended to follow many
-rules for RFC 822 headers. For example they do not follow the line
-folding rules, the encoding rules and probably many other rules.
+rules for RFC 822 headers. For example they do not follow
+the encoding rules and probably many other rules.
OPTIONS
-------
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 7f5cd2a..195cdd3 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -157,6 +157,145 @@ test_expect_success 'with non-trailer lines only' '
test_cmp expected actual
'
+test_expect_success 'multiline field treated as atomic for placement' '
+ q_to_tab >patch <<-\EOF &&
+
+ another: trailer
+ name: value on
+ Qmultiple lines
+ another: trailer
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: value on
+ Qmultiple lines
+ name: value
+ another: trailer
+ EOF
+ test_config trailer.name.where after &&
+ git interpret-trailers --trailer "name: value" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for replacement' '
+ q_to_tab >patch <<-\EOF &&
+
+ another: trailer
+ name: value on
+ Qmultiple lines
+ another: trailer
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ another: trailer
+ name: value
+ EOF
+ test_config trailer.name.ifexists replace &&
+ git interpret-trailers --trailer "name: value" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for difference check' '
+ q_to_tab >patch <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ EOF
+ test_config trailer.name.ifexists addIfDifferent &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line
+ Qsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line
+ Qsecond line *DIFFERENT*
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ name: first line
+ Qsecond line *DIFFERENT*
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line *DIFFERENT*
+ Qsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ name: first line *DIFFERENT*
+ Qsecond line
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for neighbor check' '
+ q_to_tab >patch <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ EOF
+ test_config trailer.name.where after &&
+ test_config trailer.name.ifexists addIfDifferentNeighbor &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line
+ Qsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line
+ Qsecond line *DIFFERENT*
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ name: first line
+ Qsecond line *DIFFERENT*
+ another: trailer
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'with config setup' '
git config trailer.ack.key "Acked-by: " &&
cat >expected <<-\EOF &&
diff --git a/trailer.c b/trailer.c
index 97e96a9..907baa0 100644
--- a/trailer.c
+++ b/trailer.c
@@ -31,7 +31,7 @@ struct trailer_item {
* (excluding the terminating newline) and token is NULL.
*/
char *token;
- char *value;
+ struct strbuf value;
};
struct arg_item {
@@ -81,7 +81,7 @@ static int same_token(const struct trailer_item *a, const struct arg_item *b)
static int same_value(const struct trailer_item *a, const struct arg_item *b)
{
- return !strcasecmp(a->value, b->value);
+ return !strcasecmp(a->value.buf, b->value);
}
static int same_trailer(const struct trailer_item *a, const struct arg_item *b)
@@ -107,7 +107,7 @@ 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);
+ strbuf_release(&item->value);
free(item);
}
@@ -152,8 +152,8 @@ static void print_all(FILE *outfile, struct trailer_item *first, int trim_empty)
{
struct trailer_item *item;
for (item = first; item; item = item->next) {
- if (!trim_empty || strlen(item->value) > 0)
- print_tok_val(outfile, item->token, item->value);
+ if (!trim_empty || item->value.len > 0)
+ print_tok_val(outfile, item->token, item->value.buf);
}
}
@@ -195,8 +195,8 @@ static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg
if (arg_tok->value && arg_tok->value[0]) {
arg = arg_tok->value;
} else {
- if (in_tok && in_tok->value)
- arg = xstrdup(in_tok->value);
+ if (in_tok)
+ arg = xstrdup(in_tok->value.buf);
else
arg = xstrdup("");
}
@@ -209,7 +209,9 @@ 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;
+ strbuf_init(&new->value, 0);
+ strbuf_attach(&new->value, arg_tok->value, strlen(arg_tok->value),
+ strlen(arg_tok->value));
arg_tok->token = arg_tok->value = NULL;
free_arg_item(arg_tok);
return new;
@@ -587,14 +589,17 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
return 0;
}
-static void add_trailer_item(struct trailer_item ***tail, char *tok, char *val)
+static struct trailer_item *add_trailer_item(struct trailer_item ***tail, char *tok,
+ char *val)
{
struct trailer_item *new = xcalloc(sizeof(*new), 1);
new->token = tok;
- new->value = val;
+ strbuf_init(&new->value, 0);
+ strbuf_attach(&new->value, val, strlen(val), strlen(val));
**tail = new;
*tail = &new->next;
+ return new;
}
static void add_arg_item(struct arg_item ***tail, char *tok, char *val,
@@ -750,6 +755,7 @@ 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;
+ struct trailer_item *last = NULL;
/* Get the line count */
while (lines[count])
@@ -767,16 +773,24 @@ static int process_input_file(FILE *outfile,
/* Parse trailer lines */
for (i = trailer_start; i < trailer_end; i++) {
+ if (last && isspace(lines[i]->buf[0])) {
+ /* continuation line of the last trailer item */
+ strbuf_addch(&last->value, '\n');
+ strbuf_addbuf(&last->value, lines[i]);
+ strbuf_strip_suffix(&last->value, "\n");
+ continue;
+ }
if (!parse_trailer(&tok, &val, NULL, lines[i]->buf, 0))
- add_trailer_item(in_tok_tail,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL));
+ last = add_trailer_item(in_tok_tail,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL));
else {
strbuf_addbuf(&val, lines[i]);
strbuf_strip_suffix(&val, "\n");
add_trailer_item(in_tok_tail,
NULL,
strbuf_detach(&val, NULL));
+ last = NULL;
}
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 0/5] allow non-trailers and multiple-line trailers
From: Jonathan Tan @ 2016-10-12 1:23 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, christian.couder, gitster
In [1], Junio explained a possible redesign of trailer support in
interpret-trailers and cherry-pick, something along these lines:
1. Support non-trailers and multi-line trailers in interpret-trailers
2. Create a helper function so that the new interpretation can be used
elsewhere (e.g. in sequencer)
3. Modify "Signed-off-by" and "(cherry picked by" to use the helper
function
My current plans for step 1 and step 2 require relatively significant
refactoring in trailer.c, so I thought that I should send out a patch
set covering only step 1 first for discussion, before continuing with my
work on top of this patch set.
Support for steps 2 and 3, including my original use case of being
looser in the definition of a trailer when invoking "cherry-pick -x"
(and thus suppressing the insertion of a newline) will come in a
subsequent patch set.
[1] Message ID <xmqqwphouivf.fsf@gitster.mtv.corp.google.com>
Jonathan Tan (5):
trailer: use singly-linked list, not doubly
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 | 638 +++++++++++++++----------------
3 files changed, 480 insertions(+), 342 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [PATCH 1/5] trailer: use singly-linked list, not doubly
From: Jonathan Tan @ 2016-10-12 1:23 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, christian.couder, gitster
In-Reply-To: <cover.1476232683.git.jonathantanmy@google.com>
Use singly-linked lists (instead of doubly-linked lists) in trailer to
keep track of arguments (whether implicit from configuration or explicit
from the command line) and trailer items.
This change significantly reduces the code length and simplifies the code.
There are now fewer pointers to be manipulated, but most trailer
manipulations now require seeking from beginning to end, so there might
be a slight net decrease in performance; however the number of trailers
is usually small (10 to 15 at the most) so this should not cause a big
impact.
---
trailer.c | 357 ++++++++++++++++++++++----------------------------------------
1 file changed, 125 insertions(+), 232 deletions(-)
diff --git a/trailer.c b/trailer.c
index c6ea9ac..bf3d7d0 100644
--- a/trailer.c
+++ b/trailer.c
@@ -25,7 +25,6 @@ struct conf_info {
static struct conf_info default_conf_info;
struct trailer_item {
- struct trailer_item *previous;
struct trailer_item *next;
const char *token;
const char *value;
@@ -56,7 +55,8 @@ 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(const struct trailer_item *a,
+ const struct trailer_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));
@@ -65,12 +65,14 @@ 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(const struct trailer_item *a,
+ const struct trailer_item *b)
{
return !strcasecmp(a->value, b->value);
}
-static int same_trailer(struct trailer_item *a, struct trailer_item *b)
+static int same_trailer(const struct trailer_item *a,
+ const struct trailer_item *b)
{
return same_token(a, b) && same_value(a, b);
}
@@ -129,92 +131,6 @@ static void print_all(FILE *outfile, struct trailer_item *first, int trim_empty)
}
}
-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);
- }
-}
-
-static int check_if_different(struct trailer_item *in_tok,
- struct trailer_item *arg_tok,
- int check_all)
-{
- enum action_where where = arg_tok->conf.where;
- 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;
- } 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 const char *apply_command(const char *command, const char *arg)
{
struct strbuf cmd = STRBUF_INIT;
@@ -263,122 +179,116 @@ 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)
+static void apply_existing_arg(struct trailer_item **found_next,
+ struct trailer_item *arg_tok,
+ struct trailer_item **position_to_add,
+ const struct trailer_item *in_tok_head,
+ const struct trailer_item *neighbor)
{
- switch (arg_tok->conf.if_exists) {
- case EXISTS_DO_NOTHING:
+ if (arg_tok->conf.if_exists == EXISTS_DO_NOTHING) {
free_trailer_item(arg_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);
- 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);
- 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);
- 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);
- else
+ return;
+ }
+
+ apply_item_command(*found_next, arg_tok);
+
+ if (arg_tok->conf.if_exists == EXISTS_ADD_IF_DIFFERENT_NEIGHBOR) {
+ if (neighbor && same_trailer(neighbor, arg_tok)) {
free_trailer_item(arg_tok);
- break;
+ return;
+ }
+ } else if (arg_tok->conf.if_exists == EXISTS_ADD_IF_DIFFERENT) {
+ const struct trailer_item *in_tok;
+ for (in_tok = in_tok_head; in_tok; in_tok = in_tok->next) {
+ if (same_trailer(in_tok, arg_tok)) {
+ free_trailer_item(arg_tok);
+ return;
+ }
+ }
+ }
+
+ arg_tok->next = *position_to_add;
+ *position_to_add = arg_tok;
+
+ if (arg_tok->conf.if_exists == EXISTS_REPLACE) {
+ struct trailer_item *new_next = (*found_next)->next;
+ free_trailer_item(*found_next);
+ *found_next = new_next;
}
}
-static void apply_arg_if_missing(struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last,
- struct trailer_item *arg_tok)
+static void apply_missing_arg(struct trailer_item *arg_tok,
+ struct trailer_item **position_to_add)
{
- struct trailer_item **in_tok;
- enum action_where where;
-
- switch (arg_tok->conf.if_missing) {
- case MISSING_DO_NOTHING:
+ if (arg_tok->conf.if_missing == MISSING_DO_NOTHING) {
free_trailer_item(arg_tok);
- 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;
+ return;
}
+
+ apply_item_command(NULL, arg_tok);
+
+ arg_tok->next = *position_to_add;
+ *position_to_add = arg_tok;
}
-static int find_same_and_apply_arg(struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last,
- struct trailer_item *arg_tok)
+static void apply_arg(struct trailer_item **in_tok_head,
+ struct trailer_item *arg_tok)
{
- struct trailer_item *in_tok;
- struct trailer_item *on_tok;
- struct trailer_item *following_tok;
-
+ struct trailer_item **next, **found_next = NULL, *last = NULL;
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;
- for (in_tok = start_tok; in_tok; in_tok = following_tok) {
- following_tok = backwards ? in_tok->previous : in_tok->next;
- 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);
- return 1;
+ for (next = in_tok_head; *next; next = &(*next)->next) {
+ last = *next;
+ if ((!found_next || backwards) &&
+ same_token(*next, arg_tok))
+ found_next = next;
+ }
+
+ if (found_next) {
+ struct trailer_item **position_to_add, *neighbor;
+ switch (where) {
+ case WHERE_START:
+ position_to_add = in_tok_head;
+ neighbor = *in_tok_head;
+ break;
+ case WHERE_BEFORE:
+ position_to_add = found_next;
+ neighbor = *found_next;
+ break;
+ case WHERE_AFTER:
+ position_to_add = &(*found_next)->next;
+ neighbor = *found_next;
+ break;
+ default:
+ position_to_add = next;
+ neighbor = last;
+ break;
+ }
+ apply_existing_arg(found_next, arg_tok, position_to_add,
+ *in_tok_head, neighbor);
+ } else {
+ struct trailer_item **position_to_add;
+ switch (where) {
+ case WHERE_START:
+ case WHERE_BEFORE:
+ position_to_add = in_tok_head;
+ break;
+ default:
+ position_to_add = next;
+ break;
+ }
+ apply_missing_arg(arg_tok, position_to_add);
}
- 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 trailer_item **in_tok_head,
+ struct trailer_item *arg_tok_head)
{
- 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) {
- int applied = 0;
-
- next_arg = arg_tok->next;
- remove_from_list(arg_tok, arg_tok_first, NULL);
-
- applied = find_same_and_apply_arg(in_tok_first,
- in_tok_last,
- arg_tok);
-
- if (!applied)
- apply_arg_if_missing(in_tok_first,
- in_tok_last,
- arg_tok);
+ struct trailer_item *arg_tok, *next;
+ for (arg_tok = arg_tok_head; arg_tok; arg_tok = next) {
+ next = arg_tok->next;
+ apply_arg(in_tok_head, arg_tok);
}
}
@@ -438,29 +348,19 @@ static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
static struct trailer_item *get_conf_item(const char *name)
{
+ struct trailer_item **next = &first_conf_item;
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) {
- if (!strcasecmp(item->conf.name, name))
- return item;
- }
+ for (next = &first_conf_item; *next; next = &(*next)->next)
+ if (!strcasecmp((*next)->conf.name, name))
+ return *next;
/* Item does not already exists, create it */
item = xcalloc(sizeof(struct trailer_item), 1);
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;
- }
-
+ *next = item;
return item;
}
@@ -652,26 +552,19 @@ 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,
+static void add_trailer_item(struct trailer_item ***tail,
struct trailer_item *new)
{
if (!new)
return;
- if (!*last) {
- *first = new;
- *last = new;
- } else {
- (*last)->next = new;
- new->previous = *last;
- *last = new;
- }
+ **tail = new;
+ *tail = &new->next;
}
static struct trailer_item *process_command_line_args(struct string_list *trailers)
{
- struct trailer_item *arg_tok_first = NULL;
- struct trailer_item *arg_tok_last = NULL;
+ struct trailer_item *arg_tok_head = NULL;
+ struct trailer_item **arg_tok_tail = &arg_tok_head;
struct string_list_item *tr;
struct trailer_item *item;
@@ -679,17 +572,17 @@ static struct trailer_item *process_command_line_args(struct string_list *traile
for (item = first_conf_item; item; item = item->next) {
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_tok_tail, 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_tok_tail, new);
}
- return arg_tok_first;
+ return arg_tok_head;
}
static struct strbuf **read_input_file(const char *file)
@@ -805,8 +698,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 trailer_item ***in_tok_tail)
{
int count = 0;
int patch_start, trailer_start, trailer_end, i;
@@ -829,18 +721,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(in_tok_tail, new);
}
}
return trailer_end;
}
-static void free_all(struct trailer_item **first)
+static void free_all(struct trailer_item *head)
{
- while (*first) {
- struct trailer_item *item = remove_first(first);
- free_trailer_item(item);
+ while (head) {
+ struct trailer_item *next = head->next;
+ free_trailer_item(head);
+ head = next;
}
}
@@ -877,9 +770,9 @@ 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;
+ struct trailer_item *in_tok_head = NULL;
+ struct trailer_item **in_tok_tail = &in_tok_head;
+ struct trailer_item *arg_tok_head;
struct strbuf **lines;
int trailer_end;
FILE *outfile = stdout;
@@ -894,15 +787,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, &in_tok_tail);
- arg_tok_first = process_command_line_args(trailers);
+ arg_tok_head = process_command_line_args(trailers);
- process_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);
+ process_trailers_lists(&in_tok_head, arg_tok_head);
- print_all(outfile, in_tok_first, trim_empty);
+ print_all(outfile, in_tok_head, trim_empty);
- free_all(&in_tok_first);
+ free_all(in_tok_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] sequencer: mark a file-local symbol static
From: Ramsay Jones @ 2016-10-12 0:12 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, GIT Mailing-list
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Johannes,
If you need to re-roll your 'js/prepare-sequencer' branch, could you
please squash this into commit 53f8024e ("sequencer: completely revamp
the "todo" script parsing", 10-10-2016).
Thanks.
ATB,
Ramsay Jones
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index d662c6b..aa65628 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -880,7 +880,7 @@ static void todo_list_release(struct todo_list *todo_list)
todo_list->nr = todo_list->alloc = 0;
}
-struct todo_item *append_new_todo(struct todo_list *todo_list)
+static struct todo_item *append_new_todo(struct todo_list *todo_list)
{
ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
return todo_list->items + todo_list->nr++;
--
2.10.0
^ permalink raw reply related
* [PATCHv2] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-11 23:59 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, 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.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
I think this patch is the most interesting patch, so I'll refrain from
resending the other 27 patches, though I have adressed the review comments
locally. I'll resend everything once we are in agreement for this one.
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.
Also there is no example of how to use the thread safe API for asking questions
from multiple threads.
Thanks,
Stefan
Documentation/technical/api-gitattributes.txt | 93 +++++++++++++------
archive.c | 14 ++-
attr.c | 128 +++++++++++++++++---------
attr.h | 80 ++++++++++------
builtin/check-attr.c | 30 +++---
builtin/pack-objects.c | 12 ++-
convert.c | 35 +++----
ll-merge.c | 27 ++++--
userdiff.c | 19 ++--
ws.c | 15 +--
10 files changed, 292 insertions(+), 161 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..ae98df2 100644
--- a/archive.c
+++ b/archive.c
@@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
static struct git_attr_check *check;
+ static struct git_attr_result result;
const char *path_without_prefix;
int err;
@@ -124,12 +125,15 @@ 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))
+ if (!check) {
+ 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]))
return 0;
- args->convert = ATTR_TRUE(check->check[1].value);
+ args->convert = ATTR_TRUE(result.value[1]);
}
if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
diff --git a/attr.c b/attr.c
index 0faa69f..bf01466 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,45 @@ 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);
-}
-
-int git_check_attr(const char *path, struct git_attr_check *check)
-{
- return git_check_attr_counted(path, strlen(path), check);
+ return git_check_attrs(path, strlen(path), check, result);
}
-struct git_attr_check *git_attr_check_initl(const char *one, ...)
+void git_attr_check_initl(struct git_attr_check **check_,
+ const char *one, ...)
{
- struct git_attr_check *check;
int cnt;
va_list params;
const char *param;
+ struct git_attr_check *check;
+
+ if (*check_)
+ return;
+
+ 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 +971,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 +983,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 +1007,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..a3df756 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);
}
}
}
@@ -60,14 +64,18 @@ static void check_attr(const char *prefix,
char *full_path =
prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
if (check != NULL) {
- if (git_check_attr(full_path, check))
- die("git_check_attr died");
- output_attr(check, file);
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+ git_attr_result_init(&result, check);
+ git_check_attr(full_path, check, &result);
+ output_attr(check, &result, file);
+ git_attr_result_clear(&result);
} else {
- check = git_attr_check_alloc();
- git_all_attrs(full_path, check);
- output_attr(check, file);
- git_attr_check_free(check);
+ struct git_attr_check check = GIT_ATTR_CHECK_INIT;
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+ git_all_attrs(full_path, &check, &result);
+ output_attr(&check, &result, file);
+ git_attr_result_clear(&result);
+ git_attr_check_clear(&check);
}
free(full_path);
}
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3918c07..962c87b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -899,12 +899,16 @@ static void write_pack_file(void)
static int no_try_delta(const char *path)
{
static struct git_attr_check *check;
+ static struct git_attr_result result;
- if (!check)
- check = git_attr_check_initl("delta", NULL);
- if (git_check_attr(path, check))
+ if (!check) {
+ git_attr_check_initl(&check, "delta", NULL);
+ git_attr_result_init(&result, check);
+ }
+
+ if (git_check_attr(path, check, &result))
return 0;
- if (ATTR_FALSE(check->check[0].value))
+ if (ATTR_FALSE(result.value[0]))
return 1;
return 0;
}
diff --git a/convert.c b/convert.c
index bb2435a..dd8d25b 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,25 @@ struct conv_attrs {
static void convert_attrs(struct conv_attrs *ca, const char *path)
{
static struct git_attr_check *check;
+ static struct git_attr_result result;
if (!check) {
- check = git_attr_check_initl("crlf", "ident",
- "filter", "eol", "text",
- NULL);
+ git_attr_check_initl(&check, "crlf", "ident", "filter",
+ "eol", "text", NULL);
+ git_attr_result_init(&result, check);
user_convert_tail = &user_convert;
git_config(read_convert_config, NULL);
}
- 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)
diff --git a/ll-merge.c b/ll-merge.c
index bc6479c..086f5c7 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,12 +397,16 @@ int ll_merge(mmbuffer_t *result_buf,
int ll_merge_marker_size(const char *path)
{
static struct git_attr_check *check;
+ static struct git_attr_result result;
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);
+ if (!check) {
+ 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;
}
diff --git a/userdiff.c b/userdiff.c
index 46dfd32..e88568a 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -263,21 +263,26 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {
struct userdiff_driver *userdiff_find_by_path(const char *path)
{
static struct git_attr_check *check;
+ static struct git_attr_result result;
+
+ if (!check) {
+ git_attr_check_initl(&check, "diff", NULL);
+ git_attr_result_init(&result, check);
+ }
- if (!check)
- check = git_attr_check_initl("diff", NULL);
if (!path)
return NULL;
- if (git_check_attr(path, check))
+
+ if (git_check_attr(path, check, &result))
return NULL;
- if (ATTR_TRUE(check->check[0].value))
+ if (ATTR_TRUE(result.value[0]))
return &driver_true;
- if (ATTR_FALSE(check->check[0].value))
+ if (ATTR_FALSE(result.value[0]))
return &driver_false;
- if (ATTR_UNSET(check->check[0].value))
+ if (ATTR_UNSET(result.value[0]))
return NULL;
- return userdiff_find_by_name(check->check[0].value);
+ return userdiff_find_by_name(result.value[0]);
}
struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver)
diff --git a/ws.c b/ws.c
index bb3270c..8bdbbf7 100644
--- a/ws.c
+++ b/ws.c
@@ -73,15 +73,16 @@ 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;
+ static struct git_attr_result result;
- if (!attr_whitespace_rule)
- attr_whitespace_rule = git_attr_check_initl("whitespace", NULL);
-
- if (!git_check_attr(pathname, attr_whitespace_rule)) {
- const char *value;
+ if (!check) {
+ git_attr_check_initl(&check, "whitespace", NULL);
+ git_attr_result_init(&result, check);
+ }
- value = attr_whitespace_rule->check[0].value;
+ if (!git_check_attr(pathname, check, &result)) {
+ const char *value = result.value[0];
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
* [PATCH] convert: mark a file-local symbol static
From: Ramsay Jones @ 2016-10-11 23:46 UTC (permalink / raw)
To: Lars Schneider; +Cc: Junio C Hamano, GIT Mailing-list
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Lars,
If you need to re-roll your 'ls/filter-process' branch, could you
please squash this into commit 85290197
("convert: add filter.<driver>.process option", 08-10-2016).
Thanks.
ATB,
Ramsay Jones
convert.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/convert.c b/convert.c
index fe68316..cf30380 100644
--- a/convert.c
+++ b/convert.c
@@ -568,7 +568,7 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *
free(entry);
}
-void stop_multi_file_filter(struct child_process *process)
+static void stop_multi_file_filter(struct child_process *process)
{
sigchain_push(SIGPIPE, SIG_IGN);
/* Closing the pipe signals the filter to initiate a shutdown. */
--
2.10.0
^ permalink raw reply related
* Re: git 2.10.1 test regression in t3700-add.sh
From: Jeremy Huddleston Sequoia @ 2016-10-11 23:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: t.gummerer, git
In-Reply-To: <xmqq8ttwgkyo.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 5149 bytes --]
> On Oct 10, 2016, at 10:41, Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeremy Huddleston Sequoia <jeremyhu@freedesktop.org> writes:
>
>> Actually, looks like that as just a rabbit hole. The real issue
>> looks to be because an earlier test drops down xfoo3 as a symlink,
>> which causes this test to fail due to the collision. I'll get out
>> a patch in a bit.
>
> [administrivia: please don't top-post, it is extremely hard to
> follow what is going on]
>
> There indeed is a test that creates xfoo3 as a symbolic link and
> leaves it in the filesystem pointing at xfoo2 much earlier in the
> sequence. It seems that later one of the "add ignore-errors" tests
> (there are two back-to-back) runs "git reset --hard" to make it (and
> other symbolic links that are similarly named) go away, namely this
> part of the code:
>
> test_expect_success POSIXPERM,SANITY 'git add --ignore-errors' '
> git reset --hard &&
> date >foo1 &&
> date >foo2 &&
> chmod 0 foo2 &&
> test_must_fail git add --verbose --ignore-errors . &&
> git ls-files foo1 | grep foo1
> '
>
> rm -f foo2
>
> test_expect_success POSIXPERM,SANITY 'git add (add.ignore-errors)' '
> git config add.ignore-errors 1 &&
> git reset --hard &&
> date >foo1 &&
> date >foo2 &&
> chmod 0 foo2 &&
> test_must_fail git add --verbose . &&
> git ls-files foo1 | grep foo1
> '
> rm -f foo2
>
> "git reset --hard" in the first one, because these symbolic links
> are not in the index at that point in the sequence, would leave them
> untracked and in the working tree. Then "add" is told to be
> non-atomic with "--ignore-errors", adding them to the index while
> reporting a failure. When the test after that runs "git reset --hard"
> again, because these symlinks are in the index (and not in HEAD),
> they are removed from the working tree.
>
> And that is why normal people won't see xfoo3 in later tests, like
> the one you had trouble with.
>
> Are you running without SANITY by any chance (I am not saying that
> you are doing a wrong thing--just trying to make sure I am guessing
> along the correct route)?
Ah, yep. That's the ticket:
ok 23 # skip git add should fail atomically upon an unreadable file (missing SANITY of POSIXPERM,SANITY)
ok 24 # skip git add --ignore-errors (missing SANITY of POSIXPERM,SANITY)
ok 25 # skip git add (add.ignore-errors) (missing SANITY of POSIXPERM,SANITY)
ok 26 # skip git add (add.ignore-errors = false) (missing SANITY of POSIXPERM,SANITY)
ok 27 # skip --no-ignore-errors overrides config (missing SANITY of POSIXPERM,SANITY)
I dug into it a bit, and since I'm running the tests during a staging phase which runs as root, !SANITY gets set. Should be solvable by essentially breaking test out into post-build instead of pre-install. Thanks for the pointer there.
> If that is the issue, then I think the right correction would be to
> make sure that the files that an individual test expects to be
> missing from the file system is indeed missing by explicitly
> removing it, perhaps like this.
>
> I also notice that the problematic test uses "chmod 755"; don't we
> need POSIXPERM prerequisite on this test, too, I wonder?
>
> Thanks.
>
> -- >8 --
> t3700: fix broken test under !SANITY
>
> An "add --chmod=+x" test recently added by 610d55af0f ("add: modify
> already added files when --chmod is given", 2016-09-14) used "xfoo3"
> as a test file. The paths xfoo[1-3] were used by earlier tests for
> symbolic links but they were expected to have been removed by the
> test script reached this new test.
>
> The removal with "git reset --hard" however happened in tests that
> are protected by POSIXPERM,SANITY prerequisites. Platforms and test
> environments that lacked these would have seen xfoo3 as a leftover
> symbolic link, pointing somewhere else, and chmod test would have
> given a wrong result.
>
>
>
> t/t3700-add.sh | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 924a266126..53c0cb6dea 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -350,6 +350,7 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x with symlinks' '
> '
>
> test_expect_success 'git add --chmod=[+-]x changes index with already added file' '
> + rm -f foo3 xfoo3 &&
> echo foo >foo3 &&
> git add foo3 &&
> git add --chmod=+x foo3 &&
I actually tried that, but the problem is that xfoo3 was previously added as a symlink, so test_mode_in_index still reports it as a symlink.
It's fundamentally a question of who is responsible for cleanup. Is the individual test responsible for cleaning up after itself (such that later tests can rely on a clean state), or should individual tests assume that the initial state might be undefined and try to cleanup after earlier tests? I'm in favor of either doing the former or ensuring that tests don't step on each-others toes.
--Jeremy
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4465 bytes --]
^ permalink raw reply
* Re: Make `git fetch --all` parallel?
From: Stefan Beller @ 2016-10-11 23:18 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ram Rachum, git@vger.kernel.org
In-Reply-To: <20161011225942.tvqbbzxglvu7lldi@sigill.intra.peff.net>
On Tue, Oct 11, 2016 at 3:59 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 11, 2016 at 03:50:36PM -0700, Stefan Beller wrote:
>
>> I agree. Though even for implementing the "dumb" case of fetching
>> objects twice we'd have to take care of some racing issues, I would assume.
>>
>> Why did you put a "sleep 2" below?
>> * a slow start to better spread load locally? (keep the workstation responsive?)
>> * a slow start to have different fetches in a different phase of the
>> fetch protocol?
>> * avoiding some subtle race?
>>
>> At the very least we would need a similar thing as Jeff recently sent for the
>> push case with objects quarantined and then made available in one go?
>
> I don't think so. The object database is perfectly happy with multiple
> simultaneous writers, and nothing impacts the have/wants until actual
> refs are written. Quarantining objects before the refs are written is an
> orthogonal concept.
If a remote advertises its tips, we'd need to look these up (clientside) to
decide if we have them, and I do not think we'd do that via a reachability
check, but via direct lookup in the object data base? So I do not quite
understand, what we gain from the atomic ref writes in e.g. remote/origin/.
> I'm not altogether convinced that parallel fetch would be that much
> faster, though.
Ok, time to present data... Let's assume a degenerate case first:
"up-to-date with all remotes" because that is easy to reproduce.
I have 14 remotes currently:
$ time git fetch --all
real 0m18.016s
user 0m2.027s
sys 0m1.235s
$ time git config --get-regexp remote.*.url |awk '{print $2}' |xargs
-P 14 -I % git fetch %
real 0m5.168s
user 0m2.312s
sys 0m1.167s
A factor of >3, so I suspect there is improvement ;)
Well just as Ævar pointed out, there is some improvement.
>
> I usually just do a one-off fetch of their URL in such a case, exactly
> because I _don't_ want to end up with a bunch of remotes. You can also
> mark them with skipDefaultUpdate if you only care about them
> occasionally (so you can "git fetch sbeller" when you care about it, but
> it doesn't slow down your daily "git fetch").
And I assume you don't want the remotes because it takes time to fetch and not
because your disk space is expensive. ;)
>
> -Peff
^ permalink raw reply
* Re: Make `git fetch --all` parallel?
From: Ævar Arnfjörð Bjarmason @ 2016-10-11 23:16 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, Junio C Hamano, Ram Rachum, git@vger.kernel.org
In-Reply-To: <20161011225942.tvqbbzxglvu7lldi@sigill.intra.peff.net>
On Wed, Oct 12, 2016 at 12:59 AM, Jeff King <peff@peff.net> wrote:
> I'm not altogether convinced that parallel fetch would be that much
> faster, though.
I have local aliases to use GNU parallel for stuff like this, on my
git.git which has accumulated 17 remotes:
$ time parallel -j1 'git fetch {}' ::: $(git remote)
real 0m18.265s
$ time parallel -j8 'git fetch {}' ::: $(git remote)
real 0m2.957s
In that case I didn't have any new objects to fetch, but just doing
the negotiation in parallel was a lot faster.
So there's big wins in some cases.
^ permalink raw reply
* Re: Make `git fetch --all` parallel?
From: Jeff King @ 2016-10-11 22:59 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, Ram Rachum, git@vger.kernel.org
In-Reply-To: <CAGZ79kZNvTvk4uZa8xhxZABKtzS9A5HoumJ37AacuZnHaZ4+Xw@mail.gmail.com>
On Tue, Oct 11, 2016 at 03:50:36PM -0700, Stefan Beller wrote:
> I agree. Though even for implementing the "dumb" case of fetching
> objects twice we'd have to take care of some racing issues, I would assume.
>
> Why did you put a "sleep 2" below?
> * a slow start to better spread load locally? (keep the workstation responsive?)
> * a slow start to have different fetches in a different phase of the
> fetch protocol?
> * avoiding some subtle race?
>
> At the very least we would need a similar thing as Jeff recently sent for the
> push case with objects quarantined and then made available in one go?
I don't think so. The object database is perfectly happy with multiple
simultaneous writers, and nothing impacts the have/wants until actual
refs are written. Quarantining objects before the refs are written is an
orthogonal concept.
I'm not altogether convinced that parallel fetch would be that much
faster, though. Your bottleneck for a fetch is generally the network for
most of the time, then a brief spike of CPU during delta resolution. You
might get some small benefit from overlapping the fetches so that you
spend CPU on one while you spend network on the other, but I doubt it
would be nearly as beneficial as the parallel submodule clones (which
generally have a bigger CPU segment, and also are generally considered
independent, so there's no real tradeoff of getting duplicate objects).
Sometimes the bottleneck is the server preparing the back, but if that
is the case, you should probably complain to your server admin to enable
bitmaps. :)
> I would love to see the implementation though, as over time I accumulate
> a lot or remotes. (Someone published patches on the mailing list and made
> them available somewhere hosted? Grabbing them from their hosting site
> is easier than applying patches for me, so I'd rather fetch them... so I have
> some remotes now)
I usually just do a one-off fetch of their URL in such a case, exactly
because I _don't_ want to end up with a bunch of remotes. You can also
mark them with skipDefaultUpdate if you only care about them
occasionally (so you can "git fetch sbeller" when you care about it, but
it doesn't slow down your daily "git fetch").
-Peff
^ permalink raw reply
* Re: Make `git fetch --all` parallel?
From: Junio C Hamano @ 2016-10-11 22:58 UTC (permalink / raw)
To: Stefan Beller; +Cc: Ram Rachum, git@vger.kernel.org
In-Reply-To: <CAGZ79kZNvTvk4uZa8xhxZABKtzS9A5HoumJ37AacuZnHaZ4+Xw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> Why did you put a "sleep 2" below?
The assumption was to fetch from faster and near the center of the
project universe early, so by giving them head-start, fetches that
start in later rounds may have chance to see newly updated remote
tracking refs when telling the poorer other ends what we have.
> At the very least we would need a similar thing as Jeff recently sent for the
> push case with objects quarantined and then made available in one go?
There is no race; ref updates are done only after objects are fully
finalized. You can do the quarantine but that would defeat the "let
ones from the center of universe finish early so later ones from
poor periphery have more .have's to work with" idea, I suspect.
^ permalink raw reply
* Re: Make `git fetch --all` parallel?
From: Stefan Beller @ 2016-10-11 22:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ram Rachum, git@vger.kernel.org
In-Reply-To: <CAGZ79kZNvTvk4uZa8xhxZABKtzS9A5HoumJ37AacuZnHaZ4+Xw@mail.gmail.com>
> I dunno, if documented though.
http://stackoverflow.com/questions/26373995/how-to-control-the-order-of-fetching-when-fetching-all-remotes-by-git-fetch-al
We do not give promises about the order of --all (checked with our
documentation as well), however there seems to be a
grouping scheme for remotes that you can order via
setting remotes.default (which is not documented in man git config,
but only in the git remote man page)
^ permalink raw reply
* Re: Make `git fetch --all` parallel?
From: Stefan Beller @ 2016-10-11 22:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ram Rachum, git@vger.kernel.org
In-Reply-To: <xmqqa8ea7bsh.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 11, 2016 at 3:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So I do think it would be much faster, but I also think patches for this would
>> require some thought and a lot of refactoring of the fetch code.
>> ...
>> During the negotiation phase a client would have to be able to change its
>> mind (add more "haves", or in case of the parallel fetching these become
>> "will-have-soons", although the remote figured out the client did not have it
>> earlier.)
>
> Even though a fancy optimization as you outlined might be ideal, I
> suspect that users would be happier if the network bandwidth is
> utilized to talk to multiple remotes at the same time even if they
> end up receiving the same recent objects from more than one place in
> the end.
I agree. Though even for implementing the "dumb" case of fetching
objects twice we'd have to take care of some racing issues, I would assume.
Why did you put a "sleep 2" below?
* a slow start to better spread load locally? (keep the workstation responsive?)
* a slow start to have different fetches in a different phase of the
fetch protocol?
* avoiding some subtle race?
At the very least we would need a similar thing as Jeff recently sent for the
push case with objects quarantined and then made available in one go?
>
> Is the order in which "git fetch --all" iterates over "all remotes"
> predictable and documented?
it is predictable, as it is just the same order as put by grep in
$ grep "\[remote " .git/config, i.e. in order of the file, which in my
case turns out to be sorted by importance/history quite naturally.
But reordering my config file would be not a big deal.
I dunno, if documented though.
> If so, listing the remotes from more
> powerful and well connected place to slower ones and then doing an
> equivalent of stupid
>
> for remote in $list_of_remotes_ordered_in_such_a_way
list_of_remotes_ordered_in_such_a_way is roughly:
$(git config --get-regexp remote.*.url | tr '.' ' ' |awk '{print $2}')
> do
> git fetch "$remote" &
> sleep 2
> done
>
> might be fairly easy thing to bring happiness.
I would love to see the implementation though, as over time I accumulate
a lot or remotes. (Someone published patches on the mailing list and made
them available somewhere hosted? Grabbing them from their hosting site
is easier than applying patches for me, so I'd rather fetch them... so I have
some remotes now)
^ permalink raw reply
* Re: Make `git fetch --all` parallel?
From: Junio C Hamano @ 2016-10-11 22:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: Ram Rachum, git@vger.kernel.org
In-Reply-To: <CAGZ79kZmrYZqi4+bSkRykn+Upt7bEyZ0N8VhiQ-h8DhSMym-FA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> So I do think it would be much faster, but I also think patches for this would
> require some thought and a lot of refactoring of the fetch code.
> ...
> During the negotiation phase a client would have to be able to change its
> mind (add more "haves", or in case of the parallel fetching these become
> "will-have-soons", although the remote figured out the client did not have it
> earlier.)
Even though a fancy optimization as you outlined might be ideal, I
suspect that users would be happier if the network bandwidth is
utilized to talk to multiple remotes at the same time even if they
end up receiving the same recent objects from more than one place in
the end.
Is the order in which "git fetch --all" iterates over "all remotes"
predictable and documented? If so, listing the remotes from more
powerful and well connected place to slower ones and then doing an
equivalent of stupid
for remote in $list_of_remotes_ordered_in_such_a_way
do
git fetch "$remote" &
sleep 2
done
might be fairly easy thing to bring happiness.
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Lars Schneider @ 2016-10-11 15:29 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Jakub Narębski, git, Junio C Hamano, Jeff King
In-Reply-To: <a80f6399-b344-b979-d849-1dc7f899fabe@web.de>
> On 09 Oct 2016, at 07:32, Torsten Bögershausen <tboegi@web.de> wrote:
>
> On 09.10.16 01:06, Jakub Narębski wrote:
>>> +------------------------
>>>> +packet: git< status=abort
>>>> +packet: git< 0000
>>>> +------------------------
>>>> +
>>>> +After the filter has processed a blob it is expected to wait for
>>>> +the next "key=value" list containing a command. Git will close
>>>> +the command pipe on exit. The filter is expected to detect EOF
>>>> +and exit gracefully on its own.
>> Any "kill filter" solutions should probably be put here. I guess
>> that filter exiting means EOF on its standard output when read
>> by Git command, isn't it?
>>
> Isn't it that Git closes the command pipe, then filter sees EOF on it's stdin
>
> and does a graceful exit.
Correct!
- Lars
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Lars Schneider @ 2016-10-11 22:26 UTC (permalink / raw)
To: Jakub Narębski; +Cc: git, Junio C Hamano, Jeff King
In-Reply-To: <e09a63ee-dad2-a8fb-e47f-0559d9507e1c@gmail.com>
> On 09 Oct 2016, at 01:06, Jakub Narębski <jnareb@gmail.com> wrote:
>
> Part 1 of review, starting with the protocol v2 itself.
>
> W dniu 08.10.2016 o 13:25, larsxschneider@gmail.com pisze:
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> +upon checkin. By default these commands process only a single
>> +blob and terminate. If a long running `process` filter is used
>> +in place of `clean` and/or `smudge` filters, then Git can process
>> +all blobs with a single filter command invocation for the entire
>> +life of a single Git command, for example `git add --all`. See
>> +section below for the description of the protocol used to
>> +communicate with a `process` filter.
>
> I don't remember how this part looked like in previous versions
> of this patch series, but "... is used in place of `clean` ..."
> does not tell explicitly about the precedence of those
> configuration variables. I think it should be stated explicitly
> that `process` takes precedence over any `clean` and/or `smudge`
> settings for the same `filter.<driver>` (regardless of whether
> the long running `process` filter support "clean" and/or "smudge"
> operations or not).
This is stated explicitly later on. I moved it up here:
"If a long running `process` filter is used
in place of `clean` and/or `smudge` filters, then Git can process
all blobs with a single filter command invocation for the entire
life of a single Git command, for example `git add --all`. If a
long running `process` filter is configured then it always takes
precedence over a configured single blob filter. "
OK?
>> +If the filter command (a string value) is defined via
>> +`filter.<driver>.process` then Git can process all blobs with a
>> +single filter invocation for the entire life of a single Git
>> +command. This is achieved by using a packet format (pkt-line,
>> +see technical/protocol-common.txt) based protocol over standard
>> +input and standard output as follows. All packets, except for the
>> +"*CONTENT" packets and the "0000" flush packet, are considered
>> +text and therefore are terminated by a LF.
>
> Maybe s/standard input and output/\& of filter process,/ (that is,
> add "... of filter process," to the third sentence in the above
> paragraph).
You mean "This is achieved by using a packet format (pkt-line,
see technical/protocol-common.txt) based protocol over standard
input and standard output of filter process as follows." ?
I think I like the original version better.
>> After the filter started
> Git sends a welcome message ("git-filter-client"), a list of
>> supported protocol version numbers, and a flush packet. Git expects
>> +to read a welcome response message ("git-filter-server") and exactly
>> +one protocol version number from the previously sent list. All further
>> +communication will be based on the selected version. The remaining
>> +protocol description below documents "version=2". Please note that
>> +"version=42" in the example below does not exist and is only there
>> +to illustrate how the protocol would look like with more than one
>> +version.
>> +
>> +After the version negotiation Git sends a list of all capabilities that
>> +it supports and a flush packet. Git expects to read a list of desired
>> +capabilities, which must be a subset of the supported capabilities list,
>> +and a flush packet as response:
>> +------------------------
>> +packet: git> git-filter-client
>> +packet: git> version=2
>> +packet: git> version=42
>> +packet: git> 0000
>> +packet: git< git-filter-server
>> +packet: git< version=2
>> +packet: git> clean=true
>> +packet: git> smudge=true
>> +packet: git> not-yet-invented=true
>> +packet: git> 0000
>> +packet: git< clean=true
>> +packet: git< smudge=true
>> +packet: git< 0000
>
> WARNING: This example is different from description!!!
Can you try to explain the difference more clearly? I read it multiple
times and I think this is sound.
> In example you have Git sending "git-filter-client" and list of supported
> protocol versions, terminated with flush packet,
Correct.
> then filter driver
> process sends "git-filter-server", exactly one version, *AND* list of
> supported capabilities in "<capability>=true" format, terminated with
> flush packet.
Correct. That's what I read in the text and in the example.
>
> In description above the example you have 4-part handshake, not 3-part;
> the filter is described to send list of supported capabilities last
> (a subset of what Git command supports).
Part 1: Git sends a welcome message...
Part 2: Git expects to read a welcome response message...
Part 3: After the version negotiation Git sends a list of all capabilities...
Part 4: Git expects to read a list of desired capabilities...
I think example and text match, no?
> Moreover in the example in
> previous version at least as far as v8 of this series, the response
> from filter driver was fixed length list of two lines: magic string
> "git-filter-server" and exactly one line with protocol version; this
> part was *not* terminated with a flush packet (complicating code of
> filter driver program a bit, I think).
>
> I think this version of protocol is *better*, just the text needs to
> be updated to match. I wanted to propose something like this in v9,...
I didn't change that behavior since v8:
packet: git< git-filter-server
packet: git< version=2
> By the way, now I look at it, the argument for using the
> "<capability>=true" format instead of "capability=<capability>"
> (or "supported-command=<capability>") is weak. The argument for
> using "<variable>=<value>" to make it easier to implement parsing
> is sound, but the argument for "<capability>=true" is weak.
>
> The argument was that with "<capability>=true" one can simply
> parse metadata into hash / dictionary / hashmap, and choose
> response based on that. Hash / hashmap / associative array
> needs different keys, so the reasoning went for "<capability>=true"
> over "capability=<capability>"... but the filter process still
> needs to handle lines with repeating keys, namely "version=<N>"
> lines!
>
> So the argument doesn't hold water IMVHO, and we can choose
> version which reads better / is more natural.
I have to agree that "capability=<capability>" might read a
little bit nicer. However, Peff suggested "<capability>=true"
as his preference and this is absolutely OK with me.
I am happy to change that if a second reviewer shares your
opinion.
>> +Afterwards Git sends a list of "key=value" pairs terminated with
>> +a flush packet. The list will contain at least the filter command
>> +(based on the supported capabilities) and the pathname of the file
>> +to filter relative to the repository root. Right after these packets
>
> I think you meant here "right after the flush packet", isn't it?
> It would be more explicit.
I feel "right after these packets" reads better, but I agree that your
version is more explicit. I will change it.
>> Finally, a
>> +second list of "key=value" pairs terminated with a flush packet
>> +is expected. The filter can change the status in the second list.
>
> I would add here, to be more explicit:
>
> This second list of "key=value" pairs may be empty, and usually
> would be if there is nothing wrong with response or filter; the
> terminating flush packet must be here regardless.
>
> Or something like that. The above proposal could be certainly
> improved.
How about this:
"Finally, a
second list of "key=value" pairs terminated with a flush packet
is expected. The filter can change the status in the second list
or keep the status as is with an empty list. Please note that the
empty list must be terminated with a flush packet regardless."
TBH I like the original version and I wonder if the new version
is redundant?!
>> +------------------------
>> +packet: git< status=success
>> +packet: git< 0000
>> +packet: git< SMUDGED_CONTENT
>> +packet: git< 0000
>> +packet: git< 0000 # empty list, keep "status=success" unchanged!
>
> All right, looks good. Is this exclamation mark "!" necessary / wanted?
Yes, to draw the attention towards the two flushes.
>> +------------------------
>> +
>> +If the result content is empty then the filter is expected to respond
>> +with a "success" status and an empty list.
>
> Actually, it is empty content, not empty list; that is response (filter
> output) composed entirely of flush packet.
Correct!
"If the result content is empty then the filter is expected to respond
with a "success" status and a flush packet to signal the empty content."
Better?
>
>> +------------------------
>> +packet: git< status=error
>> +packet: git< 0000
>> +------------------------
>> +
>> +If the filter experiences an error during processing, then it can
>> +send the status "error" after the content was (partially or
>> +completely) sent. Depending on the `filter.<driver>.required` flag
>> +Git will interpret that as error but it will not stop or restart the
>> +filter process.
>
> Errr... this is literal repetition. You need to decide whether to
> put it before example, or after example. Or maybe split it.
Agreed. I removed the repetition and changed the previous paragraph
to:
"In case the filter cannot or does not want to process the content,
it is expected to respond with an "error" status. Git will handle
the "error" status according to the `filter.<driver>.required` flag
but it will not stop or restart the filter process."
>> +------------------------
>> +packet: git< status=success
>> +packet: git< 0000
>> +packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT
>> +packet: git< 0000
>> +packet: git< status=error
>> +packet: git< 0000
>> +------------------------
>> +
>> +If the filter dies during the communication or does not adhere to
>> +the protocol then Git will stop the filter process and restart it
>> +with the next file that needs to be processed. Depending on the
>> +`filter.<driver>.required` flag Git will interpret that as error.
>
> Uhh... until now the order was explanation, then example. From the
> duplicated description above, it is now first example, then
> description. Consistency would be good.
OK, I moved that down after the EOF exit explanation.
>> +The error handling for all cases above mimic the behavior of
>> +the `filter.<driver>.clean` / `filter.<driver>.smudge` error
>> +handling.
>
> You have "error handling" repeated here.
True. That might not be nice from a stylistic point of view but it is
precise, no?
>> +------------------------
>> +packet: git< status=abort
>> +packet: git< 0000
>> +------------------------
>> +
>> +After the filter has processed a blob it is expected to wait for
>> +the next "key=value" list containing a command. Git will close
>> +the command pipe on exit. The filter is expected to detect EOF
>> +and exit gracefully on its own.
>
> Any "kill filter" solutions should probably be put here.
Agreed.
> I guess
> that filter exiting means EOF on its standard output when read
> by Git command, isn't it?
Yes, but at this point Git is not listing anymore.
>> +If you develop your own long running filter
>> +process then the `GIT_TRACE_PACKET` environment variables can be
>> +very helpful for debugging (see linkgit:git[1]).
>
> s/environment variables/environment variable/ - there is only
> one GIT_TRACE_PACKET. Unless you wanted to write about GIT_TRACE?
Agreed.
Thanks for the review,
Lars
^ permalink raw reply
* Re: [PATCH 26/28] attr: make git_attr_counted static
From: Stefan Beller @ 2016-10-11 21:53 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <20161011173749.GC9085@google.com>
On Tue, Oct 11, 2016 at 10:37 AM, Brandon Williams <bmwill@google.com> wrote:
> On 10/10, Stefan Beller wrote:
>> It's not used outside the attr code, so let's keep it private.
>>
>> Change-Id: I0d15e0f2ea944b31d68b9cf1a4edecac0ca2170d
>
> Looks like you forgot to remove this :)
will be fixed in a reroll.
thanks!
>
> --
> Brandon Williams
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)
From: Junio C Hamano @ 2016-10-11 21:45 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kZu71+6QmvKgXJ5t+97jvTeAhSRtHpMQR9Rmk4Ep5RHhw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Tue, Oct 11, 2016 at 2:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> * sb/submodule-ignore-trailing-slash (2016-10-10) 2 commits
>> (merged to 'next' on 2016-10-11 at e37425ed17)
>> + submodule: ignore trailing slash in relative url
>> + submodule: ignore trailing slash on superproject URL
>>
>> A minor regression fix for "git submodule".
>>
>> Will merge to 'master'.
>
> Going by the bug report, this *may* be more than
> minor and worth merging down to maint as well, eventually.
The topic was forked at a reasonably old commit so that it can be
merged as far down to maint-2.9 if we wanted to. Which means the
regression was fairly old and fix is not all that urgent as well.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/2] Support `git reset --stdin`
From: Junio C Hamano @ 2016-10-11 21:49 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20161011214713.y2fpjkrx6sspks4a@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Anyway, the existence of this discussion is probably a good argument in
> favor of Dscho's patch. I was mostly curious how close our plumbing
> tools could come.
Sure, in 100% agreement.
^ permalink raw reply
* Re: [PATCH 0/2] Support `git reset --stdin`
From: Jeff King @ 2016-10-11 21:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <xmqqoa2q7elc.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 11, 2016 at 02:36:31PM -0700, Junio C Hamano wrote:
> > True. I'd have done something more like:
> >
> > git ls-tree -r $paths | git update-index --index-info
> >
> > but there are some corner cases around deleting paths from the index.
>
> Ah, I would think read-tree has the exact same issue, even if we
> added pathspec support, around removal.
>
> So it is more like
>
> (
> printf "0 0000000000000000000000000000000000000000\t%s\n" $paths
> git --literal-pathspecs ls-tree -r --ignore-missing $paths
> ) | git update-index --index-info
>
> which does not look too bad, even though this
>
> printf "%s\n" $paths | git reset --stdin
>
> does look shorter.
Of course neither of ours solutions works when "$paths" is coming on
stdin, rather than in a variable, which I suspect was Dscho's original
motivation. :)
One reason not to do the unconditional $z40 in yours is that without it,
I would hope that update-index is smart enough not to discard the stat
information for entries which are unchanged.
I suspect the best answer is more like:
git diff-index --cached HEAD | git update-index --index-info
except that you have to munge the data in between, because update-index
does not know how to pick the correct data out of the --raw diff output.
But that's probably closer to what git-reset does internally.
Anyway, the existence of this discussion is probably a good argument in
favor of Dscho's patch. I was mostly curious how close our plumbing
tools could come.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)
From: Stefan Beller @ 2016-10-11 21:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kZu71+6QmvKgXJ5t+97jvTeAhSRtHpMQR9Rmk4Ep5RHhw@mail.gmail.com>
On Tue, Oct 11, 2016 at 2:39 PM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Oct 11, 2016 at 2:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> * sb/submodule-ignore-trailing-slash (2016-10-10) 2 commits
>> (merged to 'next' on 2016-10-11 at e37425ed17)
>> + submodule: ignore trailing slash in relative url
>> + submodule: ignore trailing slash on superproject URL
>>
>> A minor regression fix for "git submodule".
>>
>> Will merge to 'master'.
>>
>
> Going by the bug report, this *may* be more than
> minor and worth merging down to maint as well, eventually.
and here is the actual bug report:
https://public-inbox.org/git/CAL4SumgJbrirymt5+iyNbpo++xXfzJZRiHDm8=0+eCArpCX=DA@mail.gmail.com/
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)
From: Stefan Beller @ 2016-10-11 21:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqq1szm8ukf.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 11, 2016 at 2:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> * sb/submodule-ignore-trailing-slash (2016-10-10) 2 commits
> (merged to 'next' on 2016-10-11 at e37425ed17)
> + submodule: ignore trailing slash in relative url
> + submodule: ignore trailing slash on superproject URL
>
> A minor regression fix for "git submodule".
>
> Will merge to 'master'.
>
Going by the bug report, this *may* be more than
minor and worth merging down to maint as well, eventually.
^ permalink raw reply
* Re: [PATCH 0/2] Support `git reset --stdin`
From: Junio C Hamano @ 2016-10-11 21:36 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20161011212644.zzqidtcgatu3qsei@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> If read-tree had pathspec support (i.e. "read these and only these
>> paths given from the command line into the index from a given
>> tree-ish"), that would have been the most natural place to extend
>> with "oh, by the way, instead of the command line, you can feed the
>> paths on the standard input".
>>
>> But it doesn't have one.
>
> True. I'd have done something more like:
>
> git ls-tree -r $paths | git update-index --index-info
>
> but there are some corner cases around deleting paths from the index.
Ah, I would think read-tree has the exact same issue, even if we
added pathspec support, around removal.
So it is more like
(
printf "0 0000000000000000000000000000000000000000\t%s\n" $paths
git --literal-pathspecs ls-tree -r --ignore-missing $paths
) | git update-index --index-info
which does not look too bad, even though this
printf "%s\n" $paths | git reset --stdin
does look shorter.
^ 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