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

[Per Cederqvist <cederp@opera.com>]

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.
>>
>> Signed-off-by: Per Cederqvist <cederp@opera.com>
>> ---
>>  guilt | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> 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.

> Is it a problem if a patch description contains a line that starts with
> 'diff '?  (The current code has this issue as well.)

Yes.

> For the record, this ambiguity is what's on several occasions made me
> consider splitting the header and the patch into separate files.  Yeah, it'd
> be messier.  :/

I really like having the header and the patches in the same file. 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:

# usage: do_get_patch patchfile
do_get_patch()
{
    awk '
BEGIN {
    patchheader = ""
    patch = 0
}
patch == 1 {
    print $0
    next
}
/^(diff |index |=|RCS file:|retrieving revision )/ {
    patchheader = patchheader $0 "\n"
    next
}
/^(--- |((new|deleted) file|old) mode )/ {
    printf "%s", patchheader
  -/^(diff |---$|--- )/ {patch = 1}  print $0
    patch = 1
}
{
    patchheader = ""
}' < "$1"

}

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

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.

    /ceder

>>  }
>>
>>  # usage: do_get_header patchfile
>> --
>> 1.8.3.1
>>
>
> --
> Defenestration n. (formal or joc.):
>   The act of removing Windows from your computer in disgust, usually
>   followed by the installation of Linux or some other Unix-like operating
>   system.