[issue1941] test case, possible fix for built-in patch

[Patrick Mézard]

Le 10/12/09 02:14, Greg Onufer a écrit :
> This is regarding http://mercurial.selenic.com/bts/issue1941
> 
> Summary: The built-in patch implementation applies the hunks to the
> wrong lines of the file if the file in the repo has been modified to
> skew the patch line numbers and the file contains repetitive sequences
> of lines.
> 
> In my case I added a 41-line copyright/license header to the file and
> then imported a changeset from a related repo.  The result was not
> what was expected.  Old GNU patch (2.5.4, iirc) did the same thing
> (but with a scary warning), current versions (I used 2.5.8 and 2.5.9)
> of GNU patch handled it without any difficulty.
> 
> Attached are three changesets.
> 
> The first changeset modifies the patching code to warn about
> mis-ordered hunks (like GNU patch did when I used an old GNU patch)
> since that warns the user that something went horribly wrong.  For the
> case where I originally encountered this problem, the new output is:
> 
>     ...
>     Hunk #10 succeeded at 1545 (offset 41 lines).
>     Hunk #11 succeeded at 1762 (offset 41 lines).
>     misordered hunks! output would be garbled
>     Hunk #12 succeeded at 1703 (offset -26 lines).
>     Hunk #13 succeeded at 1711 (offset -26 lines).
>     ...
> 
> The second changeset modifies the built-in patch implementation to
> keep track of the offset where hunks are applied on a running basis so
> as to make a better guess at where the hunk should be applied.  This
> results in:
> 
>     ...
>     Hunk #10 succeeded at 1545 (offset 41 lines).
>     Hunk #11 succeeded at 1762 (offset 41 lines).
>     Hunk #12 succeeded at 1770 (offset 41 lines).
>     Hunk #13 succeeded at 1778 (offset 41 lines).
>     ...
> 
> Which is exactly the same as current versions of GNU patch-- both the
> patch command status output as well as resulting patched file.
> 
> The last changeset adds a test case which is a simpler version of my
> actual problematic files.   The test case works with Hg 1.4.1 if a
> recent version of GNU patch is used as an external patch program.

First, thanks for the excellent report, analysis and patches.

I missed the patch because it was prefixed like an issue notification and patches usually come with a [PATCH] prefix. If you can, try to use the patchbomb extension to contribute, it handles these things automatically, and inline patches by default which is easier for reviews.

I have folded patch 3 into 1 and pushed it in stable:

    http://hg.intevation.org/mercurial/crew/rev/9a4034b630c4

Here is an annotated version of patch 2:

> diff --git a/mercurial/patch.py b/mercurial/patch.py
> --- a/mercurial/patch.py
> +++ b/mercurial/patch.py
> @@ -286,6 +286,7 @@
>          self.hash = {}
>          self.dirty = 0
>          self.offset = 0
> +        self.lasthunk = 0
>          self.rej = []
>          self.fileprinted = False
>          self.printfile(False)
> @@ -427,9 +428,12 @@
>              if h.rmfile():
>                  self.unlink(self.fname)
>              else:
> +                if start < self.lasthunk:
> +                    self.ui.warn(_("misordered hunks! output would be garbled\n"))
>                  self.lines[start : start + h.lena] = h.new()
>                  self.offset += h.lenb - h.lena
>                  self.dirty = 1
> +                self.lasthunk = start + h.lena
>              return 0
>  
>          # ok, we couldn't match the hunk.  Lets look for offsets and fuzz it
> @@ -448,7 +452,10 @@
>                  cand = self.findlines(old[0][1:], search_start)
>                  for l in cand:
>                      if diffhelpers.testhunk(old, self.lines, l) == 0:
> +                        if l < self.lasthunk:
> +                            self.ui.warn(_("misordered hunks! output would be garbled\n"))
>                          newlines = h.new(fuzzlen, toponly)
> +                        self.lasthunk = l

Did you mean:

+                        self.lasthunk = l + len(newlines) - len(old)

here ?

>                          self.lines[l : l + len(old)] = newlines
>                          self.offset += len(newlines) - len(old)
>                          self.dirty = 1


It would be great to have a reliable test for this, appended to test-patch-offset. It could be done by switching 2 hunks of an existing diff manually. Can you resubmit patch 2 with such a test?

--
Patrick Mézard