Re: [PATCH 02/12] xdiff-merge: optionally show conflicts in "diff3 -m" style

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 30, 2008, 09:29 UTC
Message-ID: <alpine.DEB.1.00.0808301115530.24820@pacific.mpi-cbg.de.mpi-cbg.de>
In-Reply-To: <1220056963-2352-3-git-send-email-gitster@pobox.com>

Hi,

On Fri, 29 Aug 2008, Junio C Hamano wrote:

Show 12 quoted lines

> diff --git a/builtin-merge-file.c b/builtin-merge-file.c
> index 3605960..5b4f020 100644
> --- a/builtin-merge-file.c
> +++ b/builtin-merge-file.c
> @@ -25,6 +27,10 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
>  		else if (!strcmp(argv[1], "-q") ||
>  				!strcmp(argv[1], "--quiet"))
>  			freopen("/dev/null", "w", stderr);
> +		else if (!strcmp(argv[1], "--diff3")) {
> +			merge_style = XDL_MERGE_DIFF3;
> +			merge_level = XDL_MERGE_EAGER;
> +		}

FWIW I do not follow your reasoning why --diff3 does not make sense for anything more eager than MERGE_EAGER. All that ZEALOUS and ZEALOUS_ALNUM (the latter of which is useless at the moment, since it is not enabled for git-merge) do is change the way the conflicting regions are displayed, but they do not leave out conflicting regions.

So I actually suspect that ZEALOUS_ALNUM will be _especially_ useful with --diff3, since it is designed to skip the single curly brackets that would disrupt the reading pleasure otherwise.

Show 18 quoted lines

> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
> index 413082e..deebe02 100644
> --- a/xdiff/xdiff.h
> +++ b/xdiff/xdiff.h
> @@ -50,10 +50,16 @@ extern "C" {
>  #define XDL_BDOP_CPY 2
>  #define XDL_BDOP_INSB 3
>  
> +/* merge simplification levels */
>  #define XDL_MERGE_MINIMAL 0
>  #define XDL_MERGE_EAGER 1
>  #define XDL_MERGE_ZEALOUS 2
>  #define XDL_MERGE_ZEALOUS_ALNUM 3
> +#define XDL_MERGE_LEVEL_MASK 0x0f
> +
> +/* merge output styles */
> +#define XDL_MERGE_DIFF3 0x8000
> +#define XDL_MERGE_STYLE_MASK 0x8000

Hmm. This is not the Linux kernel, I think we could safely pass around two integers instead of one.

Show 6 quoted lines

> @@ -91,11 +108,13 @@ static int xdl_merge_cmp_lines(xdfenv_t *xe1, int i1, xdfenv_t *xe2, int i2,
>  	return 0;
>  }
>  
> -static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
> +static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add_nl, char *dest)

You rewrapped many function headers already; I wonder why this one was left out.

The rest looks pretty much obviously correct to me; I was too lazy/ran out of time to apply the patch and look through the resulting code, though, but I guess that you searched for "i1" and "chg1" and added the code for i0 and chg0 where necessary.

So: ACK.

Ciao, Dscho