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

STALE3320d

Revision v1 of 4 in this series.

10 messages, 4 authors, 2017-07-12 · open the first message on its own page

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

From: Paolo Bonzini <hidden>
Date: 2017-07-12 13:46:57

From: Paolo Bonzini <pbonzini@redhat.com>

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 1/3] trailers: create struct trailer_opts

From: Paolo Bonzini <hidden>
Date: 2017-07-12 13:46:59

From: Paolo Bonzini <pbonzini@redhat.com>

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 <pbonzini@redhat.com>
---
 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 2/3] trailers: export action enums and corresponding lookup functions

From: Paolo Bonzini <hidden>
Date: 2017-07-12 13:47:01

From: Paolo Bonzini <pbonzini@redhat.com>

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 <pbonzini@redhat.com>
---
 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 <chriscool@tuxfamily.org>
  */
 
-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_conf_info.if_missing, value) < 0)
 				warning(_("unknown value '%s' for key '%s'"),
 					value, conf_key);
 		} else if (!strcmp(trailer_item, "separators")) {
@@ -533,15 +532,15 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 		conf->command = xstrdup(value);
 		break;
 	case TRAILER_WHERE:
-		if (set_where(conf, value))
+		if (set_where(&conf->where, value))
 			warning(_("unknown value '%s' for key '%s'"), value, conf_key);
 		break;
 	case TRAILER_IF_EXISTS:
-		if (set_if_exists(conf, value))
+		if (set_if_exists(&conf->if_exists, value))
 			warning(_("unknown value '%s' for key '%s'"), value, conf_key);
 		break;
 	case TRAILER_IF_MISSING:
-		if (set_if_missing(conf, value))
+		if (set_if_missing(&conf->if_missing, value))
 			warning(_("unknown value '%s' for key '%s'"), value, conf_key);
 		break;
 	default:
diff --git a/trailer.h b/trailer.h
index e90ba1270..f306bf059 100644
--- a/trailer.h
+++ b/trailer.h
@@ -1,11 +1,33 @@
 #ifndef TRAILER_H
 #define TRAILER_H
 
+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 trailer_opts {
 	int in_place;
 	int trim_empty;
 };
 
+int set_where(enum action_where *item, const char *value);
+int set_if_exists(enum action_if_exists *item, const char *value);
+int set_if_missing(enum action_if_missing *item, const char *value);
+
 struct trailer_info {
 	/*
 	 * True if there is a blank line before the location pointed to by
-- 
2.13.0

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

From: Paolo Bonzini <hidden>
Date: 2017-07-12 13:47:05

From: Paolo Bonzini <pbonzini@redhat.com>

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 <pbonzini@redhat.com>
---
 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 <placement>::
+	Specify where all new trailers will be added.  This option
+	overrides all configuration variables.
+
+--if-exists <action>::
+	Specify what action will be performed when there is already at
+	least one trailer with the same <token> in the message.  This
+	option applies to all '--trailer' options and overrides all
+	configuration variables.
+
+--if-missing <action>::
+	Specify what action will be performed when there is no other
+	trailer with the same <token> 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 after --trailer "ack: Peff" \
+		complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'using "where = after"' '
 	git config trailer.ack.where "after" &&
 	cat complex_message_body >expected &&
@@ -947,6 +962,23 @@ test_expect_success 'using "ifExists = add" with "where = after"' '
 	test_cmp expected actual
 '
 
+test_expect_success 'overriding configuration with "--if-exists replace"' '
+	git config trailer.fix.key "Fixes: " &&
+	git config trailer.fix.ifExists "add" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Acked-by= Z
+		Reviewed-by:
+		Signed-off-by: Z
+		Fixes: 22
+	EOF
+	git interpret-trailers --if-exists replace --trailer "review:" \
+		--trailer "fix=53" --trailer "fix=22" --trailer "bug: 42" \
+		<complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'using "ifExists = replace"' '
 	git config trailer.fix.key "Fixes: " &&
 	git config trailer.fix.ifExists "replace" &&
@@ -1026,6 +1058,25 @@ test_expect_success 'the default is "ifMissing = add"' '
 	test_cmp expected actual
 '
 
+test_expect_success 'overriding configuration with "--if-missing doNothing"' '
+	git config trailer.ifmissing "add" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Junio
+		Acked-by= Peff
+		Reviewed-by:
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers --if-missing doNothing \
+		--trailer "review:" --trailer "fix=53" \
+		--trailer "cc=Linus" --trailer "ack: Junio" \
+		--trailer "fix=22" --trailer "bug: 42" --trailer "ack: Peff" \
+		<complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'when default "ifMissing" is "doNothing"' '
 	git config trailer.ifmissing "doNothing" &&
 	cat complex_message_body >expected &&
diff --git a/trailer.c b/trailer.c
index 558c52431..78e88f98d 100644
--- a/trailer.c
+++ b/trailer.c
@@ -549,14 +549,36 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 	return 0;
 }
 
-static void ensure_configured(void)
+static void apply_config_overrides(struct conf_info *conf, struct trailer_opts *opts)
 {
+	if (!opts)
+		return;
+	if (opts->where != WHERE_DEFAULT)
+		conf->where = opts->where;
+	if (opts->if_exists != EXISTS_DEFAULT)
+		conf->if_exists = opts->if_exists;
+	if (opts->if_missing != MISSING_DEFAULT)
+		conf->if_missing = opts->if_missing;
+}
+
+static void ensure_configured(struct trailer_opts *opts)
+{
+	struct arg_item *item;
+	struct list_head *pos;
+
 	if (configured)
 		return;
 
 	/* Default config must be setup first */
 	git_config(git_trailer_default_config, NULL);
+	apply_config_overrides(&default_conf_info, opts);
+
 	git_config(git_trailer_config, NULL);
+	list_for_each(pos, &conf_head) {
+		item = list_entry(pos, struct arg_item, list);
+		apply_config_overrides(&item->conf, opts);
+	}
+
 	configured = 1;
 }
 
@@ -977,7 +999,7 @@ void process_trailers(const char *file, struct trailer_opts *opts,
 	int trailer_end;
 	FILE *outfile = stdout;
 
-	ensure_configured();
+	ensure_configured(opts);
 
 	read_input_file(&sb, file);
 
@@ -1013,7 +1035,7 @@ void trailer_info_get(struct trailer_info *info, const char *str)
 	size_t nr = 0, alloc = 0;
 	char **last = NULL;
 
-	ensure_configured();
+	ensure_configured(NULL);
 
 	patch_start = find_patch_start(str);
 	trailer_end = find_trailer_end(str, patch_start);
diff --git a/trailer.h b/trailer.h
index f306bf059..634643c9d 100644
--- a/trailer.h
+++ b/trailer.h
@@ -2,12 +2,14 @@
 #define TRAILER_H
 
 enum action_where {
+	WHERE_DEFAULT,
 	WHERE_END,
 	WHERE_AFTER,
 	WHERE_BEFORE,
 	WHERE_START
 };
 enum action_if_exists {
+	EXISTS_DEFAULT,
 	EXISTS_ADD_IF_DIFFERENT_NEIGHBOR,
 	EXISTS_ADD_IF_DIFFERENT,
 	EXISTS_ADD,
@@ -15,6 +17,7 @@ enum action_if_exists {
 	EXISTS_DO_NOTHING
 };
 enum action_if_missing {
+	MISSING_DEFAULT,
 	MISSING_ADD,
 	MISSING_DO_NOTHING
 };
@@ -22,6 +25,9 @@ enum action_if_missing {
 struct trailer_opts {
 	int in_place;
 	int trim_empty;
+	enum action_where where;
+	enum action_if_exists if_exists;
+	enum action_if_missing if_missing;
 };
 
 int set_where(enum action_where *item, const char *value);
-- 
2.13.0

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

From: Christian Couder <hidden>
Date: 2017-07-12 14:47:16

On Wed, Jul 12, 2017 at 3:46 PM, Paolo Bonzini [off-list ref] 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.

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

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

From: Paolo Bonzini <pbonzini@redhat.com>
Date: 2017-07-12 15:02:44

On 12/07/2017 16:47, Christian Couder wrote:
On Wed, Jul 12, 2017 at 3:46 PM, Paolo Bonzini [off-list ref] wrote:
quoted
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.<token>.where", or if should just override the former.
I think it should override both, otherwise you have different behavior
depending on whether trailer.<token>.where is defined or not.

Paolo

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

From: Jonathan Tan <hidden>
Date: 2017-07-12 21:10:36

On Wed, 12 Jul 2017 15:46:46 +0200
Paolo Bonzini [off-list ref] wrote:
+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);
+}
This means that we have the following:

    $ cat message
    Hello

    a: a

    $ ./git interpret-trailers --trailer a=b message
    Hello

    a: a
    a: b

    $ ./git interpret-trailers --where start --no-where --trailer a=b message
    Hello

    a: b
    a: a

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.

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

From: Jonathan Tan <hidden>
Date: 2017-07-12 21:14:00

On Wed, 12 Jul 2017 15:46:45 +0200
Paolo Bonzini [off-list ref] wrote:
-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,
+};
I'm not sure if Git is ready to commit to using designated initializers,
so maybe avoid those for now. More information here [1].

[1] https://public-inbox.org/git/20170710070342.txmlwwq6gvjkwtw7@sigill.intra.peff.net/

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

From: Jonathan Tan <hidden>
Date: 2017-07-12 21:19:13

On Wed, 12 Jul 2017 15:46:44 +0200
Paolo Bonzini [off-list ref] wrote:
-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)
This can be "const struct trailer_opts *", I think. (Same for the other
functions in this patch.)
+struct trailer_opts {
+	int in_place;
+	int trim_empty;
+};
I was going to suggest that you make these "unsigned in_place : 1" etc.,
but I think OPT_BOOL doesn't support those. (And OPT_BIT is probably not
worth it.)

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

From: Paolo Bonzini <hidden>
Date: 2017-07-12 21:34:35

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help