From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 29 Jun 2016 21:19:55 GMT
Subject: Re: [PATCH 8/9] merge-recursive: switch to returning errors instead of dying
Message-ID: <xmqq7fd73d6s.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <06c09ab4d684c239ae4ae03373c7cc7afb3be60b.1467199553.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> @@ -743,6 +741,8 @@ static int update_file_flags(struct merge_options *o,
>  			      int update_cache,
>  			      int update_wd)
> ...
> +			ret = error_errno(_("do not know what to do with %06o %s '%s'"),
> +				mode, sha1_to_hex(sha), path);
>  free_buf:

OK, with a few more users of this label, it no longer looks so
strange to me to have this label here.

At least, match the indentation level to the existing one we see
below, though?

>  		free(buf);
>  	}
>   update_index:
> -	if (update_cache)
> +	if (!ret && update_cache)
>  		add_cacheinfo(mode, sha, path, 0, update_wd, ADD_CACHE_OK_TO_ADD);

Unlike my reaction to "if (!ret) do this" in an earlier patch, I do
think this change is sensible.  We wouldn't have reached here in the
original code if we saw errors, and some of the variable used to
call add_cacheinfo() at this point may be garbage when ret is
non-zero, i.e. we know we earlier saw an error.

> -	return 0;
> +	return ret;
>  }
> ...
>  			if ((merge_status < 0) || !result_buf.ptr)
> -				die(_("Failed to execute internal merge"));
> +				ret = error(_("Failed to execute internal merge"));
>  
> -			if (write_sha1_file(result_buf.ptr, result_buf.size,
> +			if (!ret && write_sha1_file(result_buf.ptr, result_buf.size,
>  					    blob_type, result->sha))

Likewise.

> @@ -1861,7 +1867,7 @@ static int process_entry(struct merge_options *o,
>  		 */
>  		remove_file(o, 1, path, !a_mode);
>  	} else
> -		die(_("Fatal merge failure, shouldn't happen."));
> +		return error(_("Fatal merge failure, shouldn't happen."));

Isn't this BUG()?