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

[Jeff Sipek <jeffpc@josefsipek.net>]

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?

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

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

-- 
I'm somewhere between geek and normal.
		- Linus Torvalds