[PATCH] import: abort instead of crashing when copy source does not exist (issue5375)

Pierre-Yves David pierre-yves.david at ens-lyon.org
Sat Oct 8 17:48:39 EDT 2016



On 10/08/2016 07:06 PM, Mads Kiilerich wrote:
> On 10/08/2016 06:11 PM, Ryan McElroy wrote:
>> # HG changeset patch
>> # User Ryan McElroy <rmcelroy at fb.com>
>> # Date 1475929618 25200
>> #      Sat Oct 08 05:26:58 2016 -0700
>> # Node ID 961569a2cbeebfd54c9369c6e36d03d4938aef38
>> # Parent  dbcef8918bbdd8a64d9f79a37bcfa284a26f3a39
>> import: abort instead of crashing when copy source does not exist
>> (issue5375)
>>
>> Previously, when a patch contained a move or copy from a source that
>> did not
>> exist, `hg import` would crash. This patch changes import to raise a
>> PatchError
>> with an explanantion of what is wrong with the patch to avoid the
>> stack trace
>> and bad user experience.
>>
>> diff --git a/mercurial/patch.py b/mercurial/patch.py
>> --- a/mercurial/patch.py
>> +++ b/mercurial/patch.py
>> @@ -1952,8 +1952,10 @@ def _applydiff(ui, fp, patcher, backend,
>>                   data, mode = None, None
>>                   if gp.op in ('RENAME', 'COPY'):
>>                       data, mode = store.getfile(gp.oldpath)[:2]
>> -                    # FIXME: failing getfile has never been handled here
>> -                    assert data is not None
>> +                    if data is None:
>> +                        # This means that the old path does not exist
>> +                        raise PatchError(_("source file '%s' does not
>> exist")
>> +                                           % gp.oldpath)
>>                   if gp.mode:
>>                       mode = gp.mode
>>                       if gp.op == 'ADD':
>> diff --git a/tests/test-import.t b/tests/test-import.t
>> --- a/tests/test-import.t
>> +++ b/tests/test-import.t
>> @@ -1793,3 +1793,13 @@ repository when file not found for patch
>>     1 out of 1 hunks FAILED -- saving rejects to file file1.rej
>>     abort: patch failed to apply
>>     [255]
>> +
>> +test import crash (issue5375)
>> +  $ cd ..
>> +  $ hg init repo
>> +  $ cd repo
>> +  $ printf "diff --git a/a b/b\nrename from a\nrename to b" | hg
>> import -
>> +  applying patch from stdin
>> +  a not tracked!
>> +  abort: source file 'a' does not exist
>> +  [255]
>
> Hmm.
>
> For a patch modifying a non-existing file we already get:
>
>
> unable to find 'f' for patching
> 1 out of 1 hunks FAILED -- saving rejects to file f.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
>
> $ cat f.rej
> --- f
> +++ f
> @@ -1,1 +1,2 @@
>
> +f
>
> In the tested case, I would thus also expect it to leave a .rej file
> with the failing rename "hunk" while applying the rest of the patch
> (even though a pure rename arguably *doesn't* have any hunks).
>
> BUT the logic around this check seems wrong. A rename or copy of a
> missing file should be handled exactly the same, no matter if it is a
> bare rename/copy or if it also modifies the file (= has a first hunk).
>
> I don't know if it is better to give a not-entirely-correct abort than
> to fail with an assertion error, but I think it still deserves a
> FIXME/TODO.

+1 we should keep the same erro flow than for other patch error 
(especially because I'm not sure how --partial deal with this patch.

> (The iterhunks docstring seems wrong, for example regarding 'file'
> entries and firsthunk. Let's go find pmezard!)

TGVs for Brest leave from Gare Montparnasse every hour.

Cheers,

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list