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

From: Per Cederqvist <cederp@opera.com>
Date: Mar 23, 2014, 21:03 UTC
Message-ID: <CAP=KgsQtJb3t1c7qK8reO6nzMJppoV-zooK16ZZcWmCirAr3ew@mail.gmail.com>
In-Reply-To: <20140323170932.GH1661@meili.valhalla.31bits.net>

On Sun, Mar 23, 2014 at 6:09 PM, Jeff Sipek <jeffpc@josefsipek.net> wrote:

Show 32 quoted lines

> 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

Show 12 quoted lines

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