* [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
* Re: Make `git fetch --all` parallel?
From: Jeff King @ 2016-10-12 1:34 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, Ram Rachum, git@vger.kernel.org
In-Reply-To: <CAGZ79kaKOiy-HJboaujXXc66P6CLupteDw4JyPOGetREfz_q_Q@mail.gmail.com>
On Tue, Oct 11, 2016 at 04:18:15PM -0700, Stefan Beller wrote:
> >> 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/.
It's been a while since I've dug into the fetch protocol. But I think we
cover the "do we have the objects already" check via quickfetch(), which
does do a reachability check, And then we advertise our "have" commits
by walking backwards from our ref tips, so everything there is
reachable.
Anything else would be questionable, especially under older versions of
git, as we promise only to have a complete graph for objects reachable
from the refs. Older versions of git would happily truncate unreachable
history based on the 2-week prune expiration period.
> > 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
So first, thank you (and Ævar) for providing real numbers. It's clear
that I was talking nonsense.
Second, I wonder where all that time is going. Clearly there's an
end-to-end latency issue, but I'm not sure where it is. Is it startup
time for git-fetch? Is it in getting and processing the ref
advertisement from the other side? What I'm wondering is if there are
opportunities to speed up the serial case (but nobody really cared
before because it doesn't matter unless you're doing 14 of them back to
back).
> > 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. ;)
That, and it clogs the ref namespace. You can mostly ignore the extra
refs, but they show up in the "git checkout ..." DWIM, for example.
-Peff
^ permalink raw reply
* [PATCH 3/5] trailer: make args have their own struct
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>
Improve type safety by making arguments (whether from configuration or
from the command line) have their own "struct arg_item" type, separate
from the "struct trailer_item" type used to represent the trailers in
the buffer being manipulated.
Also take the opportunity to refine the "struct trailer_item" definition
by removing the conf (which is used only by arguments) and by removing
const on the string fields, since those fields are owned by the struct.
This change also prepares "struct trailer_item" to be further
differentiated from "struct arg_item" in future patches.
---
trailer.c | 161 ++++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 99 insertions(+), 62 deletions(-)
diff --git a/trailer.c b/trailer.c
index e8b1bfb..167b2fd 100644
--- a/trailer.c
+++ b/trailer.c
@@ -26,12 +26,18 @@ static struct conf_info default_conf_info;
struct trailer_item {
struct trailer_item *next;
- const char *token;
- const char *value;
+ char *token;
+ char *value;
+};
+
+struct arg_item {
+ struct arg_item *next;
+ char *token;
+ char *value;
struct conf_info conf;
};
-static struct trailer_item *first_conf_item;
+static struct arg_item *first_conf_item;
static char *separators = ":";
@@ -55,8 +61,7 @@ static size_t token_len_without_separator(const char *token, size_t len)
return len;
}
-static int same_token(const struct trailer_item *a,
- const struct trailer_item *b)
+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));
@@ -65,14 +70,12 @@ static int same_token(const struct trailer_item *a,
return !strncasecmp(a->token, b->token, min_len);
}
-static int same_value(const struct trailer_item *a,
- const struct trailer_item *b)
+static int same_value(const struct trailer_item *a, const struct arg_item *b)
{
return !strcasecmp(a->value, b->value);
}
-static int same_trailer(const struct trailer_item *a,
- const struct trailer_item *b)
+static int same_trailer(const struct trailer_item *a, const struct arg_item *b)
{
return same_token(a, b) && same_value(a, b);
}
@@ -94,11 +97,18 @@ static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *
static void free_trailer_item(struct trailer_item *item)
{
+ free(item->token);
+ free(item->value);
+ free(item);
+}
+
+static void free_arg_item(struct arg_item *item)
+{
free(item->conf.name);
free(item->conf.key);
free(item->conf.command);
- free((char *)item->token);
- free((char *)item->value);
+ free(item->token);
+ free(item->value);
free(item);
}
@@ -131,13 +141,13 @@ static void print_all(FILE *outfile, struct trailer_item *first, int trim_empty)
}
}
-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)
@@ -162,7 +172,7 @@ static const char *apply_command(const char *command, const char *arg)
return result;
}
-static void apply_item_command(struct trailer_item *in_tok, struct trailer_item *arg_tok)
+static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
{
if (arg_tok->conf.command) {
const char *arg;
@@ -179,60 +189,77 @@ static void apply_item_command(struct trailer_item *in_tok, struct trailer_item
}
}
+static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
+{
+ struct trailer_item *new = xcalloc(sizeof(*new), 1);
+ new->token = arg_tok->token;
+ new->value = arg_tok->value;
+ arg_tok->token = arg_tok->value = NULL;
+ free_arg_item(arg_tok);
+ return new;
+}
+
static void apply_existing_arg(struct trailer_item **found_next,
- struct trailer_item *arg_tok,
+ struct arg_item *arg_tok,
struct trailer_item **position_to_add,
const struct trailer_item *in_tok_head,
const struct trailer_item *neighbor)
{
- if (arg_tok->conf.if_exists == EXISTS_DO_NOTHING) {
- free_trailer_item(arg_tok);
+ enum action_if_exists if_exists = arg_tok->conf.if_exists;
+ struct trailer_item *to_add;
+
+ if (if_exists == EXISTS_DO_NOTHING) {
+ free_arg_item(arg_tok);
return;
}
apply_item_command(*found_next, arg_tok);
- if (arg_tok->conf.if_exists == EXISTS_ADD_IF_DIFFERENT_NEIGHBOR) {
+ if (if_exists == EXISTS_ADD_IF_DIFFERENT_NEIGHBOR) {
if (neighbor && same_trailer(neighbor, arg_tok)) {
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
return;
}
- } else if (arg_tok->conf.if_exists == EXISTS_ADD_IF_DIFFERENT) {
+ } else if (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);
+ free_arg_item(arg_tok);
return;
}
}
}
- arg_tok->next = *position_to_add;
- *position_to_add = arg_tok;
+ to_add = trailer_from_arg(arg_tok);
+ to_add->next = *position_to_add;
+ *position_to_add = to_add;
- if (arg_tok->conf.if_exists == EXISTS_REPLACE) {
+ if (if_exists == EXISTS_REPLACE) {
struct trailer_item *new_next = (*found_next)->next;
free_trailer_item(*found_next);
*found_next = new_next;
}
}
-static void apply_missing_arg(struct trailer_item *arg_tok,
+static void apply_missing_arg(struct arg_item *arg_tok,
struct trailer_item **position_to_add)
{
+ struct trailer_item *to_add;
+
if (arg_tok->conf.if_missing == MISSING_DO_NOTHING) {
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
return;
}
apply_item_command(NULL, arg_tok);
- arg_tok->next = *position_to_add;
- *position_to_add = arg_tok;
+ to_add = trailer_from_arg(arg_tok);
+ to_add->next = *position_to_add;
+ *position_to_add = to_add;
}
static void apply_arg(struct trailer_item **in_tok_head,
- struct trailer_item *arg_tok)
+ struct arg_item *arg_tok)
{
struct trailer_item **next, **found_next = NULL, *last = NULL;
enum action_where where = arg_tok->conf.where;
@@ -283,9 +310,9 @@ static void apply_arg(struct trailer_item **in_tok_head,
}
static void process_trailers_lists(struct trailer_item **in_tok_head,
- struct trailer_item *arg_tok_head)
+ struct arg_item *arg_tok_head)
{
- struct trailer_item *arg_tok, *next;
+ struct arg_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);
@@ -346,10 +373,10 @@ static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
dst->command = xstrdup(src->command);
}
-static struct trailer_item *get_conf_item(const char *name)
+static struct arg_item *get_conf_item(const char *name)
{
- struct trailer_item **next = &first_conf_item;
- struct trailer_item *item;
+ struct arg_item **next = &first_conf_item;
+ struct arg_item *item;
/* Look up item with same name */
for (next = &first_conf_item; *next; next = &(*next)->next)
@@ -357,7 +384,7 @@ static struct trailer_item *get_conf_item(const char *name)
return *next;
/* Item does not already exists, create it */
- item = xcalloc(sizeof(struct trailer_item), 1);
+ item = xcalloc(sizeof(*item), 1);
duplicate_conf(&item->conf, &default_conf_info);
item->conf.name = xstrdup(name);
*next = item;
@@ -409,7 +436,7 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v
static int git_trailer_config(const char *conf_key, const char *value, void *cb)
{
const char *trailer_item, *variable_name;
- struct trailer_item *item;
+ struct arg_item *item;
struct conf_info *conf;
char *name = NULL;
enum trailer_info_type type;
@@ -467,7 +494,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
return 0;
}
-static const char *token_from_item(struct trailer_item *item, char *tok)
+static const char *token_from_item(struct arg_item *item, char *tok)
{
if (item->conf.key)
return item->conf.key;
@@ -476,7 +503,7 @@ static const char *token_from_item(struct trailer_item *item, char *tok)
return item->conf.name;
}
-static int token_matches_item(const char *tok, struct trailer_item *item, int tok_len)
+static int token_matches_item(const char *tok, struct arg_item *item, int tok_len)
{
if (!strncasecmp(tok, item->conf.name, tok_len))
return 1;
@@ -488,7 +515,7 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
{
size_t len;
struct strbuf seps = STRBUF_INIT;
- struct trailer_item *item;
+ struct arg_item *item;
int tok_len;
strbuf_addstr(&seps, separators);
@@ -513,11 +540,13 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
/* Lookup if the token matches something in the config */
tok_len = token_len_without_separator(tok->buf, tok->len);
- *conf = &default_conf_info;
+ if (conf)
+ *conf = &default_conf_info;
for (item = first_conf_item; item; item = item->next) {
if (token_matches_item(tok->buf, item, tok_len)) {
char *tok_buf = strbuf_detach(tok, NULL);
- *conf = &item->conf;
+ if (conf)
+ *conf = &item->conf;
strbuf_addstr(tok, token_from_item(item, tok_buf));
free(tok_buf);
break;
@@ -527,43 +556,53 @@ 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,
- const struct conf_info *conf)
+static void 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;
+
+ **tail = new;
+ *tail = &new->next;
+}
+
+static void add_arg_item(struct arg_item ***tail, char *tok, char *val,
+ const struct conf_info *conf)
+{
+ struct arg_item *new = xcalloc(sizeof(*new), 1);
+ new->token = tok;
+ new->value = val;
duplicate_conf(&new->conf, conf);
**tail = new;
*tail = &new->next;
}
-static struct trailer_item *process_command_line_args(struct string_list *trailers)
+static struct arg_item *process_command_line_args(struct string_list *trailers)
{
- struct trailer_item *arg_tok_head = NULL;
- struct trailer_item **arg_tok_tail = &arg_tok_head;
+ struct arg_item *arg_tok_head = NULL;
+ struct arg_item **arg_tok_tail = &arg_tok_head;
struct string_list_item *tr;
- struct trailer_item *item;
+ struct arg_item *item;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
const struct conf_info *conf;
- /* Add a trailer item for each configured trailer with a command */
+ /* Add an arg item for each configured trailer with a command */
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_arg_item(&arg_tok_tail,
+ xstrdup(token_from_item(item, NULL)),
+ xstrdup(""),
+ &item->conf);
- /* Add a trailer item for each trailer on the command line */
+ /* Add an arg item for each trailer on the command line */
for_each_string_list_item(tr, trailers) {
if (!parse_trailer(&tok, &val, &conf, tr->string))
- add_trailer_item(&arg_tok_tail,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
- conf);
+ add_arg_item(&arg_tok_tail,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf);
}
return arg_tok_head;
@@ -688,7 +727,6 @@ static int process_input_file(FILE *outfile,
int patch_start, trailer_start, trailer_end, i;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
- const struct conf_info *conf;
/* Get the line count */
while (lines[count])
@@ -707,11 +745,10 @@ static int process_input_file(FILE *outfile,
/* Parse trailer lines */
for (i = trailer_start; i < trailer_end; i++) {
if (lines[i]->buf[0] != comment_line_char &&
- !parse_trailer(&tok, &val, &conf, lines[i]->buf))
+ !parse_trailer(&tok, &val, NULL, lines[i]->buf))
add_trailer_item(in_tok_tail,
strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
- conf);
+ strbuf_detach(&val, NULL));
}
return trailer_end;
@@ -761,7 +798,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
{
struct trailer_item *in_tok_head = NULL;
struct trailer_item **in_tok_tail = &in_tok_head;
- struct trailer_item *arg_tok_head;
+ struct arg_item *arg_tok_head;
struct strbuf **lines;
int trailer_end;
FILE *outfile = stdout;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: Make `git fetch --all` parallel?
From: Jeff King @ 2016-10-12 1:52 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, Ram Rachum, git@vger.kernel.org
In-Reply-To: <20161012013428.swxmrbyxv2wo37xf@sigill.intra.peff.net>
On Tue, Oct 11, 2016 at 09:34:28PM -0400, Jeff King wrote:
> > 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
>
> So first, thank you (and Ævar) for providing real numbers. It's clear
> that I was talking nonsense.
>
> Second, I wonder where all that time is going. Clearly there's an
> end-to-end latency issue, but I'm not sure where it is. Is it startup
> time for git-fetch? Is it in getting and processing the ref
> advertisement from the other side? What I'm wondering is if there are
> opportunities to speed up the serial case (but nobody really cared
> before because it doesn't matter unless you're doing 14 of them back to
> back).
Hmm. I think it really might be just network latency. Here's my fetch
time:
$ git config remote.origin.url
git://github.com/gitster/git.git
$ time git fetch origin
real 0m0.183s
user 0m0.072s
sys 0m0.008s
14 of those in a row shouldn't take more than about 2.5 seconds, which
is still twice as fast as your parallel case. So what's going on?
One is that I live about a hundred miles from GitHub's data center, and
my ping time there is ~13ms. The other side of the country, let alone
Europe, is going to be noticeably slower just for the TCP handshake.
The second is that git:// is really cheap and simple. git-over-ssh is
over twice as slow:
$ time git fetch git@github.com:gitster/git
...
real 0m0.432s
user 0m0.100s
sys 0m0.032s
HTTP fares better than I would have thought, but is also slower:
$ time git fetch https://github.com/gitster/git
...
real 0m0.258s
user 0m0.080s
sys 0m0.032s
-Peff
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Larry Finger @ 2016-10-12 4:28 UTC (permalink / raw)
To: Matthieu Moy
Cc: Jeff King, Mathieu Lienard--Mayor, Jorge Juan Garcia Garcia,
Remi Lespinet, git
In-Reply-To: <vpq4m4iamfs.fsf@anie.imag.fr>
On 10/11/2016 11:18 AM, Matthieu Moy wrote:
> Larry Finger <Larry.Finger@lwfinger.net> writes:
>
>> That added information at the end is intended to be passed on to the
>> stable group. In this case, the patch needs to be applied to kernel
>> versions 4.8 and later.
>
> OK, but where do people fetch this information from?
This format is used in a patch for the kernel. When the patch is merged into
mainline, stable@vger.kernel.org gets sent an E-mail with a copy of the original
patch. Maintainers of the indicated systems then merge the patch with their
stable version.
>
> When you use git send-email, the content of the Cc: trailers ends up
> both in the body of the message and in the Cc: field of the same
> message.
>
> If you need the mention to appear in the body of the message, then using
> parenthesis is fine: git send-email won't remove it (more precisely,
> "send-email" will call "format-patch" which won't remove it).
>
> Not an objection to patching send-email anyway, but if there's a simple
> and RFC-compliant way to do what you're looking for, we can as well use
> it (possibly in addition to patching).
I do not want it in the body of the message. I just want to pass a hint to the
stable maintainer(s).
As noted earlier, this has worked for a very long time, and I think the previous
behavior should be restored.
Larry
>
^ permalink raw reply
* Re: git 2.10.1 test regression in t3700-add.sh
From: Junio C Hamano @ 2016-10-12 5:59 UTC (permalink / raw)
To: Jeremy Huddleston Sequoia; +Cc: t.gummerer, git
In-Reply-To: <EF7F9B45-867A-433B-9623-EBBF33D34793@freedesktop.org>
Jeremy Huddleston Sequoia <jeremyhu@freedesktop.org> writes:
>> 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.
Ah, of course. That "rm -f" needs to remove from the index and also
from the working tree, so has to be "git rm -f --ignore-unmatch" or
something like that.
> 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?
In modern tests, we strive to do the former with liberal use of
test_when_finished. I think the one that creates xfoo[123] and
leaves them behind for a long time predates the modern practice.
A minimal fix with that approach may look like this:
t/t3700-add.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 924a266126..80c7ee3e3b 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -64,6 +64,7 @@ test_expect_success 'git add: filemode=0 should not get confused by symlink' '
test_expect_success \
'git update-index --add: Test that executable bit is not used...' \
'git config core.filemode 0 &&
+ test_when_finished "git rm -f xfoo3" &&
test_ln_s_add xfoo2 xfoo3 && # runs git update-index --add
test_mode_in_index 120000 xfoo3'
^ permalink raw reply related
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 6:12 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill
In-Reply-To: <20161011235951.8358-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> 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.
What is the primary purpose of this patch? Is it to prepare callers
so that the way they interact with the attr subsystem will not have to
change when they become threaded and the attr subsystem becomes
thread ready?
I am not sure if the updates to the callers fulfill that purpose.
For example, look at this hunk.
> @@ -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;
As we discussed, this caller, even when threaded, will always want
to ask for a fixed two attributes, so "check" being static and
shared across threads is perfectly fine. But we do not want to see
"result" shared, do we?
> 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);
> + }
Are we assuming that storing and checking of a single pointer is
atomic? I would not expose that assumption to the callers. On a
platform where that assumption holds, "if check is not NULL,
somebody must have done it already, so return without doing nothing"
can be the first thing git_attr_check_initl()'s implementation does,
though. Or it may not hold anywhere without some barriers. All
that implementation details should be hidden inside _initl()'s
implementation. So this caller should instead just do an
unconditional:
git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
Also, as "result" should be per running thread, hence non-static,
and because we do not want repeated heap allocations and releases
but luckily most callers _know_ not just how many but what exact
attributes they are interested in (I think there are only two
callers that do not know it; check-all-attrs one, and your pathspec
magic one that does not exist at this point in the series), I would
think it is much more preferrable to allow the caller to prepare an
on-stack array and call it "initialized already".
In other words, ideally, I think this part of the patch should
rather read like this:
static struct git_attr_check *check;
struct git_attr_result result[2];
...
git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
if (!git_check_attr(path_without_prefix, check, result)) {
... use result[0] and result[1] ...
For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
and extra parameter to git_check_attr() so that the function can
make sure it matches (or exceeds) check->nr.
^ permalink raw reply
* Re: [PATCH 5/5] trailer: support values folded to multiple lines
From: Junio C Hamano @ 2016-10-12 6:23 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, christian.couder
In-Reply-To: <4b8616732b719ede04b90c87ab240c29b4e3a0bb.1476232683.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> 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.
Let's see how the code handles one minor detail when we see 822
folding, namely, "what happens to the leading whitespace that signals
the beginning of the second and subsequent lines?".
> 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;
> };
Is the length of value very frequently used once the list of trailer
lines are fully parsed? If not, I'd rather not to have "struct
strbuf" in a long-living structure like this one and instead prefer
keeping it a simple and stupid "char *value".
Yes, I know the existing code in trailers overuses strbuf when there
is no need, primarily because it uses the lazy "split into an array
of strbufs" function. We shouldn't make it worse.
> @@ -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])) {
It is convenient if "value" is a strbuf to do this,
> + /* continuation line of the last trailer item */
> + strbuf_addch(&last->value, '\n');
> + strbuf_addbuf(&last->value, lines[i]);
> + strbuf_strip_suffix(&last->value, "\n");
but it is easy to introduce a temporary strbuf in this scope and use
it only to create the final value and detach it to last->value, i.e.
if (last && isspace(*lines[i]->buf)) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "%s\n%s", last->value, lines[i]->buf);
strbuf_strip_suffix(&buf, "\n");
free(last->value);
last->value = strbuf_detach(&buf, NULL);
By the way, I now see that the code handles the "minor detail" to
keep the leading whitespace, which is good.
Thanks.
^ permalink raw reply
* Re: Make `git fetch --all` parallel?
From: Stefan Beller @ 2016-10-12 6:47 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ram Rachum, git@vger.kernel.org
In-Reply-To: <20161012015224.g2eb24jexepeewob@sigill.intra.peff.net>
On Tue, Oct 11, 2016 at 6:52 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 11, 2016 at 09:34:28PM -0400, Jeff King wrote:
>
>> > 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
>>
>> So first, thank you (and Ævar) for providing real numbers. It's clear
>> that I was talking nonsense.
>>
>> Second, I wonder where all that time is going. Clearly there's an
>> end-to-end latency issue, but I'm not sure where it is. Is it startup
>> time for git-fetch? Is it in getting and processing the ref
>> advertisement from the other side? What I'm wondering is if there are
>> opportunities to speed up the serial case (but nobody really cared
>> before because it doesn't matter unless you're doing 14 of them back to
>> back).
>
> Hmm. I think it really might be just network latency. Here's my fetch
> time:
>
> $ git config remote.origin.url
> git://github.com/gitster/git.git
>
> $ time git fetch origin
> real 0m0.183s
> user 0m0.072s
> sys 0m0.008s
>
> 14 of those in a row shouldn't take more than about 2.5 seconds, which
> is still twice as fast as your parallel case. So what's going on?
>
> One is that I live about a hundred miles from GitHub's data center, and
> my ping time there is ~13ms. The other side of the country, let alone
> Europe, is going to be noticeably slower just for the TCP handshake.
>
> The second is that git:// is really cheap and simple. git-over-ssh is
> over twice as slow:
>
> $ time git fetch git@github.com:gitster/git
> ...
> real 0m0.432s
> user 0m0.100s
> sys 0m0.032s
>
> HTTP fares better than I would have thought, but is also slower:
>
> $ time git fetch https://github.com/gitster/git
> ...
> real 0m0.258s
> user 0m0.080s
> sys 0m0.032s
>
> -Peff
Well 9/14 are https for me, the rest is git://
Also 9/14 (but a different set) is github, the rest is
either internal or kernel.org.
Fetching from github (https) is only 0.9s from here
(SF bay area, I'm not in Europe any more ;) )
I would have expected to have a speedup
of roughly 2 + latency gains. Factor 2 because
in the current state of affairs either the client or the
remote is working, i.e. the other sie is idle/waiting, so
factor 2 seemed reasonable (and ofc the latency), so I
was a bit surprised to see a higher yield.
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-12 6:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <xmqqvawy5c4i.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 11, 2016 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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.
>
> What is the primary purpose of this patch? Is it to prepare callers
> so that the way they interact with the attr subsystem will not have to
> change when they become threaded and the attr subsystem becomes
> thread ready?
>
> I am not sure if the updates to the callers fulfill that purpose.
> For example, look at this hunk.
>
>> @@ -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;
>
> As we discussed, this caller, even when threaded, will always want
> to ask for a fixed two attributes, so "check" being static and
> shared across threads is perfectly fine. But we do not want to see
> "result" shared, do we?
Well all of the hunks in the patch are not threaded, so they
don't follow a threading pattern, but the static pattern to not be
more expensive than needed.
>
>> 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);
>> + }
>
> Are we assuming that storing and checking of a single pointer is
> atomic? I would not expose that assumption to the callers. On a
> platform where that assumption holds, "if check is not NULL,
> somebody must have done it already, so return without doing nothing"
> can be the first thing git_attr_check_initl()'s implementation does,
> though. Or it may not hold anywhere without some barriers. All
> that implementation details should be hidden inside _initl()'s
> implementation. So this caller should instead just do an
> unconditional:
>
> git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
>
> Also, as "result" should be per running thread, hence non-static,
> and because we do not want repeated heap allocations and releases
> but luckily most callers _know_ not just how many but what exact
> attributes they are interested in (I think there are only two
> callers that do not know it; check-all-attrs one, and your pathspec
> magic one that does not exist at this point in the series), I would
> think it is much more preferrable to allow the caller to prepare an
> on-stack array and call it "initialized already".
>
> In other words, ideally, I think this part of the patch should
> rather read like this:
>
> static struct git_attr_check *check;
> struct git_attr_result result[2];
>
> ...
> git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
> if (!git_check_attr(path_without_prefix, check, result)) {
> ... use result[0] and result[1] ...
>
> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
> and extra parameter to git_check_attr() so that the function can
> make sure it matches (or exceeds) check->nr.
That seems tempting from a callers perspective; I'll look into that.
^ permalink raw reply
* Re: [PATCH 1/5] trailer: use singly-linked list, not doubly
From: Junio C Hamano @ 2016-10-12 6:24 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, christian.couder
In-Reply-To: <8e12e0954f0a23d7c7905c58a3f7d8084d9338be.1476232683.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> 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.
It is overall a very good change, but can you split this into two
independent patches? s/struct trailer_item/const &/ sprinkled all
over the place is more or less unrelated change and it is very
distracting to see the primary change of the way lists are handled.
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Matthieu Moy @ 2016-10-12 7:36 UTC (permalink / raw)
To: Larry Finger; +Cc: Jeff King, Mathieu Lienard--Mayor, Remi Lespinet, git
In-Reply-To: <b8f93bf9-bfa5-2405-437e-6bf9abf77c87@lwfinger.net>
Larry Finger <Larry.Finger@lwfinger.net> writes:
> On 10/11/2016 11:18 AM, Matthieu Moy wrote:
>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>
>>> That added information at the end is intended to be passed on to the
>>> stable group. In this case, the patch needs to be applied to kernel
>>> versions 4.8 and later.
>>
>> OK, but where do people fetch this information from?
>
> This format is used in a patch for the kernel. When the patch is
> merged into mainline, stable@vger.kernel.org gets sent an E-mail with
> a copy of the original patch. Maintainers of the indicated systems
> then merge the patch with their stable version.
Sorry, but this does not answer my question. I'll rephrase: when
people behind stable@vger.kernel.org get the message, how do they know
which version of the kernel they should apply it to?
> I do not want it in the body of the message. I just want to pass a
> hint to the stable maintainer(s).
If it's not in the body of the message, then where is it?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH v2 2/2] Feature Request: user defined suffix for temp files created by git-mergetool
From: Josef Ridky @ 2016-10-12 8:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7f9lmmol.fsf@gitster.mtv.corp.google.com>
This is update of the second variant for request to add option to change
suffix of name of temporary files generated by git mergetool. This
change is requested for cases, when is git mergetool used for local
comparison between two version of same package during package rebase.
Signed-off-by: Josef Ridky <jridky@redhat.com>
---
Documentation/git-mergetool.txt | 22 ++++++++++++++-
git-mergetool.sh | 60 +++++++++++++++++++++++++++++++++++++----
2 files changed, 76 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index e846c2e..a0466ac 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -8,7 +8,7 @@ git-mergetool - Run merge conflict resolution tools to resolve merge conflicts
SYNOPSIS
--------
[verse]
-'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [<file>...]
+'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [--local=<name>] [--remote=<name>] [--backup=<name>] [--base=<name>] [<file>...]
DESCRIPTION
-----------
@@ -79,6 +79,26 @@ success of the resolution after the custom tool has exited.
Prompt before each invocation of the merge resolution program
to give the user a chance to skip the path.
+--local=<name>::
+ Use string from <name> as part of suffix of name of temporary
+ file (local) for merging. If not set, default value is used.
+ Default suffix is LOCAL.
+
+--remote=<name>::
+ Use string from <name> as part of suffix of name of temporary
+ file (remote) for merging. If not set, default value is used.
+ Default suffix is REMOTE.
+
+--backup=<name>::
+ Use string from <name> as part of suffix of name of temporary
+ file (backup) for merging. If not set, default value is used.
+ Default suffix is BACKUP.
+
+--base=<name>::
+ Use string from <name> as part of suffix of name of temporary
+ file (base) for merging. If not set, default value is used.
+ Default suffix is BASE.
+
TEMPORARY FILES
---------------
`git mergetool` creates `*.orig` backup files while resolving merges.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index bf86270..ed9ba82 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,7 +8,7 @@
# at the discretion of Junio C Hamano.
#
-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [--local=name] [--remote=name] [--backup=name] [--base=name] [file to merge] ...'
SUBDIRECTORY_OK=Yes
NONGIT_OK=Yes
OPTIONS_SPEC=
@@ -16,6 +16,13 @@ TOOL_MODE=merge
. git-sh-setup
. git-mergetool--lib
+# Can be changed by user
+LOCAL_NAME='LOCAL'
+BASE_NAME='BASE'
+BACKUP_NAME='BACKUP'
+REMOTE_NAME='REMOTE'
+
+
# Returns true if the mode reflects a symlink
is_symlink () {
test "$1" = 120000
@@ -271,10 +278,10 @@ merge_file () {
BASE=${BASE##*/}
fi
- BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
- LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
- REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
- BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
+ BACKUP="$MERGETOOL_TMPDIR/${BASE}_${BACKUP_NAME}_$$$ext"
+ LOCAL="$MERGETOOL_TMPDIR/${BASE}_${LOCAL_NAME}_$$$ext"
+ REMOTE="$MERGETOOL_TMPDIR/${BASE}_${REMOTE_NAME}_$$$ext"
+ BASE="$MERGETOOL_TMPDIR/${BASE}_${BASE_NAME}_$$$ext"
base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}')
local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}')
@@ -396,6 +403,18 @@ do
--prompt)
prompt=true
;;
+ --local=*)
+ LOCAL_NAME=${1#--local=}
+ ;;
+ --remote=*)
+ REMOTE_NAME=${1#--remote=}
+ ;;
+ --base=*)
+ BASE_NAME=${1#--base=}
+ ;;
+ --backup=*)
+ BACKUP_NAME=${1#--backup=}
+ ;;
--)
shift
break
@@ -410,6 +429,37 @@ do
shift
done
+# sanity check after parsing command line
+case "" in
+"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
+ die "You cannot set any of --local/remote/base/backup to empty."
+ ;;
+esac
+
+case "$LOCAL_NAME" in
+"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
+ die "You cannot set any of --remote/base/backup to same as --local."
+ ;;
+esac
+
+case "$REMOTE_NAME" in
+"$LOCAL_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
+ die "You cannot set any of --local/base/backup to same as --remote."
+ ;;
+esac
+
+case "$BASE_NAME" in
+"$LOCAL_NAME"|"$REMOTE_NAME"|"$BACKUP_NAME")
+ die "You cannot set any of --local/remote/backup to same as --base."
+ ;;
+esac
+
+case "$BACKUP_NAME" in
+"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME")
+ die "You cannot set any of --local/remote/base to same as --backup."
+ ;;
+esac
+
prompt_after_failed_merge () {
while true
do
--
2.7.4
^ permalink raw reply related
* RE: git merge deletes my changes
From: Eduard Egorov @ 2016-10-12 5:51 UTC (permalink / raw)
To: 'Jakub Narębski'
Cc: 'Paul Smith', 'git@vger.kernel.org',
'Jeff King'
In-Reply-To: <94ff5fb3-6957-8983-4aa7-e1d5e2692e82@gmail.com>
Hello Jakub,
Thank you for addition. Eventually, I've merged your explanations into single answer post (http://stackoverflow.com/questions/39954265/git-merge-s-subtree-works-incorrectly/ ). I hope this will prevent other confused people from disturbing you by similar emails on this mailing list.
This is another time I can evidence the power and flexibility the git provides, thank you all for your great work!
With best regards
Eduard Egorov
-----Original Message-----
From: Jakub Narębski [mailto:jnareb@gmail.com]
Sent: Tuesday, October 11, 2016 6:57 PM
To: Paul Smith; Eduard Egorov; 'git@vger.kernel.org'
Subject: Re: git merge deletes my changes
W dniu 10.10.2016 o 19:52, Paul Smith pisze:
> On Mon, 2016-10-10 at 10:19 +0000, Eduard Egorov wrote:
>> # ~/gitbuild/git-2.10.1/git merge -s subtree --squash ceph_ansible
>>
>> Can somebody confirm this please? Doesn't "merge -s subtree" really
>> merges branches?
>
> I think possibly you're not fully understanding what the --squash flag
> does... that's what's causing your issue here, not the "-s" option.
>
> A squash merge takes the commits that would be merged from the origin
> branch and squashes them into a single patch and applies them to the
> current branch as a new commit... but this new commit is not a merge
> commit (that is, when you look at it with "git show" etc. the commit
> will have only one parent, not two--or more--parents like a normal
> merge commit).
>
> Basically, it's syntactic sugar for a diff plus patch operation plus
> some Git goodness wrapped around it to make it easier to use.
Actually this is full merge + commit surgery (as if you did merge with --no-commit, then deleted MERGE_HEAD); the state of worktree is as if it were after a merge.
>
> But ultimately once you're done, Git has no idea that this new commit
> has any relationship whatsoever to the origin branch. So the next
> time you merge, Git doesn't know that there was a previous merge and
> it will try to merge everything from scratch rather than starting at
> the previous common merge point.
>
> So either you'll have to use a normal, non-squash merge, or else
> you'll have to tell Git by hand what the previous common merge point
> was (as Jeff King's excellent email suggests). Or else, you'll have
> to live with this behavior.
The `git subtree` command (from contrib) allows yet another way: it squashes *history* of merged subproject (as if with interactive rebase 'squash'), then merges this squash commit.
Now I know why this feature is here...
--
Jakub Narębski
^ permalink raw reply
* Bug with git merge-base and a packed ref
From: Stepan Kasal @ 2016-10-12 10:37 UTC (permalink / raw)
To: git
Hello,
first, I observed a bug with git pull --rebase:
if the remote branch got rebased and the loval branch was updated,
pull tried to rebase the whole branch, not the local increment.
A reproducer would look like that
# in repo1:
git checkout tmp
cd ..
git clone repo1 repo2
cd repo1
git rebase elsewhere tmp
cd ../repo2
# edit
git commit -a -m 'Another commit'
git pull -r
The last command performs something like
git rebase new-origin/tmp
instead of
git rebase --onto new-origin/tmp old-origin/tmp
I'm using git version 2.10.1.windows.1
I tried to debug the issue:
I found that the bug happens only at the very first pull after clone.
I was able to reproduce it with git-pull.sh
The problem seems to be that command
git merge-base --fork-point refs/remotes/origin/tmp refs/heads/tmp
returns nothing, because the refs are packed.
Could you please fix merge-base so that it understands packed refs?
Thanks,
Stepan
^ permalink raw reply
* git diff
From: webmaster @ 2016-10-12 10:50 UTC (permalink / raw)
To: git
Hi.
I created a new branch named hotfix from master.
I switched to the branch, changed 1 file.
Now I want to see the diff from the both using
git diff hotfix master
I do not see any output (difference).
When I do a git status I see my file with status mofified, not staged for
commit.
Also, I can see that I am working with the correct branch, hotfix
What am I doing wrong?
-fuz
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Jakub Narębski @ 2016-10-12 10:54 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, Junio C Hamano, Jeff King
In-Reply-To: <03278DA5-34B8-42F1-B52E-A42A3BCD5FB8@gmail.com>
W dniu 12.10.2016 o 00:26, Lars Schneider pisze:
>> 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?
Looks good to me.
I think this information about precedence between one-shot `clean`
and `smudge` filter driver configuration, and multi-file `process`
filter driver should be here for two reasons.
First, if one is interested in running filter, but do not want to
write one (he or she uses existing tool, for example one of
existing LFS solutions), one can skip the "Long Running Filter
Process" section. But one still needs to know if to remove or
comment out old `clean` and `smudge` config, or how to provide
fallback for older Git (if one uses the same configuration with
pre-process Git and Git including support for this feature).
Second, the configuration belongs, in my opinion, here. It is
not a part of long running filter protocol.
>>> +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." ?
Yes.
> I think I like the original version better.
Well, I think it is better to err out on the side of being more
explicit.
>
>>> 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.
I'm sorry it was *my mistake*. I have read the example exchange wrong.
On the other hand that means that I have other comment, which I though
was addressed already in v10, namely that not all exchanges ends with
flush packet (inconsistency, and I think a bit of lack of extendability).
>> In example you have Git sending "git-filter-client" and list of supported
>> protocol versions, terminated with flush packet,
>
> Correct.
[thinking out loud]
And this serves as a 'canary' to detect single-shot driver mis-configured
to serve as multi-file filter driver.
>> 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.
Actually, the text reads that filter driver sends two lines: a line with
magic signature "git-filter-server", and exactly one line with protocol
version "version=2", *WITHOUT* terminating flush packet.
The example reads the same, I have just missed change of prefix from
"git<" to "git>" (that is "<" to mark response from filter, to ">" to
mark signal from Git).
So the text and example agrees, just me (and now you) misread the
example ;-/
IMHO this exchange should be also terminated with a flush packet,
even if in protocol version 2 it is fixed length list, and doesn't
strictly need it.
First, it would make easier to implement the filter driver process.
You would need only one 'read until flush' helper function, and two
higher-level functions: one for handling metadata, one for handling
contents (where handling = sending or receiving). Currently first
data send from filter is a bit of special case: you need to send
two pkt-lines, not send this list of lines and terminate with flush.
Second, it would allow for additional possibilities for new versions
and extending protocol, either 3-part handshake (but now I think that
4-part is better, at least in some cases), or some other "early start"
extension. OTOH we could stuff this data in additional exchange
(assuming new protocol version), and unless the exchange data goes
through slow channel (e.g. network), it shouldn't matter for the
latency that we have one more exchange.
Third, as we can see first from my error, then from yours, it would
make it easier to debug the protocol...
>>
>> 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?
Yes, it does; as I have said already, I have misread the example.
Anyway, in some cases 4-way handshake, where Git sends list of
supported capabilities first, is better. If the protocol has
to prepare something for each of capabilities, and perhaps check
those preparation status, it can do it after Git sends what it
could need, and before it sends what it does support.
Though it looks a bit strange that client (as Git is client here)
sends its capabilities first...
>> 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
Right.
>> 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.
From what I remember it was Peff stating that he thinks "<foo>=true"
is easier for parsing (it is, but we still need to support the harder
way parsing anyway), and offered that "<foo>" is good enough (if less
consistent).
> I am happy to change that if a second reviewer shares your
> opinion.
Also, with "capability=<foo>" we can be more self descriptive,
for example "supported-command=<foo>"; though "capability" is good
enough for me.
For example
packet: git> wants=clean
packet: git> wants=smudge
packet: git> wants=size
packet: git> 0000
packet: git< supports=clean
packet: git< supports=smudge
packet: git< 0000
Though coming up with good names is hard; and as I said "capability"
is good enough; OTOH with "smudge=true" etc. we don't need to come
up with good name at all... though I wonder if it is a good thing `\_o,_/
>>> +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.
Thanks. That doesn't matter much, but it matters.
Though it could go either way.
>>> 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?!
I'm a bit unsure. Original reads better and is shorter; the new
proposal is more explicit, but also more repetitive and longer.
>>> +------------------------
>>> +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.
O.K. though shouldn't it be after "empty list", then?
>>> +------------------------
>>> +
>>> +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?
Better, I think.
>>
>>> +------------------------
>>> +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."
All right, I think.
>>> +------------------------
>>> +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.
Good.
>>> +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?
All right, though you could also write it as "mimic what the ...
do in those cases"; I'm not sure if its better or worse.
>>> +------------------------
>>> +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 listening anymore.
I think it might be good idea to have here the information about
what filter process should do if it needs maybe lengthy closing
process, to not hold/stop Git command or to not be killed.
>>> +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,
You are welcome.
Thanks for working on this series,
--
Jakub Narębski
^ permalink raw reply
* Re: git diff
From: Mike Rappazzo @ 2016-10-12 11:06 UTC (permalink / raw)
To: webmaster; +Cc: git
In-Reply-To: <1066408917.43087.1476269456819.JavaMail.open-xchange@app06.ox.hosteurope.de>
On Wed, Oct 12, 2016 at 6:50 AM, <webmaster@peter-speer.de> wrote:
> Hi.
>
> I created a new branch named hotfix from master.
> I switched to the branch, changed 1 file.
>
> Now I want to see the diff from the both using
>
> git diff hotfix master
>
> I do not see any output (difference).
> When I do a git status I see my file with status mofified, not staged for
> commit.
Since you just created the branch, and did not add any content, there
is no difference to see. A branch is just a pointer to a commit. You
now have two pointers pointing at the same commit.
If you want to see the difference between your changes and the master
branch, you can omit the first reference:
git diff master
When you start adding commits to your hotfix branch, you will be able
to see the diff between that and master with the command that you
gave. However, your arguments may be in the reverse order than what
you expect. You want to specify master first because that is the
mainline branch (I presume).
When you have several commits on your hotfix branch, you can refer to
older commits to diff against. There are several ways to refer back,
but the simplest is to use a tilde '~' followed by a number to count
back. For example 'hotfix~1' refers to the parent commit on the
hotfix branch. There is a lot in the documentation[1], so take a look
there for more info.
Good luck.
_Mike
[1] https://git-scm.com/doc
> Also, I can see that I am working with the correct branch, hotfix
>
> What am I doing wrong?
>
> -fuz
On Wed, Oct 12, 2016 at 6:50 AM, <webmaster@peter-speer.de> wrote:
> Hi.
>
> I created a new branch named hotfix from master.
> I switched to the branch, changed 1 file.
>
> Now I want to see the diff from the both using
>
> git diff hotfix master
>
> I do not see any output (difference).
> When I do a git status I see my file with status mofified, not staged for
> commit.
> Also, I can see that I am working with the correct branch, hotfix
>
> What am I doing wrong?
>
> -fuz
^ permalink raw reply
* Re: [PATCH v3 12/25] sequencer: remember the onelines when parsing the todo file
From: Johannes Schindelin @ 2016-10-12 11:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqq1szmaemr.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/sequencer.c b/sequencer.c
> > index afc494e..7ba5e07 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -708,6 +708,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> > struct todo_item {
> > enum todo_command command;
> > struct commit *commit;
> > + const char *arg;
> > + int arg_len;
> > size_t offset_in_buf;
>
> micronit: you can make it to size_t and lose the cast below, no?
No. The primary users of arg_len call a printf() style function with %.*s,
expecting an int. So your suggestion would lose one cast, but introduce at
least four casts in return.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 08/25] sequencer: strip CR from the todo script
From: Johannes Schindelin @ 2016-10-12 11:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqq60oyaf7s.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/sequencer.c b/sequencer.c
> > index 678fdf3..cee7e50 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -774,6 +774,9 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
> >
> > next_p = *eol ? eol + 1 /* skip LF */ : eol;
> >
> > + if (p != eol && eol[-1] == '\r')
> > + eol--; /* skip Carriage Return */
>
> micronit: s/skip/strip/ ;-)
Okay,
Dscho
^ permalink raw reply
* Re: [PATCH v3 13/25] sequencer: prepare for rebase -i's commit functionality
From: Johannes Schindelin @ 2016-10-12 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt, Michael Haggerty
In-Reply-To: <xmqqwphe8zl2.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> > @@ -370,19 +383,79 @@ static int is_index_unchanged(void)
> > }
> >
> > /*
> > + * Read the author-script file into an environment block, ready for use in
> > + * run_command(), that can be free()d afterwards.
> > + */
> > +static char **read_author_script(void)
> > +{
> > + struct strbuf script = STRBUF_INIT;
> > + int i, count = 0;
> > + char *p, *p2, **env;
> > + size_t env_size;
> > +
> > + if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
> > + return NULL;
> > +
> > + for (p = script.buf; *p; p++)
> > + if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> > + strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> > + else if (*p == '\'')
> > + strbuf_splice(&script, p-- - script.buf, 1, "", 0);
> > + else if (*p == '\n') {
> > + *p = '\0';
> > + count++;
> > + }
>
> Hmph, didn't we recently add parse_key_value_squoted() to build
> read_author_script() in builtin/am.c on top of it, so that this
> piece of code can also take advantage of and share the parser?
I already pointed out that the author-script file may *not* be quoted.
sq_dequote() would return NULL and parse_key_value_squoted() would *fail*.
To complicate things further, the sequencer does not even need to access
the values at all. It needs to pass them to run_command() as an
environment block, which means that we would have to reconstruct the lines
after parse_key_value_squoted() painstakingly untangled the key names from
the values.
In short, this is another instance where using a function just because it
exists and is nominally related would make the resulting patch *more*
complicated than it currently is.
> > +/*
>
> Offtopic: this line and the beginning of the new comment block that
> begins with "Read the author-script" above show a suboptimal marking
> of what is added and what is left. I wonder "diff-indent-heuristic"
> topic by Michael can help to make it look better.
Maybe. I'll try to look into that once the more serious questions about
this patch series have been addressed.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 05/25] sequencer: eventually release memory allocated for the option values
From: Johannes Schindelin @ 2016-10-12 12:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqq4m4ic0gw.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> The only reason why the OPT_STRDUP appeared convenient was because
> options[] element happened to use a field in the structure directly.
> The patch under discussion does an equivalent of
>
> app.x_field = xstrdup_or_null(opt_x);
Oh, that xstrdup_or_null() function slipped by me. My local patches use it
now.
Ciao,
Dscho
^ permalink raw reply
* [PATCH] t1512: become resilient to GETTEXT_POISON build
From: Vasco Almeida @ 2016-10-12 12:25 UTC (permalink / raw)
To: git; +Cc: Vasco Almeida, Jeff King
The concerned message was marked for translation by 0c99171
("get_short_sha1: mark ambiguity error for translation", 2016-09-26).
Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
---
t/t1512-rev-parse-disambiguation.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 7c659eb..711704b 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -42,7 +42,7 @@ test_expect_success 'blob and tree' '
test_expect_success 'warn ambiguity when no candidate matches type hint' '
test_must_fail git rev-parse --verify 000000000^{commit} 2>actual &&
- grep "short SHA1 000000000 is ambiguous" actual
+ test_i18ngrep "short SHA1 000000000 is ambiguous" actual
'
test_expect_success 'disambiguate tree-ish' '
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 2/2] reset: support the --stdin option
From: Johannes Schindelin @ 2016-10-12 12:39 UTC (permalink / raw)
To: Jakub Narębski; +Cc: git, Junio C Hamano
In-Reply-To: <dc76476b-9ad5-a3b6-f12f-33cda2ca5814@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 455 bytes --]
Hi Kuba,
On Tue, 11 Oct 2016, Jakub Narębski wrote:
> W dniu 11.10.2016 o 18:09, Johannes Schindelin pisze:
>
> > SYNOPSIS
> > --------
> > [verse]
> > -'git reset' [-q] [<tree-ish>] [--] <paths>...
> > +'git reset' [-q] [--stdin [-z]] [<tree-ish>] [--] <paths>...
>
> I think you meant here
>
> +'git reset' [-q] [--stdin [-z]] [<tree-ish>]
Good point. I overlooked that the <paths>... are not optional here.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH 2/2] reset: support the --stdin option
From: Johannes Schindelin @ 2016-10-12 12:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqeg3mai1b.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > + if (read_from_stdin) {
> > + strbuf_getline_fn getline_fn = nul_term_line ?
> > + strbuf_getline_nul : strbuf_getline_lf;
> > + int flags = PATHSPEC_PREFER_FULL |
> > + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP;
> > + struct strbuf buf = STRBUF_INIT;
> > + struct strbuf unquoted = STRBUF_INIT;
> > +
> > + if (patch_mode)
> > + die(_("--stdin is incompatible with --patch"));
> > +
> > + if (pathspec.nr)
> > + die(_("--stdin is incompatible with path arguments"));
> > +
> > + if (patch_mode)
> > + flags |= PATHSPEC_PREFIX_ORIGIN;
>
> Didn't we already die above under that mode?
Oh right. Copy/paste fail.
> > + while (getline_fn(&buf, stdin) != EOF) {
> > + if (!nul_term_line && buf.buf[0] == '"') {
> > + strbuf_reset(&unquoted);
> > + if (unquote_c_style(&unquoted, buf.buf, NULL))
> > + die(_("line is badly quoted"));
> > + strbuf_swap(&buf, &unquoted);
> > + }
> > + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> > + stdin_paths[stdin_nr++] = xstrdup(buf.buf);
> > + strbuf_reset(&buf);
> > + }
> > + strbuf_release(&unquoted);
> > + strbuf_release(&buf);
> > +
> > + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> > + stdin_paths[stdin_nr++] = NULL;
>
> It makes sense to collect, but...
It does, doesn't it? I really would have loved to start resetting right
away, but if the list were not sorted and traversed at the same time as
the tree-ish, the performance would just be suboptimal.
I think that is an important point and I adjusted the commit message
accordingly.
> > + parse_pathspec(&pathspec, 0, flags, prefix,
> > + (const char **)stdin_paths);
>
> ...letting them be used as if they are pathspec is wrong when
> stdin_paths[] contain wildcard, isn't it?
>
> I think flags |= PATHSPEC_LITERAL_PATH can help fixing it. 0/2 said
> this mimicks checkout-index and I think it should by not treating
> the input as wildcarded patterns (i.e. "echo '*.c' | reset --stdin"
> shouldn't be the way to reset all .c files --- that's something we
> would want to add to the test, I guess).
True. I adjust the flags accordingly now.
Thanks,
Dscho
^ 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