Bug 552325 - Default repository folder is empty and can't be set
Summary: Default repository folder is empty and can't be set
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.14   Edit
Hardware: All All
: P3 major (vote)
Target Milestone: 4.14 M3   Edit
Assignee: Christoph Laeubrich CLA
QA Contact:
URL:
Whiteboard:
Keywords: regression
Depends on: 552329
Blocks: 551642
  Show dependency tree
 
Reported: 2019-10-22 09:09 EDT by Andrey Loskutov CLA
Modified: 2019-10-23 05:45 EDT (History)
3 users (show)

See Also:


Attachments
screenshot (73.45 KB, image/png)
2019-10-22 09:09 EDT, Andrey Loskutov CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Loskutov CLA 2019-10-22 09:09:38 EDT
Created attachment 280377 [details]
screenshot

Git, 5.6.0.201910171329, SDK I20191022-0405.

Create new workspace, go Team -> Git.
"Default repository folder" field is empty.
Type some existing path, Apply and Close, open same preference page again => nada.

Old workspace shows nothing too, even I know there was some value.
Comment 1 Andrey Loskutov CLA 2019-10-22 09:14:37 EDT
What is also interesting on the screenshot, neither defaults are selected for both SSH and HTTP clients. Not sure this is related or not, or if this is a bug or not, but I would expect to see there some values set.
Comment 2 Thomas Wolf CLA 2019-10-22 10:18:44 EDT
No idea how your Eclipse got into that state; nor why entering a new path doesn't take hold. Also utterly unclear why the two other preferences don't show anything. I didn't know it was even _possible_ to make a combo box show nothing when it has only two statically defined items. Some major corruption going on somewhere...
Comment 3 Andrey Loskutov CLA 2019-10-22 10:28:16 EDT
(In reply to Thomas Wolf from comment #2)
> No idea how your Eclipse got into that state; nor why entering a new path
> doesn't take hold. Also utterly unclear why the two other preferences don't
> show anything. I didn't know it was even _possible_ to make a combo box show
> nothing when it has only two statically defined items. Some major corruption
> going on somewhere...

So you can't reproduce? Which Eclipse version are you running?
Looks like preference defaults aren't initialized, I will debug this.
Comment 4 Andrey Loskutov CLA 2019-10-22 10:36:13 EDT
It is a regression from bug 551642.
Comment 5 Andrey Loskutov CLA 2019-10-22 10:43:00 EDT
Sarika, I believe Platform JDT or debug had preferences bug recently reported too?

I will revert original patch, current state is a NO GO from EGit point of view.
Comment 6 Christoph Laeubrich CLA 2019-10-22 10:46:24 EDT
Can you explain how/why this breaks?
Comment 7 Eclipse Genie CLA 2019-10-22 10:46:39 EDT
New Gerrit change created: https://git.eclipse.org/r/151438
Comment 8 Andrey Loskutov CLA 2019-10-22 10:49:52 EDT
(In reply to Christoph Laeubrich from comment #6)
> Can you explain how/why this breaks?

Because
1) Preference page shows up with invalid initial state
2) You can't "fix" it as a user

I have no time to debug it further, I want set preferences in that page and it doesn't work anymore. For me this is a blocker because EGit is the main part of the committers tool chain.

I plan to revert the change today to get a working Eclipse SDK build tomorrow.
Comment 9 Christoph Laeubrich CLA 2019-10-22 10:53:40 EDT
How do I start an SDK with the failing behavior? I can then debug it.
Comment 10 Andrey Loskutov CLA 2019-10-22 10:55:25 EDT
(In reply to Christoph Laeubrich from comment #9)
> How do I start an SDK with the failing behavior? I can then debug it.

Please try with https://wiki.eclipse.org/Platform/How_to_Contribute.
Comment 11 Christoph Laeubrich CLA 2019-10-22 11:20:20 EDT
Okay, GitPreferenceRoot has several field editors that try to use a different preference store in the way I have described, but this does not work as expected as described in Bug 551642 because field editors does not always use the getter, for example load/store.
The problematic code is

> public void store() {
>		if (preferenceStore == null) {
>			return;
>		}
>
>		if (isDefaultPresented) {
>			preferenceStore.setToDefault(preferenceName);
>		} else {
>			doStore();
>		}
>	}

as you can see, the preferenceStore.setToDefault(preferenceName) is always performed on the default store but NEVER on the "secondary" as the getter is simply never called, and as soon as the page is rendered overwritten by the store of the preference page!
Comment 12 Christoph Laeubrich CLA 2019-10-22 11:33:03 EDT
(In reply to Andrey Loskutov from comment #10)
> (In reply to Christoph Laeubrich from comment #9)
> > How do I start an SDK with the failing behavior? I can then debug it.
> 
> Please try with https://wiki.eclipse.org/Platform/How_to_Contribute.

I installed a "plaform-eclipse" and cloned egit and jgit, but I still can't get Eclipse to compile all modules, are there any special instructions/projectset for egit?
Comment 13 Christoph Laeubrich CLA 2019-10-22 11:41:32 EDT
Egit#Activator does the follwoing in several places:

> public static boolean autoStageDeletion() {
>		IEclipsePreferences d = DefaultScope.INSTANCE
>				.getNode(Activator.getPluginId());
>		IEclipsePreferences p = InstanceScope.INSTANCE
>				.getNode(Activator.getPluginId());
>		boolean autoStageDeletion = p.getBoolean(
>				GitCorePreferences.core_autoStageDeletion,
>				>d.getBoolean(GitCorePreferences.core_autoStageDeletion, false));
>		return autoStageDeletion;
>	}

As you can see here is always the same store used even though the code seem to be build to read first from one store and then from the other.
Comment 14 Thomas Wolf CLA 2019-10-22 11:46:23 EDT
(In reply to Christoph Laeubrich from comment #11)
> Okay, GitPreferenceRoot has several field editors that try to use a
> different preference store in the way I have described, but this does not
> work as expected as described in Bug 551642 because field editors does not
> always use the getter, for example load/store.
> The problematic code is
> 
> > public void store() {
> >		if (preferenceStore == null) {
> >			return;
> >		}
> >
> >		if (isDefaultPresented) {
> >			preferenceStore.setToDefault(preferenceName);
> >		} else {
> >			doStore();
> >		}
> >	}
> 
> as you can see, the preferenceStore.setToDefault(preferenceName) is always
> performed on the default store but NEVER on the "secondary" as the getter is
> simply never called, and as soon as the page is rendered overwritten by the
> store of the preference page!

Saw that, too. Indeed:

1. Start Eclipse (fresh workspace). Comes up with HTTP client "Apache HTTP". Change it to "Java built-in", apply. Quit Eclipse.
2. Start Eclipse (same workspace). Comes up with HTTP client "Java built-in". Click "Restore Defaults". Page shows "Apache HTTP". Apply. Quit Eclipse.
3. Start Eclipse (same workspace). Comes up with HTTP client "Java built-in".

Looks like EGit should override setPreferenceStore instead.
Comment 15 Thomas Wolf CLA 2019-10-22 11:49:40 EDT
(In reply to Christoph Laeubrich from comment #13)
> Egit#Activator does the follwoing in several places:
> 
> > public static boolean autoStageDeletion() {
> >		IEclipsePreferences d = DefaultScope.INSTANCE
> >				.getNode(Activator.getPluginId());
> >		IEclipsePreferences p = InstanceScope.INSTANCE
> >				.getNode(Activator.getPluginId());
> >		boolean autoStageDeletion = p.getBoolean(
> >				GitCorePreferences.core_autoStageDeletion,
> >				>d.getBoolean(GitCorePreferences.core_autoStageDeletion, false));
> >		return autoStageDeletion;
> >	}
> 
> As you can see here is always the same store used even though the code seem
> to be build to read first from one store and then from the other.

I fail to see what that has to do with this bug. This is maybe old code and cumbersome, but it does read the configured value or the default value from the EGit core preferences. It's not intended to read from two different bundle's InstanceScope preference stores.
Comment 16 Christoph Laeubrich CLA 2019-10-22 11:52:25 EDT
So what I can tell from the code is, that actually the preference store 'org.eclipse.egit.core' should be used for the path property, but actually the preference pages uses 'org.eclipse.egit.ui'.
Because there is code for load-delegation of old properties, that has worked before, now with the jface-fix it is written to the right store, the delegation fails to find it.

Is there nay tool in eclipse to inspect preference stores via the UI to verify this?
Comment 17 Christoph Laeubrich CLA 2019-10-22 12:01:52 EDT
(In reply to Thomas Wolf from comment #14)
> Looks like EGit should override setPreferenceStore instead.

The problem is Bug 552329 as getPreferenceStore returns non null, setPreferencestore is never called so the FieldEditor thinks there is no preference store at all. I'll provide a gerrit to fix this, this should the also fix this regression, sorry for confusion with autoStageDeletion.
Comment 18 Thomas Wolf CLA 2019-10-22 12:43:23 EDT
(In reply to Christoph Laeubrich from comment #17)
> (In reply to Thomas Wolf from comment #14)
> > Looks like EGit should override setPreferenceStore instead.
> 
> The problem is Bug 552329 as getPreferenceStore returns non null,
> setPreferencestore is never called so the FieldEditor thinks there is no
> preference store at all. I'll provide a gerrit to fix this, this should the
> also fix this regression, sorry for confusion with autoStageDeletion.

Yes, that's true, but the problem with the wrong store being used for default values will mean we'll have to fix EGit anyhow. A fix in platform (as you do for bug 552329) is nice, but EGit is supposed to work also on older platforms.
Comment 19 Christoph Laeubrich CLA 2019-10-22 13:10:55 EDT
(In reply to Thomas Wolf from comment #18)
> A fix in platform (as you do for bug 552329) is nice
Just focused on this specfic problem so Andrey has a fixed version soon :-)

> but EGit is supposed to work also on older platforms.
if you really concerned about that, you are right, if one overrides getPreferenceStore one should override setPreferenceStore in the following way:

>> public void setPreferenceStore(IPreferenceStore store) {
>>  super.setPreferenceStore(getSecondaryPreferenceStore());
>> }
Comment 20 Eclipse Genie CLA 2019-10-22 14:24:31 EDT
New Gerrit change created: https://git.eclipse.org/r/151444
Comment 21 Sarika Sinha CLA 2019-10-22 23:35:59 EDT
(In reply to Andrey Loskutov from comment #5)
> Sarika, I believe Platform JDT or debug had preferences bug recently
> reported too?
> 
> I will revert original patch, current state is a NO GO from EGit point of
> view.

Could not reproduce with I20191022-1800 and Egit 5.5. Looks like already fixed/reverted.

Could not find any preference issue reported in Debug.
Comment 22 Eclipse Genie CLA 2019-10-23 02:00:37 EDT
Gerrit change https://git.eclipse.org/r/151444 was merged to [master].
Commit: http://git.eclipse.org/c/egit/egit.git/commit/?id=f07a3e62f9944d8bdc37f7a84ab026470137027f
Comment 23 Andrey Loskutov CLA 2019-10-23 05:44:54 EDT
Thanks Christoph! Verified with I20191022-1800.