darcs

Patch 1246 issue822: Improve error message with generalized IO type

Title issue822: Improve error message with generalized IO type
Superseder Nosy List bf, netogallo
Related Issues
Status accepted Assigned To bf
Milestone 2.10.0

Created on 2015-01-09.17:54:17 by netogallo, last changed 2015-02-20.00:07:09 by gh. Tracked on DarcsWatch.

Files
File name Status Uploaded Type Edit Remove
fix_http_error.dpatch bf, 2015-01-23.15:40:49 application/x-darcs-patch
host-not-found-msg.patch netogallo, 2015-01-09.17:54:17 text/x-patch
patch-preview.txt gh, 2015-02-18.21:45:29 text/x-darcs-patch
remove-in_depth-help-of-repo-format-error_-redundant-with-scp_s-errors.dpatch gh, 2015-02-18.21:45:29 application/x-darcs-patch
unnamed netogallo, 2015-01-11.07:34:01 text/html
unnamed netogallo, 2015-02-05.16:43:01 text/html
unnamed gh, 2015-02-18.21:45:29
See mailing list archives for discussion on individual patches.
Messages
msg17930 (view) Author: netogallo Date: 2015-01-09.17:54:17
Modified darcs to read the StdErr of scp to provide better error
messages when the program fails. Also generalized the IO type of some
functions to make scp related exceptions visible in the type.
Attachments
msg17931 (view) Author: bf Date: 2015-01-09.18:47:52
The patch looks pretty good to me. One question though: Is the
generalization to MonadIO etc really necessary to achive the goal i.e.
capture the stderr output? I don't believe so, but I may be wrong. If
not, we may still want to go there but I'd rather have a bit of
discussion about the merits of the approach before screening this.
msg17932 (view) Author: bf Date: 2015-01-09.18:53:56
I tested this with a non-existing remote repo but an existing host and get

ben@sarun[1]: .../darcs/screened > darcs pull
http://hub.darcs.net/gh/darcs-screened_111

darcs failed:  Not a repository:
http://hub.darcs.net/gh/darcs-screened_111 (CouldNotResolveHost)

This is wrong, the host exists but not the path.
msg17933 (view) Author: netogallo Date: 2015-01-11.07:34:01
Thank you for the feedback.

> darcs failed:  Not a repository:
> http://hub.darcs.net/gh/darcs-screened_111
<http://hub.darcs.net/gh/darcs-screened_111> (CouldNotResolveHost)

Indeed, thanks for pointing that out. It should say "(HTTP response code
said error 404)" as it used to be. I will look into that.

> One question though: Is the generalization to MonadIO etc really
necessary to achive the goal i.e. capture the stderr output?

No it is not necessary. The issue can be resolved w/o doing this. Two
solutions are possible:

1) Throw an exception and not make visible in the type and catch the
exception in the "tryIdentifyRepoFormat" funtion as currently done. The
disatvantage of this approach is having an effect (ie. the exeption) not
visible in the type.

2) Changing the function "copyFileOrUrl", "copySsh" and some other
functions (the ones that now have the MonadIO type) to return an Either
type. The problem here is that the functions are used in many other places
in darcs so it would require significant refactoring. This would take some
more time.

I can easily  adjust my patch and implement #1, but I don't see any
shortcommings in having the exception visible in the type.

Cheers,

Ernesto


On Fri, Jan 9, 2015 at 7:53 PM, Ben Franksen <bugs@darcs.net> wrote:

>
> Ben Franksen <benjamin.franksen@helmholtz-berlin.de> added the comment:
>
> I tested this with a non-existing remote repo but an existing host and get
>
> ben@sarun[1]: .../darcs/screened > darcs pull
> http://hub.darcs.net/gh/darcs-screened_111
>
> darcs failed:  Not a repository:
> http://hub.darcs.net/gh/darcs-screened_111 (CouldNotResolveHost)
>
> This is wrong, the host exists but not the path.
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch1246>
> __________________________________
>



-- 
Ernesto Rodriguez

Masters Student
Computer Science
Utrecht University

www.netowork.me
github.com/netogallo
Attachments
msg17941 (view) Author: bf Date: 2015-01-23.15:40:48
I have screened the follow up patch that fixed the error message when
the host exists but not the path. See attached darcs patch, sent by
Ernesto Rodriguez.
Attachments
msg18025 (view) Author: bf Date: 2015-02-05.16:37:02
So, does this resolve issue822? I think so but to make it perfect I
would like to have a test.

netogallo: Could you be bothered to write one? If not, I may do it instead.
msg18027 (view) Author: netogallo Date: 2015-02-05.16:43:01
Sure, I will write one special test for this issue. It is worth mentioning
that there exists a test that checks invalid urls when using scp which
after my changes failed because it was looking or the old error message. I
updated that test to match the new error message. Nevertheless, I will
write a test specific for this case. I will do it throughout the weekend
because today and tomorrow is really busy for me :(.

Regarding wether it solves issue822. My changes do solve the specific
problem mentioned on issue822. It only covers SCP related error messages
but issue822 only talks about scp. So under my criteria it does solve it.


Cheers,

Ernesto

On Thu, Feb 5, 2015 at 5:37 PM, Ben Franksen <bugs@darcs.net> wrote:

>
> Ben Franksen <benjamin.franksen@helmholtz-berlin.de> added the comment:
>
> So, does this resolve issue822? I think so but to make it perfect I
> would like to have a test.
>
> netogallo: Could you be bothered to write one? If not, I may do it instead.
>
> ----------
> assignedto:  -> bf
> milestone:  -> 2.10.0 HEAD
> nosy: +bf
> status: needs-review -> review-in-progress
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch1246>
> __________________________________
>



-- 
Ernesto Rodriguez

Masters Student
Computer Science
Utrecht University

www.netowork.me
github.com/netogallo
Attachments
msg18028 (view) Author: bf Date: 2015-02-05.16:55:52
accepted
msg18031 (view) Author: darcswatch Date: 2015-02-05.17:05:34
This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-5a9e64c9dd770d79c747e0ba189e5e09864873e7
msg18155 (view) Author: gh Date: 2015-02-18.21:39:45
I think we want these patches into the 2.10 release, since they indeed
improve a lot the error messages (HTTP but also scp, as described in
http://bugs.darcs.net/issue2113 ).

Also, since we can see the scp errors, I would suggest removing the "If
so, check if..." message. While it can be informative for the HTTP case,
I think most of the errors are because of a bad URI.
msg18157 (view) Author: gh Date: 2015-02-18.21:45:29
Screening it and waiting for comments.

1 patch for repository http://darcs.net:

patch a24fd37e7104be1303751b0a0396f2b1bbdd8121
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Wed Feb 18 18:42:57 ART 2015
  * remove in-depth help of repo format error, redundant with scp's errors
Attachments
msg18158 (view) Author: bf Date: 2015-02-19.00:49:29
+1 from me for 2.10 and for your change, too.
msg18179 (view) Author: gh Date: 2015-02-20.00:07:09
Ok, accepted into 2.10.
History
Date User Action Args
2015-01-09 17:54:17netogallocreate
2015-01-09 18:47:52bfsetmessages: + msg17931
2015-01-09 18:53:57bfsetmessages: + msg17932
2015-01-09 20:46:28bfsetstatus: needs-screening -> needs-review
2015-01-11 07:34:02netogallosetfiles: + unnamed
messages: + msg17933
2015-01-23 15:40:49bfsetfiles: + fix_http_error.dpatch
messages: + msg17941
2015-01-23 15:42:01darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-5a9e64c9dd770d79c747e0ba189e5e09864873e7
2015-02-05 16:37:02bfsetstatus: needs-review -> review-in-progress
assignedto: bf
messages: + msg18025
nosy: + bf
milestone: 2.10.0
2015-02-05 16:43:01netogallosetfiles: + unnamed
messages: + msg18027
2015-02-05 16:55:52bfsetstatus: review-in-progress -> accepted
messages: + msg18028
2015-02-05 17:05:34darcswatchsetmessages: + msg18031
2015-02-18 21:39:46ghsetmessages: + msg18155
2015-02-18 21:45:29ghsetfiles: + patch-preview.txt, remove-in_depth-help-of-repo-format-error_-redundant-with-scp_s-errors.dpatch, unnamed
messages: + msg18157
2015-02-19 00:49:29bfsetmessages: + msg18158
2015-02-20 00:07:09ghsetmessages: + msg18179