From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Matheus Tavares <matheus.bernardino@usp.br>
Cc: git@vger.kernel.org, l.s.r@web.de, gitster@pobox.com
Subject: Re: [PATCH] format-patch: warn if commit msg contains a patch delimiter
Date: Mon, 05 Sep 2022 10:01:16 +0200 [thread overview]
Message-ID: <220905.864jxmme0a.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <d0b577825124ac684ab304d3a1395f3d2d0708e8.1662333027.git.matheus.bernardino@usp.br>
On Sun, Sep 04 2022, Matheus Tavares wrote:
> When applying a patch, `git am` looks for special delimiter strings
> (such as "---") to know where the message ends and the actual diff
> starts. If one of these strings appears in the commit message itself,
> `am` might get confused and fail to apply the patch properly. This has
> already caused inconveniences in the past [1][2]. To help avoid such
> problem, let's make `git format-patch` warn on commit messages
> containing one of the said strings.
>
> [1]: https://lore.kernel.org/git/20210113085846-mutt-send-email-mst@kernel.org/
> [2]: https://lore.kernel.org/git/16297305.cDA1TJNmNo@earendil/
I followed this topic with one eye, and have run into this myself in the
past. I'm not against this warning, but I wonder if we can't fix
"am/apply" to just be smarter. The cases I've seen are all ones where:
* We have a copy/pasted git diff, but we could disambiguate based on
(at least) the "---" line being a telltale for the "real" patch, and
the "X file changed..." diffstat.
* We have a not-quite-git-looking patch diff in the commit message
(which we'd normally detect and apply), as in your [2].
Couldn't we just be a bit smarter about applying these, and do a
look-ahead and find what the user meant.
Is any case, having such a warning won't "settle" this issue, as we're
able to deal with this non-ambiguity in commit objects/the push/fetch
protocol. It's just "format-patch/am" as a "wire protocol" that has this
issue.
But anyway, that's the state of the world now, so warning() about it is
fair, even if we had a fix for the "apply" part we might want to warn
for a while to note that it's an issue on older gits.
> + if (pp->check_in_body_patch_breaks) {
> + strbuf_reset(&linebuf);
> + strbuf_add(&linebuf, line, linelen);
> + if (patchbreak(&linebuf) || is_scissors_line(linebuf.buf)) {
> + strbuf_strip_suffix(&linebuf, "\n");
Hrm, it's a (small) shame that the patchbreak() function takes a "struct
strbuf" rather than a char */size_t in this case (seemingly for no good
reason, as it's "const"?).
Because of that you need to make a copy here, instead of just finding
the "\n" and using the %*s format, anyway, small potatoes.
> + warning("commit message has a patch delimiter: '%s'",
> + linebuf.buf);
Missing _()?
> +test_expect_success 'warn if commit message contains patch delimiter' '
> + >delim &&
> + git add delim &&
> + GIT_EDITOR="printf \"title\n\n---\" >" git commit &&
Maybe I'm missing something, but isn't this GIT_EDITOR/printf just
another way of saying something like:
cat >msg <<-\EOF &&
"title
---" >
EOF
git commit -F msg && ...
Untested, so maybe not..
next prev parent reply other threads:[~2022-09-05 8:24 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-04 23:12 [PATCH] format-patch: warn if commit msg contains a patch delimiter Matheus Tavares
2022-09-05 8:01 ` Ævar Arnfjörð Bjarmason [this message]
2022-09-05 10:57 ` René Scharfe
2022-09-07 14:44 ` [PATCH v2 0/2] " Matheus Tavares
2022-09-07 14:44 ` [PATCH v2 1/2] patchbreak(), is_scissors_line(): work with a buf/len pair Matheus Tavares
2022-09-07 18:20 ` Phillip Wood
2022-09-08 0:35 ` Eric Sunshine
2022-09-07 14:44 ` [PATCH v2 2/2] format-patch: warn if commit msg contains a patch delimiter Matheus Tavares
2022-09-07 18:09 ` Phillip Wood
2022-09-07 18:36 ` Junio C Hamano
2022-09-09 1:08 ` Matheus Tavares
2022-09-09 16:47 ` Junio C Hamano
2022-09-07 17:44 ` [PATCH v2 0/2] " René Scharfe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=220905.864jxmme0a.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
--cc=matheus.bernardino@usp.br \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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).