* [PATCH v4 8/8] trailer: support values folded to multiple lines
From: Jonathan Tan @ 2016-10-20 21:39 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, sbeller, ramsay
In-Reply-To: <cover.1476998988.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.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Documentation/git-interpret-trailers.txt | 7 +-
t/t7513-interpret-trailers.sh | 169 +++++++++++++++++++++++++++++++
trailer.c | 43 ++++++--
3 files changed, 210 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 4966b5b..e99bda6 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 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 3d94b3a..4dd1d7c 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -256,6 +256,175 @@ test_expect_success 'line with leading whitespace is not trailer' '
test_cmp expected actual
'
+test_expect_success 'multiline field treated as one trailer for 25% check' '
+ q_to_tab >patch <<-\EOF &&
+
+ Signed-off-by: a <a@example.com>
+ name: value on
+ Qmultiple lines
+ this is not a trailer
+ this is not a trailer
+ this is not a trailer
+ this is not a trailer
+ this is not a trailer
+ this is not a trailer
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ Signed-off-by: a <a@example.com>
+ name: value on
+ Qmultiple lines
+ this is not a trailer
+ this is not a trailer
+ this is not a trailer
+ this is not a trailer
+ this is not a trailer
+ this is not a trailer
+ name: value
+ EOF
+ git interpret-trailers --trailer "name: value" patch >actual &&
+ 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
+ QQQQQsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ name: first line
+ QQQQQsecond line
+ 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
+ QQQQQsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ name: first line
+ QQQQQsecond line
+ 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 3ef5576..306c387 100644
--- a/trailer.c
+++ b/trailer.c
@@ -622,12 +622,14 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,
}
}
-static void add_trailer_item(struct list_head *head, char *tok, char *val)
+static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
+ char *val)
{
struct trailer_item *new = xcalloc(sizeof(*new), 1);
new->token = tok;
new->value = val;
list_add_tail(&new->list, head);
+ return new;
}
static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
@@ -727,6 +729,14 @@ static int find_trailer_start(struct strbuf **lines, int count)
{
int start, end_of_title, only_spaces = 1;
int recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;
+ /*
+ * Number of possible continuation lines encountered. This will be
+ * reset to 0 if we encounter a trailer (since those lines are to be
+ * considered continuations of that trailer), and added to
+ * non_trailer_lines if we encounter a non-trailer (since those lines
+ * are to be considered non-trailers).
+ */
+ int possible_continuation_lines = 0;
/* The first paragraph is the title and cannot be trailers */
for (start = 0; start < count; start++) {
@@ -747,11 +757,15 @@ static int find_trailer_start(struct strbuf **lines, int count)
const char **p;
int separator_pos;
- if (lines[start]->buf[0] == comment_line_char)
+ if (lines[start]->buf[0] == comment_line_char) {
+ non_trailer_lines += possible_continuation_lines;
+ possible_continuation_lines = 0;
continue;
+ }
if (contains_only_spaces(lines[start]->buf)) {
if (only_spaces)
continue;
+ non_trailer_lines += possible_continuation_lines;
if (recognized_prefix &&
trailer_lines * 3 >= non_trailer_lines)
return start + 1;
@@ -764,6 +778,7 @@ static int find_trailer_start(struct strbuf **lines, int count)
for (p = git_generated_prefixes; *p; p++) {
if (starts_with(lines[start]->buf, *p)) {
trailer_lines++;
+ possible_continuation_lines = 0;
recognized_prefix = 1;
goto continue_outer_loop;
}
@@ -774,6 +789,7 @@ static int find_trailer_start(struct strbuf **lines, int count)
struct list_head *pos;
trailer_lines++;
+ possible_continuation_lines = 0;
if (recognized_prefix)
continue;
list_for_each(pos, &conf_head) {
@@ -785,8 +801,13 @@ static int find_trailer_start(struct strbuf **lines, int count)
break;
}
}
- } else
+ } else if (isspace(lines[start]->buf[0]))
+ possible_continuation_lines++;
+ else {
non_trailer_lines++;
+ non_trailer_lines += possible_continuation_lines;
+ possible_continuation_lines = 0;
+ }
continue_outer_loop:
;
}
@@ -835,6 +856,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])
@@ -855,19 +877,28 @@ static int process_input_file(FILE *outfile,
int separator_pos;
if (lines[i]->buf[0] == comment_line_char)
continue;
+ if (last && isspace(lines[i]->buf[0])) {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addf(&sb, "%s\n%s", last->value, lines[i]->buf);
+ strbuf_strip_suffix(&sb, "\n");
+ free(last->value);
+ last->value = strbuf_detach(&sb, NULL);
+ continue;
+ }
separator_pos = find_separator(lines[i]->buf);
if (separator_pos >= 1) {
parse_trailer(&tok, &val, NULL, lines[i]->buf,
separator_pos);
- add_trailer_item(head,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL));
+ last = add_trailer_item(head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL));
} else {
strbuf_addbuf(&val, lines[i]);
strbuf_strip_suffix(&val, "\n");
add_trailer_item(head,
NULL,
strbuf_detach(&val, NULL));
+ last = NULL;
}
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer
From: Jonathan Tan @ 2016-10-20 21:39 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, sbeller, ramsay
In-Reply-To: <cover.1476998988.git.jonathantanmy@google.com>
The parse_trailer function has a few modes of operation, all depending
on whether the separator is present in its input, and if yes, the
separator's position. Some of these modes are failure modes, and these
failure modes are handled differently depending on whether the trailer
line was sourced from a file or from a command-line argument.
Extract a function to find the separator, allowing the invokers of
parse_trailer to determine how to handle the failure modes instead of
making parse_trailer do it.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
trailer.c | 70 +++++++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 48 insertions(+), 22 deletions(-)
diff --git a/trailer.c b/trailer.c
index 99018f8..137a3fb 100644
--- a/trailer.c
+++ b/trailer.c
@@ -543,29 +543,40 @@ 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;
}
-static int parse_trailer(struct strbuf *tok, struct strbuf *val,
- const struct conf_info **conf, const char *trailer)
+/*
+ * Return the location of the first separator or '=' in line, or -1 if either a
+ * newline or the null terminator is reached first.
+ */
+static int find_separator(const char *line)
+{
+ const char *c;
+ for (c = line; ; c++) {
+ if (!*c || *c == '\n')
+ return -1;
+ if (*c == '=' || strchr(separators, *c))
+ return c - line;
+ }
+}
+
+/*
+ * Obtain the token, value, and conf from the given trailer.
+ *
+ * separator_pos must not be 0, since the token cannot be an empty string.
+ *
+ * If separator_pos is -1, interpret the whole trailer as a token.
+ */
+static void parse_trailer(struct strbuf *tok, struct strbuf *val,
+ const struct conf_info **conf, const char *trailer,
+ int separator_pos)
{
- size_t len;
- struct strbuf seps = STRBUF_INIT;
struct arg_item *item;
int tok_len;
struct list_head *pos;
- strbuf_addstr(&seps, separators);
- strbuf_addch(&seps, '=');
- len = strcspn(trailer, seps.buf);
- strbuf_release(&seps);
- if (len == 0) {
- int l = strlen(trailer);
- while (l > 0 && isspace(trailer[l - 1]))
- l--;
- return error(_("empty trailer token in trailer '%.*s'"), l, trailer);
- }
- if (len < strlen(trailer)) {
- strbuf_add(tok, trailer, len);
+ if (separator_pos != -1) {
+ strbuf_add(tok, trailer, separator_pos);
strbuf_trim(tok);
- strbuf_addstr(val, trailer + len + 1);
+ strbuf_addstr(val, trailer + separator_pos + 1);
strbuf_trim(val);
} else {
strbuf_addstr(tok, trailer);
@@ -587,8 +598,6 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
break;
}
}
-
- return 0;
}
static void add_trailer_item(struct list_head *head, char *tok, char *val)
@@ -631,11 +640,22 @@ static void process_command_line_args(struct list_head *arg_head,
/* 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))
+ int separator_pos = find_separator(tr->string);
+ if (separator_pos == 0) {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addstr(&sb, tr->string);
+ strbuf_trim(&sb);
+ error(_("empty trailer token in trailer '%.*s'"),
+ (int) sb.len, sb.buf);
+ strbuf_release(&sb);
+ } else {
+ parse_trailer(&tok, &val, &conf, tr->string,
+ separator_pos);
add_arg_item(arg_head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL),
conf);
+ }
}
}
@@ -775,11 +795,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))
+ int separator_pos;
+ if (lines[i]->buf[0] == comment_line_char)
+ continue;
+ separator_pos = find_separator(lines[i]->buf);
+ if (separator_pos >= 1) {
+ parse_trailer(&tok, &val, NULL, lines[i]->buf,
+ separator_pos);
add_trailer_item(head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL));
+ }
}
return trailer_end;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v4 6/8] trailer: allow non-trailers in trailer block
From: Jonathan Tan @ 2016-10-20 21:39 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, sbeller, ramsay
In-Reply-To: <cover.1476998988.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 "\nSigned-off-by: x\nnot trailer" |
git interpret-trailers --trailer "c: d"
would result in:
Signed-off-by: x
not trailer
c: d
Relax the definition of a trailer block to require that the trailers (i)
are all trailers, or (ii) contain at least one Git-generated trailer and
consists of at least 25% trailers.
Signed-off-by: x
not trailer
c: d
(i) is the existing functionality. (ii) allows arbitrary lines to be
included in trailer blocks, like those in [1], and still allow
interpret-trailers to be used.
[1]
https://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/e7d316a02f683864a12389f8808570e37fb90aa3
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Documentation/git-interpret-trailers.txt | 5 +-
t/t7513-interpret-trailers.sh | 115 +++++++++++++++++++++++++++++++
trailer.c | 89 ++++++++++++++++++++----
3 files changed, 194 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 93d1db6..cf4c5ea 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -48,8 +48,9 @@ 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
-the group is preceded by one or more empty (or whitespace-only) lines.
+a group of one or more lines that (i) are all trailers, or (ii) contains at
+least one Git-generated trailer and consists of at least 25% trailers.
+The group must be 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
minus signs start the patch part of the message.
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index aee785c..003e90f 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -126,6 +126,121 @@ test_expect_success 'with multiline title in the message' '
test_cmp expected actual
'
+test_expect_success 'with non-trailer lines mixed with Signed-off-by' '
+ cat >patch <<-\EOF &&
+
+ this is not a trailer
+ this is not a trailer
+ Signed-off-by: a <a@example.com>
+ this is not a trailer
+ EOF
+ cat >expected <<-\EOF &&
+
+ this is not a trailer
+ this is not a trailer
+ Signed-off-by: a <a@example.com>
+ 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 mixed with cherry picked from' '
+ cat >patch <<-\EOF &&
+
+ this is not a trailer
+ this is not a trailer
+ (cherry picked from commit x)
+ this is not a trailer
+ EOF
+ cat >expected <<-\EOF &&
+
+ this is not a trailer
+ this is not a trailer
+ (cherry picked from commit x)
+ 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 mixed with a configured trailer' '
+ cat >patch <<-\EOF &&
+
+ this is not a trailer
+ this is not a trailer
+ My-trailer: x
+ this is not a trailer
+ EOF
+ cat >expected <<-\EOF &&
+
+ this is not a trailer
+ this is not a trailer
+ My-trailer: x
+ this is not a trailer
+ token: value
+ EOF
+ test_config trailer.my.key "My-trailer: " &&
+ git interpret-trailers --trailer "token: value" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'with non-trailer lines mixed with a non-configured trailer' '
+ cat >patch <<-\EOF &&
+
+ this is not a trailer
+ this is not a trailer
+ I-am-not-configured: x
+ this is not a trailer
+ EOF
+ cat >expected <<-\EOF &&
+
+ this is not a trailer
+ this is not a trailer
+ I-am-not-configured: x
+ this is not a trailer
+
+ token: value
+ EOF
+ test_config trailer.my.key "My-trailer: " &&
+ git interpret-trailers --trailer "token: value" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'with all non-configured trailers' '
+ cat >patch <<-\EOF &&
+
+ I-am-not-configured: x
+ I-am-also-not-configured: x
+ EOF
+ cat >expected <<-\EOF &&
+
+ I-am-not-configured: x
+ I-am-also-not-configured: x
+ token: value
+ EOF
+ test_config trailer.my.key "My-trailer: " &&
+ 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 &&
diff --git a/trailer.c b/trailer.c
index 137a3fb..da15b79 100644
--- a/trailer.c
+++ b/trailer.c
@@ -27,6 +27,10 @@ static struct conf_info default_conf_info;
struct trailer_item {
struct list_head list;
+ /*
+ * 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;
};
@@ -44,6 +48,12 @@ static char *separators = ":";
#define TRAILER_ARG_STRING "$ARG"
+static const char *git_generated_prefixes[] = {
+ "Signed-off-by: ",
+ "(cherry picked from commit ",
+ NULL
+};
+
/* Iterate over the elements of the list. */
#define list_for_each_dir(pos, head, is_reverse) \
for (pos = is_reverse ? (head)->prev : (head)->next; \
@@ -70,9 +80,14 @@ static size_t token_len_without_separator(const char *token, size_t len)
static int same_token(struct trailer_item *a, struct arg_item *b)
{
- size_t a_len = token_len_without_separator(a->token, strlen(a->token));
- size_t b_len = token_len_without_separator(b->token, strlen(b->token));
- 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);
}
@@ -130,7 +145,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))
@@ -704,6 +726,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 recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;
/* The first paragraph is the title and cannot be trailers */
for (start = 0; start < count; start++) {
@@ -715,26 +738,60 @@ static int find_trailer_start(struct strbuf **lines, int count)
end_of_title = start;
/*
- * Get the start of the trailers by looking starting from the end
- * for a line with only spaces before lines with one separator.
+ * Get the start of the trailers by looking starting from the end for a
+ * blank line before a set of non-blank lines that (i) are all
+ * trailers, or (ii) contains at least one Git-generated trailer and
+ * consists of at least 25% trailers.
*/
for (start = count - 1; start >= end_of_title; start--) {
+ const char **p;
+ int separator_pos;
+
if (lines[start]->buf[0] == comment_line_char)
continue;
if (contains_only_spaces(lines[start]->buf)) {
if (only_spaces)
continue;
- return start + 1;
+ if (recognized_prefix &&
+ trailer_lines * 3 >= non_trailer_lines)
+ return start + 1;
+ if (trailer_lines && !non_trailer_lines)
+ return start + 1;
+ return count;
}
- if (strcspn(lines[start]->buf, separators) < lines[start]->len) {
- if (only_spaces)
- only_spaces = 0;
- continue;
+ only_spaces = 0;
+
+ for (p = git_generated_prefixes; *p; p++) {
+ if (starts_with(lines[start]->buf, *p)) {
+ trailer_lines++;
+ recognized_prefix = 1;
+ goto continue_outer_loop;
+ }
}
- return count;
+
+ separator_pos = find_separator(lines[start]->buf);
+ if (separator_pos >= 1) {
+ struct list_head *pos;
+
+ trailer_lines++;
+ if (recognized_prefix)
+ continue;
+ list_for_each(pos, &conf_head) {
+ struct arg_item *item;
+ item = list_entry(pos, struct arg_item, list);
+ if (token_matches_item(lines[start]->buf, item,
+ separator_pos)) {
+ recognized_prefix = 1;
+ break;
+ }
+ }
+ } else
+ non_trailer_lines++;
+continue_outer_loop:
+ ;
}
- return only_spaces ? count : 0;
+ return count;
}
/* Get the index of the end of the trailers */
@@ -805,6 +862,12 @@ static int process_input_file(FILE *outfile,
add_trailer_item(head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL));
+ } else {
+ strbuf_addbuf(&val, lines[i]);
+ strbuf_strip_suffix(&val, "\n");
+ add_trailer_item(head,
+ NULL,
+ strbuf_detach(&val, NULL));
}
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v4 2/8] trailer: use list.h for doubly-linked list
From: Jonathan Tan @ 2016-10-20 21:39 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, sbeller, ramsay
In-Reply-To: <cover.1476998988.git.jonathantanmy@google.com>
Replace the existing handwritten implementation of a doubly-linked list
in trailer.c with the functions and macros from list.h. This
significantly simplifies the code.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
trailer.c | 258 ++++++++++++++++++++++----------------------------------------
1 file changed, 91 insertions(+), 167 deletions(-)
diff --git a/trailer.c b/trailer.c
index 1f191b2..4e85aae 100644
--- a/trailer.c
+++ b/trailer.c
@@ -4,6 +4,7 @@
#include "commit.h"
#include "tempfile.h"
#include "trailer.h"
+#include "list.h"
/*
* Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
*/
@@ -25,19 +26,24 @@ struct conf_info {
static struct conf_info default_conf_info;
struct trailer_item {
- struct trailer_item *previous;
- struct trailer_item *next;
+ struct list_head list;
char *token;
char *value;
struct conf_info conf;
};
-static struct trailer_item *first_conf_item;
+static LIST_HEAD(conf_head);
static char *separators = ":";
#define TRAILER_ARG_STRING "$ARG"
+/* Iterate over the elements of the list. */
+#define list_for_each_dir(pos, head, is_reverse) \
+ for (pos = is_reverse ? (head)->prev : (head)->next; \
+ pos != (head); \
+ pos = is_reverse ? pos->prev : pos->next)
+
static int after_or_end(enum action_where where)
{
return (where == WHERE_AFTER) || (where == WHERE_END);
@@ -120,101 +126,49 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-static void print_all(FILE *outfile, struct trailer_item *first, int trim_empty)
+static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
{
+ struct list_head *pos;
struct trailer_item *item;
- for (item = first; item; item = item->next) {
+ list_for_each(pos, head) {
+ item = list_entry(pos, struct trailer_item, list);
if (!trim_empty || strlen(item->value) > 0)
print_tok_val(outfile, item->token, item->value);
}
}
-static void update_last(struct trailer_item **last)
-{
- if (*last)
- while ((*last)->next != NULL)
- *last = (*last)->next;
-}
-
-static void update_first(struct trailer_item **first)
-{
- if (*first)
- while ((*first)->previous != NULL)
- *first = (*first)->previous;
-}
-
static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct trailer_item *arg_tok,
- struct trailer_item **first,
- struct trailer_item **last)
-{
- if (after_or_end(arg_tok->conf.where)) {
- arg_tok->next = on_tok->next;
- on_tok->next = arg_tok;
- arg_tok->previous = on_tok;
- if (arg_tok->next)
- arg_tok->next->previous = arg_tok;
- update_last(last);
- } else {
- arg_tok->previous = on_tok->previous;
- on_tok->previous = arg_tok;
- arg_tok->next = on_tok;
- if (arg_tok->previous)
- arg_tok->previous->next = arg_tok;
- update_first(first);
- }
+ struct trailer_item *arg_tok)
+{
+ if (after_or_end(arg_tok->conf.where))
+ list_add(&arg_tok->list, &on_tok->list);
+ else
+ list_add_tail(&arg_tok->list, &on_tok->list);
}
static int check_if_different(struct trailer_item *in_tok,
struct trailer_item *arg_tok,
- int check_all)
+ int check_all,
+ struct list_head *head)
{
enum action_where where = arg_tok->conf.where;
+ struct list_head *next_head;
do {
- if (!in_tok)
- return 1;
if (same_trailer(in_tok, arg_tok))
return 0;
/*
* if we want to add a trailer after another one,
* we have to check those before this one
*/
- in_tok = after_or_end(where) ? in_tok->previous : in_tok->next;
+ next_head = after_or_end(where) ? in_tok->list.prev
+ : in_tok->list.next;
+ if (next_head == head)
+ break;
+ in_tok = list_entry(next_head, struct trailer_item, list);
} while (check_all);
return 1;
}
-static void remove_from_list(struct trailer_item *item,
- struct trailer_item **first,
- struct trailer_item **last)
-{
- struct trailer_item *next = item->next;
- struct trailer_item *previous = item->previous;
-
- if (next) {
- item->next->previous = previous;
- item->next = NULL;
- } else if (last)
- *last = previous;
-
- if (previous) {
- item->previous->next = next;
- item->previous = NULL;
- } else if (first)
- *first = next;
-}
-
-static struct trailer_item *remove_first(struct trailer_item **first)
-{
- struct trailer_item *item = *first;
- *first = item->next;
- if (item->next) {
- item->next->previous = NULL;
- item->next = NULL;
- }
- return item;
-}
-
static char *apply_command(const char *command, const char *arg)
{
struct strbuf cmd = STRBUF_INIT;
@@ -266,8 +220,7 @@ static void apply_item_command(struct trailer_item *in_tok, struct trailer_item
static void apply_arg_if_exists(struct trailer_item *in_tok,
struct trailer_item *arg_tok,
struct trailer_item *on_tok,
- struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last)
+ struct list_head *head)
{
switch (arg_tok->conf.if_exists) {
case EXISTS_DO_NOTHING:
@@ -275,40 +228,34 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
break;
case EXISTS_REPLACE:
apply_item_command(in_tok, arg_tok);
- add_arg_to_input_list(on_tok, arg_tok,
- in_tok_first, in_tok_last);
- remove_from_list(in_tok, in_tok_first, in_tok_last);
+ add_arg_to_input_list(on_tok, arg_tok);
+ list_del(&in_tok->list);
free_trailer_item(in_tok);
break;
case EXISTS_ADD:
apply_item_command(in_tok, arg_tok);
- add_arg_to_input_list(on_tok, arg_tok,
- in_tok_first, in_tok_last);
+ add_arg_to_input_list(on_tok, arg_tok);
break;
case EXISTS_ADD_IF_DIFFERENT:
apply_item_command(in_tok, arg_tok);
- if (check_if_different(in_tok, arg_tok, 1))
- add_arg_to_input_list(on_tok, arg_tok,
- in_tok_first, in_tok_last);
+ if (check_if_different(in_tok, arg_tok, 1, head))
+ add_arg_to_input_list(on_tok, arg_tok);
else
free_trailer_item(arg_tok);
break;
case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
apply_item_command(in_tok, arg_tok);
- if (check_if_different(on_tok, arg_tok, 0))
- add_arg_to_input_list(on_tok, arg_tok,
- in_tok_first, in_tok_last);
+ if (check_if_different(on_tok, arg_tok, 0, head))
+ add_arg_to_input_list(on_tok, arg_tok);
else
free_trailer_item(arg_tok);
break;
}
}
-static void apply_arg_if_missing(struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last,
+static void apply_arg_if_missing(struct list_head *head,
struct trailer_item *arg_tok)
{
- struct trailer_item **in_tok;
enum action_where where;
switch (arg_tok->conf.if_missing) {
@@ -317,68 +264,60 @@ static void apply_arg_if_missing(struct trailer_item **in_tok_first,
break;
case MISSING_ADD:
where = arg_tok->conf.where;
- in_tok = after_or_end(where) ? in_tok_last : in_tok_first;
apply_item_command(NULL, arg_tok);
- if (*in_tok) {
- add_arg_to_input_list(*in_tok, arg_tok,
- in_tok_first, in_tok_last);
- } else {
- *in_tok_first = arg_tok;
- *in_tok_last = arg_tok;
- }
- break;
+ if (after_or_end(where))
+ list_add_tail(&arg_tok->list, head);
+ else
+ list_add(&arg_tok->list, head);
}
}
-static int find_same_and_apply_arg(struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last,
+static int find_same_and_apply_arg(struct list_head *head,
struct trailer_item *arg_tok)
{
+ struct list_head *pos;
struct trailer_item *in_tok;
struct trailer_item *on_tok;
- struct trailer_item *following_tok;
enum action_where where = arg_tok->conf.where;
int middle = (where == WHERE_AFTER) || (where == WHERE_BEFORE);
int backwards = after_or_end(where);
- struct trailer_item *start_tok = backwards ? *in_tok_last : *in_tok_first;
+ struct trailer_item *start_tok;
- for (in_tok = start_tok; in_tok; in_tok = following_tok) {
- following_tok = backwards ? in_tok->previous : in_tok->next;
+ if (list_empty(head))
+ return 0;
+
+ start_tok = list_entry(backwards ? head->prev : head->next,
+ struct trailer_item,
+ list);
+
+ list_for_each_dir(pos, head, backwards) {
+ in_tok = list_entry(pos, struct trailer_item, list);
if (!same_token(in_tok, arg_tok))
continue;
on_tok = middle ? in_tok : start_tok;
- apply_arg_if_exists(in_tok, arg_tok, on_tok,
- in_tok_first, in_tok_last);
+ apply_arg_if_exists(in_tok, arg_tok, on_tok, head);
return 1;
}
return 0;
}
-static void process_trailers_lists(struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last,
- struct trailer_item **arg_tok_first)
+static void process_trailers_lists(struct list_head *head,
+ struct list_head *arg_head)
{
+ struct list_head *pos, *p;
struct trailer_item *arg_tok;
- struct trailer_item *next_arg;
-
- if (!*arg_tok_first)
- return;
- for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
+ list_for_each_safe(pos, p, arg_head) {
int applied = 0;
+ arg_tok = list_entry(pos, struct trailer_item, list);
- next_arg = arg_tok->next;
- remove_from_list(arg_tok, arg_tok_first, NULL);
+ list_del(pos);
- applied = find_same_and_apply_arg(in_tok_first,
- in_tok_last,
- arg_tok);
+ applied = find_same_and_apply_arg(head, arg_tok);
if (!applied)
- apply_arg_if_missing(in_tok_first,
- in_tok_last,
- arg_tok);
+ apply_arg_if_missing(head, arg_tok);
}
}
@@ -438,13 +377,12 @@ static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
static struct trailer_item *get_conf_item(const char *name)
{
+ struct list_head *pos;
struct trailer_item *item;
- struct trailer_item *previous;
/* Look up item with same name */
- for (previous = NULL, item = first_conf_item;
- item;
- previous = item, item = item->next) {
+ list_for_each(pos, &conf_head) {
+ item = list_entry(pos, struct trailer_item, list);
if (!strcasecmp(item->conf.name, name))
return item;
}
@@ -454,12 +392,7 @@ static struct trailer_item *get_conf_item(const char *name)
duplicate_conf(&item->conf, &default_conf_info);
item->conf.name = xstrdup(name);
- if (!previous)
- first_conf_item = item;
- else {
- previous->next = item;
- item->previous = previous;
- }
+ list_add_tail(&item->list, &conf_head);
return item;
}
@@ -633,6 +566,7 @@ static struct trailer_item *create_trailer_item(const char *string)
struct strbuf val = STRBUF_INIT;
struct trailer_item *item;
int tok_len;
+ struct list_head *pos;
if (parse_trailer(&tok, &val, string))
return NULL;
@@ -640,7 +574,8 @@ static struct trailer_item *create_trailer_item(const char *string)
tok_len = token_len_without_separator(tok.buf, tok.len);
/* Lookup if the token matches something in the config */
- for (item = first_conf_item; item; item = item->next) {
+ list_for_each(pos, &conf_head) {
+ item = list_entry(pos, struct trailer_item, list);
if (token_matches_item(tok.buf, item, tok_len))
return new_trailer_item(item,
strbuf_detach(&tok, NULL),
@@ -652,44 +587,34 @@ static struct trailer_item *create_trailer_item(const char *string)
strbuf_detach(&val, NULL));
}
-static void add_trailer_item(struct trailer_item **first,
- struct trailer_item **last,
- struct trailer_item *new)
+static void add_trailer_item(struct list_head *head, struct trailer_item *new)
{
if (!new)
return;
- if (!*last) {
- *first = new;
- *last = new;
- } else {
- (*last)->next = new;
- new->previous = *last;
- *last = new;
- }
+ list_add_tail(&new->list, head);
}
-static struct trailer_item *process_command_line_args(struct string_list *trailers)
+static void process_command_line_args(struct list_head *arg_head,
+ struct string_list *trailers)
{
- struct trailer_item *arg_tok_first = NULL;
- struct trailer_item *arg_tok_last = NULL;
struct string_list_item *tr;
struct trailer_item *item;
+ struct list_head *pos;
/* Add a trailer item for each configured trailer with a command */
- for (item = first_conf_item; item; item = item->next) {
+ list_for_each(pos, &conf_head) {
+ item = list_entry(pos, struct trailer_item, list);
if (item->conf.command) {
struct trailer_item *new = new_trailer_item(item, NULL, NULL);
- add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+ add_trailer_item(arg_head, new);
}
}
/* Add a trailer item for each trailer on the command line */
for_each_string_list_item(tr, trailers) {
struct trailer_item *new = create_trailer_item(tr->string);
- add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+ add_trailer_item(arg_head, new);
}
-
- return arg_tok_first;
}
static struct strbuf **read_input_file(const char *file)
@@ -805,8 +730,7 @@ static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end
static int process_input_file(FILE *outfile,
struct strbuf **lines,
- struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last)
+ struct list_head *head)
{
int count = 0;
int patch_start, trailer_start, trailer_end, i;
@@ -829,18 +753,19 @@ static int process_input_file(FILE *outfile,
for (i = trailer_start; i < trailer_end; i++) {
if (lines[i]->buf[0] != comment_line_char) {
struct trailer_item *new = create_trailer_item(lines[i]->buf);
- add_trailer_item(in_tok_first, in_tok_last, new);
+ add_trailer_item(head, new);
}
}
return trailer_end;
}
-static void free_all(struct trailer_item **first)
+static void free_all(struct list_head *head)
{
- while (*first) {
- struct trailer_item *item = remove_first(first);
- free_trailer_item(item);
+ struct list_head *pos, *p;
+ list_for_each_safe(pos, p, head) {
+ list_del(pos);
+ free_trailer_item(list_entry(pos, struct trailer_item, list));
}
}
@@ -877,9 +802,8 @@ static FILE *create_in_place_tempfile(const char *file)
void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)
{
- struct trailer_item *in_tok_first = NULL;
- struct trailer_item *in_tok_last = NULL;
- struct trailer_item *arg_tok_first;
+ LIST_HEAD(head);
+ LIST_HEAD(arg_head);
struct strbuf **lines;
int trailer_end;
FILE *outfile = stdout;
@@ -894,15 +818,15 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
outfile = create_in_place_tempfile(file);
/* Print the lines before the trailers */
- trailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);
+ trailer_end = process_input_file(outfile, lines, &head);
- arg_tok_first = process_command_line_args(trailers);
+ process_command_line_args(&arg_head, trailers);
- process_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);
+ process_trailers_lists(&head, &arg_head);
- print_all(outfile, in_tok_first, trim_empty);
+ print_all(outfile, &head, trim_empty);
- free_all(&in_tok_first);
+ free_all(&head);
/* Print the lines after the trailers as is */
print_lines(outfile, lines, trailer_end, INT_MAX);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v4 0/8] allow non-trailers and multiple-line trailers
From: Jonathan Tan @ 2016-10-20 21:39 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, sbeller, ramsay
In-Reply-To: <cover.1476232683.git.jonathantanmy@google.com>
Main changes are:
- implemented the previously discussed trailer block recognizing rule
(recognized trailer + 25% trailers or 100% trailers)
- forbidding leading whitespace in trailers to avoid false positives
Once the recognized trailer + 25% trailers rule is implemented,
implementing the 100% trailer rule gives us backwards compatibility and
is only a few lines of code, so I included it.
Summary of changes from v3:
2/6->2/8:
- squashed Ramsay Jones's "static" patch
new->5/8:
- new patch
5/6->6/8:
- new trailer block recognizing rule
- reverted to the existing behavior of ignoring comments, since the
number of trailers and non-trailers in the trailer block now
matters more
new->7/8:
- new patch
6/6->8/8:
- updated trailer block recognizing code, since the continuation
lines must not be counted if they follow a trailer line
Jonathan Tan (8):
trailer: improve const correctness
trailer: use list.h for doubly-linked list
trailer: streamline trailer item create and add
trailer: make args have their own struct
trailer: clarify failure modes in parse_trailer
trailer: allow non-trailers in trailer block
trailer: forbid leading whitespace in trailers
trailer: support values folded to multiple lines
Documentation/git-interpret-trailers.txt | 14 +-
t/t7513-interpret-trailers.sh | 299 +++++++++++++++
trailer.c | 619 +++++++++++++++++--------------
3 files changed, 651 insertions(+), 281 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [PATCH v4 1/8] trailer: improve const correctness
From: Jonathan Tan @ 2016-10-20 21:39 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, sbeller, ramsay
In-Reply-To: <cover.1476998988.git.jonathantanmy@google.com>
Change "const char *" to "char *" in struct trailer_item and in the
return value of apply_command (since those strings are owned strings).
Change "struct conf_info *" to "const struct conf_info *" (since that
struct is not modified).
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
trailer.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/trailer.c b/trailer.c
index c6ea9ac..1f191b2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -27,8 +27,8 @@ static struct conf_info default_conf_info;
struct trailer_item {
struct trailer_item *previous;
struct trailer_item *next;
- const char *token;
- const char *value;
+ char *token;
+ char *value;
struct conf_info conf;
};
@@ -95,8 +95,8 @@ static void free_trailer_item(struct trailer_item *item)
free(item->conf.name);
free(item->conf.key);
free(item->conf.command);
- free((char *)item->token);
- free((char *)item->value);
+ free(item->token);
+ free(item->value);
free(item);
}
@@ -215,13 +215,13 @@ static struct trailer_item *remove_first(struct trailer_item **first)
return item;
}
-static const char *apply_command(const char *command, const char *arg)
+static char *apply_command(const char *command, const char *arg)
{
struct strbuf cmd = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
struct child_process cp = CHILD_PROCESS_INIT;
const char *argv[] = {NULL, NULL};
- const char *result;
+ char *result;
strbuf_addstr(&cmd, command);
if (arg)
@@ -425,7 +425,7 @@ static int set_if_missing(struct conf_info *item, const char *value)
return 0;
}
-static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
+static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
{
*dst = *src;
if (src->name)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH 2/2] tag: send fully qualified refnames to verify_tag_and_format
From: Jeff King @ 2016-10-20 21:39 UTC (permalink / raw)
To: Santiago Torres; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161020165700.qwgli5mbya3d7nzz@LykOS.localdomain>
On Thu, Oct 20, 2016 at 12:57:01PM -0400, Santiago Torres wrote:
> > I think you'd really just squash the various bits of this into your
> > series at the right spots, though I don't mind it on top, either.
> >
> > The big question is to what degree we should care about the verify-tag
> > case. I don't think it's any worse off with this change than it is with
> > your series (its "kind" becomes "OTHER", but I don't think that is
> > actually used for display at all; the name remains the same). I'd be OK
> > with leaving it like this, as a known bug, until get_sha1_with_context()
> > learns to tell us about the ref. It's an unhandled corner case in a
> > brand-new feature, not a regression in an existing one.
>
> I see now, I think I can sprinkle some of these changes on 2/7 then. The
> rest should be doing 4/7 and 5/7. Does this sound ok?
Yep, that sounds about right.
-Peff
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Jeff King @ 2016-10-20 21:38 UTC (permalink / raw)
To: Stefan Beller; +Cc: Johannes Schindelin, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <CAGZ79kZpj5xXHmnA+JfLKdGmgzp7Mut1OsKMOeowpw8m1+aKGA@mail.gmail.com>
On Thu, Oct 20, 2016 at 12:54:32PM -0700, Stefan Beller wrote:
> Maybe we should stop introducing un-optimized tests.
> [...]
> * heavy use of the "git -C <dir>" pattern. When applying that
> thouroughly we'd save spanning the subshells.
Yeah, I imagine with some style changes we could drop quite a few
subshells. The problem is that the conversion work is manual and
tedious. I'd look first for spots where we can eliminate thousands of
calls with a single change.
> That said I really like the idea of having a helper that would eliminate the cat
> for you, e.g. :
>
> git_test_helper_equal_stdin_or_diff_and_die -C super_repo status
> --porcelain=v2 --branch --untracked-files=all <<-EOF
> 1 A. N... 000000 100644 100644 $_z40 $HMOD .gitmodules
> 1 AM S.M. 000000 160000 160000 $_z40 $HSUP sub1
> EOF
I think that helper still ends up using "cat" and "diff" under the hood,
unless you write those bits in pure shell. But at that point, I suspect
we could "cat" and "test_cmp" in pure shell, something like:
cat () {
# optimize common here-doc usage
if test $# -eq 0
then
while read -r line
do
printf '%s' "$line"
done
fi
command cat "$@"
}
test_cmp () {
# optimize for common "they are the same" case
# without any subshells or subprograms
while true; do
if ! read -r line1 <&3
then
if ! read -r line2 <&4
# EOF on both; good
return 0
else
# EOF only on file1; fail
break
fi
fi
if ! read -r line2 <&4
then
# EOF only on file2; fail
break
fi
test "$line1" = "$line2" || break
done 3<"$1" 4<"$2"
# if we get here, the optimized version found some
# difference. We can just "return 1", but let's run
# the real $GIT_TEST_CMP to provide pretty output.
# This should generally only happen on test failures,
# so performance isn't a big deal.
"$GIT_TEST_CMP" "$@"
}
Those are both completely untested. But maybe they are worth playing
around with for somebody on Windows to see if they make a dent in the
test runtime.
-Peff
^ permalink raw reply
* Re: [ANNOUNCE] git-log-compact v1.0
From: Kevin Daudt @ 2016-10-20 21:29 UTC (permalink / raw)
To: Kyle J. McKay; +Cc: Git mailing list, Jakub Narebski, Christian Couder
In-Reply-To: <git-log-compact-announce-v1@mackyle-at-gmail-com>
On Wed, Oct 19, 2016 at 05:13:34PM -0700, Kyle J. McKay wrote:
>
> The project page with detailed help and many screen shots is located at:
>
> https://mackyle.github.io/git-log-compact/
>
> Alternatively the repository can be cloned from:
>
> https://github.com/mackyle/git-log-compact.git
>
> Or the script file itself (which is really all you need) can be
> viewed/fetched from:
>
> https://github.com/mackyle/git-log-compact/blob/HEAD/git-log-compact
>
> The git-log-compact script should work with any version of Git released
> in the last several years.
>
> --Kyle
>
> [1] https://git.github.io/rev_news/2016/10/19/edition-20/
I've packaged up an arch AUR package[1] for it if anyone is interested.
[1]:https://aur.archlinux.org/packages/git-log-compact
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Jeff King @ 2016-10-20 21:03 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, Junio C Hamano, git
In-Reply-To: <530a3bca-e251-cb43-fb6a-e99c1e64a0a7@kdbg.org>
On Thu, Oct 20, 2016 at 10:38:23PM +0200, Johannes Sixt wrote:
> Am 20.10.2016 um 14:31 schrieb Jeff King:
> > Close to 1/3 of those processes are just invoking the bin-wrapper
> > script to set up the EXEC_PATH, etc. I imagine it would not be too hard
> > to just do that in the test script. In fact, it looks like:
> >
> > make prefix=/wherever install
> > GIT_TEST_INSTALLED=/wherever/bin make test
> >
> > might give you an immediate speedup by skipping bin-wrappers entirely.
>
> Running the tests with --with-dashes should give you the same effect, no?
Yeah, looks like it. it still uses bin-wrappers for t/helper, but that
should be a minority of calls.
Which I think explains why I saw some test failures with the
GIT_TEST_INSTALLED above. It does not know about t/helper, but relies on
those programs being present in $GIT_BUILD_DIR. So I suspect it has been
totally broken since e6e7530d10 (test helpers: move test-* to t/helper/
subdirectory, 2016-04-13).
-Peff
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Jeff King @ 2016-10-20 21:00 UTC (permalink / raw)
To: Stefan Beller; +Cc: Johannes Schindelin, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <CAGZ79ka_McRkOyKH3diCXJSdCm+4pNZfqAqJW_tQLFdhb26tRQ@mail.gmail.com>
On Thu, Oct 20, 2016 at 09:30:27AM -0700, Stefan Beller wrote:
> On Thu, Oct 20, 2016 at 5:31 AM, Jeff King <peff@peff.net> wrote:
>
> >
> > $ perl -lne '/execve\("(.*?)"/ and print $1' /tmp/foo.out | sort | uniq -c | sort -rn | head
> > 152271 /home/peff/compile/git/git
> > 57340 /home/peff/compile/git/t/../bin-wrappers/git
> > 16865 /bin/sed
> > 12650 /bin/rm
> > 11257 /bin/cat
> > 9326 /home/peff/compile/git/git-sh-i18n--envsubst
> > 9079 /usr/bin/diff
> > 8013 /usr/bin/wc
> > 5924 /bin/mv
> > 4566 /bin/grep
> >
>
> I am not an expert on perl nor tracing, but is it feasible to find out
> how many internal calls there are? i.e. either some shell script (rebase,
> submodule) calling git itself a couple of times or even from compile/git/git
> itself, e.g. some submodule operations use forking in there.
The script below is my attempt, though I think it is not quite right, as
"make" should be the single apex of the graph. You can run it like:
strace -f -o /tmp/foo.out -e clone,execve make test
perl graph.pl /tmp/foo.out | less -S
One thing that it counts (that was not counted above) is the number of
forks for subshells, which is considerable. I don't know how expensive
that is versus, say, running "cat" (if your fork() doesn't
copy-on-write, and you implement sub-programs via an efficient spawn()
call, it's possible that the subshells are significantly more
expensive).
-Peff
-- >8 --
#!/usr/bin/perl
my %clone;
my %exec;
my %is_child;
my %counter;
while (<>) {
# <pid> execve("some-prog", ...
if (/^(\d+)\s+execve\("(.*?)"/) {
push @{$exec{node($1)}}, $2;
}
# <pid> clone(...) = <child>
# or
# <pid> <... clone resumed> ...) = <child>
elsif (/^(\d+)\s+.*clone.*\) = (\d+)$/) {
push @{$clone{node($1)}}, node($2);
$is_child{node($2)} = 1;
}
# <pid> +++ exited with <code> +++
# We have to keep track of this because pids get recycled,
# and so are not unique node names in our graph.
elsif (/^(\d+)\s+.*exited with/) {
$counter{$1}++;
}
}
show($_, 0) for grep { !$is_child{$_} } keys(%clone);
sub show {
my ($pid, $indent) = @_;
my @progs = @{$exec{$pid}};
if (!@progs) {
@progs = ("(fork)");
}
print ' ' x $indent;
print "$pid: ", shift @progs;
print " => $_" for @progs;
print "\n";
show($_, $indent + 2) for @{$clone{$pid}};
}
sub node {
my $pid = shift;
my $c = $counter{$pid} || "0";
return "$pid-$c";
}
^ permalink raw reply
* [PATCH v3] rev-list: use hdr_termination instead of a always using a newline
From: Jacob Keller @ 2016-10-20 20:41 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dennis Kaarsemaker, Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
When adding support for prefixing output of log and other commands using
--line-prefix, commit 660e113ce118 ("graph: add support for
--line-prefix on all graph-aware output", 2016-08-31) accidentally
broke rev-list --header output.
In order to make the output appear with a line-prefix, the flow was
changed to always use the graph subsystem for display. Unfortunately
the graph flow in rev-list did not use info->hdr_termination as it was
assumed that graph output would never need to putput NULs.
Since we now always use the graph code in order to handle the case of
line-prefix, simply replace putchar('\n') with
putchar(info->hdr_termination) which will correct this issue.
Add a test for the --header case to make sure we don't break it in the
future.
Reported-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Changes in v2
* Squash Junio's suggested (better) test
* Add Junio's signed-off-by since he wrote the new test
Changes in v3
* Fix commit description to not reference the no longer existing test
builtin/rev-list.c | 2 +-
t/t6000-rev-list-misc.sh | 14 ++++++++++++++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git c/builtin/rev-list.c c/builtin/rev-list.c
index 8479f6ed28aa..c43decda7011 100644
--- c/builtin/rev-list.c
+++ c/builtin/rev-list.c
@@ -145,7 +145,7 @@ static void show_commit(struct commit *commit, void *data)
*/
if (buf.len && buf.buf[buf.len - 1] == '\n')
graph_show_padding(revs->graph);
- putchar('\n');
+ putchar(info->hdr_termination);
} else {
/*
* If the message buffer is empty, just show
diff --git c/t/t6000-rev-list-misc.sh c/t/t6000-rev-list-misc.sh
index 3e752ce03280..969e4e9e5261 100755
--- c/t/t6000-rev-list-misc.sh
+++ c/t/t6000-rev-list-misc.sh
@@ -100,4 +100,18 @@ test_expect_success '--bisect and --first-parent can not be combined' '
test_must_fail git rev-list --bisect --first-parent HEAD
'
+test_expect_success '--header shows a NUL after each commit' '
+ # We know that there is no Q in the true payload; names and
+ # addresses of the authors and the committers do not have
+ # any, and object names or header names do not, either.
+ git rev-list --header --max-count=2 HEAD |
+ nul_to_q |
+ grep "^Q" >actual &&
+ cat >expect <<-EOF &&
+ Q$(git rev-parse HEAD~1)
+ Q
+ EOF
+ test_cmp expect actual
+'
+
test_done
base-commit: 659889482ac63411daea38b2c3d127842ea04e4d
--
git-series 0.8.10
^ permalink raw reply related
* Re: Drastic jump in the time required for the test suite
From: Dennis Kaarsemaker @ 2016-10-20 20:40 UTC (permalink / raw)
To: Jeff King, Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <20161020123111.qnbsainul2g54z4z@sigill.intra.peff.net>
On Thu, 2016-10-20 at 08:31 -0400, Jeff King wrote:
> I'm also not entirely convinced that the test suite being a shell script
> is the main culprit for its slowness. We run git a lot of times, and
> that's inherent in testing it. I ran the whole test suite under
> "strace -f -e execve". There are ~335K execs. Here's the breakdown of
> the top ones:
You're measuring execve's, but fork (well, fork emulation. There's no
actual fork) is also expensive on windows iirc, so subshells add a lot
to this cost. That said, strace -eclone says that a 'make test' forks
~408k times, and while this is significantly more than the amount of
execs in your example, this does include cvs and svn tests and it's
still in the same ballpark.
D.
^ permalink raw reply
* [PATCH v2] rev-list: use hdr_termination instead of a always using a newline
From: Jacob Keller @ 2016-10-20 20:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dennis Kaarsemaker, Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
When adding support for prefixing output of log and other commands using
--line-prefix, commit 660e113ce118 ("graph: add support for
--line-prefix on all graph-aware output", 2016-08-31) accidentally
broke rev-list --header output.
In order to make the output appear with a line-prefix, the flow was
changed to always use the graph subsystem for display. Unfortunately
the graph flow in rev-list did not use info->hdr_termination as it was
assumed that graph output would never need to putput NULs.
Since we now always use the graph code in order to handle the case of
line-prefix, simply replace putchar('\n') with
putchar(info->hdr_termination) which will correct this issue.
Add a test for the --header case to make sure we don't break it in the
future. Implement a helper function test_ends_with_nul() to make it more
obvious what sort of check we are looking for.
Reported-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Changes in v2
* Squash Junio's suggested (better) test
* Add Junio's signed-off-by since he wrote the new test
builtin/rev-list.c | 2 +-
t/t6000-rev-list-misc.sh | 14 ++++++++++++++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git c/builtin/rev-list.c c/builtin/rev-list.c
index 8479f6ed28aa..c43decda7011 100644
--- c/builtin/rev-list.c
+++ c/builtin/rev-list.c
@@ -145,7 +145,7 @@ static void show_commit(struct commit *commit, void *data)
*/
if (buf.len && buf.buf[buf.len - 1] == '\n')
graph_show_padding(revs->graph);
- putchar('\n');
+ putchar(info->hdr_termination);
} else {
/*
* If the message buffer is empty, just show
diff --git c/t/t6000-rev-list-misc.sh c/t/t6000-rev-list-misc.sh
index 3e752ce03280..969e4e9e5261 100755
--- c/t/t6000-rev-list-misc.sh
+++ c/t/t6000-rev-list-misc.sh
@@ -100,4 +100,18 @@ test_expect_success '--bisect and --first-parent can not be combined' '
test_must_fail git rev-list --bisect --first-parent HEAD
'
+test_expect_success '--header shows a NUL after each commit' '
+ # We know that there is no Q in the true payload; names and
+ # addresses of the authors and the committers do not have
+ # any, and object names or header names do not, either.
+ git rev-list --header --max-count=2 HEAD |
+ nul_to_q |
+ grep "^Q" >actual &&
+ cat >expect <<-EOF &&
+ Q$(git rev-parse HEAD~1)
+ Q
+ EOF
+ test_cmp expect actual
+'
+
test_done
base-commit: 659889482ac63411daea38b2c3d127842ea04e4d
--
git-series 0.8.10
^ permalink raw reply related
* Re: Drastic jump in the time required for the test suite
From: Johannes Sixt @ 2016-10-20 20:38 UTC (permalink / raw)
To: Jeff King, Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <20161020123111.qnbsainul2g54z4z@sigill.intra.peff.net>
Am 20.10.2016 um 14:31 schrieb Jeff King:
> Close to 1/3 of those processes are just invoking the bin-wrapper
> script to set up the EXEC_PATH, etc. I imagine it would not be too hard
> to just do that in the test script. In fact, it looks like:
>
> make prefix=/wherever install
> GIT_TEST_INSTALLED=/wherever/bin make test
>
> might give you an immediate speedup by skipping bin-wrappers entirely.
Running the tests with --with-dashes should give you the same effect, no?
-- Hannes
^ permalink raw reply
* Re: [PATCH v4 23/25] sequencer: quote filenames in error messages
From: Junio C Hamano @ 2016-10-20 20:28 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <xmqq8ttig3zn.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> This makes the code consistent by fixing quite a couple of error messages.
>>
>> Suggested by Jakub Narębski.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>
> These finishing touches in 21-23 look all sensible to me.
Make that 21-25. I finished reading to the end and it was mostly a
pleasnt read, except for a few things I noticed and sent reviews
separately.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 23/25] sequencer: quote filenames in error messages
From: Junio C Hamano @ 2016-10-20 20:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <930ea31d6f10973807276c7a9f35f0d692d1ada6.1476450940.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> This makes the code consistent by fixing quite a couple of error messages.
>
> Suggested by Jakub Narębski.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
These finishing touches in 21-23 look all sensible to me.
^ permalink raw reply
* Re: [PATCH v4 20/25] sequencer: refactor write_message()
From: Junio C Hamano @ 2016-10-20 20:26 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <xmqqh986g49i.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> If I were doing this, I would make this into three separate steps:
>
> - move the strbuf_release(msgbuf) to the caller in
> do_pick_commit();
>
> - add the missing rollback_lock_file(), which is a bugfix; and
> then finally
>
> - allow the helper to take not a strbuf but <buf, len> pair as
> parameters.
>
> The end result of this patch achieves two thirds of the above, but
> especially given that write_message() only has two call sites in a
> single function, I think it is OK and preferrable even to do all
> three.
Ah, make that four steps. The final one is:
- add append_eol parameter that nobody uses at this step in the
series.
This is a new feature to the helper. While it is OK to have it as a
preparatory step in this series, it is easier to understand if it
kept as a separate step. It is even more preferrable if it is made
as a preparatory step in a series that adds a caller that passes
true to append_eol to this helper, or if that real change is small
enough, part of that patch that adds such a caller, not as a
separate step.
^ permalink raw reply
* Re: [PATCH v4 20/25] sequencer: refactor write_message()
From: Junio C Hamano @ 2016-10-20 20:22 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <9a66d53ebd86d35136597e534be03605bb1346bc.1476450940.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The write_message() function safely writes an strbuf to a file.
> Sometimes it is inconvenient to require an strbuf, though: the text to
> be written may not be stored in a strbuf, or the strbuf should not be
> released after writing.
>
> Let's refactor "safely writing string to a file" into
> write_with_lock_file(), and make write_message() use it.
>
> The new function will make it easy to create a new convenience function
> write_file_gently() that does not die(); as some of the upcoming callers
> of this new function will want to append a newline character, we already
> add that flag as parameter to write_with_lock_file().
>
> While at it, roll back the locked files in case of failure, as pointed
> out by Hannes Sixt.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
Once a helper function starts taking <buf, len> pair, not a strbuf,
it becomes obvious that it does not make much sense to calling
strbuf_release() from the helper. It is caller's job to do the
memory management of the strbuf it uses to pass information to the
helper, and the current api into write_message() is klunky.
If I were doing this, I would make this into three separate steps:
- move the strbuf_release(msgbuf) to the caller in
do_pick_commit();
- add the missing rollback_lock_file(), which is a bugfix; and
then finally
- allow the helper to take not a strbuf but <buf, len> pair as
parameters.
The end result of this patch achieves two thirds of the above, but
especially given that write_message() only has two call sites in a
single function, I think it is OK and preferrable even to do all
three.
^ permalink raw reply
* Re: [PATCH v4 18/25] sequencer: do not try to commit when there were merge conflicts
From: Junio C Hamano @ 2016-10-20 20:11 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <38d5f853444f80c90713f7a6e4ef1f2382549b29.1476450940.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The return value of do_recursive_merge() may be positive (indicating merge
> conflicts), or 0 (indicating success). It also may be negative, indicating
> a fatal error that requires us to abort.
>
> Now, if the return value indicates that there are merge conflicts, we
> should not try to commit those changes, of course.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index cbc3742..9ffc090 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -787,7 +787,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> res = allow;
> goto leave;
> }
> - if (!opts->no_commit)
> + if (!res && !opts->no_commit)
> res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
> opts, allow, opts->edit, 0, 0);
This by itself looks more like a bugfix than preparation for later
steps. The only reason why it is not a bugfix is because there is
nothing in this function that makes res a non-zero value and reach
this if statement at this step. We would have been caught by an
"if (res) { ... rerere(); goto leave; }" or
"if (allow < 0) { res = allow; goto leave; }"
that appear before this part of the code.
So while it is not wrong per-se, I think this should be part of an
actual change that makes it possible for the control flow to reach
here with non-zero res.
^ permalink raw reply
* Re: [PATCH v4 17/25] sequencer: support cleaning up commit messages
From: Junio C Hamano @ 2016-10-20 20:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <4173eb7716e8f4fbd083961a05dc4329686a9912.1476450940.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The run_git_commit() function already knows how to amend commits, and
> with this new option, it can also clean up commit messages (i.e. strip
> out commented lines). This is needed to implement rebase -i's 'fixup'
> and 'squash' commands as sequencer commands.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index fa77c82..cbc3742 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -481,7 +481,8 @@ static char **read_author_script(void)
> * author metadata.
> */
> static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> - int allow_empty, int edit, int amend)
> + int allow_empty, int edit, int amend,
> + int cleanup_commit_message)
> {
> char **env = NULL;
> struct argv_array array;
Looks OK to me.
This starts to look like calling for a single flag word even more,
but it is a file-local helper so we can clean it up if it becomes
necessary without affecting too many things later.
> @@ -518,9 +519,12 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> argv_array_push(&array, "-s");
> if (defmsg)
> argv_array_pushl(&array, "-F", defmsg, NULL);
> + if (cleanup_commit_message)
> + argv_array_push(&array, "--cleanup=strip");
> if (edit)
> argv_array_push(&array, "-e");
> - else if (!opts->signoff && !opts->record_origin &&
> + else if (!cleanup_commit_message &&
> + !opts->signoff && !opts->record_origin &&
> git_config_get_value("commit.cleanup", &value))
> argv_array_push(&array, "--cleanup=verbatim");
>
> @@ -785,7 +789,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> }
> if (!opts->no_commit)
> res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
> - opts, allow, opts->edit, 0);
> + opts, allow, opts->edit, 0, 0);
>
> leave:
> free_message(commit, &msg);
^ permalink raw reply
* Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline
From: Jacob Keller @ 2016-10-20 20:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list, Dennis Kaarsemaker
In-Reply-To: <xmqq4m46hmvq.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 20, 2016 at 11:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> The main part of the patch looks good. For "passing NUL to sed",
> I'd probably work it around like so:
>
Yep. I wasn't sure on the test as it was, because of the portability concern.
> t/t6000-rev-list-misc.sh | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
> index e8c6979baf..737026c34f 100755
> --- a/t/t6000-rev-list-misc.sh
> +++ b/t/t6000-rev-list-misc.sh
> @@ -4,12 +4,6 @@ test_description='miscellaneous rev-list tests'
>
> . ./test-lib.sh
>
> -test_ends_with_nul() {
> - printf "\0" >nul
> - sed '$!d' "$@" >contents
> - test_cmp_bin nul contents
> -}
> -
> test_expect_success setup '
> echo content1 >wanted_file &&
> echo content2 >unwanted_file &&
> @@ -107,8 +101,17 @@ test_expect_success '--bisect and --first-parent can not be combined' '
> '
>
> test_expect_success '--header shows a NUL after each commit' '
> - git rev-list --header --max-count=1 HEAD | sed \$!d >actual &&
> - test_ends_with_nul actual
> + # We know there is no Q in the true payload; names and
> + # addresses of the authors and the committers do not have
> + # any, and object names or header names do not, either.
> + git rev-list --header --max-count=2 HEAD |
> + nul_to_q |
> + grep "^Q" >actual &&
> + cat >expect <<-EOF &&
> + Q$(git rev-parse HEAD~1)
> + Q
> + EOF
> + test_cmp expect actual
> '
>
> test_done
I will squash this in and re-send.
Thanks,
Jake
^ permalink raw reply
* Re: tools for easily "uncommitting" parts of a patch I just commited?
From: Jacob Keller @ 2016-10-20 20:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Git mailing list
In-Reply-To: <xmqqfunqhnhf.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 20, 2016 at 11:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>>> I am not sure if that is OK. I think it is less not-OK than the use
>>> case I mentioned in my earlier message, in that this is not a case
>>> that "please don't do it" breaks. It however is an inconvenience
>>> that the user has to say "git add file" before the "git commit" (or
>>> "git commit file") to conclude the sequence.
>>>
>>> So I dunno.
>>
>> Hmmm.. Ya ok I don't think we can actually distinguish between these
>> two work flows.
>
> What we might want to have in "git commit <paths>" is a new mode
> that is totally different from -i/-o that says roughly "Start from
> the tree of HEAD, pretend as if you removed all the paths that match
> the given pathspec from the tree, and then added all the entries in
> the index that match that pathspec. Write that tree and commit.
> Take nothing from the working tree". I have a feeling that when
> people do
>
> $ git add -p file1 file2 file3
> $ git commit file2
>
> and ends up including _all_ changes made to file2, not just the ones
> they picked in the earlier part of the workflow, they are expecting
> such a behaviour.
>
Right now I think people who use it intentionally do expect it to work
that way. I just happen to not have wanted to add <file> but did so
anyways without considering, and thus I ended up including changes
that were for the next commit.
As long as there is a way to change "git commit" default from that
mode then we could make the default work and then let people configure
it to what makes sense.
I'll take a look at going this route.
Thanks,
Jake
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Stefan Beller @ 2016-10-20 19:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <alpine.DEB.2.20.1610201218060.3264@virtualbox>
On Thu, Oct 20, 2016 at 3:50 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi peff,
>
> On Wed, 19 Oct 2016, Jeff King wrote:
>
>> On Wed, Oct 19, 2016 at 10:32:12AM -0700, Junio C Hamano wrote:
>>
>> > > Maybe we should start optimizing the tests...
>> >
Maybe we should stop introducing un-optimized tests.
For other reasons I just stumbled upon t7064
(porcelain V2 output for git status)
This is how an arbitrary test looks like:
test_expect_success 'staged changes in added submodule (AM S.M.)' '
( cd super_repo &&
## stage the changes in the submodule.
( cd sub1 &&
git add file_in_sub
) &&
HMOD=$(git hash-object -t blob -- .gitmodules) &&
HSUP=$(git rev-parse HEAD) &&
HSUB=$HSUP &&
cat >expect <<-EOF &&
# branch.oid $HSUP
# branch.head master
# branch.upstream origin/master
# branch.ab +0 -0
1 A. N... 000000 100644 100644 $_z40 $HMOD .gitmodules
1 AM S.M. 000000 160000 160000 $_z40 $HSUB sub1
EOF
git status --porcelain=v2 --branch --untracked-files=all >actual &&
test_cmp expect actual
)
'
Following "modern" Git tests I would have expected:
* heavy use of the "git -C <dir>" pattern. When applying that
thouroughly we'd save spanning the subshells.
* no `cd` on the same line as the opening paren.
(This is style and would derail the performance discussion)
test_expect_success 'staged changes in added submodule (AM S.M.)' '
git -C super_repo/sub1 add file_in_sub &&
HMOD=$(git -C super_repo hash-object -t blob -- .gitmodules) &&
HSUP=$(git -C super_repo rev-parse HEAD) &&
# as a comment: HSUB is equal to HSUP, because ...
cat >expect <<-EOF &&
# branch.oid $HSUP
# branch.head master
# branch.upstream origin/master
# branch.ab +0 -0
1 A. N... 000000 100644 100644 $_z40 $HMOD .gitmodules
1 AM S.M. 000000 160000 160000 $_z40 $HSUP sub1
EOF
git -C super_repo status --porcelain=v2 --branch
--untracked-files=all >../actual &&
test_cmp expect actual
'
That said I really like the idea of having a helper that would eliminate the cat
for you, e.g. :
git_test_helper_equal_stdin_or_diff_and_die -C super_repo status
--porcelain=v2 --branch --untracked-files=all <<-EOF
1 A. N... 000000 100644 100644 $_z40 $HMOD .gitmodules
1 AM S.M. 000000 160000 160000 $_z40 $HSUP sub1
EOF
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-20 19:53 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <CAGZ79kZyCXKnt9kTbnD8KrnsjJVb_7juW2epmdP9XqcRGfg=GA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> My thought was to fix it nevertheless, such that the url recorded as
> remote.origin.url is always the first case (no l or /. at the end).
>
> If we were to add this fix to clone, then it may be easier to debug
> submodule url schemes for users as the submodule url would then
> be a concatenation of remote.origin.url and the relative part.
>
> That seems easier to understand than ${remote.origin.url%%/.} +
> relative path, maybe? (Because then the user doesn't need to guess
> or remember historical behavior that is wrong on how this)
Are you declaring that trailing / or /. will now be illegal? If you
are declaring that, then I agree that new codepaths no longer have
to worry about "strip / or /. before concatenating" and will
simplify things for them. But otherwise, such a "fix" also would
have an effect of hiding bugs from codepaths. They still need to be
prepared to see any of the three variants and cope with them
correctly, right?
^ 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