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

On Sun, Mar 23, 2014 at 6:09 PM, Jeff Sipek [off-list ref] wrote:

On Fri, Mar 21, 2014 at 08:31:44AM +0100, Per Cederqvist wrote:

quoted

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

quoted

 }

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