Community
Participate
Working Groups
Build Identifier: 1.1.0.201109060639 The current mergetool uses the default Eclipse compare editor to merge files. It would be great if this could be made customizable and use an external tool like kdiff3 or Beyond Compare. The configuration is already present in .gitconfig so it could be shared with existing command line installation. Introduce in preferences Team>Git>Merge section a select box with the following choices - Use Eclipse compare editor - Use default git merge tool - Use custom git merge tool In case the last option is selected, an additional text box appears and allow user to input the 'id' of the merge tool (could be improved by reading all merge tools available in the configuration) Reproducible: Always Steps to Reproduce: 1. Trigger a merge between 2 branches with conflicts 2. Select a file with conflict and right click > merge tool 3. The selected merge tool should
You can add this through the external tools menu.
Robin, can you explain how to perform that configuration?
(In reply to comment #2) > Robin, can you explain how to perform that configuration? The way I was able to do it was by configuring "git mergetool" to be run from external tools menu. Still it required me to install msysgit and configured that way. For now it's a "good enough" solution for me. However I would still prefer to have it the way Vicent described.
It is not a solution at all. This is a hardly workaround. The clue of this bug is to use other (than eclipse supplied) merge/diff tool from Team->Merge submenu.
I fully agree. That's only a workaround (or ugly-hack even more).
There's news information about using a custom merge tool? Currently i'm using the p4merge in 'External Tool'. Thanks. Leandro Andrade
(In reply to Leandro Andrade from comment #6) > There's news information about using a custom merge tool? AFAIK nobody is working on this, contributions welcome ;-)
We configured some different merge tools for special extensions in .gitattributes, e.g. for IBM Rhapsody files: *.rpy diff=diffwrap *.sbs diff=diffwrap and this does not work with EGit, because of reported issue. We have to merge from commandline or TortoisGit or SourceTree, that supports this configurations. Can somebody point out where in the EGit source code the internal merge or diff tools are called? May be we can add some logic to execute the external tools...
The merge tool is opened by MergeToolActionHandler [1] Comparing file versions can be started from compare() methods in CompareUtils [2] [1] http://git.eclipse.org/c/egit/egit.git/tree/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/actions/MergeToolActionHandler.java [2] http://git.eclipse.org/c/egit/egit.git/tree/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/CompareUtils.java
Thanks! Anyway just found that .gitattributes are not supported in JGit in 3.7, patches pending: https://bugs.eclipse.org/bugs/show_bug.cgi?id=342372 The standard (internal text) merge tools are called during merge and the files are modified with blocks ">>>>>" etc. But the external Rhapsody or other merge tool maybe can overwrite this broken files. I will try to extract the external tools configurations from .gitconfig file and call the selected tool like proposed in description: ... >>> Introduce in preferences Team>Git>Merge section a select box with the >>> following choices >>> - Use Eclipse compare editor >>> - Use default git merge tool >>> - Use custom git merge tool >>> In case the last option is selected, an additional text box appears and >>> allow user to input the 'id' of the merge tool (could be improved by >>> reading all merge tools available in the configuration) ...
I've pushed first version to gerrit: https://git.eclipse.org/r/#/c/50160/1 There are some TODOs and i want a discussion about using "Bash for Windows": should we support native executables / batch files only or also Bash (Git for Windows / MSYS) bash- scripts etc.? I've done some manual tests for our workflow to have same behaviour like known from "Git for Windows"commandline. We will use this version internally for next two weeks. Your feedback is appreciated :)
Why are the proposed changes in EGit? Reading config files and reading information about externel mergers should be in JGit, or? If we add that to JGit also other environments can make use of external mergers (e.g. other IDE integrations using JGit, gerrit, ...). Or is there anything special needed for calling external mergers only available in EGit?
We need this feature in our Eclipse IDE, because we are switching from ClearCase to Git now and Git does not support this behavior. Also support of gitattributes (like merge filter for binaries etc.) is not there, but this is the second part of my effort. I've no experience with JGit, but some experience with developing Eclipse-Plugins. So i've started to use JGit-API and put the logic to EGit. You are right: the whole org.eclipse.egit.ui.internal.externaltools package (managers etc.) is the bridge between reading config with JGit and the UI in EGit. Maybe it would be better to move it to JGit. Hm, for now i'm happy that it works for our developers. May be with support of gitattributes i will have an idea how to move my contribution to JGit... Thanks!
WIP - abandoned all related changes and created ONE new patch for EGit https://git.eclipse.org/r/#/c/50998/ - prepared for moving main code to JGit
WARNING: this patchset contains 2620 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Started with transition of the code from EGit to JGit.
Andre, how is your rework coming along? Is it strictly about merges or is it going to support using git difftool for non-merge diffs as well?
I had no spare time to put it to JGit and we are still using my first patches for EGit in your company. It works as mergetool and difftool in Eclipse. The same i want to have for JGit command line: git difftool and git mergetool. I have no dedicated plans and time to work on it for next month, sorry...
I have a customer who also needs this feature, and who is willing to contribute some development days towards finishing it. I have already got some pointers from Andre; since the next steps require some contributions to JGit, I'll contact the JGit mailing list first with a suggestion and an initial contribution.
(In reply to Leif Frenzel from comment #19) > I have a customer who also needs this feature, and who is willing to > contribute some development days towards finishing it. I have already got > some pointers from Andre; since the next steps require some contributions to > JGit, I'll contact the JGit mailing list first with a suggestion and an > initial contribution. Hi, we need this feature too. What's the state of your work? Did you know if this feature will soon be in implemented ?
(In reply to Andre Bossert from comment #18) > I had no spare time to put it to JGit and we are still using my first > patches for EGit in your company. It works as mergetool and difftool in > Eclipse. The same i want to have for JGit command line: git difftool and git > mergetool. I have no dedicated plans and time to work on it for next month, > sorry... Andre, do you have some time to push your initial changes for JGit? The patch https://git.eclipse.org/r/#/c/50998/ is still for EGit only. Or if you have no time, could you briefly describe what need to be done? We've got customer request for external diff tool support in EGit, so I'm looking for something to start to work with.
Andrey, i will prepare my not yet finished patches for JGit, hopefully during this weekend, and push them to new Gerrit change. Anyway i will inform here about the status. The steps i did, briefly (also see the EGit change description for the settings). Prerequisites study description http://git-scm.com/docs/git-mergetool http://git-scm.com/docs/git-difftool 1. new integration in JGit 1.1 find a place in JGit where the git-config(s) can be read and store them to new classes / instances introduced in the EGit change (code reuse from change): * merge.tool * mergetool.<tool>.path * mergetool.<tool>.cmd * mergetool.prompt * mergetool.trustExitCode * mergetool.writeToTemp * mergetool.keepTemporaries * diff.tool * difftool.<tool>.path * difftool.<tool>.cmd * difftool.prompt * difftool.trustExitCode * difftool.writeToTemp [like mergetool, not seen in C-Git] * difftool.keepTemporaries [like mergetool, not seen in C-Git] 1.2 find a place where the configured tools can be executed --> platform independent, not like it's done now in EGit change with needed path to Bash / Windows MinGW / Git Bash, see comment "Bash path: <the path to bash> [only for windows, to be discussed]" --> use the JGit infrastructure also used for Git Hooks execution -> it should handle how start with command-line interpreter etc., needed for shell-scripts under Windows. 1.3 implement command line usage like "git difftool" or "git mergetool" in JGit 1.4 write tests for config read, checks, exceptions etc. 2. new integration in EGit 2.1 refactor EGit changes to use the new JGit API for read the configured diff/merge-tools and show them in preferences (code reuse from change) 2.2 running the tools with new JGit API if compare or merge etc. was triggered in the EGit UI (parameters, exceptions, dialogs, auto-add-to-index after mergetool run etc.) (code reuse from change) 2.3 add more features to JGit / EGit: * Show message about failed tool execution to user * Added embedded diff/merge tool definitions like known in C-Git * Added support for tool option without $BASE file * Add external merged file to the index (after a successful merge only) * Added setting to Git preferences to enable or disable "Add external merged file to the index (after a successful merge only)" I hope it helps... * Added working directory for external tool call
(In reply to Andre Bossert from comment #22) > Andrey, i will prepare my not yet finished patches for JGit, hopefully > during this weekend, and push them to new Gerrit change. Anyway i will > inform here about the status. Next weekend may be? :-) Thanks for hints.
*This* weekend? :-)
I'm on it, this week ;)
(In reply to Andre Bossert from comment #25) > I'm on it, this week ;) May be *this* week?
New Gerrit change created: https://git.eclipse.org/r/137301
(In reply to Eclipse Genie from comment #27) > New Gerrit change created: https://git.eclipse.org/r/137301 I just pushed my version of reading the git config for the difftool and mergetool parts. @Andre Bossert: If this is roughly compatible with your command line (pgm) and execute tool parts, you would not need to debug your version of read config.
New Gerrit change created: https://git.eclipse.org/r/137468
(In reply to Tim Neumann from comment #28) > (In reply to Eclipse Genie from comment #27) > > New Gerrit change created: https://git.eclipse.org/r/137301 > > I just pushed my version of reading the git config for the difftool and > mergetool parts. > > @Andre Bossert: If this is roughly compatible with your command line (pgm) > and execute tool parts, you would not need to debug your version of read > config. @Tim: Thanks, i've found some time to finish the changes. The config classes are a bit different. I've pushed first part with https://git.eclipse.org/r/137468 and will push all changes based at it. We can do refactoring or integration of you version if it fits later.
New Gerrit change created: https://git.eclipse.org/r/137481
FYI: i've prepared next two steps and will push them tonight after more testing: - Add difftool compare feature (execute external tool) - Add config reader for user-defined mergetools and combine them with difftools I would appreciate it if somebody help me with reviewing and testing.
New Gerrit change created: https://git.eclipse.org/r/137575
New Gerrit change created: https://git.eclipse.org/r/138003
New Gerrit change created: https://git.eclipse.org/r/138080
New Gerrit change created: https://git.eclipse.org/r/138410
Thank you for your work Andre. I do currently not have the time to review everything. I hope I will get to it next week. Andrey Loskutov is also very busy right now. So please excuse the lack of reviews this week.
New Gerrit change created: https://git.eclipse.org/r/139262
New Gerrit change created: https://git.eclipse.org/r/139545
@Andre: I did some more trying out of your patches today, and except for some minor problems (see comments on patches) I could do all things I tried and everything behaved as I expected. We're getting close to a very good solution! Thanks for the good work! I will continue with reviewing the code next week.
New Gerrit change created: https://git.eclipse.org/r/140275
New Gerrit change created: https://git.eclipse.org/r/140280
TODO: Keep temporary files if no change was detected after merge tool closes and users answers no to the question if the merge was successful. See https://git.eclipse.org/r/#/c/139262/7/org.eclipse.jgit/src/org/eclipse/jgit/diffmergetool/MergeToolManager.java@144
@Andre Bossert: Do you have any updates? Do you need a hand with something particular?
(In reply to Tim Neumann from comment #46) > @Andre Bossert: Do you have any updates? Do you need a hand with something > particular? I've worked at more important task for us "Git native hooks support at Windows (needed for Git LFS)", because it blocks migration to Git LFS. I will continue with this issue end of this week. The last two commits for EGit integration are prepared for better solution and i will push it then as new patch set.
I just downloaded the latest version of eclipse 2019-06 and i am unable to find an option to be able integrate my external diff and merge tool (winmerge). Can the eclipse community kindly provide the steps to integrate winmerge. Thanks.
(In reply to Prinka Gupta from comment #48) > I just downloaded the latest version of eclipse 2019-06 and i am unable to > find an option to be able integrate my external diff and merge tool > (winmerge). > > Can the eclipse community kindly provide the steps to integrate winmerge. > > Thanks. This feature is not yet completed. Currently the only way is a workaround: Installing git and using the mergetool feature of it with winmerge. Then you can add that mergetool command as an external tool to eclipse. Regards, Tim
Hi all, we (me and Tim) are starting to look into this again :-) First, I will move this bug to JGit and keep here only jgit related patches (original one https://git.eclipse.org/r/50998 and https://git.eclipse.org/r/140280 for EGit will go to the extra bug 552338 for EGit). All other patches are for JGit only and will remain here. Tim rebased the patches on master and I will re-push them to Gerrit now, with the goal to merge one-by-one as soon as the review is OK. After that we will see that we re-work / finish EGit patch https://git.eclipse.org/r/140280 (all discussion please to bug 552338) so that we can use that in UI too.
Thanks, that's great. Thank you for working on this.
(In reply to Andrey Loskutov from comment #50) > Hi all, we (me and Tim) are starting to look into this again :-) > > First, I will move this bug to JGit and keep here only jgit related patches > (original one https://git.eclipse.org/r/50998 and > https://git.eclipse.org/r/140280 for EGit will go to the extra bug 552338 > for EGit). > > All other patches are for JGit only and will remain here. > Tim rebased the patches on master and I will re-push them to Gerrit now, > with the goal to merge one-by-one as soon as the review is OK. > > After that we will see that we re-work / finish EGit patch > https://git.eclipse.org/r/140280 (all discussion please to bug 552338) so > that we can use that in UI too. Thank You for looking on this again! I hope the most JGit patches will be accepted soon. I will try to help asap if there are still some issues.
Hi all, a note about the current state of the contribution and future plans. I hoped we could finish review last year, but due internal priorities I had no time to work on it at all and Tim left the project to continue his studies before we were done with all the review work. Mykola (my new co-worker) jumped quickly in and contributed https://git.eclipse.org/r/153906 patch for connected bug 552840, but now he is back to his main work area unrelated to JGit. So for now, Advantest (me and Simeon) plans to push as much as we can to drive this work to some final state. For those who don't know him, Simeon Andreev is my co-worker and a committer on platform. He is new in JGit / EGit, so please welcome him. Now to the contribution itself. Here is an ordered list of all pending patches that need to be reviewed / merged for JGit. On top of that we have also two patches for EGit. The JGit patch series starts with https://git.eclipse.org/r/137468 and ends with https://git.eclipse.org/r/153906. https://git.eclipse.org/r/137468 Add command line support for "git difftool" https://git.eclipse.org/r/137481 Add config reader for user-defined difftools https://git.eclipse.org/r/137575 Add difftool compare feature (execute external tool) https://git.eclipse.org/r/137942 Add config reader for user-defined mergetools https://git.eclipse.org/r/138003 Add command line support for "git mergetool" https://git.eclipse.org/r/138410 Add mergetool merge feature (execute external tool) https://git.eclipse.org/r/139262 Add handling for CR/LF and smudge filters for diff- and mergetool https://git.eclipse.org/r/139545 Add availability check of pre-defined tools https://git.eclipse.org/r/140275 Adapt diff- and merge tool code for PGM and EGit usage https://git.eclipse.org/r/153906 Teach JGit to handle external diff/merge tools defined in .gitattributes The EGit patch series relies on the JGit patches above and looks currently like this: https://git.eclipse.org/r/140280 [WIP] Add support for external diff and merge tools in EGit https://git.eclipse.org/r/153907 Allow use of external difftools defined in git attributes I plan to rebase all patches on master HEAD later today, and Simeon will start polishing patches & addressing review comments in the next days. I've also squashed all the Andre's patches 137468 - 140275 into https://git.eclipse.org/r/155606. This is NOT intended for merging, but only to get a "big picture" of the patch series in one single context. One big issue with the contribution is that is does not have tests, not at all. We would appreciate if someone could contribute to it, at least put some comments on patches, in which area which existing tests could be added / improved. One problem with external tools that we can't rely on their existence in the test environment, but I guess we can configure "dummy" test scripts to serve as the external tool and just echo back something useful - on Linux this would work, but Windows is probably out of the game here. The over optimistic plan is to get this all inside 5.7 release, or at least get a "go" with the overall design direction.
@Andre: I've now re-based existing patch series on the master and pushed back to gerrit. Doing so, I've updated "since" comments to 5.7 and license headers on all new code as requested on gerrit. Please note, that the current gerrit state includes some smaller code changes (all pushed as update to your original patches) for issues we've found during the test & review of the patches. I see that you mentioned on the gerrit that you have internally *another* version of the patch series that is improved in some places, and a new developer for working on all this. Would be nice if all further work could happen on gerrit and not closed source. It would be really helpful if we could synchronize our work to avoid unnecessary merge issues / duplicated work - could you please take the current gerrit state as a basis for a merge with your internal state, and do not simply overwrite existing gerrits with new patch sets? I also hope you will find time to do this soon! Simeon will start to look into the tests next week, so it would be really great if we could get the "latest greatest" code state from you.
(In reply to Andrey Loskutov from comment #54) > @Andre: I've now re-based existing patch series on the master and pushed > back to gerrit. > > Doing so, I've updated "since" comments to 5.7 and license headers on all > new code as requested on gerrit. Thank You! > Please note, that the current gerrit state includes some smaller code > changes (all pushed as update to your original patches) for issues we've > found during the test & review of the patches. > > I see that you mentioned on the gerrit that you have internally *another* > version of the patch series that is improved in some places, and a new > developer for working on all this. Would be nice if all further work could > happen on gerrit and not closed source. > It would be really helpful if we could synchronize our work to avoid > unnecessary merge issues / duplicated work - could you please take the > current gerrit state as a basis for a merge with your internal state, and do > not simply overwrite existing gerrits with new patch sets? ACK, i've rebased our changes on top of your work and continue with manual testing at Windows and Linux. We need unit tests here, but it somehow not easy for all the tools supported, as you wrote we need scripts that just return error codes. > I also hope you will find time to do this soon! > > Simeon will start to look into the tests next week, so it would be really > great if we could get the "latest greatest" code state from you. I'm working on it im my free time now and will see how it can be done next week together...
New Gerrit change created: https://git.eclipse.org/r/155670
I've updated all patches and added new one as refactoring step from review see below 155670. The JGit patch series starts with https://git.eclipse.org/r/137468 and ends with https://git.eclipse.org/r/153906. https://git.eclipse.org/r/137468 Add command line support for "git difftool" https://git.eclipse.org/r/137481 Add config reader for user-defined difftools https://git.eclipse.org/r/137575 Add difftool compare feature (execute external tool) https://git.eclipse.org/r/137942 Add config reader for user-defined mergetools https://git.eclipse.org/r/138003 Add command line support for "git mergetool" https://git.eclipse.org/r/138410 Add mergetool merge feature (execute external tool) https://git.eclipse.org/r/#/c/155670 Extract method refactoring in class DirCacheCheckout https://git.eclipse.org/r/139262 Add handling for CR/LF and smudge filters for diff- and mergetool https://git.eclipse.org/r/139545 Add availability check of pre-defined tools https://git.eclipse.org/r/140275 Adapt diff- and merge tool code for PGM and EGit usage https://git.eclipse.org/r/153906 Teach JGit to handle external diff/merge tools defined in .gitattributes The EGit patch series relies on the JGit patches above and looks currently like this: https://git.eclipse.org/r/140280 [WIP] Add support for external diff and merge tools in EGit https://git.eclipse.org/r/153907 Allow use of external difftools defined in git attributes Squashed all the Andre's patches 137468 - 140275 into https://git.eclipse.org/r/155606. Looking forward to have help with review and implementing tests... Thanks!
Hi Matthias, how do I run JGit tests from the IDE? I get this exception when I try to run the "theory" tests for merging: Class not found org.eclipse.jgit.api.MergeCommandTest java.lang.ClassNotFoundException: org.eclipse.jgit.api.MergeCommandTest cannot be found by org.eclipse.jgit.test_5.7.0.qualifier at org.eclipse.osgi.internal.loader.BundleLoader.findClassInternal(BundleLoader.java:463) at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:425) at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:417) at org.eclipse.osgi.internal.loader.ModuleClassLoader.loadClass(ModuleClassLoader.java:171) at java.lang.ClassLoader.loadClass(ClassLoader.java:357) at org.eclipse.osgi.internal.framework.EquinoxBundle.loadClass(EquinoxBundle.java:620) at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner$BundleClassLoader.findClass(RemotePluginTestRunner.java:44) at java.lang.ClassLoader.loadClass(ClassLoader.java:424) at java.lang.ClassLoader.loadClass(ClassLoader.java:357) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.loadClass(RemoteTestRunner.java:780) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.loadClasses(RemoteTestRunner.java:503) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:526) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:770) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:464) at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:171) at org.eclipse.pde.internal.junit.runtime.PlatformUITestHarness.lambda$0(PlatformUITestHarness.java:45) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:185) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4975) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4496) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1160) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1049) at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155) at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:660) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:559) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:154) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:150) at org.eclipse.pde.internal.junit.runtime.NonUIThreadTestApplication.runApp(NonUIThreadTestApplication.java:55) at org.eclipse.pde.internal.junit.runtime.UITestApplication.runApp(UITestApplication.java:46) at org.eclipse.pde.internal.junit.runtime.NonUIThreadTestApplication.start(NonUIThreadTestApplication.java:49) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:137) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:107) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:401) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:661) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:597) at org.eclipse.equinox.launcher.Main.run(Main.java:1476) at org.eclipse.equinox.launcher.Main.main(Main.java:1449) I thought this is a good starting point to understand how merging is tested, but the test doesn't run. Are the tests only runnable from the command line? Best regards and thanks, Simeon
(In reply to Simeon Andreev from comment #59) > how do I run JGit tests from the IDE? I get this exception when I try to run > the "theory" tests for merging: > ... > I thought this is a good starting point to understand how merging is tested, > but the test doesn't run. Are the tests only runnable from the command line? Please note, *J*Git tests are executed as simple JUnit tests (*not* as Plugin JUnit tests), and *E*Git tests are executed as SWTBot tests. So you can run both types of tests from IDE if you use right launch config.
(In reply to Simeon Andreev from comment #59) > Hi Matthias, > > how do I run JGit tests from the IDE? I get this exception when I try to run > the "theory" tests for merging: > > Class not found org.eclipse.jgit.api.MergeCommandTest > java.lang.ClassNotFoundException: org.eclipse.jgit.api.MergeCommandTest > cannot be found by org.eclipse.jgit.test_5.7.0.qualifier > at > org.eclipse.osgi.internal.loader.BundleLoader.findClassInternal(BundleLoader. > java:463) > at > org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java: > 425) > at > org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java: > 417) > at > org.eclipse.osgi.internal.loader.ModuleClassLoader. > loadClass(ModuleClassLoader.java:171) > at java.lang.ClassLoader.loadClass(ClassLoader.java:357) > at > org.eclipse.osgi.internal.framework.EquinoxBundle.loadClass(EquinoxBundle. > java:620) > at > org.eclipse.pde.internal.junit.runtime. > RemotePluginTestRunner$BundleClassLoader.findClass(RemotePluginTestRunner. > java:44) > at java.lang.ClassLoader.loadClass(ClassLoader.java:424) > at java.lang.ClassLoader.loadClass(ClassLoader.java:357) > at > org.eclipse.jdt.internal.junit.runner.RemoteTestRunner. > loadClass(RemoteTestRunner.java:780) > at > org.eclipse.jdt.internal.junit.runner.RemoteTestRunner. > loadClasses(RemoteTestRunner.java:503) > at > org.eclipse.jdt.internal.junit.runner.RemoteTestRunner. > runTests(RemoteTestRunner.java:526) > at > org.eclipse.jdt.internal.junit.runner.RemoteTestRunner. > runTests(RemoteTestRunner.java:770) > at > org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner. > java:464) > at > org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner. > main(RemotePluginTestRunner.java:171) > at > org.eclipse.pde.internal.junit.runtime.PlatformUITestHarness. > lambda$0(PlatformUITestHarness.java:45) > at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40) > at > org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:185) > at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4975) > at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4496) > at > org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5. > run(PartRenderingEngine.java:1160) > at > org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338) > at > org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine. > run(PartRenderingEngine.java:1049) > at > org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench. > java:155) > at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:660) > at > org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338) > at > org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:559) > at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:154) > at > org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication. > java:150) > at > org.eclipse.pde.internal.junit.runtime.NonUIThreadTestApplication. > runApp(NonUIThreadTestApplication.java:55) > at > org.eclipse.pde.internal.junit.runtime.UITestApplication. > runApp(UITestApplication.java:46) > at > org.eclipse.pde.internal.junit.runtime.NonUIThreadTestApplication. > start(NonUIThreadTestApplication.java:49) > at > org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java: > 203) > at > org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher. > runApplication(EclipseAppLauncher.java:137) > at > org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher. > start(EclipseAppLauncher.java:107) > at > org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:401) > at > org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255) > at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) > at > sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) > at > sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl. > java:43) > at java.lang.reflect.Method.invoke(Method.java:498) > at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:661) > at org.eclipse.equinox.launcher.Main.basicRun(Main.java:597) > at org.eclipse.equinox.launcher.Main.run(Main.java:1476) > at org.eclipse.equinox.launcher.Main.main(Main.java:1449) > > I thought this is a good starting point to understand how merging is tested, > but the test doesn't run. Are the tests only runnable from the command line? > > Best regards and thanks, > Simeon - ensure a jgit or egit target platform is set and the projects compile without errors in Eclipse, the easiest way to get there is to use the automatic workspace setup [1] - select MergeCommandTest e.g. in package explorer - click "Run as > JUnit test" - this works here successfully [1] https://wiki.eclipse.org/EGit/Contributor_Guide#Automated_Developer_Setup
(In reply to Andrey Loskutov from comment #60) > (In reply to Simeon Andreev from comment #59) > > how do I run JGit tests from the IDE? I get this exception when I try to run > > the "theory" tests for merging: > > ... > > I thought this is a good starting point to understand how merging is tested, > > but the test doesn't run. Are the tests only runnable from the command line? > > Please note, *J*Git tests are executed as simple JUnit tests (*not* as > Plugin JUnit tests), and *E*Git tests are executed as SWTBot tests. So you > can run both types of tests from IDE if you use right launch config. - for JGit tests use "Run as JUNit Test" (only need Java + library dependencies) - for EGit Core tests use "Run as Plugin Test" (needs in addition OSGi runtime/core Eclipse platform) - for EGit UI tests use "Run as SWTBot Test" (needs in addtion OSGi runtime, core Eclipse platform, SWT + Eclipse platform UI)
@Matthias, @JGit committers: I have a general question with all the patch series here. As of today, we have 10 commits and zero tests in gerrit. Usual practice is to contribute tests *together* with the original change in the same commit. Doing this with the current patch set will be difficult and time consuming for few reasons (lot of rebases, lot of re-work etc). Can we contribute the tests *on top* of the current patch series, or do we really want every commit to be enhanced with tests? Simeon started to write tests *on top* of the current patch series, but the way how the patches are setup, would make his tests more complicated / he would need to write different tests (more "plain" unit tests, less "integration" tests). WDYT?
(In reply to Andrey Loskutov from comment #63) > @Matthias, @JGit committers: I have a general question with all the patch > series here. > > As of today, we have 10 commits and zero tests in gerrit. Usual practice is > to contribute tests *together* with the original change in the same commit. > Doing this with the current patch set will be difficult and time consuming > for few reasons (lot of rebases, lot of re-work etc). > > Can we contribute the tests *on top* of the current patch series, or do we > really want every commit to be enhanced with tests? > > Simeon started to write tests *on top* of the current patch series, but the > way how the patches are setup, would make his tests more complicated / he > would need to write different tests (more "plain" unit tests, less > "integration" tests). > > WDYT? As long as tests with good coverage for the new functionality are implemented I am fine putting them into separate commits. If we don't have proper tests when the next release is around the corner I'd propose to revert the commits which brought in the new functionality.
(In reply to Matthias Sohn from comment #64) > As long as tests with good coverage for the new functionality are > implemented I am fine putting them into separate commits. If we don't have > proper tests when the next release is around the corner I'd propose to > revert the commits which brought in the new functionality. OK, thanks for fast reply, we will work on extra commit with tests on top of the patch series.
I've updated all patches and added new one as FIRST refactoring step from review (can be merged first independently): https://git.eclipse.org/r/155670 Extract method refactoring in class DirCacheCheckout The JGit patch series starts with https://git.eclipse.org/r/137468 and ends with https://git.eclipse.org/r/153906: https://git.eclipse.org/r/137468 Add command line support for "git difftool" https://git.eclipse.org/r/137481 Add config reader for user-defined difftools https://git.eclipse.org/r/137575 Add difftool compare feature (execute external tool) https://git.eclipse.org/r/137942 Add config reader for user-defined mergetools https://git.eclipse.org/r/138003 Add command line support for "git mergetool" https://git.eclipse.org/r/138410 Add mergetool merge feature (execute external tool) https://git.eclipse.org/r/139262 Add handling for CR/LF and smudge filters for diff- and mergetool https://git.eclipse.org/r/139545 Add availability check of pre-defined tools https://git.eclipse.org/r/140275 Adapt diff- and merge tool code for PGM and EGit usage https://git.eclipse.org/r/153906 Teach JGit to handle external diff/merge tools defined in .gitattributes The EGit patch series relies on the JGit patches above and looks currently like this: https://git.eclipse.org/r/140280 Add support for external diff and merge tools in EGit https://git.eclipse.org/r/153907 Allow use of external difftools defined in git attributes
New Gerrit change created: https://git.eclipse.org/r/155806
While running my tests on Windows, I ran into some test code that swallows exception causes. We should make this change at some point: diff --git a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/LocalDiskRepositoryTestCase.java b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/LocalDiskRepositoryTestCase.java index 494d2f3ba..610a0a74f 100644 --- a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/LocalDiskRepositoryTestCase.java +++ b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/LocalDiskRepositoryTestCase.java @@ -14,7 +14,6 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.fail; import java.io.File; import java.io.IOException; @@ -236,10 +235,9 @@ private static void reportDeleteFailure(boolean failOnError, File f, String severity = failOnError ? "ERROR" : "WARNING"; String msg = severity + ": Failed to delete " + f; if (failOnError) { - fail(msg); - } else { - System.err.println(msg); + throw new AssertionError(msg, cause); } + System.err.println(msg); cause.printStackTrace(new PrintStream(System.err)); } I guess in a separate ticket.
I've updated again all the patches after review. The main change to move from CommandExecuter to own FS for MSYS is not yet done. I need more time as it may change and break the basic already tested execution of "real" tools at Windows with MSYS2...
Gerrit change https://git.eclipse.org/r/155670 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=eca34be43ce3309344cd1ec2914d3201121af893
(In reply to Andre Bossert from comment #69) > The main change to move > from CommandExecuter to own FS for MSYS is not yet done. I need more time as > it may change and break the basic already tested execution of "real" tools > at Windows with MSYS2... Anything I can do to help with this?
https://github.com/eclipse/jgit/commit/8ada9048c5add754c3b34851b1bd501ce28b3321?_pjax=%23js-repo-pjax-container How do I fetch assertj into Eclipse with this change? After importing JGit projects, I can't compile the JGit tests any more: ` Description Resource Path Location Type No available bundle exports package 'org.assertj.core.api' MANIFEST.MF /org.eclipse.jgit.test/META-INF line 20 Plug-in Problem `
(In reply to Simeon Andreev from comment #73) > https://github.com/eclipse/jgit/commit/ > 8ada9048c5add754c3b34851b1bd501ce28b3321?_pjax=%23js-repo-pjax-container > > How do I fetch assertj into Eclipse with this change? You either use Oomph setup or just do this: https://wiki.eclipse.org/Platform/How_to_Contribute#.5B2.5D_Install_the_development_tools.
(In reply to Simeon Andreev from comment #73) > https://github.com/eclipse/jgit/commit/ > 8ada9048c5add754c3b34851b1bd501ce28b3321?_pjax=%23js-repo-pjax-container > > How do I fetch assertj into Eclipse with this change? After importing JGit > projects, I can't compile the JGit tests any more: > > ` > Description Resource Path Location Type > No available bundle exports package 'org.assertj.core.api' MANIFEST.MF > /org.eclipse.jgit.test/META-INF line 20 Plug-in Problem > > ` Set a jgit target platform defined in the org.eclipse.jgit.target project, the target platform defines the classpath and brings all the jgit dependencies. Open e.g. jgit-4.14-staging.target with the target editor and click "Set as active target platform". If you setup Eclipse for JGit/EGit development using the automated setup [1] this is done automatically. [1] https://wiki.eclipse.org/EGit/Contributor_Guide#Automated_Developer_Setup
(In reply to Andre Bossert from comment #69) > I've updated again all the patches after review. The main change to move > from CommandExecuter to own FS for MSYS is not yet done. I need more time as > it may change and break the basic already tested execution of "real" tools > at Windows with MSYS2... Hi Andre, should I try to do this? We would like to continue working on this ticket, but are not sure how we can help. Best regards, Simeon
(In reply to Simeon Andreev from comment #76) > (In reply to Andre Bossert from comment #69) > > I've updated again all the patches after review. The main change to move > > from CommandExecuter to own FS for MSYS is not yet done. I need more time as > > it may change and break the basic already tested execution of "real" tools > > at Windows with MSYS2... > > Hi Andre, > > should I try to do this? We would like to continue working on this ticket, > but are not sure how we can help. > > Best regards, > Simeon Hi Simeon, sorry for late answer, i was sick after long vacation. I will prepare the rework for this and it would be good if you can help polishing and test. I hope to do until end of the week. Thanks! Regards, Andre
(In reply to Andre Bossert from comment #77) > sorry for late answer, i was sick after long vacation. I will prepare the > rework for this and it would be good if you can help polishing and test. I > hope to do until end of the week. Thanks! Great. Please note, we will target 5.8 release now, so all the @since tags should be set to 5.8. @Matthias, can you please add 5.8 target version in bugzilla?
(In reply to Andrey Loskutov from comment #78) > (In reply to Andre Bossert from comment #77) > > sorry for late answer, i was sick after long vacation. I will prepare the > > rework for this and it would be good if you can help polishing and test. I > > hope to do until end of the week. Thanks! > > Great. Please note, we will target 5.8 release now, so all the @since tags > should be set to 5.8. > > @Matthias, can you please add 5.8 target version in bugzilla? I added the 5.8 milestone for jgit and egit
(In reply to Matthias Sohn from comment #79) > > Great. Please note, we will target 5.8 release now, so all the @since tags > > should be set to 5.8. > > > > @Matthias, can you please add 5.8 target version in bugzilla? > > I added the 5.8 milestone for jgit and egit I've updated the @since tags to 5.8 but JGit versions are 5.7. Should i wait until JGit version is incremented to 5.8 at master? Thanks.
(In reply to Andre Bossert from comment #80) > I've updated the @since tags to 5.8 but JGit versions are 5.7. Should i wait > until JGit version is incremented to 5.8 at master? Thanks. Just push, we can always rebase, and there will be anyway some changes needed after review.
New Gerrit change created: https://git.eclipse.org/r/c/jgit/jgit/+/138003
New Gerrit change created: https://git.eclipse.org/r/c/jgit/jgit/+/140275
New Gerrit change created: https://git.eclipse.org/r/c/jgit/jgit/+/166945
Hi, just curious as I wait for this feature fore some years now. Is somebody still working on it? Best regards Michael
Hi all, we (me and Andrey) would like to resume work here, as we plan to move to latest EGit and cherry-picking patches for this feature is not optimal (we already did so once for our move to EGit 5.7). Last year we got nowhere with merging the changes here. This time around we would like to try merging the changes one at a time (since hopefully that increases the changes to merge at least *something*). I've rebased the first change, and will look into whether there is leftover review work: https://git.eclipse.org/r/c/jgit/jgit/+/137468/ Meanwhile, are there any objections to merging the changes one at a time? Such as: 1. "no, this change makes no sense alone" 2. "this/each change needs dedicated tests" 3. etc. In particular with the first change in mind, https://git.eclipse.org/r/c/jgit/jgit/+/137468/, since merging that will be our first priority. Best regards and thanks, Simeon
(In reply to Simeon Andreev from comment #86) > Hi all, > > we (me and Andrey) would like to resume work here, as we plan to move to > latest EGit and cherry-picking patches for this feature is not optimal (we > already did so once for our move to EGit 5.7). > > Last year we got nowhere with merging the changes here. This time around we > would like to try merging the changes one at a time (since hopefully that > increases the changes to merge at least *something*). We (me and Simeon) discussed about the first change on a phone and we have an updated proposal how we could continue here. The way how code is distributed over commits conflicts with the proposed merge "one by one" strategy and the code/patches structure. As of today, all the new proposed JGit code is put into org.eclipse.jgit.diffmergetool and exported as API. If we merge the first commit adding some API in 5.12 but don't manage to merge the last commit where the same API is adopted for the EGit use once again, we may end in 5.12 containing API that can't be changed anymore in 5.13 without breaking it. We may try to re-work all the commits again and shift code here and there, to avoid "API changes" between different commits, but that is hard task and still we may end up with the state where we either put too many changes into one commit (violating magic 1000 lines IP border like in comment 15) or have different commits touching same API. Add to this that the tests for the whole thing were added in last commits and we have again the state which can't be merged or will be reverted in 5.12 (see comment 64). But *is* this a hard requirement that this contribution must be API? The goal of this bug is first and foremost to provide external diff/merge functionality in JGit / EGit. *If* that can be re-used as API from 3rd party code is IMHO "nice to have" but not mandatory for this feature, at least not for the most users. So here is the revisited proposal how we can merge that all into JGit 5.12: 1) Let move proposed code contribution to org.eclipse.jgit.internal.diffmergetool package. This is *not* API by default, but can be made visible to dedicated (non-core JGit) bundles similar to org.eclipse.jgit.internal.storage.file and other packages consumed outside core jgit bundle. 2) The non-API code is easier to merge and allows us to incrementally merge commits into 5.12 release without fear that some parts of the API would be incomplete/broken if we don't manage to merge everything or find new issues with the code after the merge. Also, once merged, we can polish the code as much as we like without fear to break new API. 3) After everything is merged into internal package we may decide which parts of the code could/should be API and which not. Many classes that are "API" in the current patch series are only used internally, only a small subset of them could be ever interesting for clients as API. So we can decide what is needed and how for clients and refactor the code targeting 5.13+ without time pressure, while the most users that don't need the API could just use the feature "as is" in 5.12 without waiting for the "final cut" in some future. 4) If agreed to continue this way, we (me and Simeon) could re-work all the patches and move the code to the org.eclipse.jgit.internal.diffmergetool package. 5) @Matthias, Andre (and other JGit committers interested in this feature): do we want to meet together on zoom (or whatever else suitable technology) to speed up the discussion? 6) Other ideas are welcome. It is really pity to see this useful code not merged for years...
(In reply to Andrey Loskutov from comment #87) Thank You Simeon and Andrey for pushing this forward again :) > 5) @Matthias, Andre (and other JGit committers interested in this feature): > do we want to meet together on zoom (or whatever else suitable technology) > to speed up the discussion? Yes, that would be good - we can use MS teams or zoom. Should i invite you a to a call? > 6) Other ideas are welcome. It is really pity to see this useful code not > merged for years... I'm rebasing our internal pathes again now to Eclipse 2021/03 and want to help.
Andrey's proposal makes sense. Good to see that someone with resources is backing this.
(In reply to Andrey Loskutov from comment #87) > (In reply to Simeon Andreev from comment #86) > > Hi all, > > > > we (me and Andrey) would like to resume work here, as we plan to move to > > latest EGit and cherry-picking patches for this feature is not optimal (we > > already did so once for our move to EGit 5.7). > > > > Last year we got nowhere with merging the changes here. This time around we > > would like to try merging the changes one at a time (since hopefully that > > increases the changes to merge at least *something*). +1 for bringing in the changes incrementally > We (me and Simeon) discussed about the first change on a phone and we have > an updated proposal how we could continue here. > > The way how code is distributed over commits conflicts with the proposed > merge "one by one" strategy and the code/patches structure. > > As of today, all the new proposed JGit code is put into > org.eclipse.jgit.diffmergetool and exported as API. If we merge the first > commit adding some API in 5.12 but don't manage to merge the last commit > where the same API is adopted for the EGit use once again, we may end in > 5.12 containing API that can't be changed anymore in 5.13 without breaking > it. we could - do the work in org.eclipse.jgit.diffmergetool and document in the release notes that this API is experimental until it's finished and stable - do the work in org.eclipse.jgit.experimental.diffmergetool and rename the package later when the API is complete and stable > We may try to re-work all the commits again and shift code here and there, > to avoid "API changes" between different commits, but that is hard task and > still we may end up with the state where we either put too many changes into > one commit (violating magic 1000 lines IP border like in comment 15) or have > different commits touching same API. Add to this that the tests for the > whole thing were added in last commits and we have again the state which > can't be merged or will be reverted in 5.12 (see comment 64). In order to ensure the increments work and don't break by later changes I strongly recommend that all increments should come with unit tests for the functionality they are adding. This also simplifies reviewing the changes. > But *is* this a hard requirement that this contribution must be API? > > The goal of this bug is first and foremost to provide external diff/merge > functionality in JGit / EGit. > *If* that can be re-used as API from 3rd party code is IMHO "nice to have" > but not mandatory for this feature, at least not for the most users. > > So here is the revisited proposal how we can merge that all into JGit 5.12: > > 1) Let move proposed code contribution to > org.eclipse.jgit.internal.diffmergetool package. This is *not* API by > default, but can be made visible to dedicated (non-core JGit) bundles > similar to org.eclipse.jgit.internal.storage.file and other packages > consumed outside core jgit bundle. > > 2) The non-API code is easier to merge and allows us to incrementally merge > commits into 5.12 release without fear that some parts of the API would be > incomplete/broken if we don't manage to merge everything or find new issues > with the code after the merge. Also, once merged, we can polish the code as > much as we like without fear to break new API. > > 3) After everything is merged into internal package we may decide which > parts of the code could/should be API and which not. Many classes that are > "API" in the current patch series are only used internally, only a small > subset of them could be ever interesting for clients as API. So we can > decide what is needed and how for clients and refactor the code targeting > 5.13+ without time pressure, while the most users that don't need the API > could just use the feature "as is" in 5.12 without waiting for the "final > cut" in some future. +1 for this proposal > 4) If agreed to continue this way, we (me and Simeon) could re-work all the > patches and move the code to the org.eclipse.jgit.internal.diffmergetool > package. > > 5) @Matthias, Andre (and other JGit committers interested in this feature): > do we want to meet together on zoom (or whatever else suitable technology) > to speed up the discussion? I am open to join a meeting if you think this helps. I will take one week off next week. If you want to meet soon we could do that tomorrow. I have time all afternoon CET. > 6) Other ideas are welcome. It is really pity to see this useful code not > merged for years...
I think it would also help to get the APIs right by implementing (basic) support for external diff and merge tools also in jgit command line.
(In reply to Matthias Sohn from comment #90) > we could > - do the work in org.eclipse.jgit.diffmergetool and document in the release > notes that this API is experimental until it's finished and stable > - do the work in org.eclipse.jgit.experimental.diffmergetool and rename the > package later when the API is complete and stable Not sure anyone reads release notes, also API baseline would need filters. I feel always bad to change "public API" even if that is not API yet :-) So my first preference would be "internal", second - "experimental", third - keep "as is" and add release notes. If you strongly prefer "experimental", I'm OK, I just want to have an agreement soon so we can move forward. > In order to ensure the increments work and don't break by later changes I > strongly recommend that all increments should come with unit tests for the > functionality they are adding. This also simplifies reviewing the changes. We've added the tests "after the fact" for the entire feature, at the time the feature set that was already fully implemented. I believe not all sub-features were "complete" in a single commit, most commits start with some default dummy implementation and evolve later to fully implemented version, so the tests would need to be adopted with every commit. That is the probably something coming from the code pushed initially as a "big bang", missing tests from the beginning and (probably) split to smaller chunks because of the "1000 lines" restriction. Something to discuss together with Andre, because if we decide to do that, it would probably need another major rework/shifting content between commits. > > 3) After everything is merged into internal package we may decide which > > parts of the code could/should be API and which not. Many classes that are > > "API" in the current patch series are only used internally, only a small > > subset of them could be ever interesting for clients as API. So we can > > decide what is needed and how for clients and refactor the code targeting > > 5.13+ without time pressure, while the most users that don't need the API > > could just use the feature "as is" in 5.12 without waiting for the "final > > cut" in some future. > > +1 for this proposal Thanks for supporting this. (In reply to Matthias Sohn from comment #91) > I think it would also help to get the APIs right by implementing (basic) > support for external diff and merge tools also in jgit command line. In theory yes, but I think that missing resources/experience to add this support can delay the whole feature again. The small API *actually* needed by clients can be tested / tried with the Egit changes and unit tests we have already on top of this patch iceberg. > I am open to join a meeting if you think this helps. > I will take one week off next week. If you want to meet soon we could do > that tomorrow. I have time all afternoon CET. I'm also available for tomorrow afternoon CET. @Andre, how about your time tomorrow? I would really like to have agreement on how to proceed soon, as long as we (me and Simeon) have some time to do the work.
(In reply to Andrey Loskutov from comment #92) > (In reply to Matthias Sohn from comment #90) > > we could > > - do the work in org.eclipse.jgit.diffmergetool and document in the release > > notes that this API is experimental until it's finished and stable > > - do the work in org.eclipse.jgit.experimental.diffmergetool and rename the > > package later when the API is complete and stable > > Not sure anyone reads release notes, also API baseline would need filters. > I feel always bad to change "public API" even if that is not API yet :-) > > So my first preference would be "internal", second - "experimental", third - > keep "as is" and add release notes. If you strongly prefer "experimental", > I'm OK, I just want to have an agreement soon so we can move forward. ok, then let's do it in an internal package > > In order to ensure the increments work and don't break by later changes I > > strongly recommend that all increments should come with unit tests for the > > functionality they are adding. This also simplifies reviewing the changes. > > We've added the tests "after the fact" for the entire feature, at the time > the feature set that was already fully implemented. I believe not all > sub-features were "complete" in a single commit, most commits start with > some default dummy implementation and evolve later to fully implemented > version, so the tests would need to be adopted with every commit. That is > the probably something coming from the code pushed initially as a "big > bang", missing tests from the beginning and (probably) split to smaller > chunks because of the "1000 lines" restriction. > > Something to discuss together with Andre, because if we decide to do that, > it would probably need another major rework/shifting content between commits. I am asking for this from the angle as a reviewer. If changes don't come with tests reviewers have the following choices: - 1. try to estimate if the tests 20 changes above in the change series could succeed - 2. move the tests proving the commit which is at the bottom of the series works down to that commit - 3. don't review these changes since all this is too much work Option 1 seems risky, I don't have the time for 2 so I ended up with option 3 in earlier attempts to review this series. > > > 3) After everything is merged into internal package we may decide which > > > parts of the code could/should be API and which not. Many classes that are > > > "API" in the current patch series are only used internally, only a small > > > subset of them could be ever interesting for clients as API. So we can > > > decide what is needed and how for clients and refactor the code targeting > > > 5.13+ without time pressure, while the most users that don't need the API > > > could just use the feature "as is" in 5.12 without waiting for the "final > > > cut" in some future. > > > > +1 for this proposal > > Thanks for supporting this. > > (In reply to Matthias Sohn from comment #91) > > I think it would also help to get the APIs right by implementing (basic) > > support for external diff and merge tools also in jgit command line. > > In theory yes, but I think that missing resources/experience to add this > support can delay the whole feature again. The small API *actually* needed > by clients can be tested / tried with the Egit changes and unit tests we > have already on top of this patch iceberg. this was just a suggestion, I don't insist
(In reply to Matthias Sohn from comment #93) > > Something to discuss together with Andre, because if we decide to do that, > > it would probably need another major rework/shifting content between commits. > > I am asking for this from the angle as a reviewer. If changes don't come > with tests > reviewers have the following choices: > - 1. try to estimate if the tests 20 changes above in the change series > could succeed > - 2. move the tests proving the commit which is at the bottom of the series > works down to that commit > - 3. don't review these changes since all this is too much work > > Option 1 seems risky, I don't have the time for 2 so I ended up with option > 3 in earlier attempts to review this series. Yes, the current state is not reviewers friendly. I've ended up starting second workspace with the full patch series opened, so I can see the "final" version and check the code working there.
We had a very good meeting with Matthias, Andre, Simeon, I took some notes. 1) Andre has already some additional patches that improve PGM support, and will try to insert them somewhere into the patch series. 2) It is agreed to move entire contribution from org.eclipse.jgit.diffmergetool to org.eclipse.jgit.internal.diffmergetool namespace to avoid that we release the contribution as API before it is understood / agreed, which part of the contribution should be API at all. Egit & Jgit bundles (that need this internal API and are outside core Jgit bundle) will be added as x-friends to the new package so they can consume new code. This should significantly simplify the review process, because in internal code we are free to move things around as we like without the fear to break API clients, and the review criteria for non-API code are not that strict as for API. 3) Since Andre plans to rebase the patch series on latest master anyway, and to address concerns that there were no tests added with every commit, he will try to add some simple unit tests where appropriate (may be not with every commit and may be not for the entire functionality, and may be with some TODO pointing on possible future test opportunities). In doubt, no tests is OK if testing is difficult because of relying on external command line tooling, but it should be mentioned in the commit message how the change could be tested manually (if possible). 4) While doing this rebase Andre will also try to move API changes made in later commits (caused by the added Egit integration) down to the earlier commits, so that newly added API's aren't changed later in the patch series. 5) There were comments from Matthias on changes related to msys2 support, the agreement is that Andre will try to create FS_Win32_Msys2 class (name is TBD) to encapsulate msys2 specific tool requirements. The class will be loaded if some system property (or env. variable?) is set - TBD. 6) Simeon and me would be there to provide feedback timely & help with Linux testing or with additional JUnit tests (Andre is on Windows). 7) Our goal is to complete this for 5.12 release, Andre plans to start working on this ASAP and he has two weeks planned already.
Hi Andre, since there have been no changes done from you so far (or did we miss any notifications?), should we (me & Andrey) start with the changes we discussed previously (listed in comment 95)? We would like to prevent conflicts, so we are double checking whether changes are still coming from you. If not, we will start working on this. Best regards, Simeon
Andre, unless there are objections from your side, we plan to start working on the planned changes. We would start probably latest next week.
> Hallo zusammen, > > leider gab es bei uns im Projekt wieder einige Verschiebungen. > Stand heute: ein Kollege bereitet unseren aktuelle Version bis Ende der Woche > (Mittwoch) vor damit ich es am Freitag reviewen kann > und Anfang der Woche (Mo-Di 16./17.05.) ggf. pushen. Hi Andre, could you give a status update on this? I answered the e-mail a week ago or so (asking the same), but got a notification that you are out-of-office. Best regards, Simeon
Hi Simeon, i was in vacation until today. We have rebased to latest master and are testing with latest master now. I hope to push new version soon... Regards Andre
(In reply to Andre Bossert from comment #99) > Hi Simeon, > > i was in vacation until today. We have rebased to latest master and are > testing with latest master now. I hope to push new version soon... > > Regards > Andre Hi Andre, any news here? Its been a very long while. Best regards, Simeon
(In reply to Simeon Andreev from comment #100) > (In reply to Andre Bossert from comment #99) > > Hi Simeon, > > > > i was in vacation until today. We have rebased to latest master and are > > testing with latest master now. I hope to push new version soon... > > > > Regards > > Andre > > Hi Andre, > > any news here? Its been a very long while. > > Best regards, > Simeon Hi Simeon, i'm working now on integration of latest internal patches. I'm rebasing the latest Gerrit version first to have same baseline and improve the commits. Regards Andre
Now receiving failure if want to push with valid SSH keys for Gerrit: ... "Can't connect to any repository: ssh://<myuser>@git.eclipse.org:29418/jgit/jgit.git (ssh://<myuser>@git.eclipse.org:29418/jgit/jgit.git: Too many authentication failures: 7) ... Is something different at Gerrit or Eclipse 2021-06 ? I have to investigate...
ok, the right gerrit private key was 5th in the configuration dialog of eclipse and .".ssh/config" with per server "IdentityFile" and "IdentitiesOnly yes" is still not supported -> moving the key to first in the list can be used as workaround... P.S.: looks similar to error message in https://bugs.eclipse.org/bugs/show_bug.cgi?id=425672
Hi Andre, finally me and Andrey have more time to invest in this ticket. Before we start work here, do you plan any immediate changes? I would like to avoid conflicts. I propose starting with the first change: https://git.eclipse.org/r/c/jgit/jgit/+/137468/ We should get it into a good enough shape to merge the change, so the first item would be to add tests. If tests can't be added meaningfully, we likely should flesh out the change a bit more (so that it would make more sense to commit it standalone, with tests). Then we can proceed with the rest of the changes, similarly. If you don't plan to adjust the changes in near future, do you have objections to the above. And does anyone of the watchers have objections? Best regards, Simeon
Gerrit change https://git.eclipse.org/r/c/jgit/jgit/+/137468 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=48f4d97a226a2c9be4577b916cc0b99ce870939a
New Gerrit change created: https://git.eclipse.org/r/c/jgit/jgit/+/188164
Gerrit change https://git.eclipse.org/r/c/jgit/jgit/+/188164 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=8d2650773778f282e0af1bc8bcca220b4f4ce586
Gerrit change https://git.eclipse.org/r/c/jgit/jgit/+/137481 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=14a59bdc7b37c85591eed07bc60c3f4fdcde2a8a
Gerrit change https://git.eclipse.org/r/c/jgit/jgit/+/137575 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=b63c2f39a16f1607cc65e82c0271c8c29a6038f1
Gerrit change https://git.eclipse.org/r/c/jgit/jgit/+/137942 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=24171b05f0db8487d0a1dc40072cc313bb6e2087
Gerrit change https://git.eclipse.org/r/c/jgit/jgit/+/138003 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=85734356351ec2df4067b2472a37f6d9bcbb7350
Gerrit change https://git.eclipse.org/r/c/jgit/jgit/+/138410 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=eaf4d500b886a7e776f50bf53497fe463e714b25
New Gerrit change created: https://git.eclipse.org/r/c/jgit/jgit/+/193813
Gerrit change https://git.eclipse.org/r/c/jgit/jgit/+/193813 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=8c681aac500d0755bd68f8b322f7fdb20cd2b8a1
Gerrit change https://git.eclipse.org/r/c/jgit/jgit/+/139262 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=e81085944f1a039566f2972c863d189724988b46
Gerrit change https://git.eclipse.org/r/c/jgit/jgit/+/139545 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=973e955ead1a9bf41efb3168baf5b68527e78023
Gerrit change https://git.eclipse.org/r/c/jgit/jgit/+/140275 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=ff77d412a9f1a956b8d76b139489038ee3b0870c
We should be mostly done here. Surely current implementation can be improved and there will be bugs reported, but the main functionality based on Andre Bossert patch series is merged now. Thanks to Andre, Tim, Simeon, and all other people involved. Closing this as resolved, please report new bugs for all issues or missing pieces found. As next step, we plan to proceed with EGit support, in 3 commits we should be done there too.
(In reply to Andrey Loskutov from comment #118) > We should be mostly done here. Great! Could we please have a little description int the JGit New & Noteworthy at [1] explaining how to define and use an external merge tool (and diff tool)? [1] https://wiki.eclipse.org/JGit/New_and_Noteworthy/6.2#Support_for_External_Diff_and_Merge_Tools
(In reply to Thomas Wolf from comment #119) > (In reply to Andrey Loskutov from comment #118) > > We should be mostly done here. > > Great! Could we please have a little description int the JGit New & > Noteworthy at [1] explaining how to define and use an external merge tool > (and diff tool)? > > [1] > https://wiki.eclipse.org/JGit/New_and_Noteworthy/6. > 2#Support_for_External_Diff_and_Merge_Tools I'll see if I can come up with some helpful info tomorrow. I'll need some help though with specifics, we can discuss on the review.
OK, I added some content to the page: https://wiki.eclipse.org/JGit/New_and_Noteworthy/6.2#Support_for_External_Diff_and_Merge_Tools This is based off the documentation found at: * https://git-scm.com/docs/git-mergetool * https://git-scm.com/docs/git-difftool (there are a few parameters not supported by the implementation in JGit, as well as a few parameters not listed in the documentation pages above that JGit supports) Thomas, please go over the page and let me know if there is anything to change (that you have no time to change yourself).
(In reply to Simeon Andreev from comment #121) > (there are a few parameters not supported by the implementation in JGit, as > well as a few parameters not listed in the documentation pages above that > JGit supports) > > Thomas, please go over the page and let me know if there is anything to > change (that you have no time to change yourself). Thanks a lot Simeon! I did shorten it a bit. Also, we generate the in-Eclipse doc bundle from this Wiki page -- I don't think the generator knows about possible extensions in MediaWiki, like GesHi, so I made sure we use only basic built-in MediaWiki formatting. From your list of command options and config settings, it seems JGit has fairly complete support. mergetool.hideResolved seems to be missing, as is diff.orderFile and the mergetool -O<orderFile> option. For difftool, it looks like --rotate-to, --skip-to, and -x are missing. Not sure it's worth pointing out that. What are the JGit-specific settings or options, and why do we need them?
> What are the JGit-specific settings or options, and why > do we need them? For the diff tool, there is "--cached"/"--staged", I didn't find a parameter like this in the difftool git documentation page. For the diff and merge tool, there is "--". Maybe both "just work" for egit and there is no extra mention in the documentation page.
> Maybe both "just work" for egit Sorry I meant "git", not "egit". Typo.
(In reply to Simeon Andreev from comment #123) > > What are the JGit-specific settings or options, and why > > do we need them? > > For the diff tool, there is "--cached"/"--staged", I didn't find a parameter > like this in the difftool git documentation page. Missed that. So that might be worth documenting. What does do? Your description was "Invoke the diff tool with files that contain staged changes.", but it's not clear to me what that means. Does it show a diff between working tree and index, or a diff between index and HEAD? > For the diff and merge tool, there is "--". Is listed in the synopsis of git difftool, and I'd expect it to work likewise for git mergetool, too. Not worth pointing out in a N&N I think.
> > For the diff tool, there is "--cached"/"--staged", I didn't find a parameter > > like this in the difftool git documentation page. > > Missed that. So that might be worth documenting. What does do? Your > description was "Invoke the diff tool with files that contain staged > changes.", but it's not clear to me what that means. Does it show a diff > between working tree and index, or a diff between index and HEAD? Sorry I don't know the proper terminology. If I change a file in the repository, "git diff" will show me the change. If I "git add" the file, "git diff" no longer shows the change. But "git diff --cached" shows the change. So the "--staged"/"--cached" option does the same for the external difftool (I don't know if "git difftool" supports it too, maybe yes but I didn't see a mention). You can e.g. check the preparations done in DiffToolTest.testToolCached() to check whether the parameter is working.
(In reply to Andrey Loskutov from comment #118) > We should be mostly done here. Surely current implementation can be improved > and there will be bugs reported, but the main functionality based on Andre > Bossert patch series is merged now. > > Thanks to Andre, Tim, Simeon, and all other people involved. > > Closing this as resolved, please report new bugs for all issues or missing > pieces found. > > As next step, we plan to proceed with EGit support, in 3 commits we should > be done there too. Wow! Thank You all for your effort to review, help and finaly merge it :) I'm rebasing to this base and hope to contribute small pacthes we have internaly.
(In reply to Simeon Andreev from comment #126) > > > For the diff tool, there is "--cached"/"--staged", I didn't find a parameter > > > like this in the difftool git documentation page. > > > > Missed that. So that might be worth documenting. What does do? Your > > description was "Invoke the diff tool with files that contain staged > > changes.", but it's not clear to me what that means. Does it show a diff > > between working tree and index, or a diff between index and HEAD? > > Sorry I don't know the proper terminology. If I change a file in the > repository, "git diff" will show me the change. If I "git add" the file, > "git diff" no longer shows the change. But "git diff --cached" shows the > change. So the "--staged"/"--cached" option does the same for the external > difftool (I don't know if "git difftool" supports it too, maybe yes but I > didn't see a mention). > > You can e.g. check the preparations done in DiffToolTest.testToolCached() to > check whether the parameter is working. The N&N looks good - Thanks! The difftool should also support most commands from diff, see https://git-scm.com/docs/git-difftool ``` git difftool is a frontend to git diff and accepts the same options and arguments. ``` We are using `--staged a synonym of --cached` daily. Some of the new commands are not yet implemented in JGit, but for daily work we do not see the need for other options. I think they can implemented if there is a user and test...
