* Possible bug regarding trailers
@ 2023-06-15 17:46 eric.frederich
2023-06-15 19:03 ` Kristoffer Haugsbakk
0 siblings, 1 reply; 11+ messages in thread
From: eric.frederich @ 2023-06-15 17:46 UTC (permalink / raw
To: git@vger.kernel.org
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible bug regarding trailers
2023-06-15 17:46 Possible bug regarding trailers eric.frederich
@ 2023-06-15 19:03 ` Kristoffer Haugsbakk
2023-06-16 4:24 ` Jeff King
2023-06-16 15:07 ` eric.frederich
0 siblings, 2 replies; 11+ messages in thread
From: Kristoffer Haugsbakk @ 2023-06-15 19:03 UTC (permalink / raw
To: eric.frederich@siemens.com; +Cc: git@vger.kernel.org, peff
Hi
On Thu, Jun 15, 2023, at 19:46, eric.frederich@siemens.com wrote:
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible bug regarding trailers
2023-06-15 19:03 ` Kristoffer Haugsbakk
@ 2023-06-16 4:24 ` Jeff King
2023-06-16 18:19 ` Christian Couder
2023-06-16 22:28 ` Possible bug regarding trailers Linus Arver
2023-06-16 15:07 ` eric.frederich
1 sibling, 2 replies; 11+ messages in thread
From: Jeff King @ 2023-06-16 4:24 UTC (permalink / raw
To: Kristoffer Haugsbakk
Cc: Christian Couder, eric.frederich@siemens.com, git@vger.kernel.org
On Thu, Jun 15, 2023 at 09:03:42PM +0200, Kristoffer Haugsbakk wrote:
> 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: Possible bug regarding trailers
2023-06-15 19:03 ` Kristoffer Haugsbakk
2023-06-16 4:24 ` Jeff King
@ 2023-06-16 15:07 ` eric.frederich
2023-06-16 18:37 ` Christian Couder
2023-06-16 23:49 ` Linus Arver
1 sibling, 2 replies; 11+ messages in thread
From: eric.frederich @ 2023-06-16 15:07 UTC (permalink / raw
To: Kristoffer Haugsbakk; +Cc: git@vger.kernel.org, peff@peff.net
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:
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible bug regarding trailers
2023-06-16 4:24 ` Jeff King
@ 2023-06-16 18:19 ` Christian Couder
2023-06-17 4:26 ` [PATCH] commit: pass --no-divider to interpret-trailers Jeff King
2023-06-16 22:28 ` Possible bug regarding trailers Linus Arver
1 sibling, 1 reply; 11+ messages in thread
From: Christian Couder @ 2023-06-16 18:19 UTC (permalink / raw
To: Jeff King
Cc: Kristoffer Haugsbakk, Christian Couder,
eric.frederich@siemens.com, git@vger.kernel.org
On Fri, Jun 16, 2023 at 6:24 AM Jeff King <peff@peff.net> wrote:
>
> 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.
> 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
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)."
> 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.
> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible bug regarding trailers
2023-06-16 15:07 ` eric.frederich
@ 2023-06-16 18:37 ` Christian Couder
2023-06-16 23:49 ` Linus Arver
1 sibling, 0 replies; 11+ messages in thread
From: Christian Couder @ 2023-06-16 18:37 UTC (permalink / raw
To: eric.frederich@siemens.com
Cc: Kristoffer Haugsbakk, git@vger.kernel.org, peff@peff.net
On Fri, Jun 16, 2023 at 6:04 PM eric.frederich@siemens.com
<eric.frederich@siemens.com> wrote:
>
> 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)?
> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible bug regarding trailers
2023-06-16 4:24 ` Jeff King
2023-06-16 18:19 ` Christian Couder
@ 2023-06-16 22:28 ` Linus Arver
2023-06-17 4:27 ` Jeff King
1 sibling, 1 reply; 11+ messages in thread
From: Linus Arver @ 2023-06-16 22:28 UTC (permalink / raw
To: Jeff King, Kristoffer Haugsbakk
Cc: Christian Couder, eric.frederich@siemens.com, git@vger.kernel.org
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:
> 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).
I would like to work on this patch. I should be able to push something
up in a couple weeks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: Possible bug regarding trailers
2023-06-16 15:07 ` eric.frederich
2023-06-16 18:37 ` Christian Couder
@ 2023-06-16 23:49 ` Linus Arver
1 sibling, 0 replies; 11+ messages in thread
From: Linus Arver @ 2023-06-16 23:49 UTC (permalink / raw
To: eric.frederich@siemens.com, Kristoffer Haugsbakk
Cc: git@vger.kernel.org, peff@peff.net
Hello Eric,
"eric.frederich@siemens.com" <eric.frederich@siemens.com> writes:
> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] commit: pass --no-divider to interpret-trailers
2023-06-16 18:19 ` Christian Couder
@ 2023-06-17 4:26 ` Jeff King
2023-06-19 15:13 ` Christian Couder
0 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2023-06-17 4:26 UTC (permalink / raw
To: Christian Couder
Cc: Junio C Hamano, Kristoffer Haugsbakk, Christian Couder,
eric.frederich@siemens.com, git@vger.kernel.org
On Fri, Jun 16, 2023 at 08:19:58PM +0200, Christian Couder wrote:
> > 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: Possible bug regarding trailers
2023-06-16 22:28 ` Possible bug regarding trailers Linus Arver
@ 2023-06-17 4:27 ` Jeff King
0 siblings, 0 replies; 11+ messages in thread
From: Jeff King @ 2023-06-17 4:27 UTC (permalink / raw
To: Linus Arver
Cc: Kristoffer Haugsbakk, Christian Couder,
eric.frederich@siemens.com, git@vger.kernel.org
On Fri, Jun 16, 2023 at 03:28:53PM -0700, Linus Arver wrote:
> > 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] commit: pass --no-divider to interpret-trailers
2023-06-17 4:26 ` [PATCH] commit: pass --no-divider to interpret-trailers Jeff King
@ 2023-06-19 15:13 ` Christian Couder
0 siblings, 0 replies; 11+ messages in thread
From: Christian Couder @ 2023-06-19 15:13 UTC (permalink / raw
To: Jeff King
Cc: Junio C Hamano, Kristoffer Haugsbakk, Christian Couder,
eric.frederich@siemens.com, git@vger.kernel.org
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!
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-06-19 15:13 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-15 17:46 Possible bug regarding trailers eric.frederich
2023-06-15 19:03 ` Kristoffer Haugsbakk
2023-06-16 4:24 ` Jeff King
2023-06-16 18:19 ` Christian Couder
2023-06-17 4:26 ` [PATCH] commit: pass --no-divider to interpret-trailers Jeff King
2023-06-19 15:13 ` Christian Couder
2023-06-16 22:28 ` Possible bug regarding trailers Linus Arver
2023-06-17 4:27 ` Jeff King
2023-06-16 15:07 ` eric.frederich
2023-06-16 18:37 ` Christian Couder
2023-06-16 23:49 ` Linus Arver
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).