Re: [PATCH 3/7] ll-merge: add API for capturing warnings in a strbuf instead of stderr

From: Jeff King <peff@peff.net>
Date: Sep 28, 2021, 22:37 UTC
Message-ID: <YVOZRhWttzF18Xql@coredump.intra.peff.net>
In-Reply-To: <2bf5efb5169b71ee7c1da006a74aa67794f1399d.1630376800.git.gitgitgadget@gmail.com>

On Tue, Aug 31, 2021 at 02:26:36AM +0000, Elijah Newren via GitGitGadget wrote:

Show 7 quoted lines

> From: Elijah Newren <newren@gmail.com>
> 
> Whenever ll-merge encounters a binary file, it prints a warning to
> stderr.  However, for the --remerge-diff option we want to add, we need
> to capture all conflict messages and show them in a diff instead of
> dumping them straight to stdout or stderr.  Add some new API that will
> allow us to capture this output and display it in our preferred method.

This is a reasonable strategy for error-handling in general (though some more thoughts below).

Show 11 quoted lines

> @@ -71,8 +75,11 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
>  	} else {
>  		switch (opts->variant) {
>  		default:
> -			warning("Cannot merge binary files: %s (%s vs. %s)",
> -				path, name1, name2);
> +			if (warnings) {
> +				strbuf_addstr(warnings, "Warning: ");
> +				strbuf_addf(warnings, msg, path, name1, name2);
> +			} else
> +				warning(msg, path, name1, name2);

The usual warn_builtin() has a lowercase "warning: " prefix, but your strbuf variant uses uppercase.

Show 8 quoted lines

> @@ -98,6 +105,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
>  
>  static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
>  			mmbuffer_t *result,
> +			struct strbuf *warnings,
>  			const char *path,
>  			mmfile_t *orig, const char *orig_name,
>  			mmfile_t *src1, const char *name1,

There's a lot of plumbing this variable through. This is probably too gross, but another option would be to call set_warn_routine() to override it temporarily. It's gross because it affects everything, not just this call stack (and I also think we'd need to beef up the warn routine code to handle some of the rough edges).

I do wonder if the ll_merge() code should avoid calling warning() in the first place. It is after all, meant to be "low-level". We already return an error code from the function. I wonder if returning a more detailed code instead, like:

  enum LL_MERGE_RESULT {
	LL_MERGE_OK = 0,
	LL_MERGE_CONFLICT,
	LL_MERGE_BINARY_CONFLICT,
  };

would let the caller do the sensible thing.

-Peff