On Fri, Jun 16, 2023 at 6:24 AM Jeff King <peff@xxxxxxxx> 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.
