threads / bug / 59873

Possible bug regarding trailers

Subject: Possible bug regarding trailers

## tl;dr

11 messages between Jun 15, 2023 and Jun 19, 2023.

replies: 10people: 5as markdown or json

eric.frederich@siemens.com· Jun 15, 2023, 17:46 UTC · lore
I am able to produce a commit with a trailer which does not show up in:
    `git log --format="%(trailers:key=old-scm-change-id,valueonly,separator=%x2c) %H %T" HEAD`
But does show up in:
    `git cat-file commit HEAD | git interpret-trailers --parse`

The following script reproduces the issue. It deletes directory /tmp/foo (from previous runs if you want to change something and run it again) then creates a new git directory in /tmp/foo and operates within there. The comments at the bottom of the script were from run of the script. You'll see the "WE WILL SEE THIS TRAILER" but you will not see the "THIS TRAILER GETS LOST" even though they both show up on an explicit call to `git interpret-trailers --parse`

I'm looking for a reliable way to store some meta-data from an old SCM system as we migrate to Git. I'd prefer to utilize Git's native trailers and not have to resort to using a (foreign to Git) sqlite database or something.

Thanks, ~Eric

#!/bin/bash

rm -rf /tmp/foo mkdir /tmp/foo cd /tmp/foo git init git config user.email a@b.com git config user.name "example" git config commit.gpgSign false

echo "CHANGE" >> README.md git add . git commit -m "A bad commit

--- let's mess stuff up ---

Have fun " --trailer "old-scm-change-id:THIS TRAILER GETS LOST"

echo "CHANGE" >> README.md git add . git commit -m "A good commit

*** no problem here ***

Have fun " --trailer "old-scm-change-id:WE WILL SEE THIS TRAILER"

echo "******************** git log ********************" git log

echo "******************** git cat-file commit HEAD^ ********************" git cat-file commit HEAD^

echo "******************** git cat-file commit HEAD ********************" git cat-file commit HEAD

echo "*****" echo "*****" echo "*****" echo "*****" echo "*****"

echo -e "\n\n***** git cat-file commit HEAD^ | git interpret-trailers --parse *****" git cat-file commit HEAD^ | git interpret-trailers --parse

echo -e "\n\n***** git cat-file commit HEAD | git interpret-trailers --parse *****" git cat-file commit HEAD | git interpret-trailers --parse

echo -e "\n\n******************** git log w/ trailers in format ********************" git log --format="%(trailers:key=old-scm-change-id,valueonly,separator=%x2c) %H %T" HEAD

echo -e "\n\n******************** git version ********************" git version

# OUTPUT:

# Initialized empty Git repository in /tmp/foo/.git/ # [main (root-commit) 62d1150] A bad commit # 1 file changed, 1 insertion(+) # create mode 100644 README.md # [main 0deff83] A good commit # 1 file changed, 1 insertion(+) # ******************** git log ******************** # commit 0deff83a2955ab5bb8aef24eca77e5399feabacc # Author: example <a@b.com> # Date: Thu Jun 15 13:39:37 2023 -0400 # # A good commit # # *** no problem here *** # # Have fun # # old-scm-change-id: WE WILL SEE THIS TRAILER # # commit 62d11500db250440c190ebe60945789e13fb0bbf # Author: example <a@b.com> # Date: Thu Jun 15 13:39:37 2023 -0400 # # A bad commit # # old-scm-change-id: THIS TRAILER GETS LOST # # --- let's mess stuff up --- # # Have fun # ******************** git cat-file commit HEAD^ ******************** # tree 1dd017bdd070190005e5e162d87afebcd61e0fa8 # author example <a@b.com> 1686850777 -0400 # committer example <a@b.com> 1686850777 -0400 # # A bad commit # # old-scm-change-id: THIS TRAILER GETS LOST # # --- let's mess stuff up --- # # Have fun # ******************** git cat-file commit HEAD ******************** # tree cd83b4c038415891bff254e2954113df2f78aa62 # parent 62d11500db250440c190ebe60945789e13fb0bbf # author example <a@b.com> 1686850777 -0400 # committer example <a@b.com> 1686850777 -0400 # # A good commit # # *** no problem here *** # # Have fun # # old-scm-change-id: WE WILL SEE THIS TRAILER # ***** # ***** # ***** # ***** # ***** # # # ***** git cat-file commit HEAD^ | git interpret-trailers --parse ***** # old-scm-change-id: THIS TRAILER GETS LOST # # # ***** git cat-file commit HEAD | git interpret-trailers --parse ***** # old-scm-change-id: WE WILL SEE THIS TRAILER # # # ******************** git log w/ trailers in format ******************** # WE WILL SEE THIS TRAILER 0deff83a2955ab5bb8aef24eca77e5399feabacc cd83b4c038415891bff254e2954113df2f78aa62 # 62d11500db250440c190ebe60945789e13fb0bbf 1dd017bdd070190005e5e162d87afebcd61e0fa8 # # # ******************** git version ******************** # git version 2.41.0.187.g15d354cc3d

Kristoffer Haugsbakk· Jun 15, 2023, 19:03 UTC · re: eric.frederich@siemens.com · lore

Re: Possible bug regarding trailers

Hi
On Thu, Jun 15, 2023, at 19:46, eric.frederich@siemens.com wrote:
Show 6 quoted lines
> I am able to produce a commit with a trailer which does not show up in:
>     `git log
> --format="%(trailers:key=old-scm-change-id,valueonly,separator=%x2c) %H
> %T" HEAD`
> But does show up in:
>     `git cat-file commit HEAD | git interpret-trailers --parse`

It seems that the `--- ` (note the space) is interpreted as marking the start of the patch part (as in a patch file which contains a commit message followed by a patch).

See `trailers.c:find_patch_start` (here on d7d8841f67 (Start the 2.42 cycle, 2023-06-13):

	for (s = str; *s; s = next_line(s)) {
		const char *v;
		if (skip_prefix(s, "---", &v) && isspace(*v))
			return s - str;
	}
I’m not good with C but it seems that this line will match:
    --- let's mess stuff up ---
Which instructs the code to put the trailer *before* this “patch part”.

(Or at least: if I remove this if-block then your script seems to work like you want it to.)

This seems to be in line with the documentation in `man git interpret-trailers`:

> The group must either be at the end of the message or be the last
> non-whitespace lines before a line that starts with --- (followed
> by a space or the end of the line). Such three minus signs start
> the patch part of the message. See also --no-divider below.
Note “by a space or the end of the line”.

This check used to be simpler: before it only checked for a line that started with `---`, no matter what came after on that line. But that was changed to match on `---` followed by `isspace(v*)` in c188668e38 (interpret-trailers: tighten check for "---" patch boundary, 2018-08-22). Reading the commit message it seems that the change was conservative. Maybe it would have been more strict (like demanding only lines like either `---\n` or `---\n `) if there weren’t concerns about how the behavior had been documented to match loosely up until that point.

(+Cc the commit author)
-- 
Kristoffer Haugsbakk
Jeff King· Jun 16, 2023, 04:24 UTC · re: Kristoffer Haugsbakk · lore

Re: Possible bug regarding trailers

On Thu, Jun 15, 2023 at 09:03:42PM +0200, Kristoffer Haugsbakk wrote:
Show 9 quoted lines
> This check used to be simpler: before it only checked for a line that
> started with `---`, no matter what came after on that line. But that was
> changed to match on `---` followed by `isspace(v*)` in c188668e38
> (interpret-trailers: tighten check for "---" patch boundary,
> 2018-08-22). Reading the commit message it seems that the change was
> conservative. Maybe it would have been more strict (like demanding only
> lines like either `---\n` or `---\n `) if there weren’t concerns about
> how the behavior had been documented to match loosely up until that
> point.

It's possible that we should be more strict about the separator, but I think the real bug is that we are respecting a separator at all in this example.

That "---" handling is meant for format-patch/am-style processing, where the commit is embedded in an email. And there it is an unfortunate but accepted limitation that you can't put "---" separators in your commit message, or the parsing will get confused.

But when we run "git commit --trailer", we know that we have a complete commit message, not an email. And so we should not look for "---" at all. But we do, and the commit object we write is broken (the trailer is in the wrong spot):

  $ git commit -m "A bad commit
  
  --- let's mess stuff up ---
  
  Have fun
  " --trailer "old-scm-change-id:THIS TRAILER GETS LOST"
  $ git cat-file commit HEAD^
  tree 1dd017bdd070190005e5e162d87afebcd61e0fa8
  author example <a@b.com> 1686888121 -0400
  committer example <a@b.com> 1686888121 -0400
  
  A bad commit
  
  old-scm-change-id: THIS TRAILER GETS LOST
  
  --- let's mess stuff up ---
  
  Have fun

On the display side, I think "--format=%(trailers)" is doing the right thing here; it is not respecting the "---", because it knows we have a complete commit message from the object, not something we're trying to pull out of the email format (so it finds nothing, because the trailer is not at the end). Likewise, "cat-file | interpret-trailers" is doing the right thing; by default it respects the divider. These examples probably ought to be doing:

  git cat-file commit HEAD^ |
  git interpret-trailers --parse --no-divider

to tell interpret-trailers that we are feeding it a pure commit message, not an email.

So I think the only bug is that "commit --trailer" should not respect the divider. And the fix presumably:

diff --git a/builtin/commit.c b/builtin/commit.c
index e67c4be221..9f4448f6b9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1043,7 +1043,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		struct child_process run_trailer = CHILD_PROCESS_INIT;
 
 		strvec_pushl(&run_trailer.args, "interpret-trailers",
-			     "--in-place", git_path_commit_editmsg(), NULL);
+			     "--in-place", "--no-divider",
+			     git_path_commit_editmsg(), NULL);
 		strvec_pushv(&run_trailer.args, trailer_args.v);
 		run_trailer.git_cmd = 1;
 		if (run_command(&run_trailer))

I cannot think of a reason we wouldn't want to do that (though obviously
it would need a test to become a patch). But it's possible I'm missing
some use case (e.g., would anybody feed format-patch-ish output to "git
commit --trailer ... -F" and expect it to handle the "---" divider? That
seems odd to me).

I'm adding Christian to the cc, since he wrote most of the original
interpret-trailers code and may have thoughts.

-Peff
Christian Couder· Jun 16, 2023, 18:19 UTC · re: Jeff King · lore

Re: Possible bug regarding trailers

On Fri, Jun 16, 2023 at 6:24 AM Jeff King <peff@peff.net> wrote:
Show 14 quoted lines
>
> It's possible that we should be more strict about the separator, but I
> think the real bug is that we are respecting a separator at all in this
> example.
>
> That "---" handling is meant for format-patch/am-style processing, where
> the commit is embedded in an email. And there it is an unfortunate but
> accepted limitation that you can't put "---" separators in your commit
> message, or the parsing will get confused.
>
> But when we run "git commit --trailer", we know that we have a complete
> commit message, not an email. And so we should not look for "---" at
> all. But we do, and the commit object we write is broken (the trailer is
> in the wrong spot):
Yeah, I agree with the above analysis.
Show 5 quoted lines
> On the display side, I think "--format=%(trailers)" is doing the right
> thing here; it is not respecting the "---", because it knows we have a
> complete commit message from the object, not something we're trying to
> pull out of the email format (so it finds nothing, because the trailer
> is not at the end).
Show 6 quoted lines
> Likewise, "cat-file | interpret-trailers" is doing
> the right thing; by default it respects the divider. These examples
> probably ought to be doing:
>
>   git cat-file commit HEAD^ |
>   git interpret-trailers --parse --no-divider

I agree that here we know that we will only pipe a commit message into git interpret-trailers, so we should use --no-divider. The doc for this option says:

"Use this when you know your input contains just the commit message itself (and not an email or the output of git format-patch)."

Show 20 quoted lines
> to tell interpret-trailers that we are feeding it a pure commit message,
> not an email.
>
> So I think the only bug is that "commit --trailer" should not respect
> the divider. And the fix presumably:
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index e67c4be221..9f4448f6b9 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1043,7 +1043,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>                 struct child_process run_trailer = CHILD_PROCESS_INIT;
>
>                 strvec_pushl(&run_trailer.args, "interpret-trailers",
> -                            "--in-place", git_path_commit_editmsg(), NULL);
> +                            "--in-place", "--no-divider",
> +                            git_path_commit_editmsg(), NULL);
>                 strvec_pushv(&run_trailer.args, trailer_args.v);
>                 run_trailer.git_cmd = 1;
>                 if (run_command(&run_trailer))
Yeah, that looks like the right fix.
Show 5 quoted lines
> I cannot think of a reason we wouldn't want to do that (though obviously
> it would need a test to become a patch). But it's possible I'm missing
> some use case (e.g., would anybody feed format-patch-ish output to "git
> commit --trailer ... -F" and expect it to handle the "---" divider? That
> seems odd to me).
Yeah, I don't think you have missed a use case.
Jeff King· Jun 17, 2023, 04:26 UTC · re: Christian Couder · lore

[PATCH] commit: pass --no-divider to interpret-trailers

On Fri, Jun 16, 2023 at 08:19:58PM +0200, Christian Couder wrote:
Show 6 quoted lines
> > But when we run "git commit --trailer", we know that we have a complete
> > commit message, not an email. And so we should not look for "---" at
> > all. But we do, and the commit object we write is broken (the trailer is
> > in the wrong spot):
> 
> Yeah, I agree with the above analysis.
Thanks for confirming. Here it is with a test and commit message.
-- >8 --
Subject: [PATCH] commit: pass --no-divider to interpret-trailers

When git-commit sees any "--trailer" options, it passes the COMMIT_EDITMSG file through git-interpret-trailers. But it does so without passing --no-divider, which means that interpret-trailers will look for a "---" divider to signal the end of the commit message.

That behavior doesn't make any sense in this context; we know we have a complete and solitary commit message, not something we have to further parse. And as a result, we'll do the wrong thing if the commit message contains a "---" marker (which otherwise is not syntactically significant), inserting any new trailers at the wrong spot.

We can fix this by passing --no-divider. This is the exact situation for which it was added in 1688c9a489 (interpret-trailers: allow suppressing "---" divider, 2018-08-22). As noted in the message for that commit, it just adds the mechanism, and further patches were needed to trigger it from various callers. We did that back then in a few spots, like ffce7f590f (sequencer: ignore "---" divider when parsing trailers, 2018-08-22), but obviously missed this one.

Reported-by: <eric.frederich@siemens.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/commit.c            |  3 ++-
 t/t7502-commit-porcelain.sh | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index e67c4be221..9f4448f6b9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1043,7 +1043,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		struct child_process run_trailer = CHILD_PROCESS_INIT;
 
 		strvec_pushl(&run_trailer.args, "interpret-trailers",
-			     "--in-place", git_path_commit_editmsg(), NULL);
+			     "--in-place", "--no-divider",
+			     git_path_commit_editmsg(), NULL);
 		strvec_pushv(&run_trailer.args, trailer_args.v);
 		run_trailer.git_cmd = 1;
 		if (run_command(&run_trailer))
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 38a532d81c..b5bf7de7cd 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -466,6 +466,25 @@ test_expect_success 'commit --trailer with -c and command' '
 	test_cmp expected actual
 '
 
+test_expect_success 'commit --trailer not confused by --- separator' '
+	cat >msg <<-\EOF &&
+	subject
+
+	body with dashes
+	---
+	in it
+	EOF
+	git commit --allow-empty --trailer="my-trailer: value" -F msg &&
+	{
+		cat msg &&
+		echo &&
+		echo "my-trailer: value"
+	} >expected &&
+	git cat-file commit HEAD >commit.msg &&
+	sed -e "1,/^\$/d" commit.msg >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'multiple -m' '
 
 	>negative &&

-- 
2.41.0.373.g569912ea65
Christian Couder· Jun 19, 2023, 15:13 UTC · re: Jeff King · lore

Re: [PATCH] commit: pass --no-divider to interpret-trailers

On Sat, Jun 17, 2023 at 6:26 AM Jeff King <peff@peff.net> wrote:
> Thanks for confirming. Here it is with a test and commit message.
Reviewed and it looks great. Thanks!
Linus Arver· Jun 16, 2023, 22:28 UTC · re: Jeff King · lore

Re: Possible bug regarding trailers

Hello Peff,
Jeff King <peff@peff.net> writes:
> So I think the only bug is that "commit --trailer" should not respect
> the divider. And the fix presumably:
Show 7 quoted lines
> diff --git a/builtin/commit.c b/builtin/commit.c
> index e67c4be221..9f4448f6b9 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1043,7 +1043,8 @@ static int prepare_to_commit(const char  
> *index_file, const char *prefix,
>   		struct child_process run_trailer = CHILD_PROCESS_INIT;
Show 7 quoted lines
>   		strvec_pushl(&run_trailer.args, "interpret-trailers",
> -			     "--in-place", git_path_commit_editmsg(), NULL);
> +			     "--in-place", "--no-divider",
> +			     git_path_commit_editmsg(), NULL);
>   		strvec_pushv(&run_trailer.args, trailer_args.v);
>   		run_trailer.git_cmd = 1;
>   		if (run_command(&run_trailer))
> I cannot think of a reason we wouldn't want to do that (though obviously
> it would need a test to become a patch).

I would like to work on this patch. I should be able to push something up in a couple weeks.

Jeff King· Jun 17, 2023, 04:27 UTC · re: Linus Arver · lore

Re: Possible bug regarding trailers

On Fri, Jun 16, 2023 at 03:28:53PM -0700, Linus Arver wrote:
Show 5 quoted lines
> > I cannot think of a reason we wouldn't want to do that (though obviously
> > it would need a test to become a patch).
> 
> I would like to work on this patch. I should be able to push something
> up in a couple weeks.

Sorry, I had already written up the test and commit message while waiting for Christian's response. :) I just sent it to the list (though of course if you think there's more to be done, please speak up).

-Peff
eric.frederich@siemens.com· Jun 16, 2023, 15:07 UTC · re: Kristoffer Haugsbakk · lore

RE: Possible bug regarding trailers

Thanks for the reply and finding that code. I'm wondering a couple of things.

A) Is it reasonable to expect that a trailer added during commit with `git commit --trailer some-key:some-value` always be able to be retrieved regardless of the contents of the commit message? I am migrating source code history from an older SCM to Git and would like to preserve the change messages.

B)
Should anything that is retrieved via:
    `git cat-file commit $SHA | git interpret-trailers --parse`
also be displayed via:
   `git log -1 --format="%(trailers:key=some-key,valueonly,separator=%x2c) %H %T" $SHA`
... why is there a difference?  (Explicit call to interpret-trailers shows the trailer, but the log command does not).

With some minimal investigation (I added a printf at the top of find_patch_start), I noticed that find_patch_start is called during call to `git interpret-trailers` but it is NOT called during call to `git log`. This means the same code paths are not being followed in those two cases dealing w/ trailers. I would expect that it should use the same code paths in both cases.

Thanks, ~Eric

-----Original Message-----
From: Kristoffer Haugsbakk <code@khaugsbakk.name> 
Sent: Thursday, June 15, 2023 3:04 PM
To: Frederich, Eric (DI SW T&I TO CLD XCD) <eric.frederich@siemens.com>
Cc: git@vger.kernel.org; peff@peff.net
Subject: Re: Possible bug regarding trailers
Hi
On Thu, Jun 15, 2023, at 19:46, eric.frederich@siemens.com wrote:
Show 5 quoted lines
> I am able to produce a commit with a trailer which does not show up in:
>     `git log
> --format="%(trailers:key=old-scm-change-id,valueonly,separator=%x2c) 
> %H %T" HEAD` But does show up in:
>     `git cat-file commit HEAD | git interpret-trailers --parse`
It seems that the `--- ` (note the space) is interpreted as marking the start of the patch part (as in a patch file which contains a commit message followed by a patch).
See `trailers.c:find_patch_start` (here on d7d8841f67 (Start the 2.42 cycle, 2023-06-13):
	for (s = str; *s; s = next_line(s)) {
		const char *v;
		if (skip_prefix(s, "---", &v) && isspace(*v))
			return s - str;
	}
I’m not good with C but it seems that this line will match:
    --- let's mess stuff up ---
Which instructs the code to put the trailer *before* this “patch part”.
(Or at least: if I remove this if-block then your script seems to work like you want it to.)

This seems to be in line with the documentation in `man git interpret-trailers`:

> The group must either be at the end of the message or be the last 
> non-whitespace lines before a line that starts with --- (followed by a 
> space or the end of the line). Such three minus signs start the patch 
> part of the message. See also --no-divider below.
Note “by a space or the end of the line”.

This check used to be simpler: before it only checked for a line that started with `---`, no matter what came after on that line. But that was changed to match on `---` followed by `isspace(v*)` in c188668e38 (interpret-trailers: tighten check for "---" patch boundary, 2018-08-22). Reading the commit message it seems that the change was conservative. Maybe it would have been more strict (like demanding only lines like either `---\n` or `---\n `) if there weren’t concerns about how the behavior had been documented to match loosely up until that point.

(+Cc the commit author)

-- Kristoffer Haugsbakk

Christian Couder· Jun 16, 2023, 18:37 UTC · re: eric.frederich@siemens.com · lore

Re: Possible bug regarding trailers

On Fri, Jun 16, 2023 at 6:04 PM eric.frederich@siemens.com <eric.frederich@siemens.com> wrote:

Show 6 quoted lines
>
> Thanks for the reply and finding that code.
> I'm wondering a couple of things.
>
> A)
> Is it reasonable to expect that a trailer added during commit with `git commit --trailer some-key:some-value` always be able to be retrieved regardless of the contents of the commit message?
I think the following 3 conditions would be needed:
  - `git commit --trailer ...` should be fixed using a patch like the
one Peff proposed, and the trailer should be added using this fixed
command,
  -  a command that outputs only a commit message, or perhaps commits
headers and the commit message, like `git cat-file commit rev`, should
be used to retrieve the commit message that is passed to `git
interpret-trailers` for retrieving the trailer,
  - and `git interpret-trailers` should be used with both --parse and
--no-divider.
> I am migrating source code history from an older SCM to Git and would like to preserve the change messages.
Have you looked at existing tools to do that (like maybe reposurgeon)?
Show 7 quoted lines
> B)
> Should anything that is retrieved via:
>     `git cat-file commit $SHA | git interpret-trailers --parse`
> also be displayed via:
>    `git log -1 --format="%(trailers:key=some-key,valueonly,separator=%x2c) %H %T" $SHA`
>
> ... why is there a difference?  (Explicit call to interpret-trailers shows the trailer, but the log command does not).
I believe Peff explained that.
> With some minimal investigation (I added a printf at the top of find_patch_start), I noticed that find_patch_start is called during call to `git interpret-trailers` but it is NOT called during call to `git log`.
> This means the same code paths are not being followed in those two cases dealing w/ trailers.
> I would expect that it should use the same code paths in both cases.

The log command shows all the content of the commit message as is, without any parsing. So yeah it doesn't use interpret-trailers' code which is useful for modifying or parsing trailers contained in commit messages.

Linus Arver· Jun 16, 2023, 23:49 UTC · re: eric.frederich@siemens.com · lore

RE: Possible bug regarding trailers

Hello Eric,
"eric.frederich@siemens.com" <eric.frederich@siemens.com> writes:
Show 6 quoted lines
> B)
> Should anything that is retrieved via:
>      `git cat-file commit $SHA | git interpret-trailers --parse`
> also be displayed via:
>     `git log -1  
> --format="%(trailers:key=some-key,valueonly,separator=%x2c) %H %T" $SHA`
> ... why is there a difference?  (Explicit call to interpret-trailers  
> shows the trailer, but the log command does not).
For reference (using your repro script), I get:
     $ git cat-file commit HEAD~ | sed -e '1,/^$/d'
     A bad commit
     old-scm-change-id: THIS TRAILER GETS LOST
     --- let's mess stuff up ---
     Have fun
     $ git cat-file commit HEAD~ | git interpret-trailers --parse
     old-scm-change-id: THIS TRAILER GETS LOST
     $ git cat-file commit HEAD~ | git interpret-trailers --parse  
--no-divider
     $ git log -1  
--format="%(trailers:key=some-key,valueonly,separator=%x2c) %H %T" HEAD~
      a1d5cfede3cbd109a353235a495e66f86e7456e4  
1dd017bdd070190005e5e162d87afebcd61e0fa8

I agree with Peff that interpret-trailers and git-log are both behaving correctly. For

     $ git cat-file commit HEAD~ | git interpret-trailers --parse
     old-scm-change-id: THIS TRAILER GETS LOST

we detect the trailer because interpret-trailers treats the "--- let's mess stuff up ---" line as a git-format-patch-like divider separating the commit message from the patch diff. This is working as intended.

Contrast this with the invocation where we pass in "--no-divider" and interpret-trailers (correctly) treats the "---" as _not_ a divider and so considers the "old-scm-change-id: THIS TRAILER GETS LOST" line as part of the commit message's body text. As there are no trailers at the bottom, it parses no trailers and nothing is printed on the output. This is working as intended.

For
     $ git log -1  
--format="%(trailers:key=some-key,valueonly,separator=%x2c) %H %T" HEAD~
      a1d5cfede3cbd109a353235a495e66f86e7456e4  
1dd017bdd070190005e5e162d87afebcd61e0fa8

the git-log command already knows that it is dealing with input that is only the commit message. And so it behaves the same way as interpret-trailers with the "--no-divider" flag (treating the entire input as a commit message). This is working as intended.

The issue (as Peff pointed out) is the insertion of the trailer by git-commit before the "--- let's mess stuff up ---" line. Instead we should make git-commit insert the

     old-scm-change-id: THIS TRAILER GETS LOST

line _after_ the "Have fun" line. If so, then the calls to interpret-trailers and git-log should behave more intuitively.

← back to recent threads