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

From: Jeff King <peff@peff.net>
Date: Sep 9, 2016, 10:40 UTC
Message-ID: <20160909104007.pwki2ir6et3vvk55@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1609091219350.129229@virtualbox>

On Fri, Sep 09, 2016 at 12:28:38PM +0200, Johannes Schindelin wrote:

Show 10 quoted lines

> 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