git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Per Cederqvist <cederp@opera.com>
Date
May 7, 2014, 20:44 UTC
Message-ID
<CAP=KgsSTX-z4Z2qqDNtSRUwk9XT0dQ8Do3noesB=UgNRBye-ig@mail.gmail.com>
In-Reply-To
<20140506190830.GJ1655@meili.valhalla.31bits.net>
n Tue, May 6, 2014 at 9:08 PM, Jeff Sipek <jeffpc@josefsipek.net> wrote:
Show 30 quoted lines
> 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.

Show 48 quoted lines
> ...
>> >> 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*.
Show 30 quoted lines
>> 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
Previous: Jeff SipekNext: Per Cederqvist
Message 16 of 61 in “[GUILT 00/28] Teach guilt import-commit how create legal patch names, and more”
  1. Per CederqvistMar 21, 2014
  2. 01/28 The tests should not fail if guilt.diffstat is set.Per Cederqvist, Mar 21, 2014
  3. 02/28 Allow "guilt delete -f" to run from a dir which contains spaces.Per Cederqvist, Mar 21, 2014
  4. 03/28 Added test case for "guilt delete -f".Per Cederqvist, Mar 21, 2014
  5. 04/28 Allow "guilt import-commit" to run from a dir which contains spaces.Per Cederqvist, Mar 21, 2014
  6. Jeff SipekMar 23, 2014
  7. Per CederqvistMar 23, 2014
  8. Jeff SipekMar 23, 2014
  9. Per CederqvistMar 23, 2014
  10. Jeff SipekMay 6, 2014
  11. 05/28 "guilt new": Accept more than 4 arguments.Per Cederqvist, Mar 21, 2014
  12. 06/28 Fix and simplify the do_get_patch function.Per Cederqvist, Mar 21, 2014
  13. Jeff SipekMar 23, 2014
  14. Per CederqvistMar 23, 2014
  15. Jeff SipekMay 6, 2014
  16. Per CederqvistMay 7, 2014
  17. 07/28 Added test cases for "guilt fold".Per Cederqvist, Mar 21, 2014
  18. Jeff SipekMay 6, 2014
  19. Per CederqvistMay 7, 2014
  20. Jeff SipekMay 7, 2014
  21. Per CederqvistMay 8, 2014
  22. Jeff SipekMay 8, 2014
  23. 08/28 Added more test cases for "guilt new": empty patches.Per Cederqvist, Mar 21, 2014
  24. 09/28 Test suite: properly check the exit status of commands.Per Cederqvist, Mar 21, 2014
  25. 10/28 Run test_failed if the exit status of a test script is bad.Per Cederqvist, Mar 21, 2014
  26. Jeff SipekMay 6, 2014
  27. 11/28 test suite: remove pointless redirection.Per Cederqvist, Mar 21, 2014
  28. Jeff SipekMay 6, 2014
  29. 12/28 "guilt header": more robust header selection.Per Cederqvist, Mar 21, 2014
  30. 13/28 Check that "guilt header '.*'" fails.Per Cederqvist, Mar 21, 2014
  31. 14/28 Use "git check-ref-format" to validate patch names.Per Cederqvist, Mar 21, 2014
  32. 15/28 Produce legal patch names in guilt-import-commit.Per Cederqvist, Mar 21, 2014
  33. 16/28 Fix backslash handling when creating names of imported patches.Per Cederqvist, Mar 21, 2014
  34. 17/28 "guilt graph" no longer loops when no patches are applied.Per Cederqvist, Mar 21, 2014
  35. Jeff SipekMay 6, 2014
  36. 18/28 guilt-graph: Handle commas in branch names.Per Cederqvist, Mar 21, 2014
  37. Jeff SipekMay 6, 2014
  38. 19/28 Check that "guilt graph" works when working on a branch with a comma.Per Cederqvist, Mar 21, 2014
  39. Jeff SipekMay 6, 2014
  40. Jeff SipekMay 13, 2014
  41. Jeff SipekMay 13, 2014
  42. 20/28 "guilt graph": Handle patch names containing quotes.Per Cederqvist, Mar 21, 2014
  43. Eric SunshineMar 21, 2014
  44. Jeff SipekMay 6, 2014
  45. Per CederqvistMay 8, 2014
  46. 21/28 The log.decorate setting should not influence import-commit.Per Cederqvist, Mar 21, 2014
  47. Jeff SipekMay 6, 2014
  48. 22/28 The log.decorate setting should not influence patchbomb.Per Cederqvist, Mar 21, 2014
  49. Jeff SipekMay 6, 2014
  50. 23/28 The log.decorate setting should not influence guilt rebase.Per Cederqvist, Mar 21, 2014
  51. Jeff SipekMay 6, 2014
  52. 24/28 disp no longer processes backslashes.Per Cederqvist, Mar 21, 2014
  53. Jeff SipekMay 7, 2014
  54. 25/28 "guilt push" now fails when there are no more patches to push.Per Cederqvist, Mar 21, 2014
  55. Jeff SipekMay 7, 2014
  56. Per CederqvistMay 8, 2014
  57. 26/28 "guilt pop" now fails when there are no more patches to pop.Per Cederqvist, Mar 21, 2014
  58. 27/28 Minor testsuite fix.Per Cederqvist, Mar 21, 2014
  59. Jeff SipekMay 7, 2014
  60. 28/28 Added guilt.reusebranch configuration option.Per Cederqvist, Mar 21, 2014
  61. Jeff SipekMar 21, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.