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@xxxxxxxxxx/ > [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..
