From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 26 Aug 2010 16:39:26 GMT
Subject: Re: [PATCH 2/4] ll-merge: replace flag argument with options struct
Message-ID: <7vpqx5mipt.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20100826054953.GE9708@burratino>

Jonathan Nieder <jrnieder@gmail.com> writes:

> Keeping track of the flag bits is proving more trouble than it's
> worth.  Instead, use a pointer to an options struct like most similar
> APIs do.
>
> Callers with no special requests can pass NULL to request the default
> options.
>
> Cc: Bert Wesarg <bert.wesarg@googlemail.com>
> Cc: Avery Pennarun <apenwarr@gmail.com>
> Helped-by: Justin Frankel <justin@cockos.com>
> Helped-by: Bert Wesarg <bert.wesarg@googlemail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> This time, with updated documentation.

Thanks.

> diff --git a/ll-merge.c b/ll-merge.c
> index 6bb3095..9bd3732 100644
> --- a/ll-merge.c
> +++ b/ll-merge.c
> ...
> @@ -96,14 +102,17 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,
>  			  mmfile_t *orig, const char *orig_name,
>  			  mmfile_t *src1, const char *name1,
>  			  mmfile_t *src2, const char *name2,
> -			  int flag, int marker_size)
> +			  const struct ll_merge_options *opts,
> +			  int marker_size)
>  {
>  	/* Use union favor */
> -	flag &= ~LL_OPT_FAVOR_MASK;
> -	flag |= create_ll_flag(XDL_MERGE_FAVOR_UNION);
> +	struct ll_merge_options o;
> +	assert(opts);
> +	o = *opts;
> +	o.variant = XDL_MERGE_FAVOR_UNION;
>  	return ll_xdl_merge(drv_unused, result, path_unused,
>  			    orig, NULL, src1, NULL, src2, NULL,
> -			    flag, marker_size);
> +			    &o, marker_size);
>  	return 0;

Hmph, two returns...

> @@ -337,15 +348,21 @@ int ll_merge(mmbuffer_t *result_buf,
>  	     mmfile_t *ancestor, const char *ancestor_label,
>  	     mmfile_t *ours, const char *our_label,
>  	     mmfile_t *theirs, const char *their_label,
> -	     int flag)
> +	     const struct ll_merge_options *opts)
>  {
>  	static struct git_attr_check check[2];
>  	const char *ll_driver_name = NULL;
>  	int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>  	const struct ll_merge_driver *driver;
> -	int virtual_ancestor = flag & LL_OPT_VIRTUAL_ANCESTOR;
>  
> +	if (!opts) {
> +		struct ll_merge_options default_opts = {0};
> +		return ll_merge(result_buf, path, ancestor, ancestor_label,
> +				ours, our_label, theirs, their_label,
> +				&default_opts);

Fun---expecting tail recursion elimination ;-)?

> +	}
> +
> -	if (flag & LL_OPT_RENORMALIZE) {
> +	if (opts->renormalize) {
>  		normalize_file(ancestor, path);
>  		normalize_file(ours, path);
>  		normalize_file(theirs, path);
> ...
>  		}
>  	}
>  	driver = find_ll_merge_driver(ll_driver_name);

A tangent, as this comment is not about the "richer ll-merge" series.

The above strikes me that the low level merge driver *ought* to have a say
in the use of renormalize.  For example, ll_merge_binary() may probably
not want to have its input renormalized, no?