bug? illegal text in commit log

It seems that it's not too hard to make commits such that the standard way of git format-patch -> email -> git am fails.

For example:

[mst@tuck ~]$ mkdir gittest
[mst@tuck ~]$ cd gittest/
[mst@tuck gittest]$ git init
Initialized empty Git repository in /home/mst/gittest/.git/
[mst@tuck gittest]$ echo AA > a.txt
[mst@tuck gittest]$ git add a.txt
[mst@tuck gittest]$ git commit
[master (root-commit) 46e2cd1] original
 1 file changed, 1 insertion(+)
 create mode 100644 a.txt
[mst@tuck gittest]$ echo AA > b.txt
[mst@tuck gittest]$ echo BB > c.txt
[mst@tuck gittest]$ diff -u b.txt c.txt > bc.diff
[mst@tuck gittest]$ echo "this change" > commitlog.txt
[mst@tuck gittest]$ echo >> commitlog.txt
[mst@tuck gittest]$ cat bc.diff >> commitlog.txt
[mst@tuck gittest]$ echo BB >> a.txt
[mst@tuck gittest]$ git commit -a -F commitlog.txt 
[master c5f40b2] this change
 1 file changed, 1 insertion(+)
[mst@tuck gittest]$ git format-patch HEAD~1..
0001-this-change.patch
[mst@tuck gittest]$ git reset --hard HEAD~1
HEAD is now at 46e2cd1 original
[mst@tuck gittest]$ git am 0001-this-change.patch
Applying: this change
error: sha1 information is lacking or useless (c.txt).
error: could not build fake ancestor
Patch failed at 0001 this change
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

A work around is "don't do it" - avoid putting anything that looks like a unified diff in the commit log.

However:
- Users don't know what other restrictions exist
- User sending the patch has no way to detect failure,
  it's only visible to user receiving the patch

Ideas:
- validate commit log and warn users?
- find a way to escape text in git format-patch, and unescape in git am?

Thanks,

Am 04.02.20 um 07:14 schrieb Michael S. Tsirkin:

Putting a diff in a commit message can be useful, and forbidding it is hard to justify.

That's only better than the status quo insofar it turns the issue from a hidden pitfall into an open one.

Like a Lines: header specifying the number of lines in the commit message?

René

René Scharfe <l.s.r@web.de> writes:

I think the existing practice is to indent such a diff in the message by a few characters. And I think that makes sense even without counting its value "as a workaround"---we indent materials inserted in the message for illustration, and a block of diff is just that.

On Tue, Feb 04, 2020 at 01:25:40PM -0800, Junio C Hamano wrote:

So you are for validating commit log and warning users then?

It might be intended for other purposes, for example instructions:

To try out the new foobar flag, execute the following script:

git apply --foobar << EOF --- a.txt 2020-02-04 01:09:27.927026571 -0500 +++ c.txt 2020-02-04 01:06:57.404688233 -0500

"Michael S. Tsirkin" <mst@redhat.com> writes:

Sorry, but that sounds like a made-up example to me.

Why would anybody write that in the log message, instead of placing it in the documentation? The log message will be lost to those who receive tarballs, and even to those who clone the source it needs active effort to dig one particular change out.

René Scharfe <l.s.r@web.de> writes:

The only part of the workflow that can count the number reliably is format-patch. But the output of format-patch is designed to be edited further, and expecting users to adjust the number when they make such an edit is a bit too much.

As nobody in the discussion seem to care any e-mail source other than format-patch, we may probably be able to teach mailinfo that

 * the first line that 
   - begins with "diff " or "Index", or
   - is "---" (and nothing else on the line)
   signals that the line no longer is part of the log

 * but if it finds a line that begins with "diff --git" (or
   optionally just "diff "), do not blindly assume that is the end
   of the log, and instead try to find the first "---" line.  If
   there isn't any "---", then take that "diff" line the beginning
   of the patch, but if there is, "---" is the end of the message.

The latter rule is the new one. And there is no need to change format-patch output.

Am 06.02.20 um 17:45 schrieb Junio C Hamano:

I didn't think of that, but certainly did edit the output of format-patch before myself. I suspect the effort to update the header would be bearable, as it doesn't need to be exactly right and is only needs to shield the part that looks like a patch.

We could not actually use "Lines" as the name for the header, by the way, as it's already defined to be the number of lines in the whole email. Mailinfo would ignored any patches sent using a MUA that adds such headers.

Anyway, adding a header would not cover the case where someone adds a diff manually (and without indentation).

I like this idea. It will probably be tricky to implement, though, as mailinfo currently goes through the input line by line and has no easy way to look ahead.

René

René Scharfe <l.s.r@web.de> writes:

Another issue with the approach is that it will be fooled if the patch is about removing a line with double-dash and nothing else on it. Unless we can trust the numbers on hunk header lines in the "sample patch" embedded in the log message, we cannot reliably tell if a line with "---" on it is such a line, or the true end of the log message.

Am 07.02.20 um 06:40 schrieb Junio C Hamano:

Such a patch could be cited in the message as well, with or without a correct hunk header.

Full patches in messages might be rare, lines starting with "diff -" (perhaps talking about a certain diff invocation) or "Index: " (in a language where nouns are capitalized) are probably more likely to occur in the wild.

My problem with the situation is that innocent-looking commits that can be diffed, pushed and pulled suddenly fall apart when used in a mail-based workflow or with rebase am.

Supporting hand-edited messages is especially hard. Git cannot warn the sender, as it's out of the loop.

Bottling the message by adding some kind of header would be watertight, but senders of patch-like messages would need to take care to use a large enough bottle (update the header). Clever rules can cover common cases, but will leak sometimes and might be hard to implement.

René

On Fri, Feb 07, 2020 at 09:30:59PM +0100, René Scharfe wrote:

Maybe we could automatically write ">diff" in such mails? :)

Speaking of which, try this:

  git init
  echo base >file
  git add file
  git commit -m base

  echo change >file
  {
    echo subject
    echo
    echo "From fc4ee3386d6db204b00b45889ef42400b9e18eed Mon Sep 17 00:00:00 2001"
  } | git commit -a -F -

  git format-patch --stdout -1 >patch
  git reset --hard HEAD^
  git am patch

mboxrd fixes this, but it's not the default.

-Peff

On Thu, Feb 06, 2020 at 08:45:11AM -0800, Junio C Hamano wrote:

sounds good to me. any takers?

On 06/02/20 08:45AM, Junio C Hamano wrote:

I haven't thought this through, but I'll throw the idea out anyway:

How about counting lines in the diff (the one generated by format-patch, not the one in the commit message) instead of lines in the commit message? This way, the bottom n lines in the mail (barring the signature) are the diff contents.

I personally have often edited the commit message and added comments below the '---' line in the patches output by format-patch, but I rarely ever manually edit the diff. It is just really easy to corrupt the patch when you manually edit it.

Am 07.02.20 um 12:02 schrieb Pratyush Yadav:

That's a good idea from a usability point of view. However, mailinfo() parses patches line by line. It doesn't know the total number of lines until it's done. (It would if the Lines header from RFC 1036 was mandatory for emails; RFC 2076 says it's "not standardized for use in e-mail".) It cannot easily go back at that point.

Reading the whole message into a buffer or a temporary file would be an option, but that would give up the nice property that patches are streamed.

René

René Scharfe <l.s.r@web.de> writes:

True, but we could do a two-pass approach, perhaps?  That is

 * The first pass does exactly the same as what today's code does,
   PLUS it prepares for the case where we thought the log message
   ended because we saw "diff -" or "Index: " before seeing "---",
   by scanning for the first "---"while running handle_patch().

   If there is no such "oops, a 'diff -' in the log message fooled
   us" event, we complete with what today's code does.

 * But if we detect such a case during the first pass, we run
   ourselves again with the same input and arguments, PLUS an extra,
   new option, which tells us that "we know '---' exists in the
   input and it *is* the patchbreak.  This of course can be done
   only when the standard input is seekable, but builtin/am.c does
   store the mail in a file, so...