From: Shawn O. Pearce <spearce@spearce.org>
Date: Mon, 19 Jan 2009 17:42:54 GMT
Subject: Re: [JGIT PATCH 8/8] Define a basic merge API, and a two-way tree merge strategy
Message-ID: <20090119174254.GC14053@spearce.org>
In-Reply-To: <f299b4f30901171116y216835c9jc11df2d424ee0377@mail.gmail.com>

Tomi Pakarinen <tomi.pakarinen@gmail.com> wrote:
> testTrivialTwoWay_disjointhistories() failed because merge strategy
> didn't handle missing base
> version. Am'i right?

I don't think so.
 
> @@ -119,13 +120,26 @@ protected boolean mergeImpl() throws IOException {
>  				}
> 
>  				final int modeB = tw.getRawMode(T_BASE);

Under a missing base condition modeB == 0.  So,

> -				if (modeB == modeO && tw.idEqual(T_BASE, T_OURS))
> -					add(T_THEIRS, DirCacheEntry.STAGE_0);

If modeB == 0 and modeO == 0 its not in the base and its not in ours.
Both SHA-1s will be 0{40} and are thus idEqual, so we should enter
this add(T_THEIRS) block.  Which is what you tried to write below in
your else block, isn't it?.

> -				else if (modeB == modeT && tw.idEqual(T_BASE, T_THEIRS))
> -					add(T_OURS, DirCacheEntry.STAGE_0);

Again, if modeB == 0 and modeT == 0 both SHA-1s will be 0{40} and
are idEqual, so we should enter this add(T_OURS) block if both base
and theirs are missing.  Which again is what you tried to write in
your else block.

If that isn't coming out right then perhaps tw.idEqual() is busted
for when FileMode is FileMode.MISSING (aka 0).  Granted, doing
idEqual on FileMode.MISSING is pointless and just wastes clock
cycles, but it shouldn't harm the algorithm's correctness.

> -				else {
> -					conflict();
> -					hasConflict = true;
> +				if (!FileMode.MISSING.equals(modeB)) {
> +					if (modeB == modeO && tw.idEqual(T_BASE, T_OURS))
> +						add(T_THEIRS, DirCacheEntry.STAGE_0);
> +					else if (modeB == modeT && tw.idEqual(T_BASE, T_THEIRS))
> +						add(T_OURS, DirCacheEntry.STAGE_0);
> +					else {
> +						conflict();
> +						hasConflict = true;
> +					}
> +				} else {
> +					if (!FileMode.MISSING.equals(modeO)
> +							&& FileMode.MISSING.equals(modeT))
> +						add(T_OURS, DirCacheEntry.STAGE_0);
> +					else if (FileMode.MISSING.equals(modeO)
> +							&& !FileMode.MISSING.equals(modeT))
> +						add(T_THEIRS, DirCacheEntry.STAGE_0);
> +					else {
> +						conflict();
> +						hasConflict = true;
> +					}
>  				}
>  			}
>  			builder.finish();
> -- 
> 1.6.0.4

-- 
Shawn.