Re: [PATCH 2/5] interpret-trailers: add an option to show only the trailers

[Jeff King]

On Wed, Aug 09, 2017 at 11:35:27AM -0700, Jonathan Tan 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 process_trailer_options *opts)
> 
> 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.

> > @@ -897,9 +899,10 @@ static int process_input_file(FILE *outfile,
> > trailer_info_get(, 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.

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, );
+   trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
+sb.buf, );

-Peff

Re: [PATCH 2/5] interpret-trailers: add an option to show only the trailers

[Jonathan Tan]

On Wed, 9 Aug 2017 08:24:03 -0400
Jeff King  wrote:

> diff --git a/trailer.c b/trailer.c
> index 0a0c2c264d..a4ff99f98a 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -164,13 +164,15 @@ 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 process_trailer_options *opts)

This can be const, I think. (Same thing for patch 1.)

>  {
>   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) &&
> + (!opts->only_trailers || item->token))
>   print_tok_val(outfile, item->token, item->value);
>   }
>  }
> @@ -897,9 +899,10 @@ static int process_input_file(FILE *outfile,
>   trailer_info_get(, 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++) {

Re: [PATCH 2/5] interpret-trailers: add an option to show only the trailers

[Stefan Beller]

On Wed, Aug 9, 2017 at 10:52 AM, Stefan Beller  wrote:
> On Wed, Aug 9, 2017 at 5:24 AM, Jeff King  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 
>> ---
> ...
>> +test_expect_success 'only-trailers omits non-trailer in middle of block' '
>> +   cat >expected <<-\EOF &&
>> +   Signed-off-by: nobody 
>> +   Signed-off-by: somebody 
>> +   sign: A U Thor 
>> +   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 
>> +   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?\

Nevermind, 4/5 solves that problem.

Re: [PATCH 2/5] interpret-trailers: add an option to show only the trailers

[Stefan Beller]

On Wed, Aug 9, 2017 at 5:24 AM, Jeff King  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 
> ---
...
> +test_expect_success 'only-trailers omits non-trailer in middle of block' '
> +   cat >expected <<-\EOF &&
> +   Signed-off-by: nobody 
> +   Signed-off-by: somebody 
> +   sign: A U Thor 
> +   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 
> +   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?