Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 343198

Summary:	[server] Git status response should give a clear criteria on conflicting file
Product:	[ECD] Orion	Reporter:	libing wang <libingw>
Component:	Client	Assignee:	Tomasz Zarna <tomasz.zarna>
Status:	CLOSED FIXED	QA Contact:
Severity:	normal
Priority:	P3	CC:	bokowski, Szymon.Brandys
Version:	0.2
Target Milestone:	0.2
Hardware:	PC
OS:	Windows 7
Whiteboard:
Bug Depends on:	343514
Bug Blocks:	339045, 342044

Description libing wang

2011-04-18 15:45:24 EDT

As described at comment 6 in bug 339045 , we had a simple solution in git
status page to highlight a conflicting file and tell user why conflicts
happened.
I've gone through the 3 test cases described in bug 342044 and myself got
lost on test case 2 and 3.

Here are summaries of the test cases.
test case 1.Both remote repo and local repo modified the same file.
test case 2.Remote repo modified and local repo deleted the same file
test case 3.Remote repo deleted and local repo modified the same file

To summarize the response, the git status gives 4 status of the same file on test case 1 & 3 ,and 3 status on test case 2.

As client side is checking "multiple status" on every file in the
response , once we know that 3 or 4 status happens on the same file , we flag
it as conflicting and visualize it with red flag in the git status page.

The solution is kind of hacking , but we can still live with it because we know
which file is conflicting and needs to be resolved.

But lets look at the further actions.
As a user , my first reaction on the conflicting files is to see why they are conflicting.
Diff is the only way from the git status page for me to do so.

Internally , please be aware that all the /git/diff/ response gives back
multiple segments of diffs , where I can only use one of them for the 2-way
compare.

In test case 1 , I had to use the first segment from /git/diff/Default/
response as it has the "<<<<<<<" and ">>>>>>" markers.
Although not perfect but the diff tells me that the blocks with "<<<<<<<" and
">>>>>>" are conflicting places and I could manage to resolve them.

In test case 2, /git/diff/Default/ gives me 2 segments of diffs but neither of
them tells me why there is a conflict.
The first segment tells me I am changing some thing and second one tells me I
am deleting something.

In test case 3, the second segment from /git/diff/Default/ response seems
telling me the right diff but there is no internal criteria to know when I
should use which segment.

My suggestion is to rework on test case 2&3 from server response and gives
clear criteria on :
1.How to detect a conflicting file from git status response.
2.What diff should server respond to client even we are not using 3-way
compare.

If we do not resolve this , thee is hard for the user to know why there is
conflicts and how to resolve them in test case 2 and 3.

Comment 1 Tomasz Zarna

2011-04-20 11:14:15 EDT

As discussed on the call, I will look at it tomorrow morning. Sorry with the late response.

Comment 2 Tomasz Zarna

2011-04-21 07:55:41 EDT

The merge result I'm getting on the server side from JGit for test cases 2 & 3 is not what I would expect. I filed bug 343514 against JGit.

I haven't looked at the diffs, because apparently the repository is in a bad shape after the merge (pull). So I don't expect diffs to be returning anything meaningful. Bug 343514 needs to be fixed first.

In the meantime, I would like to add an extra "bucket" to /git/status response for conflicting files to make Libing's life easier ;) The only problem is that the change in JGit making it possible is not yet in their nightly build (201104191910). I will check it again in a day or two.

Comment 3 libing wang

2011-04-21 09:59:22 EDT

(In reply to comment #2)

> In the meantime, I would like to add an extra "bucket" to /git/status response
> for conflicting files to make Libing's life easier ;) The only problem is that
> the change in JGit making it possible is not yet in their nightly build
> (201104191910). I will check it again in a day or two.
Will that be falgging a file as conflicting ? If yes , the type of the conflicting (both  modified , one side deleted , etc) will be nicer.Then the  client code should stop computing the flag and the redundant response will go away .
I am asking this because I just checked in the code for showing  red border on diff blocks and overview annotation. Basically when I am parsing a file's diff with a conflicting flag , I am checking each diff hunk to see it has the special string "<<<<<" or ">>>>>".Then in the compare editor you will see them as red .So you can click on the red markers from the overview and go to the places where you want to resolve the conflicts.This works now for test case 1.
For test case 2 and 3 , if there is no special strings in the diff we have to figure out other ways to mark the conflicts.
If we can go with this , the solution in M7 is still kind of hacking but much better.

I did the same test cases in CVS and we understand that without structural compare during fetch , it is hard to compare 3-way. In another word , if a merge already happened and a conflicting file is generated , it is too late to do 3-way without knowing the common ancestor.

Comment 4 Tomasz Zarna

2011-04-21 10:36:40 EDT

(In reply to comment #3)
> Will that be falgging a file as conflicting ? If yes , the type of the
> conflicting (both  modified , one side deleted , etc) will be nicer.

There will be an extra "bucket" named "Conflicts" which will contain files with conflicts. I guess, the *type of conflict* can be still deducted from the file being present in other "buckets" i.e. "Added"+"Changed"+"Missing"+"Modified" = both modified, "Modified"+"Removed"+...=deleted by us etc.

> In another word , if a merge
> already happened and a conflicting file is generated , it is too late to do
> 3-way without knowing the common ancestor.

I totally agree it's too late in that case, but IMO it's a separate bug about being able to diff *fetched* (not pulled/merged) objects against your local repository with an option to modify the later to prevent possible conflicts. Here, we're talking about git status response when the merge has been done, no matter if it failed or succeed.

Comment 5 Tomasz Zarna

2011-05-09 07:18:40 EDT

(In reply to comment #4)
> There will be an extra "bucket" named "Conflicts" which will contain files with
> conflicts. 

It's done[1], I had to adopt to a change in JGit[2], which caused our tests to fail (bug 344930). Libing, please update the client side asap.

> I guess, the *type of conflict* can be still deducted from the file
> being present in other "buckets" i.e. "Added"+"Changed"+"Missing"+"Modified" =
> both modified, "Modified"+"Removed"+...=deleted by us etc.

This is no longer an option since guys in JGit decided that a conflicting file will not be included in any of the other "bucket"[2].

[1] http://git.eclipse.org/c/e4/org.eclipse.orion.server.git/commit/?id=fde2fe7a64a4558cac53dfb9b66c7a1ef513ecb3
[2] http://egit.eclipse.org/r/#change,3295

Comment 6 libing wang

2011-05-10 11:06:31 EDT

(In reply to comment #5)
> (In reply to comment #4)
> > There will be an extra "bucket" named "Conflicts" which will contain files with
> > conflicts. 
> 
> It's done[1], I had to adopt to a change in JGit[2], which caused our tests to
> fail (bug 344930). Libing, please update the client side asap.
> 
Client followed up on ee6aaa01e244f638b0c45889960f34be9baed69a

Comment 7 Tomasz Zarna

2011-06-13 06:49:47 EDT

(In reply to comment #2)
> The merge result I'm getting on the server side from JGit for test cases 2 & 3
> is not what I would expect. I filed bug 343514 against JGit.

Change from http://egit.eclipse.org/r/#change,3294 has fixed bug 343514. 

Libing, could you please confirm that for cases 2 & 3 you're now getting a proper response from the server (a conflict)? If so, is there anything else we should address on this bug or can we close it?

Comment 8 libing wang

2011-06-13 09:30:52 EDT

I've confirmed that both case 2 and 3 worked well.
Closing now.