Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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