Re: [PATCH 3/5] interpret-trailers: add an option to show only existing trailers
- From
Jeff King <peff@peff.net>
- Date
- Aug 10, 2017, 07:36 UTC
- Message-ID
- <20170810073622.ojm2ezqodkcflj5y@sigill.intra.peff.net>
- In-Reply-To
- <20170809113820.50cfb247@twelve2.svl.corp.google.com>
On Wed, Aug 09, 2017 at 11:38:20AM -0700, Jonathan Tan wrote:
Show 25 quoted lines
> > diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh > > index e5b0718ef6..525fd53e5b 100755 > > --- a/t/t7513-interpret-trailers.sh > > +++ b/t/t7513-interpret-trailers.sh > > @@ -1312,4 +1312,19 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' ' > > test_cmp expected actual > > ' > > > > +test_expect_success 'only existing' ' > > + cat >expected <<-\EOF && > > + existing: existing-value > > + EOF > > + git interpret-trailers \ > > + --only-trailers --only-existing >actual <<-\EOF && > > + my subject > > + > > + my body > > + > > + existing: existing-value > > + EOF > > + test_cmp expected actual > > 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.
Show 19 quoted lines
> > diff --git a/trailer.c b/trailer.c
> > index a4ff99f98a..88f6efe523 100644
> > --- a/trailer.c
> > +++ b/trailer.c
> > @@ -991,9 +991,10 @@ void process_trailers(const char *file, struct process_trailer_options *opts,
> > 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);
> > + if (!opts->only_existing) {
> > + process_command_line_args(&arg_head, trailers);
> > + process_trailers_lists(&head, &arg_head);
> > + }
>
> 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.
Agreed.
-Peff