[PATCH 3/3] interpret-trailers: add options for actions

[Paolo Bonzini]

From: Paolo Bonzini 

Allow using non-default values for trailers without having to set
them up in .gitconfig first.  For example, if you have the following
configuration

 trailer.signed-off-by.where = end

you may use "--where before" when a patch author forgets his
Signed-off-by and provides it in a separate email.

Signed-off-by: Paolo Bonzini 
---
 Documentation/git-interpret-trailers.txt | 16 ++
 builtin/interpret-trailers.c | 40 +
 t/t7513-interpret-trailers.sh| 51 
 trailer.c| 28 --
 trailer.h|  6 
 5 files changed, 138 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index 31cdeaecd..f4d67b8a1 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -80,6 +80,22 @@ OPTIONS
trailer to the input messages. See the description of this
command.
 
+--where ::
+   Specify where all new trailers will be added.  This option
+   overrides all configuration variables.
+
+--if-exists ::
+   Specify what action will be performed when there is already at
+   least one trailer with the same  in the message.  This
+   option applies to all '--trailer' options and overrides all
+   configuration variables.
+
+--if-missing ::
+   Specify what action will be performed when there is no other
+   trailer with the same  in the message.  This option
+   applies to all '--trailer' options and overrides all
+   configuration variables.
+
 CONFIGURATION VARIABLES
 ---
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 6528680b5..5f05f0af0 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -16,6 +16,39 @@ static const char * const git_interpret_trailers_usage[] = {
NULL
 };
 
+static int option_parse_where(const struct option *opt,
+ const char *arg, int unset)
+{
+   enum action_where *where = opt->value;
+
+   if (unset)
+   return 0;
+
+   return set_where(where, arg);
+}
+
+static int option_parse_if_exists(const struct option *opt,
+ const char *arg, int unset)
+{
+   enum action_if_exists *if_exists = opt->value;
+
+   if (unset)
+   return 0;
+
+   return set_if_exists(if_exists, arg);
+}
+
+static int option_parse_if_missing(const struct option *opt,
+  const char *arg, int unset)
+{
+   enum action_if_missing *if_missing = opt->value;
+
+   if (unset)
+   return 0;
+
+   return set_if_missing(if_missing, arg);
+}
+
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
struct trailer_opts opts = { 0 };
@@ -24,6 +57,13 @@ int cmd_interpret_trailers(int argc, const char **argv, 
const char *prefix)
struct option options[] = {
OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in 
place")),
OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty 
trailers")),
+   OPT_CALLBACK(0, "where", &opts.where, N_("action"),
+N_("where to place the new trailer"), 
option_parse_where),
+   OPT_CALLBACK(0, "if-exists", &opts.if_exists, N_("action"),
+N_("action if trailer already exists"), 
option_parse_if_exists),
+   OPT_CALLBACK(0, "if-missing", &opts.if_missing, N_("action"),
+N_("action if trailer is missing"), 
option_parse_if_missing),
+
OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
N_("trailer(s) to add")),
OPT_END()
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 0c6f91c43..49f4f823d 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -681,6 +681,21 @@ test_expect_success 'using "where = before"' '
test_cmp expected actual
 '
 
+test_expect_success 'overriding configuration with "--where after"' '
+   git config trailer.ack.where "before" &&
+   cat complex_message_body >expected &&
+   sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+   Fixes: Z
+   Acked-by= Z
+   Acked-by= Peff
+   Reviewed-by: Z
+   Signed-off-by: Z
+   EOF
+   git interpret-trailers --where 

[PATCH 1/3] trailers: create struct trailer_opts

[Paolo Bonzini]

From: Paolo Bonzini 

Pass the command-line arguments as a pointer to a new struct.  This
will be extended soon to include more options.

Signed-off-by: Paolo Bonzini 
---
 builtin/interpret-trailers.c | 13 ++---
 trailer.c| 14 --
 trailer.h|  7 ++-
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 175f14797..6528680b5 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -18,13 +18,12 @@ static const char * const git_interpret_trailers_usage[] = {
 
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
-   int in_place = 0;
-   int trim_empty = 0;
+   struct trailer_opts opts = { 0 };
struct string_list trailers = STRING_LIST_INIT_NODUP;
 
struct option options[] = {
-   OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
-   OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty 
trailers")),
+   OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in 
place")),
+   OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty 
trailers")),
OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
N_("trailer(s) to add")),
OPT_END()
@@ -36,11 +35,11 @@ int cmd_interpret_trailers(int argc, const char **argv, 
const char *prefix)
if (argc) {
int i;
for (i = 0; i < argc; i++)
-   process_trailers(argv[i], in_place, trim_empty, 
&trailers);
+   process_trailers(argv[i], &opts, &trailers);
} else {
-   if (in_place)
+   if (opts.in_place)
die(_("no input file given for in-place editing"));
-   process_trailers(NULL, in_place, trim_empty, &trailers);
+   process_trailers(NULL, &opts, &trailers);
}
 
string_list_clear(&trailers, 0);
diff --git a/trailer.c b/trailer.c
index 751b56c00..a3eb42818 100644
--- a/trailer.c
+++ b/trailer.c
@@ -164,13 +164,14 @@ 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 list_head *head, int trim_empty)
+static void print_all(FILE *outfile, struct list_head *head,
+ struct trailer_opts *opts)
 {
struct list_head *pos;
struct trailer_item *item;
list_for_each(pos, head) {
item = list_entry(pos, struct trailer_item, list);
-   if (!trim_empty || strlen(item->value) > 0)
+   if (!opts->trim_empty || strlen(item->value) > 0)
print_tok_val(outfile, item->token, item->value);
}
 }
@@ -968,7 +969,8 @@ static FILE *create_in_place_tempfile(const char *file)
return outfile;
 }
 
-void process_trailers(const char *file, int in_place, int trim_empty, struct 
string_list *trailers)
+void process_trailers(const char *file, struct trailer_opts *opts,
+ struct string_list *trailers)
 {
LIST_HEAD(head);
LIST_HEAD(arg_head);
@@ -980,7 +982,7 @@ void process_trailers(const char *file, int in_place, int 
trim_empty, struct str
 
read_input_file(&sb, file);
 
-   if (in_place)
+   if (opts->in_place)
outfile = create_in_place_tempfile(file);
 
/* Print the lines before the trailers */
@@ -990,14 +992,14 @@ void process_trailers(const char *file, int in_place, int 
trim_empty, struct str
 
process_trailers_lists(&head, &arg_head);
 
-   print_all(outfile, &head, trim_empty);
+   print_all(outfile, &head, opts);
 
free_all(&head);
 
/* Print the lines after the trailers as is */
fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
-   if (in_place)
+   if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
die_errno(_("could not rename temporary file to %s"), 
file);
 
diff --git a/trailer.h b/trailer.h
index 65cc5d79c..e90ba1270 100644
--- a/trailer.h
+++ b/trailer.h
@@ -1,6 +1,11 @@
 #ifndef TRAILER_H
 #define TRAILER_H
 
+struct trailer_opts {
+   int in_place;
+   int trim_empty;
+};
+
 struct trailer_info {
/*
 * True if there is a blank line before the location pointed to by
@@ -22,7 +27,7 @@ struct trailer_info {
size_t trailer_nr;
 };
 
-void process_trailers(const char *file, int in_place, int trim_empty,
+void process_trailers(const char *file, struct trailer_opts *opts,
  struct string_list *trailers);
 
 void trailer_info_get(struct trailer_info *info, const char *str);
-- 
2.13.0

[PATCH 0/3] interpret-trailers: add --where, --if-exists, --if-missing

[Paolo Bonzini]

From: Paolo Bonzini 

These options are useful to experiment with "git interpret-trailers"
without having to tinker with .gitconfig.  It can also be useful in the
oddball case where you want a different placement for the trailer.

The case that stimulated the creation of the patches was configuring

 trailer.signed-off-by.where = end

and then wanting "--where before" when a patch author forgets his
Signed-off-by and provides it in a separate email.

Paolo

Paolo Bonzini (3):
  trailers: create struct trailer_opts
  trailers: export action enums and corresponding lookup functions
  interpret-trailers: add options for actions

 Documentation/git-interpret-trailers.txt | 16 ++
 builtin/interpret-trailers.c | 53 +++---
 t/t7513-interpret-trailers.sh| 51 ++
 trailer.c| 93 
 trailer.h| 35 +++-
 5 files changed, 205 insertions(+), 43 deletions(-)

-- 
2.13.0

[PATCH 2/3] trailers: export action enums and corresponding lookup functions

[Paolo Bonzini]

From: Paolo Bonzini 

Separate the mechanical changes out of the next patch.  The functions
are changed to take a pointer to enum, because struct conf_info is not
going to be public.

Write down the defaults explicitly in default_conf_info, since they are
not anymore close to default_conf_info and it's not obvious which
constant has value 0.

Signed-off-by: Paolo Bonzini 
---
 trailer.c | 51 +--
 trailer.h | 22 ++
 2 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/trailer.c b/trailer.c
index a3eb42818..558c52431 100644
--- a/trailer.c
+++ b/trailer.c
@@ -10,11 +10,6 @@
  * Copyright (c) 2013, 2014 Christian Couder 
  */
 
-enum action_where { WHERE_END, WHERE_AFTER, WHERE_BEFORE, WHERE_START };
-enum action_if_exists { EXISTS_ADD_IF_DIFFERENT_NEIGHBOR, 
EXISTS_ADD_IF_DIFFERENT,
-   EXISTS_ADD, EXISTS_REPLACE, EXISTS_DO_NOTHING };
-enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };
-
 struct conf_info {
char *name;
char *key;
@@ -24,7 +19,11 @@ struct conf_info {
enum action_if_missing if_missing;
 };
 
-static struct conf_info default_conf_info;
+static struct conf_info default_conf_info = {
+   .where = WHERE_END,
+   .if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR,
+   .if_missing = MISSING_ADD,
+};
 
 struct trailer_item {
struct list_head list;
@@ -374,44 +373,44 @@ static void process_trailers_lists(struct list_head *head,
}
 }
 
-static int set_where(struct conf_info *item, const char *value)
+int set_where(enum action_where *item, const char *value)
 {
if (!strcasecmp("after", value))
-   item->where = WHERE_AFTER;
+   *item = WHERE_AFTER;
else if (!strcasecmp("before", value))
-   item->where = WHERE_BEFORE;
+   *item = WHERE_BEFORE;
else if (!strcasecmp("end", value))
-   item->where = WHERE_END;
+   *item = WHERE_END;
else if (!strcasecmp("start", value))
-   item->where = WHERE_START;
+   *item = WHERE_START;
else
return -1;
return 0;
 }
 
-static int set_if_exists(struct conf_info *item, const char *value)
+int set_if_exists(enum action_if_exists *item, const char *value)
 {
if (!strcasecmp("addIfDifferent", value))
-   item->if_exists = EXISTS_ADD_IF_DIFFERENT;
+   *item = EXISTS_ADD_IF_DIFFERENT;
else if (!strcasecmp("addIfDifferentNeighbor", value))
-   item->if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+   *item = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
else if (!strcasecmp("add", value))
-   item->if_exists = EXISTS_ADD;
+   *item = EXISTS_ADD;
else if (!strcasecmp("replace", value))
-   item->if_exists = EXISTS_REPLACE;
+   *item = EXISTS_REPLACE;
else if (!strcasecmp("doNothing", value))
-   item->if_exists = EXISTS_DO_NOTHING;
+   *item = EXISTS_DO_NOTHING;
else
return -1;
return 0;
 }
 
-static int set_if_missing(struct conf_info *item, const char *value)
+int set_if_missing(enum action_if_missing *item, const char *value)
 {
if (!strcasecmp("doNothing", value))
-   item->if_missing = MISSING_DO_NOTHING;
+   *item = MISSING_DO_NOTHING;
else if (!strcasecmp("add", value))
-   item->if_missing = MISSING_ADD;
+   *item = MISSING_ADD;
else
return -1;
return 0;
@@ -471,15 +470,15 @@ static int git_trailer_default_config(const char 
*conf_key, const char *value, v
variable_name = strrchr(trailer_item, '.');
if (!variable_name) {
if (!strcmp(trailer_item, "where")) {
-   if (set_where(&default_conf_info, value) < 0)
+   if (set_where(&default_conf_info.where, value) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "ifexists")) {
-   if (set_if_exists(&default_conf_info, value) < 0)
+   if (set_if_exists(&default_conf_info.if_exists, value) 
< 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "ifmissing")) {
-   if (set_if_missing(&default_conf_info, value) < 0)
+   if (set_if_missing(&default_c

Re: [PATCH 0/3] interpret-trailers: add --where, --if-exists, --if-missing

[Paolo Bonzini]

On 12/07/2017 16:47, Christian Couder wrote:
> On Wed, Jul 12, 2017 at 3:46 PM, Paolo Bonzini  wrote:
>>
>> These options are useful to experiment with "git interpret-trailers"
>> without having to tinker with .gitconfig.  It can also be useful in the
>> oddball case where you want a different placement for the trailer.
>>
>> The case that stimulated the creation of the patches was configuring
>>
>>  trailer.signed-off-by.where = end
>>
>> and then wanting "--where before" when a patch author forgets his
>> Signed-off-by and provides it in a separate email.
> 
> Maybe you could have used the following to temporarily override the config:
> 
> git -c trailer.signed-off-by.where=before interpret-trailers ...
> 
> But it could be helpful and more straightforward to provide the
> options you implemented.

That works indeed---and I have now learnt that $GIT_CONFIG_PARAMETERS
makes "git -c" work across my thick layers of aliases!  The main
disadvantage is that it is harder to discover than a command-line option.

Also, I have scripts which pass the --trailer argument is passed
unmodified to "git interpret-trailers", and the command-line argument
avoids the need to parse the trailer to figure out the -c option.  In
particular, in my case the separator is always ":", but in general that
may not be the case.

> I am not sure also if --where should override both "trailer.where" and
> "trailer..where", or if should just override the former.

I think it should override both, otherwise you have different behavior
depending on whether trailer..where is defined or not.

Paolo

Re: [PATCH 3/3] interpret-trailers: add options for actions

[Paolo Bonzini]

On 12/07/2017 23:10, Jonathan Tan wrote:
> When I would expect the last 2 commands to produce the same output. Maybe
> invoke set_where(where, NULL) when "unset" is true? And change set_where()
> accordingly. Same for the other two option parsing functions.

Sounds good, and I'll also add a test case of course.

In fact arg is already NULL if unset is true, so the code of the three
option parsing functions becomes simpler.

Thanks!

Paolo

[PATCH v2 1/3] trailers: create struct trailer_opts

[Paolo Bonzini]

From: Paolo Bonzini 

Pass the command-line arguments as a pointer to a new struct.  This
will be extended in the next patch to include more options.

Signed-off-by: Paolo Bonzini 
---
v1->v2: constify

 builtin/interpret-trailers.c | 13 ++---
 trailer.c| 14 --
 trailer.h|  7 ++-
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 175f14797..6528680b5 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -18,13 +18,12 @@ static const char * const git_interpret_trailers_usage[] = {
 
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
-   int in_place = 0;
-   int trim_empty = 0;
+   struct trailer_opts opts = { 0 };
struct string_list trailers = STRING_LIST_INIT_NODUP;
 
struct option options[] = {
-   OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
-   OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty 
trailers")),
+   OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in 
place")),
+   OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty 
trailers")),
OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
N_("trailer(s) to add")),
OPT_END()
@@ -36,11 +35,11 @@ int cmd_interpret_trailers(int argc, const char **argv, 
const char *prefix)
if (argc) {
int i;
for (i = 0; i < argc; i++)
-   process_trailers(argv[i], in_place, trim_empty, 
&trailers);
+   process_trailers(argv[i], &opts, &trailers);
} else {
-   if (in_place)
+   if (opts.in_place)
die(_("no input file given for in-place editing"));
-   process_trailers(NULL, in_place, trim_empty, &trailers);
+   process_trailers(NULL, &opts, &trailers);
}
 
string_list_clear(&trailers, 0);
diff --git a/trailer.c b/trailer.c
index 751b56c00..a3eb42818 100644
--- a/trailer.c
+++ b/trailer.c
@@ -164,13 +164,14 @@ 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 list_head *head, int trim_empty)
+static void print_all(FILE *outfile, struct list_head *head,
+ const struct trailer_opts *opts)
 {
struct list_head *pos;
struct trailer_item *item;
list_for_each(pos, head) {
item = list_entry(pos, struct trailer_item, list);
-   if (!trim_empty || strlen(item->value) > 0)
+   if (!opts->trim_empty || strlen(item->value) > 0)
print_tok_val(outfile, item->token, item->value);
}
 }
@@ -968,7 +969,8 @@ static FILE *create_in_place_tempfile(const char *file)
return outfile;
 }
 
-void process_trailers(const char *file, int in_place, int trim_empty, struct 
string_list *trailers)
+void process_trailers(const char *file, const struct trailer_opts *opts,
+ struct string_list *trailers)
 {
LIST_HEAD(head);
LIST_HEAD(arg_head);
@@ -980,7 +982,7 @@ void process_trailers(const char *file, int in_place, int 
trim_empty, struct str
 
read_input_file(&sb, file);
 
-   if (in_place)
+   if (opts->in_place)
outfile = create_in_place_tempfile(file);
 
/* Print the lines before the trailers */
@@ -990,14 +992,14 @@ void process_trailers(const char *file, int in_place, int 
trim_empty, struct str
 
process_trailers_lists(&head, &arg_head);
 
-   print_all(outfile, &head, trim_empty);
+   print_all(outfile, &head, opts);
 
free_all(&head);
 
/* Print the lines after the trailers as is */
fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
-   if (in_place)
+   if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
die_errno(_("could not rename temporary file to %s"), 
file);
 
diff --git a/trailer.h b/trailer.h
index 65cc5d79c..e90ba1270 100644
--- a/trailer.h
+++ b/trailer.h
@@ -1,6 +1,11 @@
 #ifndef TRAILER_H
 #define TRAILER_H
 
+struct trailer_opts {
+   int in_place;
+   int trim_empty;
+};
+
 struct trailer_info {
/*
 * True if there is a blank line before the location pointed to by
@@ -22,7 +27,7 @@ struct trailer_info {
size_t trailer_nr;
 };
 
-void process_trailers(const char *file, int in_place, int trim_empty,
+void process_trailers(const char *file, const struct trailer_opts *opts,
  struct string_list *trailers);
 
 void trailer_info_get(struct trailer_info *info, const char *str);
-- 
2.13.0

[PATCH v2 0/3] interpret-trailers: add --where, --if-exists, --if-missing

[Paolo Bonzini]

From: Paolo Bonzini 

These options are useful to experiment with "git interpret-trailers"
without having to tinker with .gitconfig.  It can also be useful in the
oddball case where you want a different placement for the trailer.

Compared to "git -c", they are more easily discoverable, and also have
slightly different behavior because they override all trailer.*
configuration keys.

Paolo

v1->v2: support --no-* options, minor code fixes

Paolo Bonzini (3):
  trailers: create struct trailer_opts
  trailers: export action enums and corresponding lookup functions
  interpret-trailers: add options for actions

 Documentation/git-interpret-trailers.txt |  16 +
 builtin/interpret-trailers.c |  44 ++---
 t/t7513-interpret-trailers.sh|  66 
 trailer.c| 102 ---
 trailer.h|  35 ++-
 5 files changed, 218 insertions(+), 45 deletions(-)

-- 
2.13.0

[PATCH v2 2/3] trailers: export action enums and corresponding lookup functions

[Paolo Bonzini]

From: Paolo Bonzini 

Separate the mechanical changes out of the next patch.  The functions
are changed to take a pointer to enum, because struct conf_info is not
going to be public.

Set the default values explicitly in default_conf_info, since they are
not anymore close to default_conf_info and it's not obvious which
constant has value 0.  With the next patch, in fact, the values will
not be zero anymore!

Signed-off-by: Paolo Bonzini 
---
v1->v2: avoid use of designated initializers

 trailer.c | 48 +++-
 trailer.h | 22 ++
 2 files changed, 45 insertions(+), 25 deletions(-)

diff --git a/trailer.c b/trailer.c
index a3eb42818..a371c7b45 100644
--- a/trailer.c
+++ b/trailer.c
@@ -10,11 +10,6 @@
  * Copyright (c) 2013, 2014 Christian Couder 
  */
 
-enum action_where { WHERE_END, WHERE_AFTER, WHERE_BEFORE, WHERE_START };
-enum action_if_exists { EXISTS_ADD_IF_DIFFERENT_NEIGHBOR, 
EXISTS_ADD_IF_DIFFERENT,
-   EXISTS_ADD, EXISTS_REPLACE, EXISTS_DO_NOTHING };
-enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };
-
 struct conf_info {
char *name;
char *key;
@@ -374,44 +369,44 @@ static void process_trailers_lists(struct list_head *head,
}
 }
 
-static int set_where(struct conf_info *item, const char *value)
+int set_where(enum action_where *item, const char *value)
 {
if (!strcasecmp("after", value))
-   item->where = WHERE_AFTER;
+   *item = WHERE_AFTER;
else if (!strcasecmp("before", value))
-   item->where = WHERE_BEFORE;
+   *item = WHERE_BEFORE;
else if (!strcasecmp("end", value))
-   item->where = WHERE_END;
+   *item = WHERE_END;
else if (!strcasecmp("start", value))
-   item->where = WHERE_START;
+   *item = WHERE_START;
else
return -1;
return 0;
 }
 
-static int set_if_exists(struct conf_info *item, const char *value)
+int set_if_exists(enum action_if_exists *item, const char *value)
 {
if (!strcasecmp("addIfDifferent", value))
-   item->if_exists = EXISTS_ADD_IF_DIFFERENT;
+   *item = EXISTS_ADD_IF_DIFFERENT;
else if (!strcasecmp("addIfDifferentNeighbor", value))
-   item->if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+   *item = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
else if (!strcasecmp("add", value))
-   item->if_exists = EXISTS_ADD;
+   *item = EXISTS_ADD;
else if (!strcasecmp("replace", value))
-   item->if_exists = EXISTS_REPLACE;
+   *item = EXISTS_REPLACE;
else if (!strcasecmp("doNothing", value))
-   item->if_exists = EXISTS_DO_NOTHING;
+   *item = EXISTS_DO_NOTHING;
else
return -1;
return 0;
 }
 
-static int set_if_missing(struct conf_info *item, const char *value)
+int set_if_missing(enum action_if_missing *item, const char *value)
 {
if (!strcasecmp("doNothing", value))
-   item->if_missing = MISSING_DO_NOTHING;
+   *item = MISSING_DO_NOTHING;
else if (!strcasecmp("add", value))
-   item->if_missing = MISSING_ADD;
+   *item = MISSING_ADD;
else
return -1;
return 0;
@@ -471,15 +466,15 @@ static int git_trailer_default_config(const char 
*conf_key, const char *value, v
variable_name = strrchr(trailer_item, '.');
if (!variable_name) {
if (!strcmp(trailer_item, "where")) {
-   if (set_where(&default_conf_info, value) < 0)
+   if (set_where(&default_conf_info.where, value) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "ifexists")) {
-   if (set_if_exists(&default_conf_info, value) < 0)
+   if (set_if_exists(&default_conf_info.if_exists, value) 
< 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "ifmissing")) {
-   if (set_if_missing(&default_conf_info, value) < 0)
+   if (set_if_missing(&default_conf_info.if_missing, 
value) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strc

[PATCH v2 3/3] interpret-trailers: add options for actions

[Paolo Bonzini]

From: Paolo Bonzini 

Allow using non-default values for trailers without having to set
them up in .gitconfig first.  For example, if you have the following
configuration

 trailer.signed-off-by.where = end

you may use "--where before" when a patch author forgets his
Signed-off-by and provides it in a separate email.

Signed-off-by: Paolo Bonzini 
---
v1->v2: fix and test behavior of --no-where and similar

 Documentation/git-interpret-trailers.txt | 16 
 builtin/interpret-trailers.c | 31 +++
 t/t7513-interpret-trailers.sh| 66 
 trailer.c| 40 ---
 trailer.h|  6 +++
 5 files changed, 153 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index 31cdeaecd..f4d67b8a1 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -80,6 +80,22 @@ OPTIONS
trailer to the input messages. See the description of this
command.
 
+--where ::
+   Specify where all new trailers will be added.  This option
+   overrides all configuration variables.
+
+--if-exists ::
+   Specify what action will be performed when there is already at
+   least one trailer with the same  in the message.  This
+   option applies to all '--trailer' options and overrides all
+   configuration variables.
+
+--if-missing ::
+   Specify what action will be performed when there is no other
+   trailer with the same  in the message.  This option
+   applies to all '--trailer' options and overrides all
+   configuration variables.
+
 CONFIGURATION VARIABLES
 ---
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 6528680b5..bb93412ac 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -16,6 +16,30 @@ static const char * const git_interpret_trailers_usage[] = {
NULL
 };
 
+static int option_parse_where(const struct option *opt,
+ const char *arg, int unset)
+{
+   enum action_where *where = opt->value;
+
+   return set_where(where, arg);
+}
+
+static int option_parse_if_exists(const struct option *opt,
+ const char *arg, int unset)
+{
+   enum action_if_exists *if_exists = opt->value;
+
+   return set_if_exists(if_exists, arg);
+}
+
+static int option_parse_if_missing(const struct option *opt,
+  const char *arg, int unset)
+{
+   enum action_if_missing *if_missing = opt->value;
+
+   return set_if_missing(if_missing, arg);
+}
+
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
struct trailer_opts opts = { 0 };
@@ -24,6 +48,13 @@ int cmd_interpret_trailers(int argc, const char **argv, 
const char *prefix)
struct option options[] = {
OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in 
place")),
OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty 
trailers")),
+   OPT_CALLBACK(0, "where", &opts.where, N_("action"),
+N_("where to place the new trailer"), 
option_parse_where),
+   OPT_CALLBACK(0, "if-exists", &opts.if_exists, N_("action"),
+N_("action if trailer already exists"), 
option_parse_if_exists),
+   OPT_CALLBACK(0, "if-missing", &opts.if_missing, N_("action"),
+N_("action if trailer is missing"), 
option_parse_if_missing),
+
OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
N_("trailer(s) to add")),
OPT_END()
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 0c6f91c43..adbdf54f8 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -681,6 +681,36 @@ test_expect_success 'using "where = before"' '
test_cmp expected actual
 '
 
+test_expect_success 'overriding configuration with "--where after"' '
+   git config trailer.ack.where "before" &&
+   cat complex_message_body >expected &&
+   sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+   Fixes: Z
+   Acked-by= Z
+   Acked-by= Peff
+   Reviewed-by: Z
+   Signed-off-by: Z
+   EOF
+   git interpret-trailers --where after --trailer "ack: Peff" \
+   complex_message >actual &&
+   t

Re: [PATCH v2 0/3] interpret-trailers: add --where, --if-exists, --if-missing

[Paolo Bonzini]

On 13/07/2017 01:02, Junio C Hamano wrote:
> Paolo Bonzini  writes:
> 
>> From: Paolo Bonzini 
>>
>> These options are useful to experiment with "git interpret-trailers"
>> without having to tinker with .gitconfig.  It can also be useful in the
>> oddball case where you want a different placement for the trailer.
>>
>> Compared to "git -c", they are more easily discoverable, and also have
>> slightly different behavior because they override all trailer.*
>> configuration keys.
> 
> I think this is a very good idea (we shouldn't have started the
> command only with the configurations; we rather should have done
> this first and then added configuration after that).

Actually we can do better: we can have --where only refer to
*subsequent* --trailer options.  This will require more refactoring
(probably making a new struct that can be passed to process_trailers),
but the first two patches should be the same.

I'll have time for this only after vacation (so at the end of July), and
it's backwards-incompatible with this series.  Reviews are welcome
anyway. :)

Thanks for encouraging me.  It's always a pleasure when I can scratch my
git itches!

Paolo

> Looking forward to reviewing them, but I am cutting a maint release
> now, so it may have to wait a bit.
> 
> Thanks.
> 
>>
>> Paolo
>>
>> v1->v2: support --no-* options, minor code fixes
>>
>> Paolo Bonzini (3):
>>   trailers: create struct trailer_opts
>>   trailers: export action enums and corresponding lookup functions
>>   interpret-trailers: add options for actions
>>
>>  Documentation/git-interpret-trailers.txt |  16 +
>>  builtin/interpret-trailers.c |  44 ++---
>>  t/t7513-interpret-trailers.sh|  66 
>>  trailer.c| 102 
>> ---
>>  trailer.h|  35 ++-
>>  5 files changed, 218 insertions(+), 45 deletions(-)
> 

[PATCH v3 0/3] interpret-trailers: add --where, --if-exists, --if-missing

[Paolo Bonzini]

From: Paolo Bonzini 

These options are useful to experiment with "git interpret-trailers"
without having to tinker with .gitconfig (Junio said git should ahve
done this first and only added configuration afterwards).  It can
be useful in the case where you want a different placement for the trailer,
or for scripts/aliases that don't want to rely on specific .gitconfig
settings.

Compared to v2, the main change is that option order on the command-line
is respected.  That is,

--trailer 'acked-by: foo' --where end --trailer 'signed-off-by: me'

will only apply where=end to the second trailer.  Likewise,

--where end --trailer 'signed-off-by: me' --no-where \
--trailer 'acked-by: foo'

will only apply it to the first, reverting to trailer.*.where for the
"acked-by" trailer.

Paolo

v1->v2: support --no-* options, minor code fixes

v2->v3: largely rewritten to respect option order on the command-line;
keep trailer.h namespace clean (Christian)

Paolo Bonzini (3):
  trailers: export action enums and corresponding lookup functions
  trailers: introduce struct new_trailer_item
  interpret-trailers: add options for actions

 Documentation/git-interpret-trailers.txt |  24 +++
 builtin/interpret-trailers.c |  73 ++--
 t/t7513-interpret-trailers.sh|  66 ++
 trailer.c| 113 ++-
 trailer.h|  43 +++-
 5 files changed, 267 insertions(+), 52 deletions(-)

-- 
2.13.3

[PATCH v3 3/3] interpret-trailers: add options for actions

[Paolo Bonzini]

From: Paolo Bonzini 

Allow using non-default values for trailers without having to set
them up in .gitconfig first.  For example, if you have the following
configuration

 trailer.signed-off-by.where = end

you may use "--where before" when a patch author forgets his
Signed-off-by and provides it in a separate email.  Likewise for
--if-exists and --if-missing

Reverting to the behavior specified by .gitconfig is done with
--no-where, --no-if-exists and --no-if-missing.

Signed-off-by: Paolo Bonzini 
---
 Documentation/git-interpret-trailers.txt | 23 +++
 builtin/interpret-trailers.c | 32 
 t/t7513-interpret-trailers.sh| 66 
 trailer.c| 27 ++---
 trailer.h|  7 
 5 files changed, 149 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index 31cdeaecd..76d5fdfaf 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -80,6 +80,29 @@ OPTIONS
trailer to the input messages. See the description of this
command.
 
+--where ::
+--no-where::
+   Specify where all new trailers will be added.  A setting
+   provided with '--where' overrides all configuration variables
+   and applies to all '--trailer' options until the next occurrence of
+   '--where' or '--no-where'.
+
+--if-exists ::
+--no-if-exists::
+   Specify what action will be performed when there is already at
+   least one trailer with the same  in the message.  A setting
+   provided with '--if-exists' overrides all configuration variables
+   and applies to all '--trailer' options until the next occurrence of
+   '--if-exists' or '--no-if-exists'.
+
+--if-missing ::
+--no-if-missing::
+   Specify what action will be performed when there is no other
+   trailer with the same  in the message.  A setting
+   provided with '--if-missing' overrides all configuration variables
+   and applies to all '--trailer' options until the next occurrence of
+   '--if-missing' or '--no-if-missing'.
+
 CONFIGURATION VARIABLES
 ---
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 8f38fa318..83249e3eb 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -16,6 +16,28 @@ static const char * const git_interpret_trailers_usage[] = {
NULL
 };
 
+static enum trailer_where where;
+static enum trailer_if_exists if_exists;
+static enum trailer_if_missing if_missing;
+
+static int option_parse_where(const struct option *opt,
+ const char *arg, int unset)
+{
+   return trailer_set_where(&where, arg);
+}
+
+static int option_parse_if_exists(const struct option *opt,
+ const char *arg, int unset)
+{
+   return trailer_set_if_exists(&if_exists, arg);
+}
+
+static int option_parse_if_missing(const struct option *opt,
+  const char *arg, int unset)
+{
+   return trailer_set_if_missing(&if_missing, arg);
+}
+
 static void new_trailers_clear(struct list_head *trailers)
 {
struct list_head *pos, *tmp;
@@ -44,6 +66,9 @@ static int option_parse_trailer(const struct option *opt,
 
item = xmalloc(sizeof *item);
item->text = arg;
+   item->where = where;
+   item->if_exists = if_exists;
+   item->if_missing = if_missing;
list_add_tail(&item->list, trailers);
return 0;
 }
@@ -58,6 +83,13 @@ int cmd_interpret_trailers(int argc, const char **argv, 
const char *prefix)
OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty 
trailers")),
 
+   OPT_CALLBACK(0, "where", NULL, N_("action"),
+N_("where to place the new trailer"), 
option_parse_where),
+   OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
+N_("action if trailer already exists"), 
option_parse_if_exists),
+   OPT_CALLBACK(0, "if-missing", NULL, N_("action"),
+N_("action if trailer is missing"), 
option_parse_if_missing),
+
OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
N_("trailer(s) to add"), option_parse_trailer),
OPT_END()
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 0c6f91c43..adbdf54f8 100755
--- a/t

[PATCH v3 1/3] trailers: export action enums and corresponding lookup functions

[Paolo Bonzini]

From: Paolo Bonzini 

Separate the mechanical changes out of the next patch.  The functions
are changed to take a pointer to enum, because struct conf_info is not
going to be public.

Set the default values explicitly in default_conf_info, since they are
not anymore close to default_conf_info and it's not obvious which
constant has value 0.  With the next patches, in fact, the values will
not be zero anymore!

Signed-off-by: Paolo Bonzini 
---
 trailer.c | 65 ---
 trailer.h | 22 +
 2 files changed, 55 insertions(+), 32 deletions(-)

diff --git a/trailer.c b/trailer.c
index 751b56c00..f02895373 100644
--- a/trailer.c
+++ b/trailer.c
@@ -10,18 +10,13 @@
  * Copyright (c) 2013, 2014 Christian Couder 
  */
 
-enum action_where { WHERE_END, WHERE_AFTER, WHERE_BEFORE, WHERE_START };
-enum action_if_exists { EXISTS_ADD_IF_DIFFERENT_NEIGHBOR, 
EXISTS_ADD_IF_DIFFERENT,
-   EXISTS_ADD, EXISTS_REPLACE, EXISTS_DO_NOTHING };
-enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };
-
 struct conf_info {
char *name;
char *key;
char *command;
-   enum action_where where;
-   enum action_if_exists if_exists;
-   enum action_if_missing if_missing;
+   enum trailer_where where;
+   enum trailer_if_exists if_exists;
+   enum trailer_if_missing if_missing;
 };
 
 static struct conf_info default_conf_info;
@@ -63,7 +58,7 @@ static const char *git_generated_prefixes[] = {
pos != (head); \
pos = is_reverse ? pos->prev : pos->next)
 
-static int after_or_end(enum action_where where)
+static int after_or_end(enum trailer_where where)
 {
return (where == WHERE_AFTER) || (where == WHERE_END);
 }
@@ -201,7 +196,7 @@ static int check_if_different(struct trailer_item *in_tok,
  int check_all,
  struct list_head *head)
 {
-   enum action_where where = arg_tok->conf.where;
+   enum trailer_where where = arg_tok->conf.where;
struct list_head *next_head;
do {
if (same_trailer(in_tok, arg_tok))
@@ -306,7 +301,7 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
 static void apply_arg_if_missing(struct list_head *head,
 struct arg_item *arg_tok)
 {
-   enum action_where where;
+   enum trailer_where where;
struct trailer_item *to_add;
 
switch (arg_tok->conf.if_missing) {
@@ -331,7 +326,7 @@ static int find_same_and_apply_arg(struct list_head *head,
struct trailer_item *in_tok;
struct trailer_item *on_tok;
 
-   enum action_where where = arg_tok->conf.where;
+   enum trailer_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;
@@ -373,44 +368,44 @@ static void process_trailers_lists(struct list_head *head,
}
 }
 
-static int set_where(struct conf_info *item, const char *value)
+int trailer_set_where(enum trailer_where *item, const char *value)
 {
if (!strcasecmp("after", value))
-   item->where = WHERE_AFTER;
+   *item = WHERE_AFTER;
else if (!strcasecmp("before", value))
-   item->where = WHERE_BEFORE;
+   *item = WHERE_BEFORE;
else if (!strcasecmp("end", value))
-   item->where = WHERE_END;
+   *item = WHERE_END;
else if (!strcasecmp("start", value))
-   item->where = WHERE_START;
+   *item = WHERE_START;
else
return -1;
return 0;
 }
 
-static int set_if_exists(struct conf_info *item, const char *value)
+int trailer_set_if_exists(enum trailer_if_exists *item, const char *value)
 {
if (!strcasecmp("addIfDifferent", value))
-   item->if_exists = EXISTS_ADD_IF_DIFFERENT;
+   *item = EXISTS_ADD_IF_DIFFERENT;
else if (!strcasecmp("addIfDifferentNeighbor", value))
-   item->if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+   *item = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
else if (!strcasecmp("add", value))
-   item->if_exists = EXISTS_ADD;
+   *item = EXISTS_ADD;
else if (!strcasecmp("replace", value))
-   item->if_exists = EXISTS_REPLACE;
+   *item = EXISTS_REPLACE;
else if (!strcasecmp("doNothing", value))
-   item->if_exists = EXISTS_DO_NOTHING;
+   *item = EXISTS_DO_NOTHING;
else
return -1;
return 0;
 }
 
-static int set_if_missing(struct conf_info *item, const char *value)
+int trailer_set_if_missing(enum trailer_if_missing *item, 

[PATCH v3 2/3] trailers: introduce struct new_trailer_item

[Paolo Bonzini]

From: Paolo Bonzini 

This will provide a place to store the current state of the
--where, --if-exists and --if-missing options.

Signed-off-by: Paolo Bonzini 
---
 builtin/interpret-trailers.c | 41 +
 trailer.c| 21 -
 trailer.h| 14 +-
 3 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 175f14797..8f38fa318 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -16,17 +16,50 @@ static const char * const git_interpret_trailers_usage[] = {
NULL
 };
 
+static void new_trailers_clear(struct list_head *trailers)
+{
+   struct list_head *pos, *tmp;
+   struct new_trailer_item *item;
+
+   list_for_each_safe(pos, tmp, trailers) {
+   item = list_entry(pos, struct new_trailer_item, list);
+   list_del(pos);
+   free(item);
+   }
+}
+
+static int option_parse_trailer(const struct option *opt,
+  const char *arg, int unset)
+{
+   struct list_head *trailers = opt->value;
+   struct new_trailer_item *item;
+
+   if (unset) {
+   new_trailers_clear(trailers);
+   return 0;
+   }
+
+   if (!arg)
+   return -1;
+
+   item = xmalloc(sizeof *item);
+   item->text = arg;
+   list_add_tail(&item->list, trailers);
+   return 0;
+}
+
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
int in_place = 0;
int trim_empty = 0;
-   struct string_list trailers = STRING_LIST_INIT_NODUP;
+   LIST_HEAD(trailers);
 
struct option options[] = {
OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty 
trailers")),
-   OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
-   N_("trailer(s) to add")),
+
+   OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
+   N_("trailer(s) to add"), option_parse_trailer),
OPT_END()
};
 
@@ -43,7 +76,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const 
char *prefix)
process_trailers(NULL, in_place, trim_empty, &trailers);
}
 
-   string_list_clear(&trailers, 0);
+   new_trailers_clear(&trailers);
 
return 0;
 }
diff --git a/trailer.c b/trailer.c
index f02895373..aed5fa1f8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -669,14 +669,15 @@ static void add_arg_item(struct list_head *arg_head, char 
*tok, char *val,
 }
 
 static void process_command_line_args(struct list_head *arg_head,
- struct string_list *trailers)
+ struct list_head *new_trailer_head)
 {
-   struct string_list_item *tr;
+   struct new_trailer_item *tr;
struct arg_item *item;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
const struct conf_info *conf;
struct list_head *pos;
+   const char *string;
 
/*
 * In command-line arguments, '=' is accepted (in addition to the
@@ -695,18 +696,19 @@ 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) {
-   int separator_pos = find_separator(tr->string, cl_separators);
+   list_for_each(pos, new_trailer_head) {
+   tr = list_entry(pos, struct new_trailer_item, list);
+   string = tr->text;
+   int separator_pos = find_separator(string, cl_separators);
if (separator_pos == 0) {
struct strbuf sb = STRBUF_INIT;
-   strbuf_addstr(&sb, tr->string);
+   strbuf_addstr(&sb, 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);
+   parse_trailer(&tok, &val, &conf, string, separator_pos);
add_arg_item(arg_head,
 strbuf_detach(&tok, NULL),
 strbuf_detach(&val, NULL),
@@ -969,7 +971,8 @@ static FILE *create_in_place_tempfile(const char *file)
r

[PATCH v4 0/4] interpret-trailers: add --where, --if-exists, --if-missing

[Paolo Bonzini]

From: Paolo Bonzini 

These options are useful to experiment with "git interpret-trailers"
without having to tinker with .gitconfig (Junio said git should ahve
done this first and only added configuration afterwards).  It can
be useful in the case where you want a different placement for the trailer,
or for scripts/aliases that don't want to rely on specific .gitconfig
settings.

Compared to v2, the main change is that option order on the command-line
is respected.  That is,

--trailer 'acked-by: foo' --where end --trailer 'signed-off-by: me'

will only apply where=end to the second trailer.  Likewise,

--where end --trailer 'signed-off-by: me' --no-where \
--trailer 'acked-by: foo'

will only apply it to the first, reverting to trailer.*.where for the
"acked-by" trailer.

Paolo

v1->v2: support --no-* options, minor code fixes

v2->v3: largely rewritten to respect option order on the command-line;
keep trailer.h namespace clean (Christian)

v3->v4: fix compilation warnings (Junio), added documentation fix

Paolo Bonzini (4):
  trailers: export action enums and corresponding lookup functions
  trailers: introduce struct new_trailer_item
  interpret-trailers: add options for actions
  interpret-trailers: fix documentation typo

 Documentation/git-interpret-trailers.txt |  27 ++-
 builtin/interpret-trailers.c |  73 +--
 t/t7513-interpret-trailers.sh|  66 +
 trailer.c| 118 +++
 trailer.h|  43 ++-
 5 files changed, 274 insertions(+), 53 deletions(-)

-- 
2.13.3

[PATCH v4 2/4] trailers: introduce struct new_trailer_item

[Paolo Bonzini]

From: Paolo Bonzini 

This will provide a place to store the current state of the
--where, --if-exists and --if-missing options.

Signed-off-by: Paolo Bonzini 
---
 builtin/interpret-trailers.c | 41 +
 trailer.c| 19 +++
 trailer.h| 14 +-
 3 files changed, 61 insertions(+), 13 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 175f14797..8f38fa318 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -16,17 +16,50 @@ static const char * const git_interpret_trailers_usage[] = {
NULL
 };
 
+static void new_trailers_clear(struct list_head *trailers)
+{
+   struct list_head *pos, *tmp;
+   struct new_trailer_item *item;
+
+   list_for_each_safe(pos, tmp, trailers) {
+   item = list_entry(pos, struct new_trailer_item, list);
+   list_del(pos);
+   free(item);
+   }
+}
+
+static int option_parse_trailer(const struct option *opt,
+  const char *arg, int unset)
+{
+   struct list_head *trailers = opt->value;
+   struct new_trailer_item *item;
+
+   if (unset) {
+   new_trailers_clear(trailers);
+   return 0;
+   }
+
+   if (!arg)
+   return -1;
+
+   item = xmalloc(sizeof *item);
+   item->text = arg;
+   list_add_tail(&item->list, trailers);
+   return 0;
+}
+
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
int in_place = 0;
int trim_empty = 0;
-   struct string_list trailers = STRING_LIST_INIT_NODUP;
+   LIST_HEAD(trailers);
 
struct option options[] = {
OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty 
trailers")),
-   OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
-   N_("trailer(s) to add")),
+
+   OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
+   N_("trailer(s) to add"), option_parse_trailer),
OPT_END()
};
 
@@ -43,7 +76,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const 
char *prefix)
process_trailers(NULL, in_place, trim_empty, &trailers);
}
 
-   string_list_clear(&trailers, 0);
+   new_trailers_clear(&trailers);
 
return 0;
 }
diff --git a/trailer.c b/trailer.c
index f02895373..6941da799 100644
--- a/trailer.c
+++ b/trailer.c
@@ -669,9 +669,8 @@ static void add_arg_item(struct list_head *arg_head, char 
*tok, char *val,
 }
 
 static void process_command_line_args(struct list_head *arg_head,
- struct string_list *trailers)
+ struct list_head *new_trailer_head)
 {
-   struct string_list_item *tr;
struct arg_item *item;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
@@ -695,17 +694,20 @@ 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) {
-   int separator_pos = find_separator(tr->string, cl_separators);
+   list_for_each(pos, new_trailer_head) {
+   struct new_trailer_item *tr =
+   list_entry(pos, struct new_trailer_item, list);
+   int separator_pos = find_separator(tr->text, cl_separators);
+
if (separator_pos == 0) {
struct strbuf sb = STRBUF_INIT;
-   strbuf_addstr(&sb, tr->string);
+   strbuf_addstr(&sb, tr->text);
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,
+   parse_trailer(&tok, &val, &conf, tr->text,
  separator_pos);
add_arg_item(arg_head,
 strbuf_detach(&tok, NULL),
@@ -969,7 +971,8 @@ static FILE *create_in_place_tempfile(const char *file)
return outfile;
 }
 
-void process_trailers(const char *file, int in_place, int trim_empty, struct 
string_list *trailers)
+void process_trailers(const char *file, int in_place, int trim_empty,
+ struct list_head *new_trailer_head)
 {
LIST_HEAD(head);
LIST_H

[PATCH v4 4/4] interpret-trailers: fix documentation typo

[Paolo Bonzini]

From: Paolo Bonzini 

Self-explanatory... trailer.ifexists is documented with the
right name, but after a while it switches to ifexist.

Signed-off-by: Paolo Bonzini 
---
 Documentation/git-interpret-trailers.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index 76d5fdfaf..0ef93204f 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -193,8 +193,8 @@ trailer..where::
configuration variable and it overrides what is specified by
that option for trailers with the specified .
 
-trailer..ifexist::
-   This option takes the same values as the 'trailer.ifexist'
+trailer..ifexists::
+   This option takes the same values as the 'trailer.ifexists'
configuration variable and it overrides what is specified by
that option for trailers with the specified .
 
-- 
2.13.3

[PATCH v4 3/4] interpret-trailers: add options for actions

[Paolo Bonzini]

From: Paolo Bonzini 

Allow using non-default values for trailers without having to set
them up in .gitconfig first.  For example, if you have the following
configuration

 trailer.signed-off-by.where = end

you may use "--where before" when a patch author forgets his
Signed-off-by and provides it in a separate email.  Likewise for
--if-exists and --if-missing

Reverting to the behavior specified by .gitconfig is done with
--no-where, --no-if-exists and --no-if-missing.

Signed-off-by: Paolo Bonzini 
---
 Documentation/git-interpret-trailers.txt | 23 +++
 builtin/interpret-trailers.c | 32 
 t/t7513-interpret-trailers.sh| 66 
 trailer.c| 34 +---
 trailer.h|  7 
 5 files changed, 156 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index 31cdeaecd..76d5fdfaf 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -80,6 +80,29 @@ OPTIONS
trailer to the input messages. See the description of this
command.
 
+--where ::
+--no-where::
+   Specify where all new trailers will be added.  A setting
+   provided with '--where' overrides all configuration variables
+   and applies to all '--trailer' options until the next occurrence of
+   '--where' or '--no-where'.
+
+--if-exists ::
+--no-if-exists::
+   Specify what action will be performed when there is already at
+   least one trailer with the same  in the message.  A setting
+   provided with '--if-exists' overrides all configuration variables
+   and applies to all '--trailer' options until the next occurrence of
+   '--if-exists' or '--no-if-exists'.
+
+--if-missing ::
+--no-if-missing::
+   Specify what action will be performed when there is no other
+   trailer with the same  in the message.  A setting
+   provided with '--if-missing' overrides all configuration variables
+   and applies to all '--trailer' options until the next occurrence of
+   '--if-missing' or '--no-if-missing'.
+
 CONFIGURATION VARIABLES
 ---
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 8f38fa318..83249e3eb 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -16,6 +16,28 @@ static const char * const git_interpret_trailers_usage[] = {
NULL
 };
 
+static enum trailer_where where;
+static enum trailer_if_exists if_exists;
+static enum trailer_if_missing if_missing;
+
+static int option_parse_where(const struct option *opt,
+ const char *arg, int unset)
+{
+   return trailer_set_where(&where, arg);
+}
+
+static int option_parse_if_exists(const struct option *opt,
+ const char *arg, int unset)
+{
+   return trailer_set_if_exists(&if_exists, arg);
+}
+
+static int option_parse_if_missing(const struct option *opt,
+  const char *arg, int unset)
+{
+   return trailer_set_if_missing(&if_missing, arg);
+}
+
 static void new_trailers_clear(struct list_head *trailers)
 {
struct list_head *pos, *tmp;
@@ -44,6 +66,9 @@ static int option_parse_trailer(const struct option *opt,
 
item = xmalloc(sizeof *item);
item->text = arg;
+   item->where = where;
+   item->if_exists = if_exists;
+   item->if_missing = if_missing;
list_add_tail(&item->list, trailers);
return 0;
 }
@@ -58,6 +83,13 @@ int cmd_interpret_trailers(int argc, const char **argv, 
const char *prefix)
OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty 
trailers")),
 
+   OPT_CALLBACK(0, "where", NULL, N_("action"),
+N_("where to place the new trailer"), 
option_parse_where),
+   OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
+N_("action if trailer already exists"), 
option_parse_if_exists),
+   OPT_CALLBACK(0, "if-missing", NULL, N_("action"),
+N_("action if trailer is missing"), 
option_parse_if_missing),
+
OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
N_("trailer(s) to add"), option_parse_trailer),
OPT_END()
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 0c6f91c43..adbdf54f8 100755
--- a/t

[PATCH v4 1/4] trailers: export action enums and corresponding lookup functions

[Paolo Bonzini]

From: Paolo Bonzini 

Separate the mechanical changes out of the next patch.  The functions
are changed to take a pointer to enum, because struct conf_info is not
going to be public.

Set the default values explicitly in default_conf_info, since they are
not anymore close to default_conf_info and it's not obvious which
constant has value 0.  With the next patches, in fact, the values will
not be zero anymore!

Signed-off-by: Paolo Bonzini 
---
 trailer.c | 65 ---
 trailer.h | 22 +
 2 files changed, 55 insertions(+), 32 deletions(-)

diff --git a/trailer.c b/trailer.c
index 751b56c00..f02895373 100644
--- a/trailer.c
+++ b/trailer.c
@@ -10,18 +10,13 @@
  * Copyright (c) 2013, 2014 Christian Couder 
  */
 
-enum action_where { WHERE_END, WHERE_AFTER, WHERE_BEFORE, WHERE_START };
-enum action_if_exists { EXISTS_ADD_IF_DIFFERENT_NEIGHBOR, 
EXISTS_ADD_IF_DIFFERENT,
-   EXISTS_ADD, EXISTS_REPLACE, EXISTS_DO_NOTHING };
-enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };
-
 struct conf_info {
char *name;
char *key;
char *command;
-   enum action_where where;
-   enum action_if_exists if_exists;
-   enum action_if_missing if_missing;
+   enum trailer_where where;
+   enum trailer_if_exists if_exists;
+   enum trailer_if_missing if_missing;
 };
 
 static struct conf_info default_conf_info;
@@ -63,7 +58,7 @@ static const char *git_generated_prefixes[] = {
pos != (head); \
pos = is_reverse ? pos->prev : pos->next)
 
-static int after_or_end(enum action_where where)
+static int after_or_end(enum trailer_where where)
 {
return (where == WHERE_AFTER) || (where == WHERE_END);
 }
@@ -201,7 +196,7 @@ static int check_if_different(struct trailer_item *in_tok,
  int check_all,
  struct list_head *head)
 {
-   enum action_where where = arg_tok->conf.where;
+   enum trailer_where where = arg_tok->conf.where;
struct list_head *next_head;
do {
if (same_trailer(in_tok, arg_tok))
@@ -306,7 +301,7 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
 static void apply_arg_if_missing(struct list_head *head,
 struct arg_item *arg_tok)
 {
-   enum action_where where;
+   enum trailer_where where;
struct trailer_item *to_add;
 
switch (arg_tok->conf.if_missing) {
@@ -331,7 +326,7 @@ static int find_same_and_apply_arg(struct list_head *head,
struct trailer_item *in_tok;
struct trailer_item *on_tok;
 
-   enum action_where where = arg_tok->conf.where;
+   enum trailer_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;
@@ -373,44 +368,44 @@ static void process_trailers_lists(struct list_head *head,
}
 }
 
-static int set_where(struct conf_info *item, const char *value)
+int trailer_set_where(enum trailer_where *item, const char *value)
 {
if (!strcasecmp("after", value))
-   item->where = WHERE_AFTER;
+   *item = WHERE_AFTER;
else if (!strcasecmp("before", value))
-   item->where = WHERE_BEFORE;
+   *item = WHERE_BEFORE;
else if (!strcasecmp("end", value))
-   item->where = WHERE_END;
+   *item = WHERE_END;
else if (!strcasecmp("start", value))
-   item->where = WHERE_START;
+   *item = WHERE_START;
else
return -1;
return 0;
 }
 
-static int set_if_exists(struct conf_info *item, const char *value)
+int trailer_set_if_exists(enum trailer_if_exists *item, const char *value)
 {
if (!strcasecmp("addIfDifferent", value))
-   item->if_exists = EXISTS_ADD_IF_DIFFERENT;
+   *item = EXISTS_ADD_IF_DIFFERENT;
else if (!strcasecmp("addIfDifferentNeighbor", value))
-   item->if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+   *item = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
else if (!strcasecmp("add", value))
-   item->if_exists = EXISTS_ADD;
+   *item = EXISTS_ADD;
else if (!strcasecmp("replace", value))
-   item->if_exists = EXISTS_REPLACE;
+   *item = EXISTS_REPLACE;
else if (!strcasecmp("doNothing", value))
-   item->if_exists = EXISTS_DO_NOTHING;
+   *item = EXISTS_DO_NOTHING;
else
return -1;
return 0;
 }
 
-static int set_if_missing(struct conf_info *item, const char *value)
+int trailer_set_if_missing(enum trailer_if_missing *item, 

Re: [PATCH] send-email: new option to walkaround email server limits

[Paolo Bonzini]

On 29/04/2017 14:26, xiaoqiang zhao wrote:
> Some email server(e.g. smtp.163.com) limits a fixed number emails to be send 
> per
> session(connection) and this will lead to a send faliure.
> With --split  option, a auto reconnection will occur when number of 
> sended
> email reaches  and the problem is solved.
> 
> Signed-off-by: xiaoqiang zhao 

I think you should also add a matching configuration option, or you are
going to forget it on the command line sooner or later!

Paolo

> ---
>  git-send-email.perl | 22 +-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index eea0a517f..0de9b7058 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -81,6 +81,8 @@ git send-email --dump-aliases
>   This setting forces to use one of the 
> listed mechanisms.
>  --smtp-debug<0|1>  * Disable, enable Net::SMTP debug.
>  
> +--split   * send \$num message per connection.
> +
>Automating:
>  --identity* Use the sendemail. options.
>  --to-cmd  * Email To: via ` \$patch_path`
> @@ -153,6 +155,7 @@ my $have_email_valid = eval { require Email::Valid; 1 };
>  my $have_mail_address = eval { require Mail::Address; 1 };
>  my $smtp;
>  my $auth;
> +my $send_count = 0;
>  
>  # Regexes for RFC 2047 productions.
>  my $re_token = qr/[^][()<>@,;:\\"\/?.= \000-\037\177-\377]+/;
> @@ -186,6 +189,7 @@ my $format_patch;
>  my $compose_filename;
>  my $force = 0;
>  my $dump_aliases = 0;
> +my $split = 0;
>  
>  # Handle interactive edition of files.
>  my $multiedit;
> @@ -358,6 +362,7 @@ $rc = GetOptions(
>   "force" => \$force,
>   "xmailer!" => \$use_xmailer,
>   "no-xmailer" => sub {$use_xmailer = 0},
> + "split=i" => \$split,
>);
>  
>  usage() if $help;
> @@ -1158,10 +1163,15 @@ sub smtp_host_string {
>  # (smtp_user was not specified), and 0 otherwise.
>  
>  sub smtp_auth_maybe {
> - if (!defined $smtp_authuser || $auth) {
> + if (!defined $smtp_authuser || $send_count != 0) {
>   return 1;
>   }
>  
> + if ($auth && $send_count == 0) {
> + print "Auth use saved password. \n";
> + return !!$smtp->auth($smtp_authuser, $smtp_authpass);
> + }
> +
>   # Workaround AUTH PLAIN/LOGIN interaction defect
>   # with Authen::SASL::Cyrus
>   eval {
> @@ -1187,6 +1197,7 @@ sub smtp_auth_maybe {
>   'password' => $smtp_authpass
>   }, sub {
>   my $cred = shift;
> + $smtp_authpass = $cred->{'password'};
>  
>   if ($smtp_auth) {
>   my $sasl = Authen::SASL->new(
> @@ -1442,6 +1453,15 @@ EOF
>   }
>   }
>  
> + $send_count++;
> + if ($send_count == $split) {
> + $smtp->quit;
> + $smtp = undef;
> + $send_count = 0;
> + print "Reconnect SMTP server required. \n";
> +
> + }
> +
>   return 1;
>  }
>  
> 

Re: [PATCH v4 0/4] interpret-trailers: add --where, --if-exists, --if-missing

[Paolo Bonzini]

On 01/08/2017 11:03, Paolo Bonzini wrote:
> From: Paolo Bonzini 
> 
> These options are useful to experiment with "git interpret-trailers"
> without having to tinker with .gitconfig (Junio said git should ahve
> done this first and only added configuration afterwards).  It can
> be useful in the case where you want a different placement for the trailer,
> or for scripts/aliases that don't want to rely on specific .gitconfig
> settings.
> 
> Compared to v2, the main change is that option order on the command-line
> is respected.  That is,
> 
>   --trailer 'acked-by: foo' --where end --trailer 'signed-off-by: me'
> 
> will only apply where=end to the second trailer.  Likewise,
> 
>   --where end --trailer 'signed-off-by: me' --no-where \
>   --trailer 'acked-by: foo'
> 
> will only apply it to the first, reverting to trailer.*.where for the
> "acked-by" trailer.

Junio, I see you haven't yet applied this v4 to origin/pu, did you miss it?

Thanks,

Paolo

> Paolo
> 
> v1->v2: support --no-* options, minor code fixes
> 
> v2->v3: largely rewritten to respect option order on the command-line;
>   keep trailer.h namespace clean (Christian)
> 
> v3->v4: fix compilation warnings (Junio), added documentation fix
> 
> Paolo Bonzini (4):
>   trailers: export action enums and corresponding lookup functions
>   trailers: introduce struct new_trailer_item
>   interpret-trailers: add options for actions
>   interpret-trailers: fix documentation typo
> 
>  Documentation/git-interpret-trailers.txt |  27 ++-
>  builtin/interpret-trailers.c |  73 +--
>  t/t7513-interpret-trailers.sh|  66 +
>  trailer.c| 118 
> +++
>  trailer.h|  43 ++-
>  5 files changed, 274 insertions(+), 53 deletions(-)
> 

Re: [Bug/Solution] Git hangs in compat/poll/poll.c on HPE NonStop

[Paolo Bonzini]

On 29/09/2017 00:47, Junio C Hamano wrote:
> [jch: a patch that hopefully is applicable is attached at the end;
>  I'd appreciate input from Paolo, the contributor of the original
>  upstream]

I wasn't involved in upstream commit d42461c3, but Linux is also always
overwriting the revents output with zero.

Reviewed-by: Paolo Bonzini 

Paolo

> "Randall S. Becker"  writes:
> 
>> After a whole lot of investigating, we (it is a large "we") have discovered
>> the reason for the hang we occasionally get in git-upload-pack on HPE
>> NonStop servers - reported here well over a year ago. This resulted from a
>> subtle check that the operating system does on file descriptors. When it
>> sees random values in pfd[i].revents,...
> 
> What do you mean by "random values"?  Where do these random values
> come from?  The original code the patch touches is _not_ setting
> anything, so it is leaving it uninitialized and that is seen by the
> system?  If that is the case perhaps should we clear it before we
> call into this codepath?
> 
>>  There is a non-destructive fix
>> that I would like to propose for this that I have already tested.
> 
> I am not sure in what sense this is "non-destructive"; we left the
> value as it was when we didn't notice anything happening in
> compute_revents().  Now we explicitly destroy the value there was
> (just like we do in the case where the corresponding file descriptor
> was negative).  Maybe overwriting with 0 is the right thing, but I
> wouldn't call it "non-destructive", though.  Puzzling...
> 
>> --- a/compat/poll/poll.c
>> +++ b/compat/poll/poll.c
>> @@ -438,6 +438,10 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
>> pfd[i].revents = happened;
>> rc++;
>>   }
>> +else
>> +  {
>> +pfd[i].revents = 0;
>> +  }
>>}
>>
>>return rc;
> 
> FYI, the upstream gnulib rewrites this part of the code with
> d42461c3 ("poll: fixes for large fds", 2015-02-20) quite a bit, and
> it reads like this:
> 
> +{
> +  pfd[i].revents = (pfd[i].fd < 0
> +? 0
> +: compute_revents (pfd[i].fd, pfd[i].events,
> +   &rfds, &wfds, &efds));
> +  rc += pfd[i].revents != 0;
> +}
> 
> The code before your fix (and before d42461c3) used to assign to
> pfd[i].revents only when compute_revents() gave us non-zero value,
> but with d42461c3 in the upstream, it pfd[i].revents is assigned
> the return value from compute_revents() even if the value returned
> is 0.  And rc is incremented only when that value is non-zero.
> 
> The result of applying your patch is equivalent, so after all, I
> tend to think that your patch is the right fix in the context of the
> code we have (i.e. the older code we borrowed from them).
> 
> I wonder if we want to refresh the borrowed copy of poll.c to a
> newer snapshot someday, but that is totally a separate topic.  At
> least, I now know that your fix in this patch will not be broken due
> to d42461c3 updating the code in a slightly different way ;-)
> 
> Randall, I forged your Sign-off in the patch below, but please say
> it is OK, after reading Documentation/SubmittingPatches.
> 
> Thanks.
> 
> -- >8 --
> From: Randall S. Becker 
> Subject: poll.c: always set revents, even if to zero.
> 
> Match what happens to pfd[i].revents when compute_revents() returns
> 0 to the upstream gnulib's commit d42461c3 ("poll: fixes for large
> fds", 2015-02-20).  The revents field is set to 0, without
> incrementing the value rc to be returned from the function.
> 
> This fixes occasional hangs in git-upload-pack on NPE NonStop.
> 
> Signed-off-by: Randall S. Becker 
> Signed-off-by: Junio C Hamano 
> ---
>  compat/poll/poll.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/compat/poll/poll.c b/compat/poll/poll.c
> index b10adc780f..ae03b74a6f 100644
> --- a/compat/poll/poll.c
> +++ b/compat/poll/poll.c
> @@ -438,6 +438,10 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
>   pfd[i].revents = happened;
>   rc++;
> }
> + else
> +   {
> + pfd[i].revents = 0;
> +   }
>}
>  
>return rc;
> 

[RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

The purpose of this action is for scripts to be able to keep the
user's Signed-off-by at the end.  For example say I have a script
that adds a Reviewed-by tag:

#! /bin/sh
them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
trailer="Reviewed-by: $them"
git log -1 --pretty=format:%B | \
  git interpret-trailers --where end --if-exists doNothing --trailer 
"$trailer" | \
  git commit --amend -F-

Now, this script will leave my Signed-off-by line in a non-canonical
place, like

   Signed-off-by: Paolo Bonzini 
   Reviewed-by: Junio C Hamano 

This new option enables the following improvement:

#! /bin/sh
me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')
them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
trailer="Reviewed-by: $them"
sob="Signed-off-by: $me"
git log -1 --pretty=format:%B | \
  git interpret-trailers --where end --if-exists doNothing --trailer 
"$trailer" \
 --where end --if-exists move --if-missing 
doNothing --trailer "$sob" | \
  git commit --amend -F-

which lets me keep the SoB line at the end, as it should be.
Posting as RFC because it's possible that I'm missing a simpler
way to achieve this...

Paolo Bonzini (4):
  trailer: push free_arg_item up
  trailer: simplify check_if_different
  trailer: create a new function to handle adding trailers
  trailer: add "move" configuration for trailer.ifExists

 Documentation/git-interpret-trailers.txt |  13 ++-
 t/t7513-interpret-trailers.sh|  37 +++
 trailer.c| 169 ++-
 trailer.h|   1 +
 4 files changed, 149 insertions(+), 71 deletions(-)

-- 
2.14.2

[PATCH 1/4] trailer: push free_arg_item up

[Paolo Bonzini]

All callees of process_trailers_lists are calling free_arg_item.
Just do it in process_trailers_lists itself.

Signed-off-by: Paolo Bonzini 
---
 trailer.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/trailer.c b/trailer.c
index 3ba157ed0..4ba28ae33 100644
--- a/trailer.c
+++ b/trailer.c
@@ -178,7 +178,6 @@ static struct trailer_item *trailer_from_arg(struct 
arg_item *arg_tok)
new->token = arg_tok->token;
new->value = arg_tok->value;
arg_tok->token = arg_tok->value = NULL;
-   free_arg_item(arg_tok);
return new;
 }
 
@@ -271,7 +270,6 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
 {
switch (arg_tok->conf.if_exists) {
case EXISTS_DO_NOTHING:
-   free_arg_item(arg_tok);
break;
case EXISTS_REPLACE:
apply_item_command(in_tok, arg_tok);
@@ -287,15 +285,11 @@ static void apply_arg_if_exists(struct trailer_item 
*in_tok,
apply_item_command(in_tok, arg_tok);
if (check_if_different(in_tok, arg_tok, 1, head))
add_arg_to_input_list(on_tok, arg_tok);
-   else
-   free_arg_item(arg_tok);
break;
case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
apply_item_command(in_tok, arg_tok);
if (check_if_different(on_tok, arg_tok, 0, head))
add_arg_to_input_list(on_tok, arg_tok);
-   else
-   free_arg_item(arg_tok);
break;
default:
die("BUG: trailer.c: unhandled value %d",
@@ -311,7 +305,6 @@ static void apply_arg_if_missing(struct list_head *head,
 
switch (arg_tok->conf.if_missing) {
case MISSING_DO_NOTHING:
-   free_arg_item(arg_tok);
break;
case MISSING_ADD:
where = arg_tok->conf.where;
@@ -374,6 +367,8 @@ static void process_trailers_lists(struct list_head *head,
 
if (!applied)
apply_arg_if_missing(head, arg_tok);
+
+   free_arg_item(arg_tok);
}
 }
 
-- 
2.14.2

[PATCH 3/4] trailer: create a new function to handle adding trailers

[Paolo Bonzini]

Create a new function apply_arg that takes care of computing the new
trailer's "neighbor", checking for duplicates through a pluggable
callback, and adding the new argument according to "trailer.where".

Rename after_or_end, and don't use it in apply_arg.  It's a coincidence
that the conditions for "scan backwards" and "add after" are the same.

This simplifies find_same_and_apply_arg so that it does exactly what
the name says.  apply_arg_if_missing can also use the new function;
before, it was redoing add_arg_to_input_list's job in a slightly
different fashion.

Signed-off-by: Paolo Bonzini 
---
 trailer.c | 125 +-
 1 file changed, 75 insertions(+), 50 deletions(-)

diff --git a/trailer.c b/trailer.c
index 91f89db7f..ce0d94074 100644
--- a/trailer.c
+++ b/trailer.c
@@ -58,7 +58,7 @@ static const char *git_generated_prefixes[] = {
pos != (head); \
pos = is_reverse ? pos->prev : pos->next)
 
-static int after_or_end(enum trailer_where where)
+static int scan_backwards(enum trailer_where where)
 {
return (where == WHERE_AFTER) || (where == WHERE_END);
 }
@@ -181,18 +181,8 @@ static struct trailer_item *trailer_from_arg(struct 
arg_item *arg_tok)
return new;
 }
 
-static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct arg_item *arg_tok)
-{
-   int aoe = after_or_end(arg_tok->conf.where);
-   struct trailer_item *to_add = trailer_from_arg(arg_tok);
-   if (aoe)
-   list_add(&to_add->list, &on_tok->list);
-   else
-   list_add_tail(&to_add->list, &on_tok->list);
-}
-
 static int check_if_different(struct trailer_item *in_tok,
+ struct trailer_item *neighbor,
  struct arg_item *arg_tok,
  struct list_head *head)
 {
@@ -203,8 +193,8 @@ static int check_if_different(struct trailer_item *in_tok,
 * if we want to add a trailer after another one,
 * we have to check those before this one
 */
-   next_head = after_or_end(where) ? in_tok->list.prev
-   : in_tok->list.next;
+   next_head = scan_backwards(where) ? in_tok->list.prev
+ : in_tok->list.next;
if (next_head == head)
return 1;
in_tok = list_entry(next_head, struct trailer_item, list);
@@ -212,6 +202,14 @@ static int check_if_different(struct trailer_item *in_tok,
return 0;
 }
 
+static int check_if_different_neighbor(struct trailer_item *in_tok,
+  struct trailer_item *neighbor,
+  struct arg_item *arg_tok,
+  struct list_head *head)
+{
+   return !same_trailer(neighbor, arg_tok);
+}
+
 static char *apply_command(const char *command, const char *arg)
 {
struct strbuf cmd = STRBUF_INIT;
@@ -260,33 +258,80 @@ static void apply_item_command(struct trailer_item 
*in_tok, struct arg_item *arg
}
 }
 
+static int apply_arg(struct trailer_item *in_tok,
+struct arg_item *arg_tok,
+struct list_head *head,
+int (*check)(struct trailer_item *in_tok,
+ struct trailer_item *neighbor,
+ struct arg_item *arg_tok,
+ struct list_head *head),
+int replace)
+{
+   struct trailer_item *to_add, *neighbor;
+   struct list_head *place;
+   int add_after;
+
+   enum trailer_where where = arg_tok->conf.where;
+   int middle = (where == WHERE_AFTER) || (where == WHERE_BEFORE);
+
+   /*
+* No other trailer to apply arg_tok one before/after.  Put it
+* before/after _all_ other trailers.
+*/
+   if (!in_tok && middle) {
+   where = (where == WHERE_AFTER) ? WHERE_END : WHERE_START;
+   middle = 0;
+   }
+
+   if (list_empty(head)) {
+   add_after = 1;
+   place = head;
+   neighbor = NULL;
+   } else if (middle) {
+   add_after = (where == WHERE_AFTER);
+   place = &in_tok->list;
+   neighbor = in_tok;
+   } else {
+   add_after = (where == WHERE_END);
+   place = (where == WHERE_END) ? head->prev : head->next;
+   neighbor = list_entry(place, struct trailer_item, list);
+   }
+
+   apply_item_command(in_tok, arg_tok);
+   if (check && !check(in_tok, neighbor, arg_tok, head))
+   return 0;
+
+   to_add = trailer_from_arg(arg

[PATCH 4/4] trailer: add "move" configuration for trailer.ifExists

[Paolo Bonzini]

In some cases, people apply patches to a queue branch immediately
with "git am -3 -s", and later collect Reviewed-by or Acked-by
trailers as they come in from the mailing list.

In this case, "where=after" does not have the desired behavior,
because it will add the trailer in an unorthodox position, after the
committer's Signed-off-by line.  The "move" configuration is intended
to be applied in such a case to the Signed-off-by header, like

git interpret-trailers \
--where end --if-missing doNothing --if-exists move
"Signed-off-by: A U Thor "

or perhaps with an automated configuration

[trailer "move-sob"]
command = "'echo \"$(git config user.name) <$(git config user.email)>\"'"
key = Signed-off-by
where = end
ifMissing = doNothing
ifExists = move

Though this of course makes the most sense if the last Signed-off-by is
from the committer itself, and thus is not necessarily a good idea for
everyone.

Signed-off-by: Paolo Bonzini 
---
 Documentation/git-interpret-trailers.txt | 13 ---
 t/t7513-interpret-trailers.sh| 37 
 trailer.c| 26 +-
 trailer.h|  1 +
 4 files changed, 69 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index 9dd19a1dd..1cdde492c 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -165,7 +165,7 @@ trailer.ifexists::
same  in the message.
 +
 The valid values for this option are: `addIfDifferentNeighbor` (this
-is the default), `addIfDifferent`, `add`, `replace` or `doNothing`.
+is the default), `addIfDifferent`, `add`, `replace`, `move` or `doNothing`.
 +
 With `addIfDifferentNeighbor`, a new trailer will be added only if no
 trailer with the same (, ) pair is above or below the line
@@ -182,13 +182,20 @@ deleted and the new trailer will be added. The deleted 
trailer will be
 the closest one (with the same ) to the place where the new one
 will be added.
 +
+With `move`, an existing trailer with the same (, ) will be
+deleted and the new trailer will be added.  If more equal pairs exists,
+the deleted trailer will be the closest one to the place where the new one
+will be added.
++
 With `doNothing`, nothing will be done; that is no new trailer will be
 added if there is already one with the same  in the message.
 
 trailer.ifmissing::
This option makes it possible to choose what action will be
-   performed when there is not yet any trailer with the same
-in the message.
+   performed when the `ifexists` action fails.  This means that
+   there is not yet any trailer with the same  in the message
+   or, for the `move` action only, there is no identical trailer
+   in the message.
 +
 The valid values for this option are: `add` (this is the default) and
 `doNothing`.
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 164719d1c..a0f21fefd 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1052,6 +1052,43 @@ test_expect_success 'using "ifExists = doNothing"' '
test_cmp expected actual
 '
 
+test_expect_success 'require token and value match when "ifExists = move"' '
+   git config trailer.fix.ifExists "move" &&
+   git config trailer.fix.where "start" &&
+   cat complex_message_body >expected &&
+   sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+   Fixes: 22
+   Fixes: Z
+   Acked-by= Z
+   Reviewed-by:
+   Signed-off-by: Z
+   Fixes: 53
+   EOF
+   (cat complex_message; echo Fixes: 53) | \
+   git interpret-trailers \
+   --trailer "fix=22" \
+   >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'look for all trailers when "ifExists = move"' '
+   git config trailer.fix.ifMissing "doNothing" &&
+   cat complex_message_body >expected &&
+   sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+   Fixes: 22
+   Fixes: Z
+   Acked-by= Z
+   Reviewed-by:
+   Signed-off-by: Z
+   Fixes: 53
+   EOF
+   (cat complex_message; echo Fixes: 22; echo Fixes: 53) | \
+   git interpret-trailers \
+   --trailer "fix=22" \
+   >actual &&
+   test_cmp expected actual
+'
+
 test_expect_success 'the default is "ifMissing = add"' '
git config tr

[PATCH 2/4] trailer: simplify check_if_different

[Paolo Bonzini]

The check_all argument is pointless, because the function degenerates
to !same_trailer when check_all==0 (if same_trailer fails, it always
ends up returning 1).  Remove it, switching the check_all==0 caller
to use same_trailer directly.

Signed-off-by: Paolo Bonzini 
---
 trailer.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/trailer.c b/trailer.c
index 4ba28ae33..91f89db7f 100644
--- a/trailer.c
+++ b/trailer.c
@@ -194,14 +194,11 @@ static void add_arg_to_input_list(struct trailer_item 
*on_tok,
 
 static int check_if_different(struct trailer_item *in_tok,
  struct arg_item *arg_tok,
- int check_all,
  struct list_head *head)
 {
enum trailer_where where = arg_tok->conf.where;
struct list_head *next_head;
-   do {
-   if (same_trailer(in_tok, arg_tok))
-   return 0;
+   while (!same_trailer(in_tok, arg_tok)) {
/*
 * if we want to add a trailer after another one,
 * we have to check those before this one
@@ -209,10 +206,10 @@ static int check_if_different(struct trailer_item *in_tok,
next_head = after_or_end(where) ? in_tok->list.prev
: in_tok->list.next;
if (next_head == head)
-   break;
+   return 1;
in_tok = list_entry(next_head, struct trailer_item, list);
-   } while (check_all);
-   return 1;
+   }
+   return 0;
 }
 
 static char *apply_command(const char *command, const char *arg)
@@ -283,12 +280,12 @@ static void apply_arg_if_exists(struct trailer_item 
*in_tok,
break;
case EXISTS_ADD_IF_DIFFERENT:
apply_item_command(in_tok, arg_tok);
-   if (check_if_different(in_tok, arg_tok, 1, head))
+   if (check_if_different(in_tok, arg_tok, head))
add_arg_to_input_list(on_tok, arg_tok);
break;
case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
apply_item_command(in_tok, arg_tok);
-   if (check_if_different(on_tok, arg_tok, 0, head))
+   if (!same_trailer(on_tok, arg_tok))
add_arg_to_input_list(on_tok, arg_tok);
break;
default:
-- 
2.14.2

Re: git send-email does not work with Google anymore?!

[Paolo Bonzini]

On 05/10/2017 12:52, Lars Schneider wrote:
> Hi,
> 
> I used to use the Google SMTP server to send my patches to the list with 
> the following config:
> 
> [sendemail]
> smtpencryption = tls
> smtpserver = smtp.gmail.com
> smtpuser = larsxschnei...@gmail.com
> smtpserverport = 587
> from = larsxschnei...@gmail.com
> chainreplyto = false
> suppresscc = self
> 
> Apparently that stopped working today. I get this error:
> 
> (mbox) Adding cc: Lars Schneider  from line 
> 'From: Lars Schneider '
> Password for 'smtp://larsxschnei...@gmail.com@smtp.gmail.com:587':
> 5.7.14  5.7.14 ...> Please log in via your web browser and
> 5.7.14 then try again.
> 5.7.14  Learn more at
> 5.7.14  https://support.google.com/mail/answer/78754 ... - gsmtp
> 
> Of couse I tried to log in via web browser etc. Does anyone else use 
> Google as SMTP server? If yes, does it work for you?

It's probably a good idea to set up two-factor authentication and add an 
app-specific password for "git send-email" (at least that's what I do).

The password can be stored encrypted using the OS keychain, or if you
don't want/have one (e.g. you are running headless) you could check if
your distro installs git-credential-netrc.  Create a ~/.netrc.gpg file,
where the decrypted content should look like

machine smtp.gmail.com login larsxschnei...@gmail.com password mypassword

Then do

git config --global credential.helper netrc

and you're all set. :)

Paolo

Re: git send-email does not work with Google anymore?!

[Paolo Bonzini]

On 05/10/2017 16:21, ankostis wrote:
> 
> But this seems to have stopped working; I clicked the link in the page below
>   https://support.google.com/accounts/answer/6010255?hl=en
> and for my account it tells me "less secure apps" access is unavailable :-(

It says for me "This setting is not available for accounts with 2-Step
Verification enabled. Such accounts require an application-specific
password for less secure apps access", which is the right thing to do.

I used an app-specific password ten minutes before answering OP...

Paolo

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

On 06/10/2017 08:44, Junio C Hamano wrote:
> Paolo Bonzini  writes:
>> The purpose of this action is for scripts to be able to keep the
>> user's Signed-off-by at the end.
>>
>> #! /bin/sh
>> me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')
>> them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>> trailer="Reviewed-by: $them"
>> sob="Signed-off-by: $me"
>> git log -1 --pretty=format:%B | \
>>   git interpret-trailers --where end --if-exists doNothing --trailer 
>> "$trailer" \
>>  --where end --if-exists move --if-missing 
>> doNothing --trailer "$sob" | \
>>   git commit --amend -F-
>>
>> which lets me keep the SoB line at the end, as it should be.
>> Posting as RFC because it's possible that I'm missing a simpler
>> way to achieve this...
>
> If anything, the above (assuming that you wrote a patch, sent out
> for a review with or without signing it off, and then after getting
> a review, you are adding reviewed-by to the commit) does not
> demonstrate the need for "move".  The use of "move" in the example
> looks like a mere workaround that reviewed-by was added at the wrong
> place (i.e. --where end) in the first place.

Yes, I agree.  Though I also tried implementing "--where beforeLastSOB", 
and in the end decided against it because there were more corner cases.
I include the patch for reference at the end of this message, to be
applied on top of these four.

> But that is not the primary reason why I find the example using
> S-o-b convincing.  If the patch in your example originally did not
> have just one S-o-b by you, but yours was at the end of the chain of
> patch passing, use of "move" may become even more problematic.  Your
> friend may write an original, sign it off and pass it to you, who
> then signs it off and sends to the mailng list.  It gets picked up
> by somebody else, who tweaks and adds her sign off, then you pick it
> up and relay it to the final destination (i.e. the first sign-off is
> by your friend, then you have two sign-offs of yours, one sign off
> from somebody else in between, and the chain records how the patch
> "flowed").  And then Linus says "yeah, this is good, I throughly
> reviewed it."  Where would you place that reviewed-by?  Before your
> second (and last) sign-off?  What makes that last one special?

So:

   Signed-off-by: Me
   Signed-off-by: Friend
   Signed-off-by: Me   <<<
   Reviewed-by: Linus
   Signed-off-by: Me

I do think the SoB line marked with "<<<" is a bit "special".  SoB lines 
before it represent the path followed by the contribution, according to 
clause (c) of the Developer Certificate of Origin.  Multiple 
*consecutive* SoB lines from the same person do not add much, while 
multiple separate SoB lines from the same person must be kept.

Of course, using "--where start" for Reviewed/Tested/Acked-by lines *is* 
an option.  On the other hand, for "Cc: sta...@vger.kernel.org" the 
placement hints at *who* decided the patch to be worth of inclusion in a 
stable version.  That person might be the right one to bug if the patch 
doesn't apply and needs a manual backport.  It's not science of course, 
but in practice I found the "always apply with -s, and use 'move' to 
keep my SoB at the end" workflow to be the least error-prone.

> Would it more faithfully reflect the order of events if you added
> Linus's reviewed-by and then your own sign-off to conclude the
> chain?

Possibly, but the DCO doesn't care and SoB lines are first and firemost 
about the DCO.

On the other hand, "move" does not provide exactly what we want in the 
case where the user's SoB is there, but is not the last.  So, the above 
script pretty much assumes that you apply the patch with "-s"; if you 
didn't, you'd need something more like "moveLast".  It is trivial to 
implement "moveLast" on top of the first three patches in this series, 
but things start getting a bit out of hand perhaps...

Paolo

> So I am not opposed to the idea of "move", but I am not sure in what
> situation it is useful and what use case it makes it easier to use.
> The example makes me suspect that what we want is not a new
> operation, but a way to specify "where" in a richer way.

--- 8< 
>From a238b973821586eaaf26d608172cdc72f19d6071 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini 
Date: Wed, 12 Jul 2017 18:11:44 +0200
Subject: [PATCH] trailer: add "beforeLastSOB" configuration

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

On 06/10/2017 12:30, Christian Couder wrote:
> On Thu, Oct 5, 2017 at 3:22 PM, Paolo Bonzini  wrote:
>> The purpose of this action is for scripts to be able to keep the
>> user's Signed-off-by at the end.  For example say I have a script
>> that adds a Reviewed-by tag:
>>
>> #! /bin/sh
>> them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>> trailer="Reviewed-by: $them"
>> git log -1 --pretty=format:%B | \
>>   git interpret-trailers --where end --if-exists doNothing --trailer 
>> "$trailer" | \
>>   git commit --amend -F-
>>
>> Now, this script will leave my Signed-off-by line in a non-canonical
>> place, like
>>
>>Signed-off-by: Paolo Bonzini 
>>Reviewed-by: Junio C Hamano 
>>
>> This new option enables the following improvement:
>>
>> #! /bin/sh
>> me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')
>> them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>> trailer="Reviewed-by: $them"
>> sob="Signed-off-by: $me"
>> git log -1 --pretty=format:%B | \
>>   git interpret-trailers --where end --if-exists doNothing --trailer 
>> "$trailer" \
>>  --where end --if-exists move --if-missing 
>> doNothing --trailer "$sob" | \
>>   git commit --amend -F-
>>
>> which lets me keep the SoB line at the end, as it should be.
>> Posting as RFC because it's possible that I'm missing a simpler
>> way to achieve this...
> 
> Did you try using `--where end --if-exists replace --trailer "$sob"`?

Yes, it's a different behavior; "--if-exists replace" matches on others'
SoB as well, so it would eat the original author's SoB if I didn't have one.

So "move" does get it wrong for

Signed-off-by: Me
Signed-off-by: Friend

(Me gets moved last, which may not be what you want) but "replace" gets
it wrong in the arguably more common case of

Signed-off-by: Friend

which is damaged to just "Signed-off-by: Me".

Paolo

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

On 06/10/2017 14:33, Christian Couder wrote:
> Ok. I think you might want something called for example
> "replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
> trailer with the same (, ) pair above or below the line
> where the replaced trailer will be put when ignoring trailers with a
> different ".

So basically "moveIfClosest" (move if last for where=end, move if first
for where=begin; for where=after and where=before it would just end up
doing nothing)?  It's not hard to implement, but I'm wondering if it's
too ad hoc.

Paolo

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

On 06/10/2017 15:19, Christian Couder wrote:
> Now it would be strange to have "moveIfClosest" without having "move"
> first and I don't see how "move" would be different from the existing
> "replace".
> Or maybe "move" means "replaceIfIdentical", in this case I think it
> would help users to just call it "replaceIfIdentical".

Well, the effect of "replacing if identical" *is* to move the existing
identical trailer to the new position. :)

Paolo

Re: [PATCH 01/12] t4150: am.messageid really adds the message id

[Paolo Bonzini]

On 02/07/2015 20:16, Paul Tan wrote:
> Since a078f73 (git-am: add --message-id/--no-message-id, 2014-11-25),
> the am.messageid setting determines whether the --message-id option is
> set by default.
> 
> Add a test for this.
> 
> Cc: Paolo Bonzini 
> Signed-off-by: Paul Tan 
> ---
>  t/t4150-am.sh | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index b822a39..3f54bdf 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -563,6 +563,17 @@ test_expect_success 'am --message-id really adds the 
> message id' '
>   test_cmp expected actual
>  '
>  
> +test_expect_success 'am.messageid really adds the message id' '
> + rm -fr .git/rebase-apply &&
> + git checkout -f HEAD^ &&
> + test_config am.messageid true &&
> + git am patch1.eml &&
> + test_path_is_missing .git/rebase-apply &&
> + git cat-file commit HEAD | tail -n1 >actual &&
> + grep Message-Id patch1.eml >expected &&
> + test_cmp expected actual
> +'
> +
>  test_expect_success 'am --message-id -s signs off after the message id' '
>   rm -fr .git/rebase-apply &&
>   git reset --hard &&
> 

Reviewed-by: Paolo Bonzini 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*

[Paolo Bonzini]

On 27/05/19 00:54, Ævar Arnfjörð Bjarmason wrote:
> This resulted in a case[1] where someone on LKML did:
> 
> git push kvm +HEAD:tags/for-linus
> 
> Which would have created a new "tags/for-linus" branch in their "kvm"
> repository, except because they happened to have an existing
> "refs/tags/for-linus" reference we pushed there instead, and replaced
> an annotated tag with a lightweight tag.

Actually, I would not be surprised even if "git push foo
someref:tags/foo" _always_ created a lightweight tag (i.e. push to
refs/tags/foo).

In my opinion, the bug is that "git request-pull" should warn if the tag
is lightweight remotely but not locally, and possibly even vice versa.
Here is a simple testcase:

  # setup "local" repo
  mkdir -p testdir/a
  cd testdir/a
  git init
  echo a > test
  git add test
  git commit -minitial

  # setup "remote" repo
  git clone --bare . ../b

  # setup "local" tag
  echo b >> test
  git commit -msecond test
  git tag -mtag tag1

  # create remote lightweight tag and prepare a pull request
  git push ../b HEAD:refs/tags/tag1
  git request-pull HEAD^ ../b tags/tag1

Paolo

Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*

[Paolo Bonzini]

On 27/05/19 17:39, Junio C Hamano wrote:
> I do not think lightweight vs annotated should be the issue.  The
> tag that the requestor asks to be pulled (from repository ../b)
> should be what the requestor has locally when writing the request
> (in repository .).  Even if both tags at remote and local are
> annotated, we should still warn if they are different objects, no?

Right, lightweight vs annotated then is the obvious special case where
one of the two is a commit and the other is a tag, hence they ought not
to have the same SHA1.  I'll take a look.

Paolo

[PATCH 1/2] request-pull: quote regex metacharacters in local ref

[Paolo Bonzini]

From: Paolo Bonzini 

The local part of the third argument of git-request-pull is used in
a regular expression without quoting it.  Use qr{} and \Q\E to ensure
that e.g. a period in a tag name does not match any character on the
remote side.

Signed-off-by: Paolo Bonzini 
---
 git-request-pull.sh |  5 ++---
 t/t5150-request-pull.sh | 18 ++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index 13c172bd94..0d128be7fd 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -83,19 +83,18 @@ die "fatal: No commits in common between $base and $head"
 # Otherwise find a random ref that matches $headrev.
 find_matching_ref='
my ($head,$headrev) = (@ARGV);
+   my $pattern = qr{/\Q$head\E$};
my ($found);
 
while () {
chomp;
my ($sha1, $ref, $deref) = /^(\S+)\s+([^^]+)(\S*)$/;
-   my ($pattern);
next unless ($sha1 eq $headrev);
 
-   $pattern="/$head\$";
if ($ref eq $head) {
$found = $ref;
}
-   if ($ref =~ /$pattern/) {
+   if ($ref =~ $pattern) {
$found = $ref;
}
if ($sha1 eq $head) {
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index fca001eb9b..c1a821a549 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -246,4 +246,22 @@ test_expect_success 'request-pull ignores 
OPTIONS_KEEPDASHDASH poison' '
 
 '
 
+test_expect_success 'request-pull quotes regex metacharacters properly' '
+
+   rm -fr downstream.git &&
+   git init --bare downstream.git &&
+   (
+   cd local &&
+   git checkout initial &&
+   git merge --ff-only master &&
+   git tag -mrelease v2.0 &&
+   git push origin refs/tags/v2.0:refs/tags/v2-0 &&
+   test_must_fail git request-pull initial "$downstream_url" 
tags/v2.0 \
+   2>../err
+   ) &&
+   grep "No match for commit .*" err &&
+   grep "Are you sure you pushed" err
+
+'
+
 test_done
-- 
2.21.0

[PATCH 2/2] request-pull: warn if the remote object is not the same as the local one

[Paolo Bonzini]

From: Paolo Bonzini 

In some cases, git request-pull might be invoked with remote and
local objects that differ even though they point to the same commit.
For example, the remote object might be a lightweight tag
vs. an annotated tag on the local side; or the user might have
reworded the tag locally and forgotten to push it.

When this happens git-request-pull will not warn, because it only
checks that "git ls-remote" returns an SHA1 that matches the local
commit (known as $headrev in the script).  This patch makes
git-request-pull retrieve the tag object SHA1 while processing
the "git ls-remote" output, so that it can be matched against the
local object.

Signed-off-by: Paolo Bonzini 
---
 git-request-pull.sh | 43 +++--
 t/t5150-request-pull.sh | 35 +
 2 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index 0d128be7fd..2d0e44656c 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -65,6 +65,8 @@ test -z "$head" && die "fatal: Not a valid revision: $local"
 headrev=$(git rev-parse --verify --quiet "$head"^0)
 test -z "$headrev" && die "fatal: Ambiguous revision: $local"
 
+local_sha1=$(git rev-parse --verify --quiet "$head")
+
 # Was it a branch with a description?
 branch_name=${head#refs/heads/}
 if test "z$branch_name" = "z$headref" ||
@@ -77,42 +79,53 @@ merge_base=$(git merge-base $baserev $headrev) ||
 die "fatal: No commits in common between $base and $head"
 
 # $head is the refname from the command line.
-# If a ref with the same name as $head exists at the remote
-# and their values match, use that.
-#
-# Otherwise find a random ref that matches $headrev.
+# Find a ref with the same name as $head that exists at the remote
+# and points to the same commit as the local object.
 find_matching_ref='
my ($head,$headrev) = (@ARGV);
my $pattern = qr{/\Q$head\E$};
-   my ($found);
+   my ($remote_sha1, $found);
 
while () {
chomp;
my ($sha1, $ref, $deref) = /^(\S+)\s+([^^]+)(\S*)$/;
-   next unless ($sha1 eq $headrev);
 
-   if ($ref eq $head) {
-   $found = $ref;
-   }
-   if ($ref =~ $pattern) {
-   $found = $ref;
-   }
if ($sha1 eq $head) {
-   $found = $sha1;
+   $found = $remote_sha1 = $sha1;
+   break;
+   }
+
+   if ($ref eq $head || $ref =~ $pattern) {
+   if ($deref eq "") {
+   # Remember the matching object on the remote 
side
+   $remote_sha1 = $sha1;
+   }
+   if ($sha1 eq $headrev) {
+   $found = $ref;
+   break;
+   }
}
}
if ($found) {
-   print "$found\n";
+   $remote_sha1 = $headrev if ! defined $remote_sha1;
+   print "$remote_sha1 $found\n";
}
 '
 
-ref=$(git ls-remote "$url" | @@PERL@@ -e "$find_matching_ref" 
"${remote:-HEAD}" "$headrev")
+set fnord $(git ls-remote "$url" | @@PERL@@ -e "$find_matching_ref" 
"${remote:-HEAD}" "$headrev")
+remote_sha1=$2
+ref=$3
 
 if test -z "$ref"
 then
echo "warn: No match for commit $headrev found at $url" >&2
echo "warn: Are you sure you pushed '${remote:-HEAD}' there?" >&2
status=1
+elif test "$local_sha1" != "$remote_sha1"
+then
+   echo "warn: $head found at $url but points to a different object" >&2
+   echo "warn: Are you sure you pushed '${remote:-HEAD}' there?" >&2
+   status=1
 fi
 
 # Special case: turn "for_linus" to "tags/for_linus" when it is correct
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index c1a821a549..852dcd913f 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -264,4 +264,39 @@ test_expect_success 'request-pull quotes regex 
metacharacters properly' '
 
 '
 
+test_expect_success 'pull request with mismatched object' '
+
+   rm -fr downstream.git &&
+   git init --bare downstream.git &&
+   (
+   cd local &&
+   git checkout initial &&
+   git merge --ff-only master &&
+   git push origin HEAD:refs/tags/full &&
+   test_must_fail git request-pull initial "$do

[PATCH 0/2] request-pull: warn if the remote object is not the same as the local one

[Paolo Bonzini]

From: Paolo Bonzini 

In some cases, git request-pull might be invoked with remote and
local objects that differ even though they point to the same commit.
For example, the remote object might be a lightweight tag
vs. an annotated tag on the local side, or the user might have
reworded the tag locally and forgotten to push it.

When this happens git-request-pull will not warn, because it only
checks that "git ls-remote" returns an SHA1 that matches the local
commit.  Patch 2 of this series makes git-request-pull remember the tag
object's SHA1 while processing the "git ls-remote" output, so that it
can be matched against the local object.

Paolo Bonzini (2):
  request-pull: quote metacharacters in local ref
  request-pull: warn if the remote object is not the same as the local one

 git-request-pull.sh | 46 ++-
 t/t5150-request-pull.sh | 53 +
 2 files changed, 82 insertions(+), 17 deletions(-)

-- 
2.21.0

Re: poll() emulation in git

[Paolo Bonzini]

Il 05/09/2012 13:24, Joachim Schmitz ha scritto:
> However: this poll implementation, while compiling OK, doesn't work properly.
> Because it uses recv(...,MSG_PEEK), it works on sockets only (returns 
> ENOTSOCK on anything else), while the real poll() works on all
> kind if file descriptors, at least that is my understanding.

Actually recv(...,MSG_PEEK) on most Unix variants works on non-sockets
too.  The trick is taken from GNU Pth in turn.

> Here on HP NonStop, when being connected via an non-interactive SSH, we get a 
> set of pipes (stdin, stdout, stderr) instead of a
> socket to talk to, so the poll() just hangs/loops.

Does your system have a working FIONREAD ioctl for pipes?

> As git's implementation is based on ('stolen' from?) gnulib's and still 
> pretty similar, CC to the gnulib list and Paolo
> 
> Any idea how this could get solved? I.e. how to implement a poll() that works 
> on non-sockets too?
> There is some code that pertains to a seemingly similar problem in Mac OS X, 
> but my problem is not identical, as that fix doesn't
> help.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: poll() emulation in git

[Paolo Bonzini]

Il 05/09/2012 15:36, Joachim Schmitz ha scritto:
>>> > > Does your system have a working FIONREAD ioctl for pipes?
>> > 
>> > It does have FIONREAD ioctl. Whether it works properly is to be 
>> > determined...
>> > I'll test if you could show me how?
> Oh, now I see what you aimed at, but no, that Mac OS X method doesn't work 
> for me, I tried (at least I think I did).
> 
> And  has
> /*
>  * Normal IOCTL's supported by the socket interface
>  */
> #define FIONREAD_IOR(0, 8, _ioctl_int)   /* Num of bytes to read 
> */
> #define FIONBIO _IOW(0, 9, _ioctl_int)   /* Non-blocking I/O 
> */
> 
> So these seem to be supported on sockets only, I guess.
> And indeed the man pages for ioctl confirms:
> 
>   Valid values for the request parameter for AF_INET or
>   AF_INET6 sockets are:
> 
> 
>   FIONREAD  Gets the number of bytes available for reading and
> stores it at the int pointed at by arg.
> 
> 
> So not even AF_UNIX sockets, not to mention pipes...

So there's no way you can support POLLHUP.  Your system is quite
crippled. :(

Paolo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: poll() emulation in git

[Paolo Bonzini]

Il 06/09/2012 16:02, Joachim Schmitz ha scritto:
> 
> But is there something that could be done to make git work even without 
> poll()?
> It is used in 5 places:
> 
> $ grep -n poll\( *.c */*.c
> credential-cache--daemon.c:175: if (poll(&pfd, 1, 1000 * wakeup) < 0) {
> daemon.c:1018:  if (poll(pfd, socklist->nr, -1) < 0) {
> help.c:361: poll(NULL, 0, autocorrect * 100);
> upload-pack.c:232:  if (poll(pfd, pollsize, -1) < 0) {
> builtin/upload-archive.c:125:   if (poll(pfd, 2, -1) < 0) {
> 
> Don't quite understand why in help.c it has that NULL, which should always 
> result in an EFAULT and other than that basically is a
> NOP (at least in the poll() emulation)? Seems a usleep(autocorrect * 100) is 
> meant to happen here instead?
> So I think here a poll() isn't needed at all. But also the 'broken' one 
> shouldn't harm too much.

Yes, it's an usleep(autocorrect * 10) basically (poll takes
milliseconds, not micro).

> ...
> # else
>   char data[64];
>   r = recv (fd, data, sizeof (data), MSG_PEEK);
>   socket_errno = (r < 0) ? errno : 0;
> # endif
>   if (r == 0)
> happened |= POLLHUP;
> 
>   /* If the event happened on an unconnected server socket,
>  that's fine. */
>   else if (r > 0 || ( /* (r == -1) && */ socket_errno == ENOTCONN))
> happened |= (POLLIN | POLLRDNORM) & sought;
> 
>   /* Distinguish hung-up sockets from other errors.  */
>   else if (socket_errno == ESHUTDOWN || socket_errno == ECONNRESET
>|| socket_errno == ECONNABORTED || socket_errno == ENETRESET)
> happened |= POLLHUP;
> 
> #ifdef __TANDEM /* as we can't recv(...,MSG_PEEK) on a non-socket */
>   else if (socket_errno == ENOTSOCK)
> happened |= (POLLIN | POLLRDNORM) & sought;
> #endif
>   else
> happened |= POLLERR;
> }
> ...
> 
> We won't detect POLLHUP that way I think. However it seems to work, we've 
> been able to clone, push, pull, branch that way with
> NonStop being the (ssh-)server, something that didn't work at all without 
> that hack (and yes, I believe it is just that).
> Someone in for a cleaner way of managing this?

I suppose it works to always handle ENOTSOCK that way, even on
non-__TANDEM systems.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: poll() emulation in git

[Paolo Bonzini]

Il 06/09/2012 16:44, Joachim Schmitz ha scritto:
>> > Yes, it's an usleep(autocorrect * 10) basically (poll takes
>> > milliseconds, not micro).
> OK, it is _supposed_ to do this usleep(), but is does not, as poll() returns 
> early with EFAULT in this case:
>   /* EFAULT is not necessary to implement, but let's do it in the
>  simplest case. */
>   if (!pfd)
> {
>   errno = EFAULT;
>   return -1;
> }
> 
> poll() is doing this before calling select(), so won't sleep.
> So there's a bug in {gnulib|git}'s poll(), right?
> 

Yes, it should be "if (!pfd && nfd)".

Paolo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: poll() emulation in git

[Paolo Bonzini]

Il 07/09/2012 09:39, Joachim Schmitz ha scritto:
>> > I suppose it works to always handle ENOTSOCK that way, even on
>> > non-__TANDEM systems.
> Will you be fixing this in gnulib? How?

I don't have access to the system, so it's best if you post the patches
yourself to bug-gnulib and git mailing lists (separately, since the code
is cross-pollinated but forked).

Paolo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] git-send-email: delay creation of MIME headers

[Paolo Bonzini]

From: Paolo Bonzini 

After the next patch, git-send-email will sometimes modify
existing Content-Transfer-Encoding headers.  Delay the addition
of the header to @xh until just before sending.  Do the same
for MIME-Version, to avoid adding it twice.

Signed-off-by: Paolo Bonzini 
---
 git-send-email.perl | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 9949db0..b29a304 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1324,6 +1324,8 @@ foreach my $t (@files) {
my $author_encoding;
my $has_content_type;
my $body_encoding;
+   my $xfer_encoding;
+   my $has_mime_version;
@to = ();
@cc = ();
@xh = ();
@@ -1394,9 +1396,16 @@ foreach my $t (@files) {
}
push @xh, $_;
}
+   elsif (/^MIME-Version/i) {
+   $has_mime_version = 1;
+   push @xh, $_;
+   }
elsif (/^Message-Id: (.*)/i) {
$message_id = $1;
}
+   elsif (/^Content-Transfer-Encoding: (.*)/i) {
+   $xfer_encoding = $1 if not defined 
$xfer_encoding;
+   }
elsif (!/^Date:\s/i && /^[-A-Za-z]+:\s+\S/) {
push @xh, $_;
}
@@ -1444,10 +1453,9 @@ foreach my $t (@files) {
if defined $cc_cmd && !$suppress_cc{'cccmd'};
 
if ($broken_encoding{$t} && !$has_content_type) {
+   $xfer_encoding = '8bit' if not defined $xfer_encoding;
$has_content_type = 1;
-   push @xh, "MIME-Version: 1.0",
-   "Content-Type: text/plain; charset=$auto_8bit_encoding",
-   "Content-Transfer-Encoding: 8bit";
+   push @xh, "Content-Type: text/plain; 
charset=$auto_8bit_encoding";
$body_encoding = $auto_8bit_encoding;
}
 
@@ -1467,14 +1475,19 @@ foreach my $t (@files) {
}
}
else {
+   $xfer_encoding = '8bit' if not defined 
$xfer_encoding;
$has_content_type = 1;
push @xh,
- 'MIME-Version: 1.0',
- "Content-Type: text/plain; 
charset=$author_encoding",
- 'Content-Transfer-Encoding: 8bit';
+ "Content-Type: text/plain; 
charset=$author_encoding";
}
}
}
+   if (defined $xfer_encoding) {
+   push @xh, "Content-Transfer-Encoding: $xfer_encoding";
+   }
+   if (defined $xfer_encoding or $has_content_type) {
+   unshift @xh, 'MIME-Version: 1.0' unless $has_mime_version;
+   }
 
$needs_confirm = (
$confirm eq "always" or
-- 
2.1.0


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] git-send-email: add --transfer-encoding option for conversion to specified encoding

[Paolo Bonzini]

From: Paolo Bonzini 

The thread at http://thread.gmane.org/gmane.comp.version-control.git/257392
details problems when applying patches with "git am" in a repository with
CRLF line endings.  In the example in the thread, the repository originated
from "git-svn" so it is not possible to use core.eol and friends on it.

Right now, the best option is to use "git am --keep-cr".  However, when
a patch create new files, the patch application process will reject the
new file because it finds a "/dev/null\r" string instead of "/dev/null".

The problem is that SMTP transport is CRLF-unsafe; "git am --keep-cr" is
mostly working by chance and it would be very problematic to have a "git
am" workflow in a repository with mixed LF and CRLF line endings.  It is
more robust to forgo readable patch files[1] and use the quoted-printable
transfer enconding.  This series adds an option and configuration key
to git-send-email that lets it automatically produce quoted-printable
or base64 messages.

Paolo

[1] A useful oneliner to decode quoted-printable files is the following:
    perl -pe 'use MIME::QuotedPrint; $_=MIME::QuotedPrint::decode($_);'

Paolo Bonzini (2):
  git-send-email: delay creation of MIME headers
  git-send-email: add --transfer-encoding option

 Documentation/config.txt   |   1 +
 Documentation/git-send-email.txt   |  10 +++
 contrib/completion/git-completion.bash |   4 +
 git-send-email.perl|  61 +++--
 t/t9001-send-email.sh  | 157 +
 5 files changed, 227 insertions(+), 6 deletions(-)

-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] git-am: add --message-id/--no-message-id options

[Paolo Bonzini]

From: Paolo Bonzini 

This series adds a --message-id option to git-mailinfo and git-am.
git-am also gets an am.messageid configuration key to set the default,
and a --no-message-id option to override the configuration key.
(I'm not sure of the usefulness of a mailinfo.messageid option, so
I left it out; this follows the example of -k instead of --scissors).

This option can be useful in order to associate commit messages with
mailing list discussions.

If both --message-id and -s are specified, the Signed-off-by goes
last.  This is coming out more or less naturally out of the git-am
implementation, but is also tested in t4150-am.sh.

Paolo Bonzini (2):
  git-mailinfo: add --message-id
  git-am: add --message-id/--no-message-id

 Documentation/git-am.txt   | 11 +++
 Documentation/git-mailinfo.txt |  5 +
 builtin/mailinfo.c | 22 +-
 git-am.sh  | 21 +++--
 t/t4150-am.sh  | 23 +++
 t/t5100-mailinfo.sh|  4 
 t/t5100/info0012--message-id   |  5 +
 t/t5100/msg0012--message-id|  8 
 t/t5100/patch0012--message-id  | 30 ++
 9 files changed, 126 insertions(+), 3 deletions(-)
 create mode 100644 t/t5100/info0012--message-id
 create mode 100644 t/t5100/msg0012--message-id
 create mode 100644 t/t5100/patch0012--message-id

-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] git-send-email: add --transfer-encoding option

[Paolo Bonzini]

From: Paolo Bonzini 

The thread at http://thread.gmane.org/gmane.comp.version-control.git/257392
details problems when applying patches with "git am" in a repository with
CRLF line endings.  In the example in the thread, the repository originated
from "git-svn" so it is not possible to use core.eol and friends on it.

Right now, the best option is to use "git am --keep-cr".  However, when
a patch create new files, the patch application process will reject the
new file because it finds a "/dev/null\r" string instead of "/dev/null".

The problem is that SMTP transport is CRLF-unsafe.  Sending a patch by
email is the same as passing it through "dos2unix | unix2dos".  The newly
introduced CRLFs are normally transparent because git-am strips them. The
keepcr=true setting preserves them, but it is mostly working by chance
and it would be very problematic to have a "git am" workflow in a
repository with mixed LF and CRLF line endings.

The MIME solution to this is the quoted-printable transfer enconding.
This is not something that we want to enable by default, since it makes
received emails horrible to look at.  However, it is a very good match
for projects that store CRLF line endings in the repository.

The only disadvantage of quoted-printable is that quoted-printable
patches fail to apply if the maintainer uses "git am --keep-cr".  This
is because the decoded patch will have two carriage returns at the end
of the line.  Therefore, add support for base64 transfer encoding too,
which makes received emails downright impossible to look at outside
a MUA, but really just works.

The patch covers all bases, including users that still live in the late
80s, by also providing a 7bit content transfer encoding that refuses
to send emails with non-ASCII character in them.  And finally, "8bit"
will add a Content-Transfer-Encoding header but otherwise do nothing.

Signed-off-by: Paolo Bonzini 
---
 Documentation/config.txt   |   1 +
 Documentation/git-send-email.txt   |  10 +++
 contrib/completion/git-completion.bash |   4 +
 git-send-email.perl|  36 
 t/t9001-send-email.sh  | 157 +
 5 files changed, 208 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9220725..cc2ff20 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2303,6 +2303,7 @@ sendemail.smtpserverport::
 sendemail.smtpserveroption::
 sendemail.smtpuser::
 sendemail.thread::
+sendemail.transferencoding::
 sendemail.validate::
See linkgit:git-send-email[1] for description.
 
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index a60776e..a9efa5c 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -131,6 +131,16 @@ Note that no attempts whatsoever are made to validate the 
encoding.
Specify encoding of compose message. Default is the value of the
'sendemail.composeencoding'; if that is unspecified, UTF-8 is assumed.
 
+--transfer-encoding=(7bit|8bit|quoted-printable|base64)::
+   Specify the transfer encoding to be used to send the message over SMTP.
+   7bit will fail upon encountering a non-ASCII message.  quoted-printable
+   can be useful when the repository contains files that contain carriage
+   returns, but makes the raw patch email file (as saved from a MUA) much
+   harder to inspect manually.  base64 is even more fool proof, but also
+   even more opaque.  Default is the value of the 
'sendemail.transferEncoding'
+   configuration value; if that is unspecified, git will use 8bit and not
+   add a Content-Transfer-Encoding header.
+
 
 Sending
 ~~~
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 2fece98..b154f70 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1875,6 +1875,10 @@ _git_config ()
__gitcomp "$__git_send_email_suppresscc_options"
return
;;
+   sendemail.transferencoding)
+   __gitcomp "7bit 8bit quoted-printable base64"
+   return
+   ;;
--get|--get-all|--unset|--unset-all)
__gitcomp_nl "$(__git_config_get_set_variables)"
return
diff --git a/git-send-email.perl b/git-send-email.perl
index b29a304..82c6fea 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -58,6 +58,7 @@ git send-email [options] 
 --compose  * Open an editor for introduction.
 --compose-encoding* Encoding to assume for introduction.
 --8bit-encoding   * Encoding to assume 8bit mails if 
undeclared
+--transfer-encoding   * Transfer encoding to use 
(quoted-printable, 8bit, b

[PATCH 1/2] git-mailinfo: add --message-id

[Paolo Bonzini]

From: Paolo Bonzini 

This option adds the content of the Message-Id header at the end of the
commit message prepared by git-mailinfo.  This is useful in order to
associate commit messages automatically with mailing list discussions.

Signed-off-by: Paolo Bonzini 
---
 Documentation/git-mailinfo.txt |  5 +
 builtin/mailinfo.c | 22 +-
 t/t5100-mailinfo.sh|  4 
 t/t5100/info0012--message-id   |  5 +
 t/t5100/msg0012--message-id|  8 
 t/t5100/patch0012--message-id  | 30 ++
 6 files changed, 73 insertions(+), 1 deletion(-)
 create mode 100644 t/t5100/info0012--message-id
 create mode 100644 t/t5100/msg0012--message-id
 create mode 100644 t/t5100/patch0012--message-id

diff --git a/Documentation/git-mailinfo.txt b/Documentation/git-mailinfo.txt
index 164a3c6..2e99603 100644
--- a/Documentation/git-mailinfo.txt
+++ b/Documentation/git-mailinfo.txt
@@ -66,6 +66,11 @@ conversion, even with this flag.
 -n::
Disable all charset re-coding of the metadata.
 
+-m::
+--message-id::
+   Copy the Message-ID header at the end of the commit message.  This 
+   is useful in order to associate commits with mailing list discussions.
+
 --scissors::
Remove everything in body before a scissors line.  A line that
mainly consists of scissors (either ">8" or "8<") and perforation
diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index 6a14d29..c8a47c1 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -15,6 +15,7 @@ static const char *metainfo_charset;
 static struct strbuf line = STRBUF_INIT;
 static struct strbuf name = STRBUF_INIT;
 static struct strbuf email = STRBUF_INIT;
+static char *message_id;
 
 static enum  {
TE_DONTCARE, TE_QP, TE_BASE64
@@ -24,6 +25,7 @@ static struct strbuf charset = STRBUF_INIT;
 static int patch_lines;
 static struct strbuf **p_hdr_data, **s_hdr_data;
 static int use_scissors;
+static int add_message_id;
 static int use_inbody_headers = 1;
 
 #define MAX_HDR_PARSED 10
@@ -198,6 +200,12 @@ static void handle_content_type(struct strbuf *line)
}
 }
 
+static void handle_message_id(const struct strbuf *line)
+{
+   if (add_message_id)
+   message_id = strdup(line->buf);
+}
+
 static void handle_content_transfer_encoding(const struct strbuf *line)
 {
if (strcasestr(line->buf, "base64"))
@@ -342,6 +350,14 @@ static int check_header(const struct strbuf *line,
ret = 1;
goto check_header_out;
}
+   if (cmp_header(line, "Message-Id")) {
+   len = strlen("Message-Id: ");
+   strbuf_add(&sb, line->buf + len, line->len - len);
+   decode_header(&sb);
+   handle_message_id(&sb);
+   ret = 1;
+   goto check_header_out;
+   }
 
/* for inbody stuff */
if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
@@ -816,6 +832,8 @@ static int handle_commit_msg(struct strbuf *line)
}
 
if (patchbreak(line)) {
+   if (message_id)
+   fprintf(cmitmsg, "Message-Id: %s\n", message_id);
fclose(cmitmsg);
cmitmsg = NULL;
return 1;
@@ -1013,7 +1031,7 @@ static int git_mailinfo_config(const char *var, const 
char *value, void *unused)
 }
 
 static const char mailinfo_usage[] =
-   "git mailinfo [-k|-b] [-u | --encoding= | -n] [--scissors | 
--no-scissors] msg patch < mail >info";
+   "git mailinfo [-k|-b] [-m | --message-id] [-u | --encoding= | 
-n] [--scissors | --no-scissors] msg patch < mail >info";
 
 int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 {
@@ -1032,6 +1050,8 @@ int cmd_mailinfo(int argc, const char **argv, const char 
*prefix)
keep_subject = 1;
else if (!strcmp(argv[1], "-b"))
keep_non_patch_brackets_in_subject = 1;
+   else if (!strcmp(argv[1], "-m") || !strcmp(argv[1], 
"--message-id"))
+   add_message_id = 1;
else if (!strcmp(argv[1], "-u"))
metainfo_charset = def_charset;
else if (!strcmp(argv[1], "-n"))
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 9e1ad1c..60df10f 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -35,6 +35,10 @@ do
then
check_mailinfo $mail --no-inbody-headers
fi
+   if test -f "$TEST_DIRECTORY"/t5100/msg$mail--message-id
+   then
+   check_mailinfo $mail --message-id
+   fi
'
 done
 
diff --git a/t/t5100/info0012--message-id b/t/t5100/info0012--m

[PATCH 2/2] git-am: add --message-id/--no-message-id

[Paolo Bonzini]

From: Paolo Bonzini 

Parse the option and pass it directly to git-mailinfo.

Signed-off-by: Paolo Bonzini 
---
 Documentation/git-am.txt | 11 +++
 git-am.sh| 21 +++--
 t/t4150-am.sh| 23 +++
 3 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 9adce37..cfb74bc 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -57,6 +57,17 @@ OPTIONS
 --no-scissors::
Ignore scissors lines (see linkgit:git-mailinfo[1]).
 
+-m::
+--message-id::
+   Pass the `-m` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]),
+   so that the Message-ID header is added to the commit message.
+   The `am.messageid` configuration variable can be used to specify
+   the default behaviour.
+
+--no-message-id::
+   Do not add the Message-ID header to the commit message.
+   `no-message-id` is useful to override `am.messageid`.
+
 -q::
 --quiet::
Be quiet. Only print error messages.
diff --git a/git-am.sh b/git-am.sh
index ee61a77..c92632f 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -17,6 +17,7 @@ s,signoff   add a Signed-off-by line to the commit message
 u,utf8  recode into utf8 (default)
 k,keep  pass -k flag to git-mailinfo
 keep-non-patch  pass -b flag to git-mailinfo
+m,message-idpass -m flag to git-mailinfo
 keep-cr pass --keep-cr flag to git-mailsplit for mbox format
 no-keep-cr  do not pass --keep-cr flag to git-mailsplit independent of 
am.keepcr
 c,scissors  strip everything before a scissors line
@@ -371,13 +372,18 @@ split_patches () {
 prec=4
 dotest="$GIT_DIR/rebase-apply"
 sign= utf8=t keep= keepcr= skip= interactive= resolved= rebasing= abort=
-resolvemsg= resume= scissors= no_inbody_headers=
+messageid= resolvemsg= resume= scissors= no_inbody_headers=
 git_apply_opt=
 committer_date_is_author_date=
 ignore_date=
 allow_rerere_autoupdate=
 gpg_sign_opt=
 
+if test "$(git config --bool --get am.messageid)" = true
+then
+messageid=t
+fi
+
 if test "$(git config --bool --get am.keepcr)" = true
 then
 keepcr=t
@@ -400,6 +406,10 @@ it will be removed. Please do not use it anymore."
utf8=t ;; # this is now default
--no-utf8)
utf8= ;;
+   -m|--message-id)
+   messageid=t ;;
+   --no-message-id)
+   messageid=f ;;
-k|--keep)
keep=t ;;
--keep-non-patch)
@@ -567,6 +577,7 @@ Use \"git am --abort\" to remove it.")"
echo "$sign" >"$dotest/sign"
echo "$utf8" >"$dotest/utf8"
echo "$keep" >"$dotest/keep"
+   echo "$messageid" >"$dotest/messageid"
echo "$scissors" >"$dotest/scissors"
echo "$no_inbody_headers" >"$dotest/no_inbody_headers"
echo "$GIT_QUIET" >"$dotest/quiet"
@@ -621,6 +632,12 @@ b)
 *)
keep= ;;
 esac
+case "$(cat "$dotest/messageid")" in
+t)
+   messageid=-m ;;
+f)
+   messageid= ;;
+esac
 case "$(cat "$dotest/scissors")" in
 t)
scissors=--scissors ;;
@@ -692,7 +709,7 @@ do
get_author_ident_from_commit "$commit" 
>"$dotest/author-script"
git diff-tree --root --binary --full-index "$commit" 
>"$dotest/patch"
else
-   git mailinfo $keep $no_inbody_headers $scissors $utf8 
"$dotest/msg" "$dotest/patch" \
+   git mailinfo $keep $no_inbody_headers $messageid 
$scissors $utf8 "$dotest/msg" "$dotest/patch" \
<"$dotest/$msgnum" >"$dotest/info" ||
stop_here $this
 
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 5edb79a..306e6f3 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -85,6 +85,7 @@ test_expect_success setup '
 
git format-patch --stdout first >patch1 &&
{
+   echo "Message-Id: 
<1226501681-24923-1-git-send-email-...@mnsspb.ru>" &&
echo "X-Fake-Field: Line One" &&
echo "X-Fake-Field: Line Two" &&
echo "X-Fake-Field: Line Three" &&
@@ -536,4 +537,26 @@ test_expect_success 'am empty-file does not infloop' '
test_i18ncmp expected actual
 '
 
+test_expect_success 'am --message-id really adds the message id' '
+   rm -fr .git/rebase-apply &&
+   git reset --hard &&
+   git checkout HEAD^ &&
+   git am --message-id 

Re: [PATCH 0/2] git-am: add --message-id/--no-message-id options

[Paolo Bonzini]

On 25/11/2014 17:27, Christian Couder wrote:
>> > From: Paolo Bonzini 
>> >
>> > This series adds a --message-id option to git-mailinfo and git-am.
>> > git-am also gets an am.messageid configuration key to set the default,
>> > and a --no-message-id option to override the configuration key.
>> > (I'm not sure of the usefulness of a mailinfo.messageid option, so
>> > I left it out; this follows the example of -k instead of --scissors).
>> >
>> > This option can be useful in order to associate commit messages with
>> > mailing list discussions.
>> >
>> > If both --message-id and -s are specified, the Signed-off-by goes
>> > last.  This is coming out more or less naturally out of the git-am
>> > implementation, but is also tested in t4150-am.sh.
> Did you have a look at git interpret-trailers currently in master?

Hmm, now I have.

As far as I understand, all the git-am hooks are called on the commit
rather than the incoming email: all headers are lost by the time
git-mailinfo exits, including the Message-Id.  And you cannot call any
hook before git-mailinfo because git-mailinfo is where the
Content-Transfer-Encoding is processed.

How would you integrate git-interpret-trailers in git-mailinfo?

Paolo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] git-am: add --message-id/--no-message-id options

[Paolo Bonzini]

On 25/11/2014 19:33, Junio C Hamano wrote:
>> > If both --message-id and -s are specified, the Signed-off-by goes
>> > last.  This is coming out more or less naturally out of the git-am
>> > implementation, but is also tested in t4150-am.sh.
> Nice.  So if you apply a message whose last sign-off is yourself
> with both of these options, what would we see?
> 
> 1. S-o-b: you and then M-id: and then another S-o-b: you?
> 2. M-id: and then S-o-b: you?
> 3. S-o-b: you and then M-id:?
> 
> I do not offhand know which one of the above possibilities to favor
> more over others myself.  Just asking to find out more about the
> thinking behind the design.

You currently get (1), which is arguably the most precise but definitely
the ugliest.

In this case (posting as maintainer), I would probably not use "git am
--message-id"; instead I would use an alias to add the Message-Id (with
git interpret-trailers!) after posting to the mailing list, resulting in
either (2) or (3).

I think (but I am not sure) that git-am could use a hook to rewrite (1)
into (2) or (3).

Paolo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] git-am: add --message-id/--no-message-id

[Paolo Bonzini]

On 26/11/2014 00:34, Junio C Hamano wrote:
> ... makes the result look questionable.  The variable is initialized
> to empty; when it is written out to $dotest/messageid and later read
> back here, that empty value is not covered by this case statement.
> 
> Perhaps clearing messageid= upon seeing "--no-message-id" and using
> "'t' or empty" makes the code a bit easier to follow?  I dunno.

Possibly.  The other side is that it would be handled differently than
scissors and keep.  Changing everything is possible but would break
continuing a "git am" operation across an update, so I chose consistency.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] git-am: add --message-id/--no-message-id options

[Paolo Bonzini]

On 25/11/2014 22:21, Christian Couder wrote:
> On Tue, Nov 25, 2014 at 6:01 PM, Paolo Bonzini  wrote:
>>
>>
>> On 25/11/2014 17:27, Christian Couder wrote:
>>>>> From: Paolo Bonzini 
>>>>>
>>>>> This series adds a --message-id option to git-mailinfo and git-am.
>>>>> git-am also gets an am.messageid configuration key to set the default,
>>>>> and a --no-message-id option to override the configuration key.
>>>>> (I'm not sure of the usefulness of a mailinfo.messageid option, so
>>>>> I left it out; this follows the example of -k instead of --scissors).
>>>>>
>>>>> This option can be useful in order to associate commit messages with
>>>>> mailing list discussions.
>>>>>
>>>>> If both --message-id and -s are specified, the Signed-off-by goes
>>>>> last.  This is coming out more or less naturally out of the git-am
>>>>> implementation, but is also tested in t4150-am.sh.
>>> Did you have a look at git interpret-trailers currently in master?
>>
>> Hmm, now I have.
>>
>> As far as I understand, all the git-am hooks are called on the commit
>> rather than the incoming email: all headers are lost by the time
>> git-mailinfo exits, including the Message-Id.  And you cannot call any
>> hook before git-mailinfo because git-mailinfo is where the
>> Content-Transfer-Encoding is processed.
>>
>> How would you integrate git-interpret-trailers in git-mailinfo?
> 
> I don't know exactly, but people may want to add trailers when they
> run git-am, see:
> 
> http://thread.gmane.org/gmane.comp.version-control.git/251412/
> 
> and we decided that it was better to let something like git
> interpret-trailers decide how they should be handled.
> 
> Maybe if git-interpret-trailers could be called from git-mailinfo with
> some arguments coming from git-am, it could be configured with
> something like:
> 
> git config trailer.Message-Id.command 'perl -ne '\''print $1 if
> m/^Message-Id: (.*)$/'\'' $ARG'
> 
> So "git am --trailer 'Message-Id: msg-file' msg-file" would call "git
> mailinfo ..." that would call "git interpret-trailers --trailer
> 'Message-Id: msg-file'" that would call "perl -ne 'print $1 if
> m/^Message-Id: (.*)$/' msg-file" and the output of this command, let's
> call it $id, would be put into a "Message-Id: $id" trailer in the
> commit message.

I think overloading trailer.Message-Id.command is not a good idea,
because it would prevent using "git interpret-trailers" to add a message
id manually ("git interpret-trailers --trailer message-id=''").

Another possibility could be to add a third output file to git-mailinfo,
including all the headers.  Then a hook could be called with the headers
and commit message.

The question is: what would it be used for?  There aren't that many mail
headers, and most of them (From, Subject, Date) are recorded in the
commit anyway.  One idea could be to record who was a recipient of the
original message, even if no Cc line was added explicitly.  In most
projects, Cc is often added randomly, but I guess that's a valid
usecase.  I can certainly code the above hook instead of this approach
if Junio thinks it's better.

In the meanwhile, I have thought of a couple additions to "git
interpret-trailers" and I can submit patches for them.

Paolo

> This way there is nothing specific to Message-Id in the code and
> people can decide using other trailer.Message-Id.* config variables
> exactly where the Message-Id trailer would be in the commit message.
> 
> Best,
> Christian.
> 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] request-pull: fix expected format in tests

[Paolo Bonzini]

From: Paolo Bonzini 

"tag foo" in requests has been replaced with "tags/foo" (commit f032d66,
request-pull: do not emit "tag" before the tagname, 2011-12-19).  Adjust
the parsing script to match; since the new format does not have spaces,
doing nothing is fine.

Signed-off-by: Paolo Bonzini 
---
 t/t5150-request-pull.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index 82c33b8..8b19279 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -67,11 +67,10 @@ test_expect_success 'setup: two scripts for reading pull 
requests' '
 
cat <<-\EOT >read-request.sed &&
#!/bin/sed -nf
-   # Note that a request could ask for "tag $tagname"
+   # Note that a request could ask for "tags/$tagname"
/ in the git repository at:$/!d
n
/^$/ n
-   s/ tag \([^ ]*\)$/ tag--\1/
s/^[]*\(.*\) \([^ ]*\)/please pull\
\1\
\2/p
-- 
2.3.0


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] request-pull: find matching tag or branch name on remote side

[Paolo Bonzini]

From: Paolo Bonzini 

If the third argument is not passed to "git request-pull", the
find_matching_ref script will look for HEAD in the remote side
which does not work.  Instead, default to the ref names found
via "git show-ref" or "git tag".

Signed-off-by: Paolo Bonzini 
---
 git-request-pull.sh | 14 ++
 t/t5150-request-pull.sh |  2 +-
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index a507006..fcbe383 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -54,8 +54,6 @@ fi
 local=${3%:*}
 local=${local:-HEAD}
 remote=${3#*:}
-pretty_remote=${remote#refs/}
-pretty_remote=${pretty_remote#heads/}
 head=$(git symbolic-ref -q "$local")
 head=${head:-$(git show-ref --heads --tags "$local" | cut -d' ' -f2)}
 head=${head:-$(git tag --points-at "$local" | sed 's,^,refs/tags/,')}
@@ -64,6 +62,14 @@ head=${head:-$(git rev-parse --quiet --verify "$local")}
 # None of the above? Bad.
 test -z "$head" && die "fatal: Not a valid revision: $local"
 
+#
+# If $3 was not there, the remote name should be the same
+# as the locally detected name
+#
+remote=${remote:-$head}
+pretty_remote=${remote#refs/}
+pretty_remote=${pretty_remote#heads/}
+
 # This also verifies that the resulting head is unique:
 # "git show-ref" could have shown multiple matching refs..
 headrev=$(git rev-parse --verify --quiet "$head"^0)
@@ -111,12 +117,12 @@ find_matching_ref='
}
 '
 
-ref=$(git ls-remote "$url" | @@PERL@@ -e "$find_matching_ref" 
"${remote:-HEAD}" "$headrev")
+ref=$(git ls-remote "$url" | @@PERL@@ -e "$find_matching_ref" "$remote" 
"$headrev")
 
 if test -z "$ref"
 then
echo "warn: No match for commit $headrev found at $url" >&2
-   echo "warn: Are you sure you pushed '${remote:-HEAD}' there?" >&2
+   echo "warn: Are you sure you pushed '$remote' there?" >&2
status=1
 fi
 
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index 8b19279..11ba8ff 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -178,7 +178,7 @@ test_expect_success 'request asks HEAD to be pulled' '
read repository &&
read branch
} http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] request-pull: do something if $3 is passed

[Paolo Bonzini]

From: Paolo Bonzini 

After updating to git 2.3.0, "git request-pull" is stubbornly complaining
that I lack a matching tag on the remote side unless I pass the third
argument.  But I did prepare and push a signed tag.

This looks like a bug to me; when $3 is not passed git will try to use
"HEAD" as the default but it cannot be resolved to a tag, neither locally
(patch 2) nor remotely (patch 3).

Patch 1 is a simple testcase fix.

Paolo

Paolo Bonzini (3):
  request-pull: fix expected format in tests
  request-pull: use "git tag --points-at" to detect local tags
  request-pull: find matching tag or branch name on remote side

 git-request-pull.sh | 15 +++
 t/t5150-request-pull.sh |  5 ++---
 2 files changed, 13 insertions(+), 7 deletions(-)

-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] request-pull: use "git tag --points-at" to detect local tags

[Paolo Bonzini]

From: Paolo Bonzini 

If the third argument is not passed, "git show-ref --tags HEAD" will
never return anything and git-request-pull will never detect a tag
name.

Instead, "git tag --points-at" can find it.  Use it if "git show-ref"
fails.

Signed-off-by: Paolo Bonzini 
---
 git-request-pull.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index d5500fd..a507006 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -58,6 +58,7 @@ pretty_remote=${remote#refs/}
 pretty_remote=${pretty_remote#heads/}
 head=$(git symbolic-ref -q "$local")
 head=${head:-$(git show-ref --heads --tags "$local" | cut -d' ' -f2)}
+head=${head:-$(git tag --points-at "$local" | sed 's,^,refs/tags/,')}
 head=${head:-$(git rev-parse --quiet --verify "$local")}
 
 # None of the above? Bad.
-- 
2.3.0


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Paolo Bonzini]

On 16/02/2015 20:47, Junio C Hamano wrote:
> Paolo Bonzini  writes:
> 
>> From: Paolo Bonzini 
>>
>> After updating to git 2.3.0, "git request-pull" is stubbornly complaining
>> that I lack a matching tag on the remote side unless I pass the third
>> argument.  But I did prepare and push a signed tag.
> 
> A few questions.
> 
>  - what old version did you update from?  I think the "correct
>over-eager dwimming" change was from v2.0 days.

I upgraded from 1.9.  My workflow is to make a signed tag, push it and
do "git request-pull origin/master ".

My branches have a different name locally vs. remotely (e.g.
"kvm-master" and "kvm-next" locally vs. refs/heads/master and
refs/heads/next remotely) exactly to avoid overeager matching in
git-request-pull.  I only ever want to request pulls based on signed tags.

>  - what exactly do you mean by "stubbornly complain"?  I think we
>say something about HEAD not matching the HEAD over there, which
>I think is bogus (we should instead say things about the branch
>you are on and the branch over there with the same name) and is
>worth fixing.

I tried both "git checkout kvm-next" and "git checkout tags/for-linus",
and it still complains.

What you refer to is, I think, fixed by patch 3.  The find_matching_ref
script does not work if its first argument is HEAD.  So patch 3 is
probably an improvement anyway for the "matching branch name" case, even
if my usecase involves tags rather than branches.

> An earlier 024d34cb (request-pull: more strictly match local/remote
> branches, 2014-01-22) deliberately disabled over-eager DWIMming when
> the $3-rd argument _is_ given.

I agree with that change.

> One part of me feel that not giving the $3-rd argument should behave
> the same way as if you gave the name of the current branch as the
> $3-rd argument.

This works well for workflows where you do pull requests based on
branches.  However Linus strongly encourages using signed tags.

I certainly can adjust my workflow for this.  For example I can add
something like this to my .gitconfig

[request-pull]
dwim = tags/for-linus

and add an alias that uses "git config request-pull.dwim" as the third
argument (other projects I work on obviously use different tag names :).

While similar, the two patches are different:

1) The usage of "git show-ref --heads --tags" looked like a feeble
attempt at DWIMming tags.  But I can see how that is supposed to work
only if $3 is specified.  Adding a usage of "git tag --points-at" would
go against the intentions of 024d34cb.  Perhaps restrict DWIMming to
signed and annotated tags only, through a new option to "git tag"?

2) Patch 3 makes sense independent of patch 2 and, as mentioned above,
it is probably a bugfix anyway.

> On the other hand, I can also understand (not necessarily agree
> with) a view that not giving the $3-rd argument is an explicit
> user's wish to us to DWIM as much as we want.  But again, that
> directly contradicts with the desire of 024d34cb.
> 
> So,... I dunno.

I don't know either.  Based on your answer, it seems like you are
focusing mostly on a branch-based workflow; the two definitely have
different requirements for DWIMming (since you cannot get a tag name via
"git symbolic-ref" for example).  On the other hand most of the
un-DWIMming changes were done by Linus who works a lot with (other
people's) signed tags...

Paolo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Paolo Bonzini]

On 17/02/2015 20:57, Junio C Hamano wrote:
> Sorry, I was asking what you mean by "complains" (i.e. the exact
> error message).  I was and am guessing it is something like this: 
> 
> warn: No match for commit 3188ab3... found at 
> warn: Are you sure you pushed 'HEAD' there?

Yes, it is.

> Asking to pull 'HEAD' may be often a wrong thing to do, and I
> wouldn't mind if this sequence:
> 
>   git checkout kvm-next
> git request-pull origin/master 
> 
> behaved the same way as
> 
> git request-pull origin/master  kvm-next

FWIW, that would always be wrong for my scenario.

> But I do not know if the implicit HEAD should DWIM locally to this:
> 
> git request-pull origin/master  for-linus

I guess only Linus could answer that, since he wrote 024d34cb0 and he
knows the intent better than anyone else.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Paolo Bonzini]

On 17/02/2015 21:42, Linus Torvalds wrote:
>   "when $3 is not passed git will try to use "HEAD" as the default but
> it cannot be resolved to a tag, neither locally (patch 2) nor remotely
> (patch 3)"
> 
> which makes absolutely no sense.

Indeed, that's why I wrote patches even though I did find the patches
that you wrote for 2.0.

Without $3, git tries to do things that make no sense like "git show-ref
--heads --tags HEAD"; or that make little sense when requesting a pull,
like looking for HEAD in the output of "git ls-remote".  But from the
release notes of 2.0 it looks like it's intended and the script is just
taking shortcuts.

> HEAD is not a tag. Never has been, never will be. If you want me to
> pull a tag, then you damn well should say what tag you want, not just
> randomly say HEAD.

Ok, in 1.9.x I used to not say anything; if the new workflow is to
always specify a tag, that's okay.

> So what is it you want to do? At no point is "HEAD should resolve as a
> tag" sensible.

I wanted git to find the matching tag on the remote side when I use "git
request-pull origin/master URL" with no third parameter, since I never
request pulls except with a single signed tag.  But I'll adjust my aliases.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Paolo Bonzini]

> Looking for HEAD in "git ls-remote"? Perfectly sensible:
> 
> [torvalds@i7 linux]$ git ls-remote origin | grep HEAD
> cc4f9c2a91b7be7b3590bb1cbe8148873556aa3f HEAD
> 
> that's the default thing when you don't specify any particular branch or tag.

Sure.  But if I got a pull request saying "please pull
git://example.org/foo.git HEAD" I would think that the sender
messed up the pull request.  So *in the context of git-request-pull*
${remote:-HEAD} makes little sense to me.

But hey, you said it's me who makes no sense.  Maybe I really don't.

> The thing is, HEAD works. Not for you, because you don't use HEAD. But
> because you don't use HEAD, you shouldn't use the default.

Oki.  Will adjust my scripts.  Junio, you may still want to apply patch
1 if only for documentation purposes (the "tag foo" functionality is
unused in the rest of the test).

Paolo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html