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

• host-not-found-msg.patch

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.

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.

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

• unnamed

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

• fix_http_error.dpatch

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.

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

• unnamed

accepted

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

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.

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

• patch-preview.txt
• remove-in_depth-help-of-repo-format-error_-redundant-with-scp_s-errors.dpatch
• unnamed

+1 from me for 2.10 and for your change, too.

Ok, accepted into 2.10.