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

From: Jeff King <peff@peff.net>
Date: Aug 10, 2017, 07:40 UTC
Message-ID: <20170810074038.3q5sclod7ag7xnti@sigill.intra.peff.net>
In-Reply-To: <20170809113527.292c1776@twelve2.svl.corp.google.com>

On Wed, Aug 09, 2017 at 11:35:27AM -0700, Jonathan Tan wrote:

Show 5 quoted lines

> > -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.

Show 15 quoted lines

> > @@ -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.

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