RE: Possible bug regarding trailers

From: eric.frederich@siemens.com <eric.frederich@siemens.com>
Date: Jun 16, 2023, 15:07 UTC
Message-ID: <PH7PR07MB9467AD34C96AEE60D5A4369F8C58A@PH7PR07MB9467.namprd07.prod.outlook.com>
In-Reply-To: <42e4373e-b700-45a6-99cc-db2f8896d5b7@app.fastmail.com>

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