Re: [GUILT 06/28] Fix and simplify the do_get_patch function.

[Per Cederqvist <cederp@opera.com>]

n Tue, May 6, 2014 at 9:08 PM, Jeff Sipek <jeffpc@josefsipek.net> wrote:
> On Sun, Mar 23, 2014 at 10:03:48PM +0100, Per Cederqvist wrote:
>> On Sun, Mar 23, 2014 at 6:09 PM, Jeff Sipek <jeffpc@josefsipek.net> wrote:
>>
>> > On Fri, Mar 21, 2014 at 08:31:44AM +0100, Per Cederqvist wrote:
>> >> When extracting the patch, we only want the actual patches.  We don't
>> >> want the "---" delimiter.  Simplify the extraction by simply deleting
>> >> everything before the first "diff " line.  (Use sed instead of awk to
>> >> simplify the code.)
>> >>
>> >> Without this patch, "guilt fold" and "guilt push" sometimes fails if
>> >> guilt.diffstat is true.
>
> Hrm, I just did a test and guilt-push seems to work with a patch such as:
>
> aoeuaoeu
> this is
> ---
> not a patch!
> ---
>  foo |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/foo b/foo
> index e69de29..203a557 100644
> --- a/foo
> +++ b/foo
> @@ -0,0 +1 @@
> +aoue
>
> What sort of thing breaks fold/push?

This sequence breaks:

  mkdir guiltdemo
  cd guiltdemo
  git init
  git commit --allow-empty -mstart
  guilt init
  git config guilt.diffstat true
  guilt new empty-1
  guilt new empty-2
  guilt pop
  guilt fold empty-2
  guilt pop
  guilt push

That is, create two empty patches, fold them together, pop them, and try to push
the combined (still empty) patch.

The last command fails with:

  Applying patch..empty-1
  fatal: unrecognized input
  To force apply this patch, use 'guilt push -f'

At this point, the patch series consists of a single patch, empty-1,
which contains
the five characters "-", "-", "-", newline, newline.

The 06 patch (Added test cases for "guilt fold".) contains a test case
for this issue.
Which opens a style issue. When you fix a bug, should you:

 - commit the bug fix first and the test case later, so that "git bisect" always
   finds commits where "make test" works, or
 - commit the test case first, and the bug fix later, so that it is more obvious
   that you are actually fixing a pre-existing bug, or
 - commit the test case and the bug fix in the same commit?

In this series I have committed bug fixes first and test cases later, but
I'm not convinced that is the right thing to do.

> ...
>> >> diff --git a/guilt b/guilt
>> >> index 8701481..c59cd0f 100755
>> >> --- a/guilt
>> >> +++ b/guilt
>> >> @@ -332,12 +332,7 @@ do_make_header()
>> >>  # usage: do_get_patch patchfile
>> >>  do_get_patch()
>> >>  {
>> >> -     awk '
>> >> -BEGIN{}
>> >> -/^(diff |---$|--- )/ {patch = 1}
>> >> -patch == 1 {print $0}
>> >> -END{}
>> >> -' < "$1"
>> >> +     sed -n '/^diff /,$p' < "$1"
>> >
>> > So, the thought behind this mess was to allow minimal patches to work.  The
>> > 'diff' line is *not* required by patch(1)!
>>
>> I see. I can see that this is sometimes useful, even though I think
>> it is more important that guilt actually works with the patches itself
>> creates.
>
> Fair enough.  I'm convinced that we should assume that all patches start
> with 'diff '.
>
> ...
>> I had to solve a similar problem when I wrote my utility for diffing two
>> diff files (https://git.lysator.liu.se/pdiffdiff). Based on the experience
>> I got doing that, I propose this new do_get_patch function:
> ...
>>
>> The idea is to collect lines that *might* start the patch in
>> patchheader. Once we detect the start of the patch (via a
>> line that matches "--- " or any of the mode change lines)
>> we print the patcheader and the current line. If we get a
>> line that neither looks like a patchheader nor starts a patch,
>> we discard the patcheader. This is the case of a commit
>> message with a line that starts with "diff ".
>>
>> The function above solves the issue with lines that
>> start with "diff " in the commit message.  On the other
>> hand, it introduces the same issue for lines that start
>> with for instance "old mode ".
>
> Hrm.  I'm open to revisiting the get-patch/get-header functions to address
> the ambiguity issues in the future.

Postponing that for the future seems like a good plan. I think
there are three possible ways to deal with the problem.

 - Store the commit message, diffstat, and diffs in separate files.
   I don't like this option, as it would make it a lot less convenient
   to work with the patch files.

 - Add a quoting mechanism, so that you need to write ">diff" or
   ">---" instead of just "diff" or "---" if you want to include those
   characters at the start of the line in a commit message.

 - Ignore the problem. This is what guilt has done in the past,
   and I see no reason why it needs to change *in this patch series*.

>> Actually, a smaller change that probably fixes the
>> issue with diffstat is to replace
>>
>> /^(diff |---$|--- )/ {patch = 1}
>>
>> witih
>>
>> /^(diff |--- )/ {patch = 1}
>>
>> as the problem with the original implementation is that
>> the "---\n" line that starts the diffstat should not start
>> the patch.
>
> (Thinking out loud...) I suppose there are three portions to a patch file...
>
> (1) the description
> (2) optional diffstat
> (3) the patch
>
> You just convinced me that the patch should start with '^diff '.  Currently,
> the diffstat begins with '^---$'.  Sadly, the description can contain what
> looks like the beginning of a diffstat or a patch.  In the case of
> do_get_patch, we're only interested in the patch, so we can just look for
> '^diff ' (because garbage before the diff itself seems to be ignored by
> git).  (If we wanted to allow patches without the 'diff ' line, we'd need
> '^(diff |--- )'.)
>
> I feel like I'm missing something.
>
> Jeff.

Today, I think the most reasonable thing to do is simply remove the
"---$". After all, we want the patch, not the diffstat + patch.  So this
patch would look like this:

diff --git a/guilt b/guilt
index 8701481..3fc524e 100755
--- a/guilt
+++ b/guilt
@@ -334,7 +334,7 @@ do_get_patch()
 {
     awk '
 BEGIN{}
-/^(diff |---$|--- )/ {patch = 1}
+/^(diff |--- )/ {patch = 1}
 patch == 1 {print $0}
 END{}
 ' < "$1"

    /ceder