Re: Possible bug regarding trailers

From: Christian Couder <christian.couder@gmail.com>
Date: Jun 16, 2023, 18:19 UTC
Message-ID: <CAP8UFD2JYAFv=iTDzvn4fzwYpJHt7TBLMXV0GV1-S7tXd7Bp=A@mail.gmail.com>
In-Reply-To: <20230616042407.GA54009@coredump.intra.peff.net>

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.