Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Jacob Keller]

On Thu, Aug 10, 2017 at 11:42 AM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>>> > The above example made me wonder if we also want a format specifier
>>> > to do the above without piping, but it turns out that we already
>>> > have "log --format=%(trailers)", so we are good ;-)
>>>
>>> I was going to say, I thought we had a way to get trailers for a
>>> commit via the pretty format, since that is what i used in the past.
>>
>> I do like that you could get the trailers for many commits in a single
>> invocation. That doesn't matter for my current use-case, but obviously
>> piping through O(n) interpret-trailers invocations is a bad idea.
>> But there are a few difficulties with using %(trailers) for this,...
>
> I think it is clear to you, but it may not be clear to others, that
> I did not mean to say "because 'log --format' already knows about
> it, this change to interpret-trailers is unnecessary".
>
>> For (1) I think many callers would prefer to see the original
>> formatting. Maybe we'd need a %(trailers:normalize) or something.
>
> Thanks; that is exactly the line of thought I had in the back of my
> head without even realizing when I brought up %(trailers) format
> element.

I'll add that I also think this patch series is good, it's useful to
have a separate command.

Thanks,
Jake

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Junio C Hamano]

Stefan Beller  writes:

> On Thu, Aug 10, 2017 at 11:03 AM, Jeff King  wrote:
>> 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:
>>>
>>> > 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 
>>> >   Helped-by: Stefan Beller 
>>> >   Signed-off-by: Ralf Thielow 
>>> >   Acked-by: Matthias Rüster 
>>>
>>> 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.
>>
>
> Looks good,

Yeah, looks good to me too.  Thanks for the "--only-input"
discussion.

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Junio C Hamano]

Jeff King  writes:

>> > The above example made me wonder if we also want a format specifier
>> > to do the above without piping, but it turns out that we already
>> > have "log --format=%(trailers)", so we are good ;-)
>> 
>> I was going to say, I thought we had a way to get trailers for a
>> commit via the pretty format, since that is what i used in the past.
>
> I do like that you could get the trailers for many commits in a single
> invocation. That doesn't matter for my current use-case, but obviously
> piping through O(n) interpret-trailers invocations is a bad idea.
> But there are a few difficulties with using %(trailers) for this,...

I think it is clear to you, but it may not be clear to others, that
I did not mean to say "because 'log --format' already knows about
it, this change to interpret-trailers is unnecessary".

> For (1) I think many callers would prefer to see the original
> formatting. Maybe we'd need a %(trailers:normalize) or something.

Thanks; that is exactly the line of thought I had in the back of my
head without even realizing when I brought up %(trailers) format
element.

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Stefan Beller]

On Thu, Aug 10, 2017 at 11:03 AM, Jeff King  wrote:
> 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:
>>
>> > 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 
>> >   Helped-by: Stefan Beller 
>> >   Signed-off-by: Ralf Thielow 
>> >   Acked-by: Matthias Rüster 
>>
>> 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.
>

Looks good,

Thanks,
Stefan

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Jeff King]

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:
> 
> > 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 
> >   Helped-by: Stefan Beller 
> >   Signed-off-by: Ralf Thielow 
> >   Acked-by: Matthias Rüster 
> 
> 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(-)

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Jeff King]

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 
>   Helped-by: Stefan Beller 
>   Signed-off-by: Ralf Thielow 
>   Acked-by: Matthias Rüster 

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

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Jeff King]

On Thu, Aug 10, 2017 at 12:04:49AM -0700, Jacob Keller wrote:

> >>   $ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
> >> git interpret-trailers --parse
> >>   Signed-off-by: Hartmut Henkel 
> >>   Helped-by: Stefan Beller 
> >>   Signed-off-by: Ralf Thielow 
> >>   Acked-by: Matthias Rüster 
> >
> > Thank-you, thank-you, thank-you.
> >
> > The above example made me wonder if we also want a format specifier
> > to do the above without piping, but it turns out that we already
> > have "log --format=%(trailers)", so we are good ;-)
> 
> I was going to say, I thought we had a way to get trailers for a
> commit via the pretty format, since that is what i used in the past.

I do like that you could get the trailers for many commits in a single
invocation. That doesn't matter for my current use-case, but obviously
piping through O(n) interpret-trailers invocations is a bad idea.

But there are a few difficulties with using %(trailers) for this, as it
shows everything between the start/end points of the trailer block. In
particular:

  1. You don't get any kind of normalization, so you're on your own for
 parsing things like whitespace continuation, extra spaces before
 separators, etc.

  2. It prints non-trailers that fall inside the block. For instance:

   $ git commit --allow-empty -F - <<-\EOF
   subject

   body

   Signed-off-by: me
   this is not a trailer
   Signed-off-by: you
   EOF
 
   $ git log -1 --format=%B | git interpret-trailers --parse
   Signed-off-by: me
   Signed-off-by: you

   $ git log -1 --format='%(trailers)'
   Signed-off-by: me
   this is not a trailer
   Signed-off-by: you

For (1) I think many callers would prefer to see the original
formatting. Maybe we'd need a %(trailers:normalize) or something.

I'm tempted to call (2) a bug, but I guess it's unclear whether callers
would want to see the whole block, or if they really want just the
individual trailers.

-Peff

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Jacob Keller]

On Wed, Aug 9, 2017 at 10:19 AM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> 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 
>>   Helped-by: Stefan Beller 
>>   Signed-off-by: Ralf Thielow 
>>   Acked-by: Matthias Rüster 
>
> Thank-you, thank-you, thank-you.
>
> The above example made me wonder if we also want a format specifier
> to do the above without piping, but it turns out that we already
> have "log --format=%(trailers)", so we are good ;-)
>

I was going to say, I thought we had a way to get trailers for a
commit via the pretty format, since that is what i used in the past.

Thanks,
Jake

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Junio C Hamano]

Jeff King  writes:

> 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 
>   Helped-by: Stefan Beller 
>   Signed-off-by: Ralf Thielow 
>   Acked-by: Matthias Rüster 

Thank-you, thank-you, thank-you.

The above example made me wonder if we also want a format specifier
to do the above without piping, but it turns out that we already
have "log --format=%(trailers)", so we are good ;-)

> 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

[PATCH 0/5] make interpret-trailers useful for parsing

[Jeff King]

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 
  Helped-by: Stefan Beller 
  Signed-off-by: Ralf Thielow 
  Acked-by: Matthias Rüster 

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