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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 9, 2016, 10:28 UTC
Message-ID: <alpine.DEB.2.20.1609091219350.129229@virtualbox>
In-Reply-To: <20160907220409.oowxymhvkof2xsk5@sigill.intra.peff.net>

Hi Peff,

On Wed, 7 Sep 2016, Jeff King wrote:

Show 16 quoted lines

> All of our errors come from diff_get_patch_id(), which has
> exactly three error conditions. The first is an internal
> assertion, which should be a die("BUG") in the first place.
> 
> The other two are caused by an inability to two diff blobs,
> which is an indication of a serious problem (probably
> repository corruption). All the rest of the diff subsystem
> dies immediately on these conditions. By passing up the
> error, in theory we can keep going even if patch-id is
> unable to function. But in practice this means we may
> generate subtly wrong results (e.g., by failing to correlate
> two commits). Let's just die(), as we're better off making
> it clear to the user that their repository is not
> functional.
> 
> As a result, we can simplify the calling code.

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.

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

	enum diff_flush_patch_id {
		DIFF_HEADER_ONLY = 1,
		SKIP_MERGES = 2
	}

But it is definitely not a good idea to reintroduce the bad practice of die()ing deep down in library code. I know, you want proper exception handling. We cannot have that. We use C. But die() is not a solution: it introduces new problems.

Mind you: I agree that there are serious problems in the cases you illustrated. But none of those problems give us license to leave the user utterly puzzled by not even telling them what is going on: spouting internals such as "unable to read files to diff" is *most definitely* not helping users who simply want to run a `git pull --rebase`.

Ciao, Dscho