bug? illegal text in commit log

bug? illegal text in commit log

[Michael S. Tsirkin]

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,
-- 
MST

Re: bug? illegal text in commit log

[René Scharfe]

Am 04.02.20 um 07:14 schrieb Michael S. Tsirkin:
> It seems that it's not too hard to make commits such that
> the standard way of git format-patch -> email -> git am
> fails.

> 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

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

> Ideas:
> - validate commit log and warn users?

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

> - find a way to escape text in git format-patch, and unescape in git am?

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

René

Re: bug? illegal text in commit log

[Junio C Hamano]

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

> Am 04.02.20 um 07:14 schrieb Michael S. Tsirkin:
>> It seems that it's not too hard to make commits such that
>> the standard way of git format-patch -> email -> git am
>> fails.
>
>> 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
>
> Putting a diff in a commit message can be useful, and forbidding it is
> hard to justify.
>
>> Ideas:
>> - validate commit log and warn users?
>
> That's only better than the status quo insofar it turns the issue from
> a hidden pitfall into an open one.
>
>> - find a way to escape text in git format-patch, and unescape in git am?
>
> Like a Lines: header specifying the number of lines in the commit message?

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.

Re: bug? illegal text in commit log

[Michael S. Tsirkin]

On Tue, Feb 04, 2020 at 01:25:40PM -0800, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
> > Am 04.02.20 um 07:14 schrieb Michael S. Tsirkin:
> >> It seems that it's not too hard to make commits such that
> >> the standard way of git format-patch -> email -> git am
> >> fails.
> >
> >> 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
> >
> > Putting a diff in a commit message can be useful, and forbidding it is
> > hard to justify.
> >
> >> Ideas:
> >> - validate commit log and warn users?
> >
> > That's only better than the status quo insofar it turns the issue from
> > a hidden pitfall into an open one.
> >
> >> - find a way to escape text in git format-patch, and unescape in git am?
> >
> > Like a Lines: header specifying the number of lines in the commit message?
> 
> I think the existing practice is to indent such a diff in the
> message by a few characters.

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

>  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.

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
@@ -1 +1 @@
-AA
+BB
EOF

indenting would break the script, and make it harder for users
to follow the instructions.

-- 
MST

Re: bug? illegal text in commit log

[Junio C Hamano]

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

> 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
> @@ -1 +1 @@
> -AA
> +BB
> EOF
>
> indenting would break the script, and make it harder for users
> to follow the instructions.

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.

Re: bug? illegal text in commit log

[Junio C Hamano]

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

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

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.

Re: bug? illegal text in commit log

[René Scharfe]

Am 06.02.20 um 17:45 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Like a Lines: header specifying the number of lines in the commit message?
>
> 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.

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).

> 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.

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é

Re: bug? illegal text in commit log

[Junio C Hamano]

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

>>  * 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.
>
> 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é

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.

Re: bug? illegal text in commit log

[René Scharfe]

Am 07.02.20 um 06:40 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>>>  * 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.
>>
>> 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é
>
> 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.

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é

Re: bug? illegal text in commit log

[Jeff King]

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

> 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.

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

Re: bug? illegal text in commit log

[Michael S. Tsirkin]

On Thu, Feb 06, 2020 at 08:45:11AM -0800, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
> > Like a Lines: header specifying the number of lines in the commit message?
> 
> 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.

sounds good to me. any takers?

Re: bug? illegal text in commit log

[Pratyush Yadav]

On 06/02/20 08:45AM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
> > Like a Lines: header specifying the number of lines in the commit message?
> 
> 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.

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.

-- 
Regards,
Pratyush Yadav

Re: bug? illegal text in commit log

[René Scharfe]

Am 07.02.20 um 12:02 schrieb Pratyush Yadav:
> On 06/02/20 08:45AM, Junio C Hamano wrote:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Like a Lines: header specifying the number of lines in the commit message?
>>
>> 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.
>
> 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.

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é

Re: bug? illegal text in commit log

[Junio C Hamano]

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

> 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.

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...