Community
Participate
Working Groups
Assume cherry-pick caused a conflict. Assume you use the merge tool to resolve the conflict. No matter whether you use the pre-merged or clean local HEAD, the merge tool suggests to adopt changes that are completely unrelated to the commit being cherry-picked. What I expect: support for adopting only the *changes* from the commit being cherry-picked. What I see: comparison of file states *including all changes* up-to and including the commit being cherry-picked. Maybe merge tool isn't the right tool in this situation, but I don't see a better option in the UI. Brainstorming some options: (A) I imagine that a much better user experience could be created by using the apply-patch wizard with this input: - local file with clean content as of local HEAD - patch generated on the fly from the commit being cherry-picked, reduced to the current file. I think it would be awesome if in the conflicting state places like the staging view would offer to fire up the the patch wizard with input like above created behind the scenes. Maybe after applying the patch I still want to use the existing merge-tool approach to review which changes from other commits I'm ignoring. (B) Thinking even more broadly, support for different kinds of changes in the merge-tool would be awesome: distinguish changes from the commit-being-cherry-picked vs. changes from earlier commits. In that case there should also be a button: "copy all non-conflicting changes from right to left, that belong to the commit-being-cherry-picked." - Admittedly quite long for a button description :) While firing up the apply-patch wizard slightly breaks the typical workflows, which use compare editors rather than modal wizard, I imagine that this approach might be more realistic implementationwise.
Could you include a screen shot of the merge tool and the unrelated changes there? I must admit that I edit the file content (with conflict markers) directly instead of using the tool.
Created attachment 245738 [details] Before the cherry-pick OK, let's let some pictures tell the story :) This is my fresh demo repo, with just one lore-ipsum file and some changes in two branches. To force the conflict I'm maintaining a changelog at the beginning of the file ... Next I will try to get exactly "Change 2", no more, no less.
Created attachment 245739 [details] Oh, we have a conflict Note, one of the goals is to get this header: Changelog - created - change 0 - made change 2 - no more - no less Next, I'm gonna fire up the merge tool
Created attachment 245740 [details] Four kinds of changes Mh, what do I learn here? 1. Top region is the conflict, I can choose between the (wrong) pre-merged content (left) or the (wrong) incoming change containing both "change 1" and "change 2" (right). 2. Next the merge tool offers to accept the incoming "change 1" in the text body. 3. The real Change 2 (look for "change 2" in the second paragraph) isn't even highlighted (it is already applied without conflict) 4. At the bottom I'm offered to unapply change 0. The only thing that makes sense to me, is the invisible change 2. Everything I see is bogus, as in: has nothing to do with cherry-picking Change 2.
Created attachment 245741 [details] Merge tool - optione "Use HEAD ..." Here I'm trying the other option of the Merge Tool (this one I normally like better). In this view I see no difference whatsoever between two "incoming" changes. Except that change 1 isn't incoming, I chose cherry-pick to *skip* that change. The tool doesn't help in skipping.
(In reply to Robin Stocker from comment #1) > Could you include a screen shot of the merge tool and the unrelated changes > there? "unrelated changes" = "change 1" Is it clearer now? > I must admit that I edit the file content (with conflict markers) > directly instead of using the tool. Don't you trust that EGit can help you? :)
Created attachment 245742 [details] Using apply-patch - conflict region For comparison: Create patch (to clipboard) and apply patch gives this wizard. Two relevant changes shown, no bogus change. In the conflict area I still see mention of "change 1", but only because that's in the direct context of the diff of Change 2 - this seems to be unavoidable. The body change 1 is correctly excluded. Nor does anybody bother with change 0.
Created attachment 245744 [details] Apply-patch, second region The second (non-conflicting) region clearly shows the incoming change 2. Like in the pre-merged version I don't have to do anything to accept this change.
I see what you mean now. In a merge, it is ok to show and compare both versions of the file, as all the changes are related to the merge. In a cherry-pick however, we only want to have *some* of the changes of the other version applied (namely those from the commit), so comparing all the contents is not helpful, but rather misleading. Bringing up the Apply Patch wizard would indeed be better for a cherry-pick. (FWIW though, the few times I used the "Apply Patch" wizard, I had some hunks that couldn't be applied, but I couldn't figure out what I had to do to finish the wizard and just handle those hunks separately later. I found the <<< >>> conflicts in the file much easier to understand.) (In reply to Stephan Herrmann from comment #6) > Don't you trust that EGit can help you? :) It helps me already by putting the <<< >>>> markers in the file. Then I can edit it using my normal editor with content assist, formatting, warnings, errors, etc and know that I just have to edit all the conflicting sections and I'm good. Try it next time :).
[Batch change] Remove pre-3.7 Target Milestones If anyone on CC list is going to fix/implement this, please assign a new 3.7+ target milestone.
Ping. I'm frequently hitting this and frankly, EGit's behavior in this situation is plain wrong and makes cherry picking unnecessarily hard and error prone. Please consider fixing this :) BTW, if preparing better input for the compare editor is not feasible, we already have an alternative proposal in bug 507229 :)
Once bug 482154 (wrong ancestor used for the merge editor for cherry-pick conflicts) is fixed additional improvements are possible: * for cherry-picks only: configure the comparison to ignore RangeDifference.LEFT (and RangeDifference.ANCESTOR) changes. * general: use neither the workpace version (merged by git/JGit, with conflict markers) _nor_ the "last HEAD" version (stage 2, ours before the merge/cherry-pick) as input but use the workspace version with all conflicts auto-resolved to the ours hunks. Already the first is a big improvement; combined with the second, it gets even better.
New Gerrit change created: https://git.eclipse.org/r/c/egit/egit/+/180111
New Gerrit change created: https://git.eclipse.org/r/c/egit/egit/+/180112
Created attachment 286478 [details] Screen recording showing how I154cbecd445ef4481a1288c87c0e6e9cf498651f behaves
(In reply to Thomas Wolf from comment #15) > Created attachment 286478 [details] > Screen recording showing how I154cbecd445ef4481a1288c87c0e6e9cf498651f > behaves This shows a cherry-pick of https://git.eclipse.org/r/c/jgit/jgit/+/181123/2 (commit 90400ca) onto current JGit master (commit ea02639d), with an EGit with https://git.eclipse.org/r/c/egit/egit/+/180112 applied This produces conflicts in two files. The conflict in PackFile.java cannot be resolved, no matter what one does -- the file has been completely refactored. But that's besides the point. The point of the video is to show the scrolling behavior I get (normal), and to show how hiding differences that occur only between left and ancestor can improve things: most irrelevant changes disappear.
(In reply to Thomas Wolf from comment #16) > (In reply to Thomas Wolf from comment #15) > > Created attachment 286478 [details] > > Screen recording showing how I154cbecd445ef4481a1288c87c0e6e9cf498651f > > behaves > > This shows a cherry-pick of https://git.eclipse.org/r/c/jgit/jgit/+/181123/2 > (commit 90400ca) onto current JGit master (commit ea02639d), with an EGit > with https://git.eclipse.org/r/c/egit/egit/+/180112 applied > > This produces conflicts in two files. The conflict in PackFile.java cannot > be resolved, no matter what one does -- the file has been completely > refactored. But that's besides the point. The point of the video is to show > the scrolling behavior I get (normal), and to show how hiding differences > that occur only between left and ancestor can improve things: most > irrelevant changes disappear. That looks very promising! Looking forward to it.
*** Bug 507229 has been marked as a duplicate of this bug. ***
Created attachment 286483 [details] Screen recording of Stephan's "lorem ipsum" example done with the proposed commits More EGit 5.12 sneak preview :-) I tried to re-do your lorem ipsum example. Looks really good :-)
(In reply to Thomas Wolf from comment #19) > Created attachment 286483 [details] > Screen recording of Stephan's "lorem ipsum" example done with the proposed > commits > > More EGit 5.12 sneak preview :-) > > I tried to re-do your lorem ipsum example. Looks really good :-) I particularly like the options to see / not see non-conflicting changes coming with the cherry-pick. Hiding is got for focused conflict resolution, seeing all changes including non-conflicting ones may help to understand the interplay of different diff hunks. Cool!
(In reply to Stephan Herrmann from comment #20) > (In reply to Thomas Wolf from comment #19) > > Created attachment 286483 [details] > > Screen recording of Stephan's "lorem ipsum" example done with the proposed > > commits > > > > More EGit 5.12 sneak preview :-) > > > > I tried to re-do your lorem ipsum example. Looks really good :-) where can I find this example ? > I particularly like the options to see / not see non-conflicting changes > coming with the cherry-pick. Hiding is got for focused conflict resolution, > seeing all changes including non-conflicting ones may help to understand the > interplay of different diff hunks. > > Cool! - looks promising, will try some examples with your latest patches - could we color the conflicting and non-conflicting hunks differently (conflicting in red) ?
(In reply to Matthias Sohn from comment #21) > where can I find this example ? In the images Stephan had attached to this bug in 2014 :-) I'll publish it on Github. > - could we color the conflicting and non-conflicting hunks differently > (conflicting in red) ? How differences are colored is hard-coded inside TextMergeViewer. Normally it shows conflicts as red. I have noticed that some are shown as normal black/gray when suppression of changes between "ours" and "ancestor" is switched off, but I don't know why. In any case I don't think there's much we can do about it. (Especially since coloring has changed in the meantime; newer Eclipse versions do it differently than Neon.3. The screen recordings are from a child Eclipse running the Neon.3 target platform.) I'd prefer to do such fine-tuning in follow-ups, maybe in the next version. I already have a few other minor UI tweaks in the pipeline. :-)
(In reply to Thomas Wolf from comment #22) > I'll publish it on Github. https://github.com/tomaswolf/jgit-loremipsum
(In reply to Thomas Wolf from comment #22) > How differences are colored is hard-coded inside TextMergeViewer. Normally > it shows conflicts as red. I have noticed that some are shown as normal > black/gray when suppression of changes between "ours" and "ancestor" is > switched off, but I don't know why. I was looking in particular at line 3 in loremipsum.txt: change 0 in "ours", "change 1" in "theirs". But also "change 1" in ancestor, thus not a conflict but a change on one side only. Hence gray. So that one's fine.
here is a screenshot showing how p4merge displays this conflict https://imgur.com/bWf1iHm
That's nice, too. Uses stage 3 as "ours" in your screenshot. Same result (modulo the coloring) in Eclipse if you * show the ancestor pane and * show the diffs between "ours" and ancestor The Eclipse differencer handles the "change 0" vs. "change 1/change 2" on line 3 a little differently; it sees "change 2" as incoming addition, and "change 0" as an outgoing change. I won't change the Eclipse diff engine. Coloration is something we could look at later. I'm not sure EGit can override the default (and if so, how), or what happens if it can and some other plug-in also does so.
(In reply to Thomas Wolf from comment #26) > That's nice, too. Uses stage 3 as "ours" in your screenshot. s/stage 3/stage 2/ of course.
(In reply to Thomas Wolf from comment #22) > (In reply to Matthias Sohn from comment #21) > > where can I find this example ? > > In the images Stephan had attached to this bug in 2014 :-) > > I'll publish it on Github. > > > - could we color the conflicting and non-conflicting hunks differently > > (conflicting in red) ? > > How differences are colored is hard-coded inside TextMergeViewer. Normally > it shows conflicts as red. I have noticed that some are shown as normal > black/gray when suppression of changes between "ours" and "ancestor" is > switched off, but I don't know why. In any case I don't think there's much > we can do about it. (Especially since coloring has changed in the meantime; > newer Eclipse versions do it differently than Neon.3. The screen recordings > are from a child Eclipse running the Neon.3 target platform.) I tried with egit-4.19.target and the lines connecting related hunks don't scroll with the text, see https://imgur.com/SQ8oBSc
(In reply to Matthias Sohn from comment #28) > I tried with egit-4.19.target and the lines connecting related hunks don't > scroll with the text, see https://imgur.com/SQ8oBSc I cannot reproduce this on OS X 10.14.6, Eclipse 2021-03 (freshly downloaded "Eclipse for Java Developers", build ID 20210312-0638), using the built-in JustJ Hotspot 15.0.2 JRE). Tried first before updating EGit: lines do scroll. Then installed EGit from the CI build at [1]: lines do scroll. Tried with different inputs for "ours": lines do scroll. Sure it's not another Big Sur bug in platform? Did you try this without these changes? BTW: any reason you post the images to an external site instead of attaching to this bug? [1] https://ci.eclipse.org/egit/job/egit.gerrit/2067/artifact/org.eclipse.egit.repository/target/repository/
(In reply to Thomas Wolf from comment #29) > (In reply to Matthias Sohn from comment #28) > > I tried with egit-4.19.target and the lines connecting related hunks don't > > scroll with the text, see https://imgur.com/SQ8oBSc > > I cannot reproduce this on OS X 10.14.6, Eclipse 2021-03 (freshly downloaded > "Eclipse for Java Developers", build ID 20210312-0638), using the built-in > JustJ Hotspot 15.0.2 JRE). > > Tried first before updating EGit: lines do scroll. Then installed EGit from > the CI build at [1]: lines do scroll. Tried with different inputs for > "ours": lines do scroll. > > Sure it's not another Big Sur bug in platform? Did you try this without > these changes? tried without your patch series and still see the same behavior, so this is probably another Big Sur issue > > BTW: any reason you post the images to an external site instead of attaching > to this bug? no, will upload it here > [1] > https://ci.eclipse.org/egit/job/egit.gerrit/2067/artifact/org.eclipse.egit. > repository/target/repository/
Created attachment 286493 [details] merge tool on 4.19 target on MacOS Big Sur
(In reply to Matthias Sohn from comment #30) > tried without your patch series and still see the same behavior, so this is > probably another Big Sur issue Would be great if you could try this with the Eclipse SDK 2020-06 RC1 [1], and then file a bug if it still shows this behavior on Big Sur. [1] https://download.eclipse.org/eclipse/downloads/drops4/S-4.20RC1-202105262310/
Gerrit change https://git.eclipse.org/r/c/egit/egit/+/180112 was merged to [master]. Commit: http://git.eclipse.org/c/egit/egit.git/commit/?id=aaed129d046863f4c76aa64beee211df594789e1
Gerrit change https://git.eclipse.org/r/c/egit/egit/+/180111 was merged to [master]. Commit: http://git.eclipse.org/c/egit/egit.git/commit/?id=5996134fd793388bf747052023bf9ffacab21072
(In reply to Thomas Wolf from comment #32) > (In reply to Matthias Sohn from comment #30) > > tried without your patch series and still see the same behavior, so this is > > probably another Big Sur issue > > Would be great if you could try this with the Eclipse SDK 2020-06 RC1 [1], > and then file a bug if it still shows this behavior on Big Sur. > > [1] > https://download.eclipse.org/eclipse/downloads/drops4/S-4.20RC1-202105262310/ I tried this version and it works properly, the lines are moving with the text being scrolled, so I can confirm this problem is fixed in 4.20RC1 :-)
(In reply to Matthias Sohn from comment #35) > ... so I can confirm this problem is fixed in 4.20RC1 :-) Cool. Probably fixed unintentionally via bug 571954.
