Hi Peff, On Fri, 9 Sep 2016, Jeff King wrote: > On Fri, Sep 09, 2016 at 12:28:38PM +0200, Johannes Schindelin wrote: > > > I like the simplification, but I *hate* the fact that the calling code has > > *no way* to inform the user about the proper next steps. > > > > You are touching code that is really quite at the bottom of a lot of call > > chains. For example in the one of `git pull --rebase`. I just spent an > > insane amount of time trying to make sure that this command will not > > simply die() somewhere deep in the code, leaving the user puzzled. > > > > Please see 3be18b4 (t5520: verify that `pull --rebase` shows the helpful > > advice when failing, 2016-07-26) for more details. > > Yes, I agree that this is the opposite direction of libification. And I > agree that the current message is not very helpful. > > But I am not sure that returning the error up the stack will actually > help somebody move forward. The reason these are all die() calls in the > rest of the diff code is that they are generally indicative of > unrecoverable repository corruption. So any advice does not really > depend on what operation you are performing; it is always "stop what you > are doing immediately, run fsck, and try to get the broken objects from > somebody else". > > So IMHO, on balance this is not hurting anything. Well, you make such a situation even worse than it already is. It would be one thing to change the code to actually say "stop what you are doing immediately, run `git fsck` and try to get the broken objects from somewhere else", *before* saying how to proceed after that. But that is not what your patch does. What your patch does is to remove *even the possibility* of saying how to proceed after getting the repository corruption fixed. And instead of saying how the corruption could be fixed, it outputs a terse "cannot read files to diff". I do not think that is a wise direction. Ciao, Dscho