Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [jgit-dev] A bug in WorkDirCheckout?

fredagen den 28 maj 2010 17.19.05 skrev  Christian Halstrick:
> Hi,
> 
> I am currently "porting" WorkDirCheckout to a new DirCacheCheckout to
> implement real merges.  While doing so I detected something which
> looks like a bug to me:
> 
> WorkDirCheckout.processEntry() should implement what is defined in a
> big table in the man page of "git read-tree" (see
> http://www.kernel.org/pub/software/scm/git/docs/git-read-tree.html ).
> I think that our implementation is not doing what is writen in the
> latest version of the man page in "case 3". E.g. the checkout should
> fail if the head-commit and merge-commit contain different content for
> a path but the index contains nothing for this path (means a deletion
> for this path was added to index). But our code doesn't respect that
> situation.
> 
> I tried to fix this and wrote a test which tests a certain aspect of
> "case 3". I'll attach this fix. With my fix my new test goes through
> (without my fix the test fails). But my fix breaks 3 tests in
> ReadTreeTest.java. Any ideas somebody what's going wrong?

I had to hand apply your patch, but some comments

testRules1thru3_NoIndexEntry, last part is the same as your 3b. So the 
existing test is wrong.

The other ones involved directories and workdireckout seem so do the wrong 
thing.

e.g, in. testDirectoryFileConflicts_4, the input is

	DF/DF(somecontent) 

	DF/DF(othercontent

	DF(othercontent)

DF is selected during read without regard for the fact that the other
two entries conflict due to directory/filename reasons. Then your change
kicks in and sees the two differnt DF/DF's without and index entry (already 
consumed) and decides that this is a conflict, which is right, so the test
is wrong here too, 

In testDirectoryFileConflicts_9 we have
	
		doit(mk("DF"), mkmap("DF", "QP"), mk("DF/DF"));

	DF(content)

	DF(othercontent)

	DF/DF

which is the case above, Two different files and no (matching) index, i.e. a 
conflict, which is to say that the current test is wrong.

-- robin

> >From d384452623e6447780fcc1df596665c8ab1aa272 Fri, 28 May 2010 17:18:38
> >+0200
> 
> From: Christian Halstrick <christian.halstrick@xxxxxxx>
> Date: Fri, 28 May 2010 17:14:40 +0200
> Subject: [PATCH] fix WorkDirCheckout
> 
> Signed-off-by: Christian Halstrick <christian.halstrick@xxxxxxx>
> 
> diff --git
> a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ReadTreeTest.java
> b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ReadTreeTest.java index
> 481691a..118ad54 100644
> --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ReadTreeTest.java
> +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ReadTreeTest.java
> @@ -90,6 +90,22 @@
>  		assertEquals(anotherId, readTree.updated.get("foo"));
>  	}
> 
> +	public void testRule3b_NoIndexEntry() throws IOException {
> +		HashMap<String, String> headMap, mergeMap, idxMap;
> +
> +		headMap = new HashMap<String, String>();
> +		headMap.put("foo", "foo");
> +		mergeMap = new HashMap<String, String>();
> +		mergeMap.put("foo", "fox");
> +		idxMap = new HashMap<String, String>();
> +
> +		setupCase(headMap, mergeMap, idxMap);
> +		theReadTree = go();
> +
> +		assertTrue(theReadTree.updated.isEmpty());
> +		assertTrue(theReadTree.conflicts.contains("foo"));
> +	}
> +
>  	void setupCase(HashMap<String, String> headEntries,
>  			HashMap<String, String> mergeEntries,
>  			HashMap<String, String> indexEntries) throws IOException {
> diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/WorkDirCheckout.java
> b/org.eclipse.jgit/src/org/eclipse/jgit/lib/WorkDirCheckout.java
> index ee78202..d2f75f9 100644
> --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/WorkDirCheckout.java
> +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/WorkDirCheckout.java
> @@ -318,13 +318,19 @@
>  			        0 nothing             nothing  nothing  (does not happen)
>  			        1 nothing             nothing  exists   use M
>  			        2 nothing             exists   nothing  remove path from 
index
> -			        3 nothing             exists   exists   use M */
> +			        3 nothing             exists   exists,  use M if "initial
> checkout", +                                                   H == M  
> keep
> index otherwise
> +                                                   exists,  fail
> +                                                   H != M */
> 
>  					if (h == null) {
>  						updated.put(name,mId);
>  					} else if (m == null) {
>  						removed.add(name);
> +					} else if (!hId.equals(mId)) { // case 3b: H!=M
> +						conflicts.add(name);
>  					} else {
> +						// TODO detect "initial checkout" and act properly
>  						updated.put(name, mId);
>  					}
>  				} else if (h == null) {
> 
> --
> Git Team Provider UI (Incubation) 0.8.0.201005251513
> _______________________________________________
> jgit-dev mailing list
> jgit-dev@xxxxxxxxxxx
> https://dev.eclipse.org/mailman/listinfo/jgit-dev


Back to the top