Call git wip asynchronously #561

Merged
merged 4 commits into from Feb 22, 2013

3 participants

@tkf

No description provided.

@sigma
Magit member

@tarsius could you review this change ? I don't know enough about git-wip to decide if that's safe or not :)

@tarsius
Magit member

If git is already running in the repository git-wip will fail regardless whether it is called asynchronously or not. So there is a small possibility that git-wip is called but does not actually save anything. But likenesses of this happening isn't increased by calling it asynchronously.

However the condition-case in magit-wip-save-safe would not catch the error anymore so the user gets a generic error message instead of one prefixed with "Magit WIP got an error:". So I would suggest not to merge this change for now.

On the other hand there are other places in magit that have the same problem so maybe there is no reason to take special care here. In that case magit-wip-save-safe should be removed.

It should be possible to pass an error handler to magit-run-git-async. I think there are other serious problems with the magit-run... functions (mostly, but not only, related to error handling). Doing all the heavy lifting in magit-run* does not seem like such a good idea. I have considered refactoring the hole thing in the past, but that's gonna be a lot of work, with huge potential of introducing serious bugs.

@tkf

I agree that magit-wip-save-safe should be removed, but I don't think loosing the prefix from the error message is a good reason for not merging this. magit-wip-save-safe is introduced by #467 and the main purpose is to not propagate error to after-save-hook caller.

I think plugin that uses external process heavily such as Magit should rely on a good library to write asynchronous tasks like deferred.el. But as it seems Magit works fast enough for many people I guess chance for refactoring is not high...

@tarsius
Magit member

I now agree with @tkf. I forgot why magit-wip-save-safe was added.

Could you please update the pull request so @sigma can merge it.

tkf added some commits Feb 22, 2013
@tkf tkf Remove magit-wip-save-safe
As git wip is run asynchronously now, magit-wip-save
should not be raising any git-related errors.
7678bf8
@tkf tkf Fix a bug in magit-process-sentinel
magit-process-client-buffer may be killed at the time
magit-process-sentinel is called.  Therefore, a check
using buffer-live-p is added.
3748dd7
@tkf

Done. I found a bug in magit-process-sentinel while using it, so this is fixed also.

@sigma sigma merged commit 7122ff0 into magit:master Feb 22, 2013

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment