Patchwork import: abort instead of crashing when copy source does not exist (issue5375)

login
register
mail settings
Submitter Mads Kiilerich
Date Oct. 8, 2016, 5:06 p.m.
Message ID <d82d21ce-3440-7891-4310-4b7bd087ef99@kiilerich.com>
Download mbox | patch
Permalink /patch/16954/
State Not Applicable
Headers show

Comments

Mads Kiilerich - Oct. 8, 2016, 5:06 p.m.
On 10/08/2016 06:11 PM, Ryan McElroy wrote:
> # HG changeset patch
> # User Ryan McElroy <rmcelroy@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

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.

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

/Mads
Pierre-Yves David - Oct. 8, 2016, 9:48 p.m.
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@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,
Ryan McElroy - Oct. 9, 2016, 11:35 a.m.
On 10/8/2016 11:48 PM, Pierre-Yves David wrote:
>
>
> 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@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,
>

1/ I can't tell if this is actually still queued or not because of the 
follow-up comments. Do I need to send a V2 that keeps the TODO/FIXME line?

2/ I think not crashing is a very good start. I'm okay opening a 
follow-up issue to move on to even better behavior, but not crashing 
seems imperative.
Pierre-Yves David - Oct. 9, 2016, 2 p.m.
On 10/09/2016 01:35 PM, Ryan McElroy wrote:
> On 10/8/2016 11:48 PM, Pierre-Yves David wrote:
>>
>>
>> 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@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,
>>
>
> 1/ I can't tell if this is actually still queued or not because of the
> follow-up comments. Do I need to send a V2 that keeps the TODO/FIXME line?

I see a changeset like yours in the hg-committed repository, so follow 
up seems appropriate.

> 2/ I think not crashing is a very good start. I'm okay opening a
> follow-up issue to move on to even better behavior, but not crashing
> seems imperative.

I agree, this is probably a good share of why this changeset is still in 
hg-committed.

Patch

--- f
+++ f
@@ -1,1 +1,2 @@ 

+f