[EGIT RFC] Commit behaviour

[EGIT RFC] Commit behaviour

[Robin Rosenberg]

I got annoyed about having change my selection to a project to be able
to commit. This tentative feature allows me to hit the commit button
when any resource is selected and figure out which resources have been
modified. This makes it much easier to commit. Only the toolbar commit
is affected for now.

Another twist would be to list all changed resources, but only enable
the selected ones, or only the ones in in the same projects as the selected
resources. Comments?

Code is not efficient either. This is question about the user interface.

Then we could ask ourselved, should we do something similar for Checkout
and reset too? I think that is not as important as those operations are
much less frequent.

-- robin

[EGIT RFC] Add utilities for figuring out repositories for selected resources

[Robin Rosenberg]

We want to go from selected resources to repositories and back
to any resource in those repositories
---
 .../egit/ui/internal/actions/RepositoryAction.java |   50 ++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/RepositoryAction.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/RepositoryAction.java
index c4e3256..8c250ca 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/RepositoryAction.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/RepositoryAction.java
@@ -13,6 +13,8 @@ import java.util.HashSet;
 import java.util.Set;
 
 import org.eclipse.core.resources.IProject;
+import org.eclipse.core.resources.IResource;
+import org.eclipse.core.resources.ResourcesPlugin;
 import org.eclipse.jface.action.IAction;
 import org.eclipse.jface.dialogs.MessageDialog;
 import org.eclipse.team.internal.ui.actions.TeamAction;
@@ -36,6 +38,54 @@ public abstract class RepositoryAction extends TeamAction {
 	}
 
 	/**
+	 * @return the projects hosting the selected resources
+	 */
+	protected IProject[] getProjectsForSelectedResources() {
+		Set<IProject> ret = new HashSet<IProject>();
+		for (IResource resource : (IResource[])getSelectedAdaptables(getSelection(), IResource.class))
+			ret.add(resource.getProject());
+		return ret.toArray(new IProject[ret.size()]);
+	}
+
+	/**
+	 * @param projects
+	 *            a list of projects
+	 * @return the repositories that projects map to iff all projects are mapped
+	 */ 
+	protected Repository[] getRepositoriesFor(final IProject[] projects) {
+		Set<Repository> ret = new HashSet<Repository>();
+		for (IProject project : projects) {
+			RepositoryMapping repositoryMapping = RepositoryMapping.getMapping(project);
+			if (repositoryMapping == null)
+				return new Repository[0];
+			ret.add(repositoryMapping.getRepository());
+		}
+		return ret.toArray(new Repository[ret.size()]);
+	}
+	
+	/**
+	 * List the projects with selected resources, if all projects are connected
+	 * to a Git repository.
+	 * 
+	 * @return the tracked projects affected by the current resource selection
+	 */
+	public IProject[] getProjectsInRepositoryOfSelectedResources() {
+		Set<IProject> ret = new HashSet<IProject>();
+		Repository[] repositories = getRepositoriesFor(getProjectsForSelectedResources());
+		final IProject[] projects = ResourcesPlugin.getWorkspace().getRoot().getProjects();
+		for (IProject project : projects) {
+			RepositoryMapping mapping = RepositoryMapping.getMapping(project);
+			for (Repository repository : repositories) {
+				if (mapping != null && mapping.getRepository() == repository) {
+					ret.add(project);
+					break;
+				}
+			}
+		}
+		return ret.toArray(new IProject[ret.size()]);
+	}
+
+	/**
 	 * Figure out which repository to use. All selected
 	 * resources must map to the same Git repository.
 	 *
-- 
1.5.5.1.178.g1f811

[EGIT RFC] Enable commit for any resource in a Git-shared project

[Robin Rosenberg]

By default the commit dialog will be populated with all changed resources
in the projects that contain the selected resources, provided the projects
are associated with a Git respository.
---
 .../egit/ui/internal/actions/CommitAction.java     |   31 +++++++++----------
 1 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java
index db38c10..da32ea5 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java
@@ -68,7 +68,7 @@ public class CommitAction extends RepositoryAction {
 			return;
 		}
 
-		Repository[] repos = getRepositories();
+		Repository[] repos = getRepositoriesFor(getProjectsForSelectedResources());
 		amendAllowed = repos.length == 1;
 		for (Repository repo : repos) {
 			if (!repo.getRepositoryState().canCommit()) {
@@ -313,21 +313,20 @@ public class CommitAction extends RepositoryAction {
 	}
 
 	private void buildIndexHeadDiffList() throws IOException {
-		for (IProject project : getSelectedProjects()) {
+		for (IProject project : getProjectsInRepositoryOfSelectedResources()) {
 			RepositoryMapping repositoryMapping = RepositoryMapping.getMapping(project);
-			if (repositoryMapping != null) {
-				Repository repository = repositoryMapping.getRepository();
-				Tree head = repository.mapTree("HEAD");
-				GitIndex index = repository.getIndex();
-				IndexDiff indexDiff = new IndexDiff(head, index);
-				indexDiff.diff();
-
-				includeList(project, indexDiff.getAdded(), indexChanges);
-				includeList(project, indexDiff.getChanged(), indexChanges);
-				includeList(project, indexDiff.getRemoved(), indexChanges);
-				includeList(project, indexDiff.getMissing(), notIndexed);
-				includeList(project, indexDiff.getModified(), notIndexed);
-			}
+			assert repositoryMapping != null;
+			Repository repository = repositoryMapping.getRepository();
+			Tree head = repository.mapTree("HEAD");
+			GitIndex index = repository.getIndex();
+			IndexDiff indexDiff = new IndexDiff(head, index);
+			indexDiff.diff();
+
+			includeList(project, indexDiff.getAdded(), indexChanges);
+			includeList(project, indexDiff.getChanged(), indexChanges);
+			includeList(project, indexDiff.getRemoved(), indexChanges);
+			includeList(project, indexDiff.getMissing(), notIndexed);
+			includeList(project, indexDiff.getModified(), notIndexed);
 		}
 	}
 
@@ -395,7 +394,7 @@ public class CommitAction extends RepositoryAction {
 
 	@Override
 	public boolean isEnabled() {
-		return getRepositories().length > 0;
+		return getProjectsInRepositoryOfSelectedResources().length > 0;
 	}
 
 }
-- 
1.5.5.1.178.g1f811

Re: [EGIT RFC] Commit behaviour

[Marek Zawirski]

Robin Rosenberg wrote:
> I got annoyed about having change my selection to a project to be able
> to commit. This tentative feature allows me to hit the commit button
> when any resource is selected and figure out which resources have been
> modified. This makes it much easier to commit. Only the toolbar commit
> is affected for now.

That's a nice idea! I also thought about that one day, as such feature 
already exists in (e.g.) Subclipse that I was used to.

> 
> Another twist would be to list all changed resources, but only enable
> the selected ones, or only the ones in in the same projects as the selected
> resources. Comments?

I think that only selected && changed ones should be listed. When user 
selects explicitly some resources, he/she is probably interested only in 
these ones. And he/she probably did it for easier selection than from 
list of all changed resources, isn't it?

> Code is not efficient either. This is question about the user interface.

Theses patches don't work for me however :/ When I introduce some change 
to a resource/file, even add it to index and click commit (toolbar or 
menu), commit action does nothing. It is - nothing happens. Can you 
reproduce this problem, or should I debug it on my instance? Or do I use 
it in some wrong way?


> Then we could ask ourselved, should we do something similar for Checkout
> and reset too? I think that is not as important as those operations are
> much less frequent.
> 
> -- robin

Well, I think that checkout (maybe reset too) would be useful too - to 
revert some file quickly.

-- 
Marek Zawirski [zawir]
marek.zawirski@gmail.com

Re: [EGIT RFC] Commit behaviour

[Marek Zawirski]

> Robin Rosenberg wrote:
>> Then we could ask ourselved, should we do something similar for Checkout
>> and reset too? I think that is not as important as those operations are
>> much less frequent.

Hey, by the way - do we already have some action for checkout? Did I 
missed something?

-- 
Marek Zawirski [zawir]
marek.zawirski@gmail.com

Re: [EGIT RFC] Commit behaviour

[Robin Rosenberg]

[-- Attachment #1: Type: text/plain, Size: 626 bytes --]

lördagen den 14 juni 2008 00.41.33 skrev Marek Zawirski:
> > Robin Rosenberg wrote:
> >> Then we could ask ourselved, should we do something similar for Checkout
> >> and reset too? I think that is not as important as those operations are
> >> much less frequent.
> 
> Hey, by the way - do we already have some action for checkout? Did I 
> missed something?

You missed it. 

It's been there for a while (thanks to Roger Soares), but it is currently only available when a project is selected. It's in the team menu or, if you enable Git in Window/Customize Perspective, in the toolbar and menu bar. 

-- robin

[-- Attachment #2: commit.png --]
[-- Type: image/png, Size: 23453 bytes --]

Re: [EGIT RFC] Commit behaviour

[Robin Rosenberg]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

lördagen den 14 juni 2008 10.40.02 skrev Robin Rosenberg:
> lördagen den 14 juni 2008 00.41.33 skrev Marek Zawirski:
> > > Robin Rosenberg wrote:
> > >> Then we could ask ourselved, should we do something similar for Checkout
> > >> and reset too? I think that is not as important as those operations are
> > >> much less frequent.
> > 
> > Hey, by the way - do we already have some action for checkout? Did I 
> > missed something?
> 
> You missed it. 
> 
> It's been there for a while (thanks to Roger Soares), but it is currently only available when a project is selected. It's in the team menu or, if you enable Git in Window/Customize Perspective, in the toolbar and menu bar. 

Sorry about that. It's the Branch action. I think the original thought was to
make the dialog do more, like create branches. We should rename it regardless.

-- robin

[-- Attachment #2: checkout.png --]
[-- Type: image/png, Size: 17344 bytes --]

Re: [EGIT RFC] Commit behaviour

[Robin Rosenberg]

lördagen den 14 juni 2008 00.07.59 skrev Marek Zawirski:
> Robin Rosenberg wrote:
> > I got annoyed about having change my selection to a project to be able
> > to commit. This tentative feature allows me to hit the commit button
> > when any resource is selected and figure out which resources have been
> > modified. This makes it much easier to commit. Only the toolbar commit
> > is affected for now.
> 
> That's a nice idea! I also thought about that one day, as such feature 
> already exists in (e.g.) Subclipse that I was used to.
> 
> > 
> > Another twist would be to list all changed resources, but only enable
> > the selected ones, or only the ones in in the same projects as the selected
> > resources. Comments?
> 
> I think that only selected && changed ones should be listed. When user 
> selects explicitly some resources, he/she is probably interested only in 
> these ones. And he/she probably did it for easier selection than from 
> list of all changed resources, isn't it?

Selection is often implicit, i.e. if you link editor and selection, which I do, the selection
change every time you switch editor. 

> > Code is not efficient either. This is question about the user interface.
> 
> Theses patches don't work for me however :/ When I introduce some change 
> to a resource/file, even add it to index and click commit (toolbar or 
> menu), commit action does nothing. It is - nothing happens. Can you 
> reproduce this problem, or should I debug it on my instance? Or do I use 
> it in some wrong way?
It it's too hard to use properly it's wrong. Anyway I could reproduce it. Not sure
why it works sometimes though. I found this in the workspace log. Please 
check if you see it too, so I know we're chasing the same bug.

java.lang.ArrayIndexOutOfBoundsException: 0
        at org.spearce.egit.ui.internal.actions.CommitAction.loadPreviousCommit(CommitAction.java:127)


> > Then we could ask ourselved, should we do something similar for Checkout
> > and reset too? I think that is not as important as those operations are
> > much less frequent.
> > 
> > -- robin
> 
> Well, I think that checkout (maybe reset too) would be useful too - to 
> revert some file quickly.

We do not have a per-file checout/reset yet. What we have checks out all files in the repository.

-- robin

Re: [EGIT RFC] Commit behaviour

[Robin Rosenberg]

lördagen den 14 juni 2008 00.07.59 skrev Marek Zawirski:
> Theses patches don't work for me however :/ When I introduce some change 
> to a resource/file, even add it to index and click commit (toolbar or 
> menu), commit action does nothing. It is - nothing happens. Can you 
> reproduce this problem, or should I debug it on my instance? Or do I use 
> it in some wrong way?

Found it. Obviously this won't work for commits to multiple repositories which is supposed
to work really, but I think that was broken earlier by yours truly. This fix should make it
possible to test the idea though this thread is about, and see if we have more selection
problems. 

-- robin

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java
index da32ea5..922649b 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java
@@ -124,7 +124,7 @@ public class CommitAction extends RepositoryAction {
        }

        private void loadPreviousCommit() {
-               IProject project = getSelectedProjects()[0];
+               IProject project = getProjectsForSelectedResources()[0];

                Repository repo = RepositoryMapping.getMapping(project).getRepository();
                try {

Re: [EGIT RFC] Commit behaviour

[Marek Zawirski]

Robin Rosenberg wrote:
> Found it. Obviously this won't work for commits to multiple repositories which is supposed
> to work really, but I think that was broken earlier by yours truly. This fix should make it
> possible to test the idea though this thread is about, and see if we have more selection
> problems. 
> 
> -- robin
> 
> diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java
> index da32ea5..922649b 100644
> --- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java
> +++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java
> @@ -124,7 +124,7 @@ public class CommitAction extends RepositoryAction {
>         }
> 
>         private void loadPreviousCommit() {
> -               IProject project = getSelectedProjects()[0];
> +               IProject project = getProjectsForSelectedResources()[0];
> 
>                 Repository repo = RepositoryMapping.getMapping(project).getRepository();
>                 try {
> 

Yeah, it works for me now, with this patch applied.

And yes, without this patch, I found mentioned exception in log:
java.lang.ArrayIndexOutOfBoundsException: 0
  at 
org.spearce.egit.ui.internal.actions.CommitAction.loadPreviousCommit(CommitAction.java:127)

-- 
Marek Zawirski [zawir]
marek.zawirski@gmail.com

Re: [EGIT RFC] Commit behaviour

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> I got annoyed about having change my selection to a project to be able
> to commit. This tentative feature allows me to hit the commit button
> when any resource is selected and figure out which resources have been
> modified. This makes it much easier to commit. Only the toolbar commit
> is affected for now.
> 
> Another twist would be to list all changed resources, but only enable
> the selected ones, or only the ones in in the same projects as the selected
> resources. Comments?

I think this is a good idea.  You really do want to make commits
that cover the entire repository, rather than a subsection of it.
I think that covering the whole repository is much more common.

> Then we could ask ourselved, should we do something similar for Checkout
> and reset too? I think that is not as important as those operations are
> much less frequent.

Yes, we do want to.  If you are doing a reset you probably need
to whack everything that is dirty and restore the state, likewise
if you are doing a checkout you are likely switching branches and
would expect the entire repository to switch at once.

-- 
Shawn.