From: Jeff King <hidden> Date: 2017-08-09 12:21:54
Parsing trailers out of a commit message is _mostly_ easy, but there
area a lot of funny corner cases (e.g., heuristics for how many
non-trailers must be present before a final paragraph isn't a trailer
block anymore). The code in trailer.c already knows about these corner
cases, but there's no way to access it from the command line.
This series teaches interpret-trailers to parse and output just the
trailers. So now you can do:
$ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
git interpret-trailers --parse
Signed-off-by: Hartmut Henkel [off-list ref]
Helped-by: Stefan Beller [off-list ref]
Signed-off-by: Ralf Thielow [off-list ref]
Acked-by: Matthias Rüster [off-list ref]
I considered a few different approaches before deciding on
what's here:
1. The output format is actually the normal "key: value" trailers. I
considered something more structured like JSON. But the "key:
value" format is quite easy to parse, once it has been normalized
(finding the trailers, unfolding whitespace continuation, etc).
2. This series introduces several orthogonal options which can be used
together to achieve my goal, when there could just be a "parse"
mode. Since interpret-trailers is plumbing, I reasoned that the
individual options might still be useful apart from each other (for
instance, you could re-normalize existing trailers while writing
your new ones from a commit hook). I did add a "--parse" for
convenience and to help point users in the right direction.
For the same reason (and so we could build on other orthogonal
features like --in-place and --trim-empty), I decided against
having a separate command like "git parse-trailers".
[1/5]: trailer: put process_trailers() options into a struct
[2/5]: interpret-trailers: add an option to show only the trailers
[3/5]: interpret-trailers: add an option to show only existing trailers
[4/5]: interpret-trailers: add an option to normalize output
[5/5]: interpret-trailers: add --parse convenience option
Documentation/git-interpret-trailers.txt | 17 ++++++++
builtin/interpret-trailers.c | 34 ++++++++++++---
t/t7513-interpret-trailers.sh | 73 ++++++++++++++++++++++++++++++++
trailer.c | 65 ++++++++++++++++++++++------
trailer.h | 12 +++++-
5 files changed, 180 insertions(+), 21 deletions(-)
-Peff
From: Jeff King <hidden> Date: 2017-08-09 12:22:38
We already have two options and are about to add a few more.
To avoid having a huge number of boolean arguments, let's
convert to an options struct which can be passed in.
Signed-off-by: Jeff King <redacted>
---
builtin/interpret-trailers.c | 13 ++++++-------
trailer.c | 9 +++++----
trailer.h | 9 ++++++++-
3 files changed, 19 insertions(+), 12 deletions(-)
@@ -980,7 +981,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct strread_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 +991,14 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct strprocess_trailers_lists(&head,&arg_head);-print_all(outfile,&head,trim_empty);+print_all(outfile,&head,opts->trim_empty);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);
From: Jeff King <hidden> Date: 2017-08-09 12:24:09
In theory it's easy for any reader who wants to parse
trailers to do so. But there are a lot of subtle corner
cases around what counts as a trailer, when the trailer
block begins and ends, etc. Since interpret-trailers already
has our parsing logic, let's let callers ask it to just
output the trailers.
They still have to parse the "key: value" lines, but at
least they can ignore all of the other corner cases.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 3 +++
builtin/interpret-trailers.c | 1 +
t/t7513-interpret-trailers.sh | 37 ++++++++++++++++++++++++++++++++
trailer.c | 19 ++++++++++------
trailer.h | 1 +
5 files changed, 54 insertions(+), 7 deletions(-)
@@ -80,6 +80,9 @@ OPTIONS trailer to the input messages. See the description of this command.+--only-trailers::+ Output only the trailers, not any other parts of the input.+ CONFIGURATION VARIABLES -----------------------
@@ -24,6 +24,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)structoptionoptions[]={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_BOOL(0,"only-trailers",&opts.only_trailers,N_("output only the trailers")),OPT_STRING_LIST(0,"trailer",&trailers,N_("trailer"),N_("trailer(s) to add")),OPT_END()
@@ -897,9 +899,10 @@ static int process_input_file(FILE *outfile,trailer_info_get(&info,str);/* Print lines before the trailers as is */-fwrite(str,1,info.trailer_start-str,outfile);+if(outfile)+fwrite(str,1,info.trailer_start-str,outfile);-if(!info.blank_line_before_trailer)+if(outfile&&!info.blank_line_before_trailer)fprintf(outfile,"\n");for(i=0;i<info.trailer_nr;i++){
@@ -985,18 +988,20 @@ void process_trailers(const char *file, struct process_trailer_options *opts,outfile=create_in_place_tempfile(file);/* Print the lines before the trailers */-trailer_end=process_input_file(outfile,sb.buf,&head);+trailer_end=process_input_file(opts->only_trailers?NULL:outfile,+sb.buf,&head);process_command_line_args(&arg_head,trailers);process_trailers_lists(&head,&arg_head);-print_all(outfile,&head,opts->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(!opts->only_trailers)+fwrite(sb.buf+trailer_end,1,sb.len-trailer_end,outfile);if(opts->in_place)if(rename_tempfile(&trailers_tempfile,file))
From: Stefan Beller <hidden> Date: 2017-08-09 17:52:17
On Wed, Aug 9, 2017 at 5:24 AM, Jeff King [off-list ref] wrote:
In theory it's easy for any reader who wants to parse
trailers to do so. But there are a lot of subtle corner
cases around what counts as a trailer, when the trailer
block begins and ends, etc. Since interpret-trailers already
has our parsing logic, let's let callers ask it to just
output the trailers.
They still have to parse the "key: value" lines, but at
least they can ignore all of the other corner cases.
Signed-off-by: Jeff King <redacted>
---
...
+test_expect_success 'only-trailers omits non-trailer in middle of block' '
+ cat >expected <<-\EOF &&
+ Signed-off-by: nobody <nobody@nowhere>
+ Signed-off-by: somebody <somebody@somewhere>
+ sign: A U Thor [off-list ref]
+ EOF
+ git interpret-trailers --only-trailers >actual <<-\EOF &&
+ subject
+
+ it is important that the trailers below are signed-off-by
+ so that they meet the "25% trailers Git knows about" heuristic
+
+ Signed-off-by: nobody <nobody@nowhere>
+ this is not a trailer
Please see 60ef86a162 (trailer: support values folded to multiple lines,
2016-10-21), maybe we also want to test for
VeryLongTrailerKey: long text with spaces and breaking
the line.
For those parsing the trailer do we unbreak the line?
Such that one line equals one trailer? or is the user of the parsed
output expected to take care of this themselves?
From: Stefan Beller <hidden> Date: 2017-08-09 17:55:16
On Wed, Aug 9, 2017 at 10:52 AM, Stefan Beller [off-list ref] wrote:
On Wed, Aug 9, 2017 at 5:24 AM, Jeff King [off-list ref] wrote:
quoted
In theory it's easy for any reader who wants to parse
trailers to do so. But there are a lot of subtle corner
cases around what counts as a trailer, when the trailer
block begins and ends, etc. Since interpret-trailers already
has our parsing logic, let's let callers ask it to just
output the trailers.
They still have to parse the "key: value" lines, but at
least they can ignore all of the other corner cases.
Signed-off-by: Jeff King <redacted>
---
...
quoted
+test_expect_success 'only-trailers omits non-trailer in middle of block' '
+ cat >expected <<-\EOF &&
+ Signed-off-by: nobody <nobody@nowhere>
+ Signed-off-by: somebody <somebody@somewhere>
+ sign: A U Thor [off-list ref]
+ EOF
+ git interpret-trailers --only-trailers >actual <<-\EOF &&
+ subject
+
+ it is important that the trailers below are signed-off-by
+ so that they meet the "25% trailers Git knows about" heuristic
+
+ Signed-off-by: nobody <nobody@nowhere>
+ this is not a trailer
Please see 60ef86a162 (trailer: support values folded to multiple lines,
2016-10-21), maybe we also want to test for
VeryLongTrailerKey: long text with spaces and breaking
the line.
For those parsing the trailer do we unbreak the line?
Such that one line equals one trailer? or is the user of the parsed
output expected to take care of this themselves?\
@@ -897,9 +899,10 @@ static int process_input_file(FILE *outfile, trailer_info_get(&info, str); /* Print lines before the trailers as is */- fwrite(str, 1, info.trailer_start - str, outfile);+ if (outfile)
Any reason why you expect outfile to possibly be NULL?
+ fwrite(str, 1, info.trailer_start - str, outfile);
- if (!info.blank_line_before_trailer)
+ if (outfile && !info.blank_line_before_trailer)
Same comment here.
fprintf(outfile, "\n");
for (i = 0; i < info.trailer_nr; i++) {
This can be const, I think. (Same thing for patch 1.)
OK. We often leave these kinds of option structs as non-const because
they can sometimes grow to carry state between functions (e.g.,
diff_opt). But it's certainly const-able now, so we can let somebody
undo it later if they want.
quoted
@@ -897,9 +899,10 @@ static int process_input_file(FILE *outfile, trailer_info_get(&info, str); /* Print lines before the trailers as is */- fwrite(str, 1, info.trailer_start - str, outfile);+ if (outfile)
Any reason why you expect outfile to possibly be NULL?
quoted
+ fwrite(str, 1, info.trailer_start - str, outfile);
- if (!info.blank_line_before_trailer)
+ if (outfile && !info.blank_line_before_trailer)
Same comment here.
Because of this hunk from later in the file where we pass in NULL:
/* Print the lines before the trailers */
- trailer_end = process_input_file(outfile, sb.buf, &head);
+ trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
+ sb.buf, &head);
-Peff
From: Jeff King <hidden> Date: 2017-08-09 12:24:46
It can be useful to invoke interpret-trailers for the
primary purpose of parsing existing trailers. But in that
case, we don't want to apply existing ifMissing or ifExists
rules from the config. Let's add a special mode where we
avoid applying those rules. Coupled with --only-trailers,
this gives us a reasonable parsing tool.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 5 +++++
builtin/interpret-trailers.c | 7 +++++++
t/t7513-interpret-trailers.sh | 15 +++++++++++++++
trailer.c | 7 ++++---
trailer.h | 1 +
5 files changed, 32 insertions(+), 3 deletions(-)
@@ -83,6 +83,11 @@ OPTIONS --only-trailers:: Output only the trailers, not any other parts of the input.+--only-existing::+ Output only trailers that exist in the input; do not add any+ from the command-line or by following configured `trailer.*`+ rules.+ CONFIGURATION VARIABLES -----------------------
@@ -25,6 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)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_BOOL(0,"only-trailers",&opts.only_trailers,N_("output only the trailers")),+OPT_BOOL(0,"only-existing",&opts.only_existing,N_("output only existing trailers")),OPT_STRING_LIST(0,"trailer",&trailers,N_("trailer"),N_("trailer(s) to add")),OPT_END()
@@ -33,6 +34,12 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)argc=parse_options(argc,argv,prefix,options,git_interpret_trailers_usage,0);+if(opts.only_existing&&trailers.nr)+usage_msg_opt(+_("--trailer with --only-existing does not make sense"),+git_interpret_trailers_usage,+options);+if(argc){inti;for(i=0;i<argc;i++)
From: Stefan Beller <hidden> Date: 2017-08-09 18:18:27
On Wed, Aug 9, 2017 at 5:24 AM, Jeff King [off-list ref] wrote:
It can be useful to invoke interpret-trailers for the
primary purpose of parsing existing trailers. But in that
case, we don't want to apply existing ifMissing or ifExists
rules from the config. Let's add a special mode where we
avoid applying those rules. Coupled with --only-trailers,
this gives us a reasonable parsing tool.
I have the impression that the name is slightly misleading
because 'only' just reduces the set. it does not enhance it.
(Do we have a configuration that says "remove this trailer
anytime"?)
So maybe this is rather worded as 'exact-trailers' ?
From: Jeff King <hidden> Date: 2017-08-10 07:32:46
On Wed, Aug 09, 2017 at 11:18:19AM -0700, Stefan Beller wrote:
On Wed, Aug 9, 2017 at 5:24 AM, Jeff King [off-list ref] wrote:
quoted
It can be useful to invoke interpret-trailers for the
primary purpose of parsing existing trailers. But in that
case, we don't want to apply existing ifMissing or ifExists
rules from the config. Let's add a special mode where we
avoid applying those rules. Coupled with --only-trailers,
this gives us a reasonable parsing tool.
I have the impression that the name is slightly misleading
because 'only' just reduces the set. it does not enhance it.
(Do we have a configuration that says "remove this trailer
anytime"?)
No, I think you can only add trailers via ifExists or ifMissing.
I actually called this --no-config originally, because to me it meant
"do not apply config". But the processing applies also to --trailer
arguments no the command line, which is how I ended up with
--only-existing.
So maybe this is rather worded as 'exact-trailers' ?
I'm not fond of that, as it's vague about which exact trailers we're
talking about. I also thought of something like --verbatim, but I'd
worry that would seem to conflict with --normalize.
I dunno. All of the names seem not quite descriptive enough to me.
-Peff
From: Stefan Beller <hidden> Date: 2017-08-10 17:27:49
On Thu, Aug 10, 2017 at 12:32 AM, Jeff King [off-list ref] wrote:
On Wed, Aug 09, 2017 at 11:18:19AM -0700, Stefan Beller wrote:
quoted
On Wed, Aug 9, 2017 at 5:24 AM, Jeff King [off-list ref] wrote:
quoted
It can be useful to invoke interpret-trailers for the
primary purpose of parsing existing trailers. But in that
case, we don't want to apply existing ifMissing or ifExists
rules from the config. Let's add a special mode where we
avoid applying those rules. Coupled with --only-trailers,
this gives us a reasonable parsing tool.
I have the impression that the name is slightly misleading
because 'only' just reduces the set. it does not enhance it.
(Do we have a configuration that says "remove this trailer
anytime"?)
No, I think you can only add trailers via ifExists or ifMissing.
I actually called this --no-config originally, because to me it meant
"do not apply config". But the processing applies also to --trailer
arguments no the command line, which is how I ended up with
--only-existing.
quoted
So maybe this is rather worded as 'exact-trailers' ?
I'm not fond of that, as it's vague about which exact trailers we're
talking about. I also thought of something like --verbatim, but I'd
worry that would seem to conflict with --normalize.
I dunno. All of the names seem not quite descriptive enough to me.
I meant 'exact' as in 'exactly from the patch/commit, no external
influence such as config', so maybe '--from-patch' or '--from-commit'
(which says the same as --no-config just the other way round.
Having --no- in config options as the standard is a UX disaster
IMHO as then we have to forbid the --no-no-X or reintroduce X
and flip the default)
Maybe --genuine ?
From: Jeff King <hidden> Date: 2017-08-10 17:33:35
On Thu, Aug 10, 2017 at 10:27:19AM -0700, Stefan Beller wrote:
quoted
I'm not fond of that, as it's vague about which exact trailers we're
talking about. I also thought of something like --verbatim, but I'd
worry that would seem to conflict with --normalize.
I dunno. All of the names seem not quite descriptive enough to me.
I meant 'exact' as in 'exactly from the patch/commit, no external
influence such as config', so maybe '--from-patch' or '--from-commit'
(which says the same as --no-config just the other way round.
Having --no- in config options as the standard is a UX disaster
IMHO as then we have to forbid the --no-no-X or reintroduce X
and flip the default)
Yes, that was definitely the other reason I didn't want to call it
"--no-config". :)
It's not always from a patch or commit. The most accurate along those
lines is "--from-input".
Maybe --genuine ?
But in the greater context I think that's vague again; we don't know
which part of the command's operation is "genuine".
Perhaps "--exact-input" hits all of those. Or maybe "--only-input" to
match the other "--only".
I think I like that last one the best. It makes it clear that we are
looking just at the input, and not anything else. Which is exactly what
the feature does.
-Peff
From: Stefan Beller <hidden> Date: 2017-08-10 17:38:52
On Thu, Aug 10, 2017 at 10:33 AM, Jeff King [off-list ref] wrote:
On Thu, Aug 10, 2017 at 10:27:19AM -0700, Stefan Beller wrote:
quoted
quoted
I'm not fond of that, as it's vague about which exact trailers we're
talking about. I also thought of something like --verbatim, but I'd
worry that would seem to conflict with --normalize.
I dunno. All of the names seem not quite descriptive enough to me.
I meant 'exact' as in 'exactly from the patch/commit, no external
influence such as config', so maybe '--from-patch' or '--from-commit'
(which says the same as --no-config just the other way round.
Having --no- in config options as the standard is a UX disaster
IMHO as then we have to forbid the --no-no-X or reintroduce X
and flip the default)
Yes, that was definitely the other reason I didn't want to call it
"--no-config". :)
It's not always from a patch or commit. The most accurate along those
lines is "--from-input".
quoted
Maybe --genuine ?
But in the greater context I think that's vague again; we don't know
which part of the command's operation is "genuine".
The input of course. ;) --genuine-input.
Perhaps "--exact-input" hits all of those. Or maybe "--only-input" to
match the other "--only".
I think I like that last one the best. It makes it clear that we are
looking just at the input, and not anything else. Which is exactly what
the feature does.
This is a bit confusing, but it is correct, since
process_command_line_args() processes both configured trailers and
command-line trailers.
Having said that, it might be worth declaring LIST_HEAD(arg_head) inside
the if block now.
@@ -1312,4 +1312,19 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' 'test_cmpexpectedactual'+test_expect_success'only existing''+cat>expected<<-\EOF&&+existing:existing-value+EOF+gitinterpret-trailers\+--only-trailers--only-existing>actual<<-\EOF&&+mysubject++mybody++existing:existing-value+EOF+test_cmpexpectedactual
Would it be worth asserting that the "sign" trailer is configured here
and would normally appear? Maybe through a grep on the output of "git
config".
I'd much rather re-add it than assert it with grep hackery. Note that
its presence is implied in all of the follow-on tests, too (so I had
sort of assumed that its presence in the --only-trailers test would
imply that it was carried through to the others). We can be more
explicit, though, I guess.
This is a bit confusing, but it is correct, since
process_command_line_args() processes both configured trailers and
command-line trailers.
Yes, it confused me, too. That combination is why "--trailer" is
disallowed with --only-existing (which otherwise could work together). I
didn't think it was worth refactoring for a case that I don't think
anybody would care about.
Having said that, it might be worth declaring LIST_HEAD(arg_head) inside
the if block now.
From: Jeff King <hidden> Date: 2017-08-09 12:25:57
The point of "--only-trailers" is to give a caller an output
that's easy for them to parse. Getting rid of the
non-trailer material helps, but we still may see more
complicated syntax like whitespace continuation. Let's add
an option to normalize the output into one "key: value" line
per trailer.
As a bonus, this could be used even without --only-trailers
to clean up unusual formatting in the incoming data.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 5 +++++
builtin/interpret-trailers.c | 1 +
t/t7513-interpret-trailers.sh | 21 ++++++++++++++++++++
trailer.c | 34 +++++++++++++++++++++++++++++++-
trailer.h | 1 +
5 files changed, 61 insertions(+), 1 deletion(-)
@@ -88,6 +88,11 @@ OPTIONS from the command-line or by following configured `trailer.*` rules.+--normalize::+ Normalize trailer output so that trailers appear exactly one per+ line, with any existing whitespace continuation folded into a+ single line.+ CONFIGURATION VARIABLES -----------------------
@@ -26,6 +26,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)OPT_BOOL(0,"trim-empty",&opts.trim_empty,N_("trim empty trailers")),OPT_BOOL(0,"only-trailers",&opts.only_trailers,N_("output only the trailers")),OPT_BOOL(0,"only-existing",&opts.only_existing,N_("output only existing trailers")),+OPT_BOOL(0,"normalize",&opts.normalize,N_("normalize trailer formatting")),OPT_STRING_LIST(0,"trailer",&trailers,N_("trailer"),N_("trailer(s) to add")),OPT_END()
@@ -1327,4 +1327,25 @@ test_expect_success 'only existing' 'test_cmpexpectedactual'+test_expect_success'normalize''+cat>expected<<-\EOF&&+foo:continuedacrossseverallines+EOF+# pass through tr to make leading and trailing whitespace more obvious+tr_" "<<-\EOF|+mysubject++mybody++foo:_+__continued+___across+____several+_____lines+___+EOF+gitinterpret-trailers--only-trailers--only-existing--normalize>actual&&+test_cmpexpectedactual+'+ test_done
@@ -887,7 +887,35 @@ static int ends_with_blank_line(const char *buf, size_t len)returnis_blank_line(buf+ll);}+staticvoidnormalize_value(structstrbuf*val)+{+structstrbufout=STRBUF_INIT;+size_ti;++strbuf_grow(&out,val->len);+i=0;+while(i<val->len){+charc=val->buf[i++];+if(c=='\n'){+/* Collapse continuation down to a single space. */+while(i<val->len&&isspace(val->buf[i]))+i++;+strbuf_addch(&out,' ');+}else{+strbuf_addch(&out,c);+}+}++/* Empty lines may have left us with whitespace cruft at the edges */+strbuf_trim(&out);++/* output goes back to val as if we modified it in-place */+strbuf_swap(&out,val);+strbuf_release(&out);+}+staticintprocess_input_file(FILE*outfile,+intnormalize,constchar*str,structlist_head*head){
@@ -914,12 +942,16 @@ static int process_input_file(FILE *outfile,if(separator_pos>=1){parse_trailer(&tok,&val,NULL,trailer,separator_pos);+if(normalize)+normalize_value(&val);add_trailer_item(head,strbuf_detach(&tok,NULL),strbuf_detach(&val,NULL));}else{strbuf_addstr(&val,trailer);strbuf_strip_suffix(&val,"\n");+if(normalize)+normalize_value(&val);add_trailer_item(head,NULL,strbuf_detach(&val,NULL));
@@ -989,7 +1021,7 @@ void process_trailers(const char *file, struct process_trailer_options *opts,/* Print the lines before the trailers */trailer_end=process_input_file(opts->only_trailers?NULL:outfile,-sb.buf,&head);+opts->normalize,sb.buf,&head);if(!opts->only_existing){process_command_line_args(&arg_head,trailers);
From: Jeff King <hidden> Date: 2017-08-09 12:26:49
The last few commits have added command line options that
can turn interpret-trailers into a parsing tool. Since
they'd most often be used together, let's provide a
convenient single option for callers to invoke this mode.
This is implemented as a callback rather than a boolean so
that its effect is applied immediately, as if those options
had been specified. Later options can then override them.
E.g.:
git interpret-trailers --parse --no-normalize
would work.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 4 ++++
builtin/interpret-trailers.c | 12 ++++++++++++
2 files changed, 16 insertions(+)
@@ -93,6 +93,10 @@ OPTIONS line, with any existing whitespace continuation folded into a single line.+--parse::+ A convenience alias for `--only-trailers --only-existing+ --normalize`.+ CONFIGURATION VARIABLES -----------------------
@@ -27,6 +37,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)OPT_BOOL(0,"only-trailers",&opts.only_trailers,N_("output only the trailers")),OPT_BOOL(0,"only-existing",&opts.only_existing,N_("output only existing trailers")),OPT_BOOL(0,"normalize",&opts.normalize,N_("normalize trailer formatting")),+{OPTION_CALLBACK,0,"parse",&opts,NULL,N_("set parsing options"),+PARSE_OPT_NOARG|PARSE_OPT_NONEG,parse_opt_parse},OPT_STRING_LIST(0,"trailer",&trailers,N_("trailer"),N_("trailer(s) to add")),OPT_END()
From: Stefan Beller <hidden> Date: 2017-08-09 18:20:19
On Wed, Aug 9, 2017 at 5:26 AM, Jeff King [off-list ref] wrote:
quoted hunk
The last few commits have added command line options that
can turn interpret-trailers into a parsing tool. Since
they'd most often be used together, let's provide a
convenient single option for callers to invoke this mode.
This is implemented as a callback rather than a boolean so
that its effect is applied immediately, as if those options
had been specified. Later options can then override them.
E.g.:
git interpret-trailers --parse --no-normalize
would work.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 4 ++++
builtin/interpret-trailers.c | 12 ++++++++++++
2 files changed, 16 insertions(+)
@@ -93,6 +93,10 @@ OPTIONS line, with any existing whitespace continuation folded into a single line.+--parse::+ A convenience alias for `--only-trailers --only-existing+ --normalize`.
Somewhere in this series, we'd want to not just describe each
of the new knobs, but reword the initial description, too?
git-interpret-trailers - help add structured information
into commit messages
Maybe a s/add/handle/ s/into// is enough?
From: Jeff King <hidden> Date: 2017-08-10 07:59:10
On Wed, Aug 09, 2017 at 11:20:12AM -0700, Stefan Beller wrote:
quoted
+--parse::
+ A convenience alias for `--only-trailers --only-existing
+ --normalize`.
Somewhere in this series, we'd want to not just describe each
of the new knobs, but reword the initial description, too?
git-interpret-trailers - help add structured information
into commit messages
Maybe a s/add/handle/ s/into// is enough?
Thanks, my initial attempt actually included that, when I thought I was
going to have a unique "parsing mode". But then as it grew into
individual options, I dropped it. And forgot how modification-centric
the rest of the manpage description is. I'll try to include something in
the final commit, which is where the parsing mode is all tied together.
-Peff
From: Jeff King <hidden> Date: 2017-08-10 08:02:55
On Wed, Aug 09, 2017 at 08:21:47AM -0400, Jeff King wrote:
This series teaches interpret-trailers to parse and output just the
trailers. So now you can do:
$ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
git interpret-trailers --parse
Signed-off-by: Hartmut Henkel [off-list ref]
Helped-by: Stefan Beller [off-list ref]
Signed-off-by: Ralf Thielow [off-list ref]
Acked-by: Matthias Rüster [off-list ref]
And here's a v2 that addresses all of the comments except one: Stefan
suggested that --only-existing wasn't a great name. I agree, but I like
everything else less.
Summary:
- opts arguments are now const
- tests that depend on the value of trailer.sign.command now set it
explicitly
- the arg_head variable is now moved into the conditional block whewre
it's used
- the interpret-trailers manpage has been updated in patch 5 to make
it clear that "add" and "parse" are the two major modes
[1/5]: trailer: put process_trailers() options into a struct
[2/5]: interpret-trailers: add an option to show only the trailers
[3/5]: interpret-trailers: add an option to show only existing trailers
[4/5]: interpret-trailers: add an option to normalize output
[5/5]: interpret-trailers: add --parse convenience option
Documentation/git-interpret-trailers.txt | 34 +++++++++++---
builtin/interpret-trailers.c | 34 +++++++++++---
t/t7513-interpret-trailers.sh | 76 ++++++++++++++++++++++++++++++++
trailer.c | 68 ++++++++++++++++++++++------
trailer.h | 13 +++++-
5 files changed, 196 insertions(+), 29 deletions(-)
-Peff
From: Jeff King <hidden> Date: 2017-08-10 08:03:27
We already have two options and are about to add a few more.
To avoid having a huge number of boolean arguments, let's
convert to an options struct which can be passed in.
Signed-off-by: Jeff King <redacted>
---
builtin/interpret-trailers.c | 13 ++++++-------
trailer.c | 10 ++++++----
trailer.h | 10 +++++++++-
3 files changed, 21 insertions(+), 12 deletions(-)
@@ -980,7 +982,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct strread_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 strprocess_trailers_lists(&head,&arg_head);-print_all(outfile,&head,trim_empty);+print_all(outfile,&head,opts->trim_empty);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);
From: Jeff King <hidden> Date: 2017-08-10 08:03:29
In theory it's easy for any reader who wants to parse
trailers to do so. But there are a lot of subtle corner
cases around what counts as a trailer, when the trailer
block begins and ends, etc. Since interpret-trailers already
has our parsing logic, let's let callers ask it to just
output the trailers.
They still have to parse the "key: value" lines, but at
least they can ignore all of the other corner cases.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 3 +++
builtin/interpret-trailers.c | 1 +
t/t7513-interpret-trailers.sh | 39 ++++++++++++++++++++++++++++++++
trailer.c | 19 ++++++++++------
trailer.h | 1 +
5 files changed, 56 insertions(+), 7 deletions(-)
@@ -80,6 +80,9 @@ OPTIONS trailer to the input messages. See the description of this command.+--only-trailers::+ Output only the trailers, not any other parts of the input.+ CONFIGURATION VARIABLES -----------------------
@@ -24,6 +24,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)structoptionoptions[]={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_BOOL(0,"only-trailers",&opts.only_trailers,N_("output only the trailers")),OPT_STRING_LIST(0,"trailer",&trailers,N_("trailer"),N_("trailer(s) to add")),OPT_END()
@@ -897,9 +899,10 @@ static int process_input_file(FILE *outfile,trailer_info_get(&info,str);/* Print lines before the trailers as is */-fwrite(str,1,info.trailer_start-str,outfile);+if(outfile)+fwrite(str,1,info.trailer_start-str,outfile);-if(!info.blank_line_before_trailer)+if(outfile&&!info.blank_line_before_trailer)fprintf(outfile,"\n");for(i=0;i<info.trailer_nr;i++){
@@ -986,18 +989,20 @@ void process_trailers(const char *file,outfile=create_in_place_tempfile(file);/* Print the lines before the trailers */-trailer_end=process_input_file(outfile,sb.buf,&head);+trailer_end=process_input_file(opts->only_trailers?NULL:outfile,+sb.buf,&head);process_command_line_args(&arg_head,trailers);process_trailers_lists(&head,&arg_head);-print_all(outfile,&head,opts->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(!opts->only_trailers)+fwrite(sb.buf+trailer_end,1,sb.len-trailer_end,outfile);if(opts->in_place)if(rename_tempfile(&trailers_tempfile,file))
From: Jeff King <hidden> Date: 2017-08-10 08:03:35
It can be useful to invoke interpret-trailers for the
primary purpose of parsing existing trailers. But in that
case, we don't want to apply existing ifMissing or ifExists
rules from the config. Let's add a special mode where we
avoid applying those rules. Coupled with --only-trailers,
this gives us a reasonable parsing tool.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 5 +++++
builtin/interpret-trailers.c | 7 +++++++
t/t7513-interpret-trailers.sh | 16 ++++++++++++++++
trailer.c | 9 +++++----
trailer.h | 1 +
5 files changed, 34 insertions(+), 4 deletions(-)
@@ -83,6 +83,11 @@ OPTIONS --only-trailers:: Output only the trailers, not any other parts of the input.+--only-existing::+ Output only trailers that exist in the input; do not add any+ from the command-line or by following configured `trailer.*`+ rules.+ CONFIGURATION VARIABLES -----------------------
@@ -25,6 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)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_BOOL(0,"only-trailers",&opts.only_trailers,N_("output only the trailers")),+OPT_BOOL(0,"only-existing",&opts.only_existing,N_("output only existing trailers")),OPT_STRING_LIST(0,"trailer",&trailers,N_("trailer"),N_("trailer(s) to add")),OPT_END()
@@ -33,6 +34,12 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)argc=parse_options(argc,argv,prefix,options,git_interpret_trailers_usage,0);+if(opts.only_existing&&trailers.nr)+usage_msg_opt(+_("--trailer with --only-existing does not make sense"),+git_interpret_trailers_usage,+options);+if(argc){inti;for(i=0;i<argc;i++)
From: Jeff King <hidden> Date: 2017-08-10 08:03:36
The point of "--only-trailers" is to give a caller an output
that's easy for them to parse. Getting rid of the
non-trailer material helps, but we still may see more
complicated syntax like whitespace continuation. Let's add
an option to normalize the output into one "key: value" line
per trailer.
As a bonus, this could be used even without --only-trailers
to clean up unusual formatting in the incoming data.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 5 +++++
builtin/interpret-trailers.c | 1 +
t/t7513-interpret-trailers.sh | 21 ++++++++++++++++++++
trailer.c | 34 +++++++++++++++++++++++++++++++-
trailer.h | 1 +
5 files changed, 61 insertions(+), 1 deletion(-)
@@ -88,6 +88,11 @@ OPTIONS from the command-line or by following configured `trailer.*` rules.+--normalize::+ Normalize trailer output so that trailers appear exactly one per+ line, with any existing whitespace continuation folded into a+ single line.+ CONFIGURATION VARIABLES -----------------------
@@ -26,6 +26,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)OPT_BOOL(0,"trim-empty",&opts.trim_empty,N_("trim empty trailers")),OPT_BOOL(0,"only-trailers",&opts.only_trailers,N_("output only the trailers")),OPT_BOOL(0,"only-existing",&opts.only_existing,N_("output only existing trailers")),+OPT_BOOL(0,"normalize",&opts.normalize,N_("normalize trailer formatting")),OPT_STRING_LIST(0,"trailer",&trailers,N_("trailer"),N_("trailer(s) to add")),OPT_END()
@@ -1330,4 +1330,25 @@ test_expect_success 'only existing' 'test_cmpexpectedactual'+test_expect_success'normalize''+cat>expected<<-\EOF&&+foo:continuedacrossseverallines+EOF+# pass through tr to make leading and trailing whitespace more obvious+tr_" "<<-\EOF|+mysubject++mybody++foo:_+__continued+___across+____several+_____lines+___+EOF+gitinterpret-trailers--only-trailers--only-existing--normalize>actual&&+test_cmpexpectedactual+'+ test_done
@@ -887,7 +887,35 @@ static int ends_with_blank_line(const char *buf, size_t len)returnis_blank_line(buf+ll);}+staticvoidnormalize_value(structstrbuf*val)+{+structstrbufout=STRBUF_INIT;+size_ti;++strbuf_grow(&out,val->len);+i=0;+while(i<val->len){+charc=val->buf[i++];+if(c=='\n'){+/* Collapse continuation down to a single space. */+while(i<val->len&&isspace(val->buf[i]))+i++;+strbuf_addch(&out,' ');+}else{+strbuf_addch(&out,c);+}+}++/* Empty lines may have left us with whitespace cruft at the edges */+strbuf_trim(&out);++/* output goes back to val as if we modified it in-place */+strbuf_swap(&out,val);+strbuf_release(&out);+}+staticintprocess_input_file(FILE*outfile,+intnormalize,constchar*str,structlist_head*head){
@@ -914,12 +942,16 @@ static int process_input_file(FILE *outfile,if(separator_pos>=1){parse_trailer(&tok,&val,NULL,trailer,separator_pos);+if(normalize)+normalize_value(&val);add_trailer_item(head,strbuf_detach(&tok,NULL),strbuf_detach(&val,NULL));}else{strbuf_addstr(&val,trailer);strbuf_strip_suffix(&val,"\n");+if(normalize)+normalize_value(&val);add_trailer_item(head,NULL,strbuf_detach(&val,NULL));
@@ -989,7 +1021,7 @@ void process_trailers(const char *file,/* Print the lines before the trailers */trailer_end=process_input_file(opts->only_trailers?NULL:outfile,-sb.buf,&head);+opts->normalize,sb.buf,&head);if(!opts->only_existing){LIST_HEAD(arg_head);
From: Stefan Beller <hidden> Date: 2017-08-10 18:35:07
On Thu, Aug 10, 2017 at 1:03 AM, Jeff King [off-list ref] wrote:
The point of "--only-trailers" is to give a caller an output
that's easy for them to parse. Getting rid of the
non-trailer material helps, but we still may see more
complicated syntax like whitespace continuation. Let's add
an option to normalize the output into one "key: value" line
per trailer.
As a bonus, this could be used even without --only-trailers
to clean up unusual formatting in the incoming data.
This is useful for the parsing part, but for the writing part we'd
rather want to have the opposite thing, such as
'--line-break=rfc822'. But this doesn't have to be part of this
series. With this in mind, I do not quite understand the latter
use case how you would use normalized trailers without
--only-trailers?
From: Jeff King <hidden> Date: 2017-08-10 18:37:19
On Thu, Aug 10, 2017 at 11:35:00AM -0700, Stefan Beller wrote:
On Thu, Aug 10, 2017 at 1:03 AM, Jeff King [off-list ref] wrote:
quoted
The point of "--only-trailers" is to give a caller an output
that's easy for them to parse. Getting rid of the
non-trailer material helps, but we still may see more
complicated syntax like whitespace continuation. Let's add
an option to normalize the output into one "key: value" line
per trailer.
As a bonus, this could be used even without --only-trailers
to clean up unusual formatting in the incoming data.
This is useful for the parsing part, but for the writing part we'd
rather want to have the opposite thing, such as
'--line-break=rfc822'. But this doesn't have to be part of this
series. With this in mind, I do not quite understand the latter
use case how you would use normalized trailers without
--only-trailers?
If you prefer the normalized form (and the input was line-broken in a
way that you don't like), then this would convert to your preferred
form. I agree that you could potentially want the opposite (folding long
lines). Perhaps something like --wrap=72.
-Peff
From: Christian Couder <hidden> Date: 2017-08-10 19:39:26
On Thu, Aug 10, 2017 at 8:37 PM, Jeff King [off-list ref] wrote:
On Thu, Aug 10, 2017 at 11:35:00AM -0700, Stefan Beller wrote:
quoted
On Thu, Aug 10, 2017 at 1:03 AM, Jeff King [off-list ref] wrote:
quoted
The point of "--only-trailers" is to give a caller an output
that's easy for them to parse. Getting rid of the
non-trailer material helps, but we still may see more
complicated syntax like whitespace continuation. Let's add
an option to normalize the output into one "key: value" line
per trailer.
As a bonus, this could be used even without --only-trailers
to clean up unusual formatting in the incoming data.
This is useful for the parsing part, but for the writing part we'd
rather want to have the opposite thing, such as
'--line-break=rfc822'. But this doesn't have to be part of this
series. With this in mind, I do not quite understand the latter
use case how you would use normalized trailers without
--only-trailers?
If you prefer the normalized form (and the input was line-broken in a
way that you don't like), then this would convert to your preferred
form. I agree that you could potentially want the opposite (folding long
lines). Perhaps something like --wrap=72.
Related to this, I wonder if people might want to "normalize" in
different ways later. If that happens, we might regret having called
this option "--normalize" instead of "--one-per-line" for example.
From: Jeff King <hidden> Date: 2017-08-10 19:42:58
On Thu, Aug 10, 2017 at 09:39:21PM +0200, Christian Couder wrote:
quoted
If you prefer the normalized form (and the input was line-broken in a
way that you don't like), then this would convert to your preferred
form. I agree that you could potentially want the opposite (folding long
lines). Perhaps something like --wrap=72.
Related to this, I wonder if people might want to "normalize" in
different ways later. If that happens, we might regret having called
this option "--normalize" instead of "--one-per-line" for example.
My assumption was that it would be OK to add other normalization later
if it brings us closer to the "key: value" form as a standard, and it
could fall under "--normalize", since that's what callers would want.
And that's why I didn't want to call it something like --one-per-line.
But if you are arguing that there can be many "standards" to normalize
to, I agree that's a possibility. I think we have an out by extending to
"--normalize=whatever-form" in the future.
-Peff
From: Christian Couder <hidden> Date: 2017-08-10 20:26:34
On Thu, Aug 10, 2017 at 9:42 PM, Jeff King [off-list ref] wrote:
On Thu, Aug 10, 2017 at 09:39:21PM +0200, Christian Couder wrote:
quoted
quoted
If you prefer the normalized form (and the input was line-broken in a
way that you don't like), then this would convert to your preferred
form. I agree that you could potentially want the opposite (folding long
lines). Perhaps something like --wrap=72.
Related to this, I wonder if people might want to "normalize" in
different ways later. If that happens, we might regret having called
this option "--normalize" instead of "--one-per-line" for example.
My assumption was that it would be OK to add other normalization later
if it brings us closer to the "key: value" form as a standard, and it
could fall under "--normalize", since that's what callers would want.
And that's why I didn't want to call it something like --one-per-line.
But if you are arguing that there can be many "standards" to normalize
to, I agree that's a possibility. I think we have an out by extending to
"--normalize=whatever-form" in the future.
If we take `git log` as an example, we now have "--oneline" which is a
shorthand for "--pretty=oneline --abbrev-commit".
And the default for "--pretty" is called "medium".
So instead of your suggestion, we could call this option "--oneline"
now, and if other normalizations are later required we could then
create "--pretty=whatever" and say that "--oneline" is a shorthand for
"--pretty=oneline".
From: Stefan Beller <hidden> Date: 2017-08-10 19:44:51
On Thu, Aug 10, 2017 at 12:39 PM, Christian Couder
[off-list ref] wrote:
On Thu, Aug 10, 2017 at 8:37 PM, Jeff King [off-list ref] wrote:
quoted
On Thu, Aug 10, 2017 at 11:35:00AM -0700, Stefan Beller wrote:
quoted
On Thu, Aug 10, 2017 at 1:03 AM, Jeff King [off-list ref] wrote:
quoted
The point of "--only-trailers" is to give a caller an output
that's easy for them to parse. Getting rid of the
non-trailer material helps, but we still may see more
complicated syntax like whitespace continuation. Let's add
an option to normalize the output into one "key: value" line
per trailer.
As a bonus, this could be used even without --only-trailers
to clean up unusual formatting in the incoming data.
This is useful for the parsing part, but for the writing part we'd
rather want to have the opposite thing, such as
'--line-break=rfc822'. But this doesn't have to be part of this
series. With this in mind, I do not quite understand the latter
use case how you would use normalized trailers without
--only-trailers?
If you prefer the normalized form (and the input was line-broken in a
way that you don't like), then this would convert to your preferred
form. I agree that you could potentially want the opposite (folding long
lines). Perhaps something like --wrap=72.
Related to this, I wonder if people might want to "normalize" in
different ways later. If that happens, we might regret having called
this option "--normalize" instead of "--one-per-line" for example.
What is normal?
Maybe --style=[one-line, wrapped:72, rfc, json, xml, ...]
then?
If you have --one-per-line, this may be orthogonal to e.g. json
(as json can be crammed into one line IIUC), but when given the
selection you cannot combine multiple styles.
Scratch that, we actually want to combine these styles with each
other.
From: Christian Couder <hidden> Date: 2017-08-10 21:06:46
On Thu, Aug 10, 2017 at 9:44 PM, Stefan Beller [off-list ref] wrote:
On Thu, Aug 10, 2017 at 12:39 PM, Christian Couder
[off-list ref] wrote:
quoted
On Thu, Aug 10, 2017 at 8:37 PM, Jeff King [off-list ref] wrote:
quoted
On Thu, Aug 10, 2017 at 11:35:00AM -0700, Stefan Beller wrote:
quoted
On Thu, Aug 10, 2017 at 1:03 AM, Jeff King [off-list ref] wrote:
quoted
The point of "--only-trailers" is to give a caller an output
that's easy for them to parse. Getting rid of the
non-trailer material helps, but we still may see more
complicated syntax like whitespace continuation. Let's add
an option to normalize the output into one "key: value" line
per trailer.
As a bonus, this could be used even without --only-trailers
to clean up unusual formatting in the incoming data.
This is useful for the parsing part, but for the writing part we'd
rather want to have the opposite thing, such as
'--line-break=rfc822'. But this doesn't have to be part of this
series. With this in mind, I do not quite understand the latter
use case how you would use normalized trailers without
--only-trailers?
If you prefer the normalized form (and the input was line-broken in a
way that you don't like), then this would convert to your preferred
form. I agree that you could potentially want the opposite (folding long
lines). Perhaps something like --wrap=72.
Related to this, I wonder if people might want to "normalize" in
different ways later. If that happens, we might regret having called
this option "--normalize" instead of "--one-per-line" for example.
What is normal?
Maybe --style=[one-line, wrapped:72, rfc, json, xml, ...]
then?
Yeah, we could go there right now (using perhaps "--pretty" or
"--format" instead of "--style", so that we are more consistent with
other commands), but on the other hand we don't know yet if the other
formats will really be needed.
If you have --one-per-line, this may be orthogonal to e.g. json
(as json can be crammed into one line IIUC), but when given the
selection you cannot combine multiple styles.
Scratch that, we actually want to combine these styles with each
other.
Yeah, that's another possibility for the future. People might want a
--json option that can be used both with and without --oneline. But as
the future is difficult to predict, let's try to make it easy for us
in both cases.
And I think starting with just "--oneline" would be easier to deal
with later than "--normalize" (or "--style" or "--pretty" or
"--format") especially in the latter case.
From: Jeff King <hidden> Date: 2017-08-10 21:10:45
On Thu, Aug 10, 2017 at 11:06:40PM +0200, Christian Couder wrote:
quoted
quoted
Related to this, I wonder if people might want to "normalize" in
different ways later. If that happens, we might regret having called
this option "--normalize" instead of "--one-per-line" for example.
What is normal?
Maybe --style=[one-line, wrapped:72, rfc, json, xml, ...]
then?
Yeah, we could go there right now (using perhaps "--pretty" or
"--format" instead of "--style", so that we are more consistent with
other commands), but on the other hand we don't know yet if the other
formats will really be needed.
But some of those things are not 1:1 mappings with normalization. For
instance, --json presumably implies --only-trailers. Or are we proposing
to break the whole commit message down into components and output it all
as json?
-Peff
From: Ramsay Jones <hidden> Date: 2017-08-10 23:02:59
On 10/08/17 22:10, Jeff King wrote:
On Thu, Aug 10, 2017 at 11:06:40PM +0200, Christian Couder wrote:
quoted
quoted
quoted
Related to this, I wonder if people might want to "normalize" in
different ways later. If that happens, we might regret having called
this option "--normalize" instead of "--one-per-line" for example.
What is normal?
Maybe --style=[one-line, wrapped:72, rfc, json, xml, ...]
then?
Yeah, we could go there right now (using perhaps "--pretty" or
"--format" instead of "--style", so that we are more consistent with
other commands), but on the other hand we don't know yet if the other
formats will really be needed.
But some of those things are not 1:1 mappings with normalization. For
instance, --json presumably implies --only-trailers. Or are we proposing
to break the whole commit message down into components and output it all
as json?
Hmm, to me the operation wasn't so much a normalization, rather
it was an --unfold (or maybe --un-fold ;-D). I suppose going in
the other direction could be --fold=72, or some such.
[blue is my favourite colour ... :-P ]
ATB,
Ramsay Jones
From: Jeff King <hidden> Date: 2017-08-10 23:10:45
On Fri, Aug 11, 2017 at 12:02:49AM +0100, Ramsay Jones wrote:
quoted
But some of those things are not 1:1 mappings with normalization. For
instance, --json presumably implies --only-trailers. Or are we proposing
to break the whole commit message down into components and output it all
as json?
Hmm, to me the operation wasn't so much a normalization, rather
it was an --unfold (or maybe --un-fold ;-D). I suppose going in
the other direction could be --fold=72, or some such.
But I really don't want callers to think of it as "unfold". I want it to
be "turn this into something I can parse simply". Hence if we were to
find another case where the output is irregular, I'd feel comfortable
calling that a bug and fixing it (one that I suspect but haven't tested
is that alternate separators probably should all be converted to
colons).
From: Ramsay Jones <hidden> Date: 2017-08-10 23:36:59
On 11/08/17 00:10, Jeff King wrote:
On Fri, Aug 11, 2017 at 12:02:49AM +0100, Ramsay Jones wrote:
quoted
quoted
But some of those things are not 1:1 mappings with normalization. For
instance, --json presumably implies --only-trailers. Or are we proposing
to break the whole commit message down into components and output it all
as json?
Hmm, to me the operation wasn't so much a normalization, rather
it was an --unfold (or maybe --un-fold ;-D). I suppose going in
the other direction could be --fold=72, or some such.
But I really don't want callers to think of it as "unfold". I want it to
be "turn this into something I can parse simply". Hence if we were to
find another case where the output is irregular, I'd feel comfortable
calling that a bug and fixing it (one that I suspect but haven't tested
is that alternate separators probably should all be converted to
colons).
From: Christian Couder <hidden> Date: 2017-08-11 07:02:30
On Fri, Aug 11, 2017 at 1:10 AM, Jeff King [off-list ref] wrote:
On Fri, Aug 11, 2017 at 12:02:49AM +0100, Ramsay Jones wrote:
quoted
quoted
But some of those things are not 1:1 mappings with normalization. For
instance, --json presumably implies --only-trailers. Or are we proposing
to break the whole commit message down into components and output it all
as json?
Well who knows what people might want/need?
Anyway in `git log` --oneline is not a direct mapping with
--pretty=oneline as it also means --abbrev-commit, and this is not a
big problem.
quoted
Hmm, to me the operation wasn't so much a normalization, rather
it was an --unfold (or maybe --un-fold ;-D). I suppose going in
the other direction could be --fold=72, or some such.
Yeah, we could call that --no-fold or --no-wrap if we expect to need
--fold=72 or --wrap=72.
At least it is more descriptive than --normalize and if we later
introduce --pretty or --format we can say that it is a shorthand for
--pretty=nofold or --pretty=unfolded.
But I really don't want callers to think of it as "unfold". I want it to
be "turn this into something I can parse simply". Hence if we were to
find another case where the output is irregular, I'd feel comfortable
calling that a bug and fixing it (one that I suspect but haven't tested
is that alternate separators probably should all be converted to
colons).
Though "fixing" this after a release has been made might introduce a
regression for people who would already use the option on trailers
with different separators. That's also why I don't like --normalize.
We just don't know if we will need to "fix" it a lot to make sure
scripts using it will work in all cases.
If we use --no-fold or --oneline instead, we don't promise too much
from this option, so users cannot say that they expect it to work for
them in all cases.
From: Jeff King <hidden> Date: 2017-08-11 09:06:24
On Fri, Aug 11, 2017 at 09:02:24AM +0200, Christian Couder wrote:
quoted
But I really don't want callers to think of it as "unfold". I want it to
be "turn this into something I can parse simply". Hence if we were to
find another case where the output is irregular, I'd feel comfortable
calling that a bug and fixing it (one that I suspect but haven't tested
is that alternate separators probably should all be converted to
colons).
Though "fixing" this after a release has been made might introduce a
regression for people who would already use the option on trailers
with different separators. That's also why I don't like --normalize.
We just don't know if we will need to "fix" it a lot to make sure
scripts using it will work in all cases.
If we use --no-fold or --oneline instead, we don't promise too much
from this option, so users cannot say that they expect it to work for
them in all cases.
But promising a normalized form is exactly what I want from the option.
That said, I'm OK to promise that via "--parse", and call this --unfold,
if you really feel strongly.
-Peff
From: Christian Couder <hidden> Date: 2017-08-11 19:02:49
On Fri, Aug 11, 2017 at 11:06 AM, Jeff King [off-list ref] wrote:
On Fri, Aug 11, 2017 at 09:02:24AM +0200, Christian Couder wrote:
quoted
quoted
But I really don't want callers to think of it as "unfold". I want it to
be "turn this into something I can parse simply". Hence if we were to
find another case where the output is irregular, I'd feel comfortable
calling that a bug and fixing it (one that I suspect but haven't tested
is that alternate separators probably should all be converted to
colons).
Though "fixing" this after a release has been made might introduce a
regression for people who would already use the option on trailers
with different separators. That's also why I don't like --normalize.
We just don't know if we will need to "fix" it a lot to make sure
scripts using it will work in all cases.
If we use --no-fold or --oneline instead, we don't promise too much
from this option, so users cannot say that they expect it to work for
them in all cases.
But promising a normalized form is exactly what I want from the option.
That said, I'm OK to promise that via "--parse", and call this --unfold,
if you really feel strongly.
Yeah, I think promising these kind of things via an higher level
option that is a shorthand for a mix of other basic options is much
better especially if it's clearly documented that the option mix could
change in case of bugs or improvements.
This way people who want something stable, know that they should use
their own mix of basic options. And those who are ok with something
not as stable as long as it evolves towards a specific goal, know that
they should use the higher level option.
From: Jeff King <hidden> Date: 2017-08-10 08:03:39
The last few commits have added command line options that
can turn interpret-trailers into a parsing tool. Since
they'd most often be used together, let's provide a
convenient single option for callers to invoke this mode.
This is implemented as a callback rather than a boolean so
that its effect is applied immediately, as if those options
had been specified. Later options can then override them.
E.g.:
git interpret-trailers --parse --no-normalize
would work.
Let's also update the documentation to make clear that this
parsing mode behaves quite differently than the normal
"add trailers to the input" mode.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 21 ++++++++++++++-------
builtin/interpret-trailers.c | 12 ++++++++++++
2 files changed, 26 insertions(+), 7 deletions(-)
@@ -3,24 +3,27 @@ git-interpret-trailers(1) NAME -----git-interpret-trailers - help add structured information into commit messages+git-interpret-trailers - add or parse structured information in commit messages SYNOPSIS -------- [verse]-'git interpret-trailers' [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]+'git interpret-trailers' [options] [(--trailer <token>[(=|:)<value>])...] [<file>...]+'git interpret-trailers' [options] [--parse] [<file>...] DESCRIPTION ------------Help adding 'trailers' lines, that look similar to RFC 822 e-mail+Help parsing or adding 'trailers' lines, that look similar to RFC 822 e-mail headers, at the end of the otherwise free-form part of a commit message. This command reads some patches or commit messages from either the-<file> arguments or the standard input if no <file> is specified. Then-this command applies the arguments passed using the `--trailer`-option, if any, to the commit message part of each input file. The-result is emitted on the standard output.+<file> arguments or the standard input if no <file> is specified. If+`--parse` is specified, the output consists of the parsed trailers.++Otherwise, the this command applies the arguments passed using the+`--trailer` option, if any, to the commit message part of each input+file. The result is emitted on the standard output. Some configuration variables control the way the `--trailer` arguments are applied to each commit message and the way any existing trailer in
@@ -93,6 +96,10 @@ OPTIONS line, with any existing whitespace continuation folded into a single line.+--parse::+ A convenience alias for `--only-trailers --only-existing+ --normalize`.+ CONFIGURATION VARIABLES -----------------------
@@ -27,6 +37,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)OPT_BOOL(0,"only-trailers",&opts.only_trailers,N_("output only the trailers")),OPT_BOOL(0,"only-existing",&opts.only_existing,N_("output only existing trailers")),OPT_BOOL(0,"normalize",&opts.normalize,N_("normalize trailer formatting")),+{OPTION_CALLBACK,0,"parse",&opts,NULL,N_("set parsing options"),+PARSE_OPT_NOARG|PARSE_OPT_NONEG,parse_opt_parse},OPT_STRING_LIST(0,"trailer",&trailers,N_("trailer"),N_("trailer(s) to add")),OPT_END()
From: Jeff King <hidden> Date: 2017-08-10 18:03:32
On Thu, Aug 10, 2017 at 04:02:46AM -0400, Jeff King wrote:
On Wed, Aug 09, 2017 at 08:21:47AM -0400, Jeff King wrote:
quoted
This series teaches interpret-trailers to parse and output just the
trailers. So now you can do:
$ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
git interpret-trailers --parse
Signed-off-by: Hartmut Henkel [off-list ref]
Helped-by: Stefan Beller [off-list ref]
Signed-off-by: Ralf Thielow [off-list ref]
Acked-by: Matthias Rüster [off-list ref]
And here's a v2 that addresses all of the comments except one: Stefan
suggested that --only-existing wasn't a great name. I agree, but I like
everything else less.
Here's a v3 that takes care of that (renaming it to --only-input).
It's otherwise the same as v2, but since the name-change ripples through
the remaining patches, I wanted to get v3 in front of people sooner
rather than later.
[1/5]: trailer: put process_trailers() options into a struct
[2/5]: interpret-trailers: add an option to show only the trailers
[3/5]: interpret-trailers: add an option to show only existing trailers
[4/5]: interpret-trailers: add an option to normalize output
[5/5]: interpret-trailers: add --parse convenience option
Documentation/git-interpret-trailers.txt | 34 +++++++++++---
builtin/interpret-trailers.c | 34 +++++++++++---
t/t7513-interpret-trailers.sh | 76 ++++++++++++++++++++++++++++++++
trailer.c | 68 ++++++++++++++++++++++------
trailer.h | 13 +++++-
5 files changed, 196 insertions(+), 29 deletions(-)
From: Jeff King <hidden> Date: 2017-08-10 18:04:05
We already have two options and are about to add a few more.
To avoid having a huge number of boolean arguments, let's
convert to an options struct which can be passed in.
Signed-off-by: Jeff King <redacted>
---
builtin/interpret-trailers.c | 13 ++++++-------
trailer.c | 10 ++++++----
trailer.h | 10 +++++++++-
3 files changed, 21 insertions(+), 12 deletions(-)
@@ -980,7 +982,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct strread_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 strprocess_trailers_lists(&head,&arg_head);-print_all(outfile,&head,trim_empty);+print_all(outfile,&head,opts->trim_empty);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);
From: Jeff King <hidden> Date: 2017-08-10 18:04:10
In theory it's easy for any reader who wants to parse
trailers to do so. But there are a lot of subtle corner
cases around what counts as a trailer, when the trailer
block begins and ends, etc. Since interpret-trailers already
has our parsing logic, let's let callers ask it to just
output the trailers.
They still have to parse the "key: value" lines, but at
least they can ignore all of the other corner cases.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 3 +++
builtin/interpret-trailers.c | 1 +
t/t7513-interpret-trailers.sh | 39 ++++++++++++++++++++++++++++++++
trailer.c | 19 ++++++++++------
trailer.h | 1 +
5 files changed, 56 insertions(+), 7 deletions(-)
@@ -80,6 +80,9 @@ OPTIONS trailer to the input messages. See the description of this command.+--only-trailers::+ Output only the trailers, not any other parts of the input.+ CONFIGURATION VARIABLES -----------------------
@@ -24,6 +24,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)structoptionoptions[]={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_BOOL(0,"only-trailers",&opts.only_trailers,N_("output only the trailers")),OPT_STRING_LIST(0,"trailer",&trailers,N_("trailer"),N_("trailer(s) to add")),OPT_END()
@@ -897,9 +899,10 @@ static int process_input_file(FILE *outfile,trailer_info_get(&info,str);/* Print lines before the trailers as is */-fwrite(str,1,info.trailer_start-str,outfile);+if(outfile)+fwrite(str,1,info.trailer_start-str,outfile);-if(!info.blank_line_before_trailer)+if(outfile&&!info.blank_line_before_trailer)fprintf(outfile,"\n");for(i=0;i<info.trailer_nr;i++){
@@ -986,18 +989,20 @@ void process_trailers(const char *file,outfile=create_in_place_tempfile(file);/* Print the lines before the trailers */-trailer_end=process_input_file(outfile,sb.buf,&head);+trailer_end=process_input_file(opts->only_trailers?NULL:outfile,+sb.buf,&head);process_command_line_args(&arg_head,trailers);process_trailers_lists(&head,&arg_head);-print_all(outfile,&head,opts->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(!opts->only_trailers)+fwrite(sb.buf+trailer_end,1,sb.len-trailer_end,outfile);if(opts->in_place)if(rename_tempfile(&trailers_tempfile,file))
From: Stefan Beller <hidden> Date: 2017-08-10 18:29:01
On Thu, Aug 10, 2017 at 11:04 AM, Jeff King [off-list ref] wrote:
In theory it's easy for any reader who wants to parse
trailers to do so. But there are a lot of subtle corner
cases around what counts as a trailer, when the trailer
block begins and ends, etc. Since interpret-trailers already
has our parsing logic, let's let callers ask it to just
output the trailers.
They still have to parse the "key: value" lines, but at
least they can ignore all of the other corner cases.
Signed-off-by: Jeff King <redacted>
Sorry for a sloppy review last round, upon reviewing
I found another nit.
You may want to use 'test_config' here, which keeps the config
only for one test. The subsequent tests seem to overwrite the
config, so this is not wrong, just not the best style.
You may want to use 'test_config' here, which keeps the config
only for one test. The subsequent tests seem to overwrite the
config, so this is not wrong, just not the best style.
Yeah, I actually considered that but decided to keep style with the rest
of the script. I agree the whole thing could possibly switch to
test_config, but I suspect there may be some fallouts (the style of the
rest of the script seems to assume some continuity between the tests).
-Peff
From: Jeff King <hidden> Date: 2017-08-10 18:04:16
It can be useful to invoke interpret-trailers for the
primary purpose of parsing existing trailers. But in that
case, we don't want to apply existing ifMissing or ifExists
rules from the config. Let's add a special mode where we
avoid applying those rules. Coupled with --only-trailers,
this gives us a reasonable parsing tool.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 5 +++++
builtin/interpret-trailers.c | 7 +++++++
t/t7513-interpret-trailers.sh | 16 ++++++++++++++++
trailer.c | 9 +++++----
trailer.h | 1 +
5 files changed, 34 insertions(+), 4 deletions(-)
@@ -83,6 +83,11 @@ OPTIONS --only-trailers:: Output only the trailers, not any other parts of the input.+--only-input::+ Output only trailers that exist in the input; do not add any+ from the command-line or by following configured `trailer.*`+ rules.+ CONFIGURATION VARIABLES -----------------------
@@ -25,6 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)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_BOOL(0,"only-trailers",&opts.only_trailers,N_("output only the trailers")),+OPT_BOOL(0,"only-input",&opts.only_input,N_("do not apply config rules")),OPT_STRING_LIST(0,"trailer",&trailers,N_("trailer"),N_("trailer(s) to add")),OPT_END()
@@ -33,6 +34,12 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)argc=parse_options(argc,argv,prefix,options,git_interpret_trailers_usage,0);+if(opts.only_input&&trailers.nr)+usage_msg_opt(+_("--trailer with --only-input does not make sense"),+git_interpret_trailers_usage,+options);+if(argc){inti;for(i=0;i<argc;i++)
From: Jeff King <hidden> Date: 2017-08-10 18:04:18
The point of "--only-trailers" is to give a caller an output
that's easy for them to parse. Getting rid of the
non-trailer material helps, but we still may see more
complicated syntax like whitespace continuation. Let's add
an option to normalize the output into one "key: value" line
per trailer.
As a bonus, this could be used even without --only-trailers
to clean up unusual formatting in the incoming data.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 5 +++++
builtin/interpret-trailers.c | 1 +
t/t7513-interpret-trailers.sh | 21 ++++++++++++++++++++
trailer.c | 34 +++++++++++++++++++++++++++++++-
trailer.h | 1 +
5 files changed, 61 insertions(+), 1 deletion(-)
@@ -88,6 +88,11 @@ OPTIONS from the command-line or by following configured `trailer.*` rules.+--normalize::+ Normalize trailer output so that trailers appear exactly one per+ line, with any existing whitespace continuation folded into a+ single line.+ CONFIGURATION VARIABLES -----------------------
@@ -1330,4 +1330,25 @@ test_expect_success 'only input' 'test_cmpexpectedactual'+test_expect_success'normalize''+cat>expected<<-\EOF&&+foo:continuedacrossseverallines+EOF+# pass through tr to make leading and trailing whitespace more obvious+tr_" "<<-\EOF|+mysubject++mybody++foo:_+__continued+___across+____several+_____lines+___+EOF+gitinterpret-trailers--only-trailers--only-input--normalize>actual&&+test_cmpexpectedactual+'+ test_done
@@ -887,7 +887,35 @@ static int ends_with_blank_line(const char *buf, size_t len)returnis_blank_line(buf+ll);}+staticvoidnormalize_value(structstrbuf*val)+{+structstrbufout=STRBUF_INIT;+size_ti;++strbuf_grow(&out,val->len);+i=0;+while(i<val->len){+charc=val->buf[i++];+if(c=='\n'){+/* Collapse continuation down to a single space. */+while(i<val->len&&isspace(val->buf[i]))+i++;+strbuf_addch(&out,' ');+}else{+strbuf_addch(&out,c);+}+}++/* Empty lines may have left us with whitespace cruft at the edges */+strbuf_trim(&out);++/* output goes back to val as if we modified it in-place */+strbuf_swap(&out,val);+strbuf_release(&out);+}+staticintprocess_input_file(FILE*outfile,+intnormalize,constchar*str,structlist_head*head){
@@ -914,12 +942,16 @@ static int process_input_file(FILE *outfile,if(separator_pos>=1){parse_trailer(&tok,&val,NULL,trailer,separator_pos);+if(normalize)+normalize_value(&val);add_trailer_item(head,strbuf_detach(&tok,NULL),strbuf_detach(&val,NULL));}else{strbuf_addstr(&val,trailer);strbuf_strip_suffix(&val,"\n");+if(normalize)+normalize_value(&val);add_trailer_item(head,NULL,strbuf_detach(&val,NULL));
@@ -989,7 +1021,7 @@ void process_trailers(const char *file,/* Print the lines before the trailers */trailer_end=process_input_file(opts->only_trailers?NULL:outfile,-sb.buf,&head);+opts->normalize,sb.buf,&head);if(!opts->only_input){LIST_HEAD(arg_head);
From: Jeff King <hidden> Date: 2017-08-10 18:04:19
The last few commits have added command line options that
can turn interpret-trailers into a parsing tool. Since
they'd most often be used together, let's provide a
convenient single option for callers to invoke this mode.
This is implemented as a callback rather than a boolean so
that its effect is applied immediately, as if those options
had been specified. Later options can then override them.
E.g.:
git interpret-trailers --parse --no-normalize
would work.
Let's also update the documentation to make clear that this
parsing mode behaves quite differently than the normal
"add trailers to the input" mode.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-interpret-trailers.txt | 21 ++++++++++++++-------
builtin/interpret-trailers.c | 12 ++++++++++++
2 files changed, 26 insertions(+), 7 deletions(-)
@@ -3,24 +3,27 @@ git-interpret-trailers(1) NAME -----git-interpret-trailers - help add structured information into commit messages+git-interpret-trailers - add or parse structured information in commit messages SYNOPSIS -------- [verse]-'git interpret-trailers' [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]+'git interpret-trailers' [options] [(--trailer <token>[(=|:)<value>])...] [<file>...]+'git interpret-trailers' [options] [--parse] [<file>...] DESCRIPTION ------------Help adding 'trailers' lines, that look similar to RFC 822 e-mail+Help parsing or adding 'trailers' lines, that look similar to RFC 822 e-mail headers, at the end of the otherwise free-form part of a commit message. This command reads some patches or commit messages from either the-<file> arguments or the standard input if no <file> is specified. Then-this command applies the arguments passed using the `--trailer`-option, if any, to the commit message part of each input file. The-result is emitted on the standard output.+<file> arguments or the standard input if no <file> is specified. If+`--parse` is specified, the output consists of the parsed trailers.++Otherwise, the this command applies the arguments passed using the+`--trailer` option, if any, to the commit message part of each input+file. The result is emitted on the standard output. Some configuration variables control the way the `--trailer` arguments are applied to each commit message and the way any existing trailer in
@@ -93,6 +96,10 @@ OPTIONS line, with any existing whitespace continuation folded into a single line.+--parse::+ A convenience alias for `--only-trailers --only-input+ --normalize`.+ CONFIGURATION VARIABLES -----------------------
From: Stefan Beller <hidden> Date: 2017-08-10 18:35:54
On Thu, Aug 10, 2017 at 11:03 AM, Jeff King [off-list ref] wrote:
On Thu, Aug 10, 2017 at 04:02:46AM -0400, Jeff King wrote:
quoted
On Wed, Aug 09, 2017 at 08:21:47AM -0400, Jeff King wrote:
quoted
This series teaches interpret-trailers to parse and output just the
trailers. So now you can do:
$ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
git interpret-trailers --parse
Signed-off-by: Hartmut Henkel [off-list ref]
Helped-by: Stefan Beller [off-list ref]
Signed-off-by: Ralf Thielow [off-list ref]
Acked-by: Matthias Rüster [off-list ref]
And here's a v2 that addresses all of the comments except one: Stefan
suggested that --only-existing wasn't a great name. I agree, but I like
everything else less.
Here's a v3 that takes care of that (renaming it to --only-input).
It's otherwise the same as v2, but since the name-change ripples through
the remaining patches, I wanted to get v3 in front of people sooner
rather than later.
From: Jeff King <hidden> Date: 2017-08-15 10:23:01
Here's a fourth version of my series to make the output of
interpret-trailers useful for callers who just want to parse existing
trailers.
The big change here is that I added similar features to pretty.c's
"%(trailers)", meaning you can now efficiently get the same output for
multiple commits with a single invocation. Like:
$ git rev-list --no-walk --format='%(trailers:only:unfold)' \
3ac53e0d13fa7483cce90eb6a1cfcdcbda5b8e35
commit 3ac53e0d13fa7483cce90eb6a1cfcdcbda5b8e35
Signed-off-by: H. Peter Anvin [off-list ref]
Signed-off-by: Junio C Hamano [off-list ref]
That doesn't look very exciting, but with just "%(trailers)" you would
see that the block also contains a "cherry-picked from" line. There
aren't good examples of unfolding in git.git because we don't tend to
fold. Nor does linux.git (though they do have a lot of non-trailer lines
in square brackets). The tests show off some samples of both.
I left %(trailers) exactly as it is: it shows the trailer block
verbatim, including funny syntax, etc. As soon as you use one of :only
or :unfold, we have to parse further, and as a side effect we normalize
a few bits (like whitespace around separators), because we round-trip
through the parser. I think that's fine and what callers would want. But
it did make me wonder if I should just have a single "parse" option that
does all that normalizing. Or alternatively, have %(trailers) always do
that round-trip normalizing. That's how interpret-trailers behaves,
after all(since it always parses and reconstructs the trailer block). I
felt better taking the conservative route, though, and leaving
%(trailers) alone.
There are a few other changes from v3:
- the --normalize option is now --unfold (and I matched it with
":unfold" for the placeholder)
- the process_input_file function now takes the
process_trailer_options to check "only_trailers" instead of taking a
hint from a NULL outfile. This is hopefully a bit more obvious.
- when only_trailers is set, I don't even bother adding non-trailers
from the middle of the trailer-block to the list (we know we'd never
output them, and they can't affect the trailer rules). This is just
a micro-optimization I noticed while writing the %(trailers) helper.
- Similarly, we no longer unfold non-trailers. So if you have:
key: value
more value
this is not a trailer
this is also not a trailer
we would unfold "key: value more value", but not the other two
lines. Because without a separator, that's not really folding. I'm
not sure it actually matters. It's hard to have non-trailers in the
block in the first place. But I think the new behavior is the right
thing if it ever does come up.
[1/8]: trailer: put process_trailers() options into a struct
[2/8]: interpret-trailers: add an option to show only the trailers
[3/8]: interpret-trailers: add an option to show only existing trailers
[4/8]: interpret-trailers: add an option to unfold values
[5/8]: interpret-trailers: add --parse convenience option
[6/8]: pretty: move trailer formatting to trailer.c
[7/8]: t4205: refactor %(trailer) tests
[8/8]: pretty: support normalization options for %(trailers)
Documentation/git-interpret-trailers.txt | 33 +++++++--
Documentation/pretty-formats.txt | 5 +-
builtin/interpret-trailers.c | 34 +++++++--
pretty.c | 26 +++----
t/t4205-log-pretty-formats.sh | 51 ++++++++++++--
t/t7513-interpret-trailers.sh | 76 ++++++++++++++++++++
trailer.c | 115 ++++++++++++++++++++++++++-----
trailer.h | 27 +++++++-
8 files changed, 315 insertions(+), 52 deletions(-)
-Peff
From: Jeff King <hidden> Date: 2017-08-15 10:23:23
We already have two options and are about to add a few more.
To avoid having a huge number of boolean arguments, let's
convert to an options struct which can be passed in.
Signed-off-by: Jeff King <redacted>
---
builtin/interpret-trailers.c | 13 ++++++-------
trailer.c | 10 ++++++----
trailer.h | 10 +++++++++-
3 files changed, 21 insertions(+), 12 deletions(-)
@@ -980,7 +982,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct strread_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 strprocess_trailers_lists(&head,&arg_head);-print_all(outfile,&head,trim_empty);+print_all(outfile,&head,opts->trim_empty);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);