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

[Jeff King <peff@peff.net>]

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.

> A much better way, in my opinion, would be to introduce a new flag, say,
> skip_merges, and pass that to the diff_flush_patch_id() function. You
> could also consider consolidating that flag with the diff_header_only flag
> into a "flags" argument via something like

diff_flush_patch_id() doesn't care about merges; that's too late. The
change has to happen in commit_patch_id(). And the problem is not one of
passing in "skip merges" (we _always_ want to skip merges). It is rather
distinguishing the reason that commit_patch_id() told us it did not fill
in the sha1: because it was an error, or because the patch id is
undefined (one triggers a die(), the other a silent continue).

I think I laid out that path already in the cover letter of the
original. If the consensus is that this is too ugly, I can implement
that approach.

-Peff