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

2016-10-09 Thread Ryan McElroy

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




___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


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

2016-10-08 Thread Pierre-Yves David



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 
# 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


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

2016-10-08 Thread Mads Kiilerich

On 10/08/2016 06:11 PM, Ryan McElroy wrote:

# HG changeset patch
# User Ryan McElroy 
# 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.


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


/Mads
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


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

2016-10-08 Thread Augie Fackler
On Sat, Oct 08, 2016 at 09:11:38AM -0700, Ryan McElroy wrote:
> # HG changeset patch
> # User Ryan McElroy 
> # 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)

Queued, thanks

>
> 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]
> ___
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


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

2016-10-08 Thread Ryan McElroy
# HG changeset patch
# User Ryan McElroy 
# 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]
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel