Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make fetch/pull/push results more intuitive #684

Merged
merged 3 commits into from May 24, 2019

Conversation

Projects
None yet
3 participants
@zshipko
Copy link
Collaborator

commented May 9, 2019

Instead of #683, where the documentation doesn't really help clarify the cases that much, I've updated the API to only use `Msg of string.

@zshipko zshipko requested review from hannesm and samoht and removed request for hannesm May 9, 2019

@samoht

This comment has been minimized.

Copy link
Member

commented May 10, 2019

I agree that we should treat empty repository properly (e.g. not throw an error when it's not needed) but I'm not sure it makes sense to completely remove the error structure. But maybe there's nothing else to do than printing with these errors?

Also your patch seems to change the semantics of the return of few of the operations, do you mind giving a summary of the changes? Did you try to avoid having No_head on both the ok and the error side?

@zshipko

This comment has been minimized.

Copy link
Collaborator Author

commented May 10, 2019

Well the `Not_available error seems misleading. It seems like it would indicate that the remote is unreachable, but really signals that the action is not supported, which seems like something you would just print rather than handle. I think the best case there is to use `Msg, with a more specific message about which function is not available.

I guess the significant changes are in pull/push where `No_head is now ignored. I agree that throwing away that information is not ideal, but hadn't thought about using `No_head in the Ok case. I think that sounds like it could be a nice way to preserve the existing semantics of push/pull while cleaning up the API a little.

Just to be clear, this would look something like:

val pull :
    db ->
    ?depth:int ->
    remote ->
    [ `Merge of Info.f | `Set ] -> 
    ([`No_head | `Success], pull_error) result Lwt.t

val push :
    db -> 
    ?depth:int -> 
    remote -> 
    ([`No_head | `Success], [ `Msg of string | `Detached_head]) result Lwt.t

@zshipko zshipko force-pushed the zshipko:sync-results branch from c9264e2 to 3b17e10 May 10, 2019

@hannesm

This comment has been minimized.

Copy link
Member

commented May 13, 2019

in fetch and pull, I do understand that this may result in `No_head, but how so in push? or, the other way around, let's assume I pulled an empty repository (--> `No_head), do a commit and push, that should lead to success (and not `No_head), no?

@zshipko

This comment has been minimized.

Copy link
Collaborator Author

commented May 13, 2019

You will only get `No_head on push when the local repository (the one you're pushing from) has no commits. So your example is correct, the result will be `Success since the repo is no longer empty.

@samoht

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I'm not totally sure about No_head. Maybe something more explicit like:

type status = [ `Empty | `Head of commit ]
... (status, {push,pull}_error) result Lwt.t

What do you think?

Also, I don't remember what's the Detached_head error means :-)

@zshipko

This comment has been minimized.

Copy link
Collaborator Author

commented May 13, 2019

Yeah, returning the commit like that seems better, I will update this PR.

The Detached_head error only occurs here:

| `Commit _ -> Lwt.return (Error `Detached_head)

@zshipko

This comment has been minimized.

Copy link
Collaborator Author

commented May 14, 2019

Hmm, these test errors on CI just started showing up and I'm not sure why. I don't think it's specific to this branch because they can be seen both here and here

@zshipko

This comment has been minimized.

Copy link
Collaborator Author

commented May 22, 2019

Well, the tests are back to passing. I'm hoping the failures were related to this: ocaml/opam-repository#14154 (comment)

@zshipko zshipko merged commit 5503fd1 into mirage:master May 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zshipko zshipko deleted the zshipko:sync-results branch May 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.