[PATCH JGIT] Propose author and committer in the commit dialog

[PATCH JGIT] Propose author and committer in the commit dialog

[Yann Simon]

Add a field 'committer'.
The fields 'author' and 'committer' are populated with the values
found in the configuration.

Validate the author and the committer.

Add the signed-off line in the comment text box when the user clicks
on the signed-off checkbox.

Use Text.DELIMITER as line break for plateform independance.

A default PersonIdent is an author and gets the values from the
configuration.

Signed-off-by: Yann Simon <yann.simon.fr@gmail.com>
---
 .../src/org/spearce/egit/ui/UIText.java            |    6 +
 .../egit/ui/internal/actions/CommitAction.java     |   55 +++++------
 .../egit/ui/internal/dialogs/CommitDialog.java     |   97 +++++++++++++++++++-
 .../src/org/spearce/egit/ui/uitext.properties      |    2 +
 .../src/org/spearce/jgit/lib/PersonIdent.java      |   20 +----
 5 files changed, 128 insertions(+), 52 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java
index d74f53e..249f2a0 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java
@@ -584,6 +584,9 @@
 	public static String CommitDialog_CommitChanges;
 
 	/** */
+	public static String CommitDialog_Committer;
+
+	/** */
 	public static String CommitDialog_CommitMessage;
 
 	/** */
@@ -596,6 +599,9 @@
 	public static String CommitDialog_ErrorInvalidAuthorSpecified;
 
 	/** */
+	public static String CommitDialog_ErrorInvalidCommitterSpecified;
+
+	/** */
 	public static String CommitDialog_ErrorMustEnterCommitMessage;
 
 	/** */
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 ae26770..9a9d494 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
@@ -99,15 +99,16 @@ public void run(IAction act) {
 		}
 
 		String author = null;
+		String committer = null;
 		if (repository != null) {
 			final RepositoryConfig config = repository.getConfig();
 			author = config.getAuthorName();
-			if (author != null && author.length() != 0) {
-				final String email = config.getAuthorEmail();
-				if (email != null && email.length() != 0) {
-					author = author + " <" + email + ">";
-				}
-			}
+			final String authorEmail = config.getAuthorEmail();
+			author = author + " <" + authorEmail + ">";
+
+			committer = config.getCommitterName();
+			final String committerEmail = config.getAuthorEmail();
+			committer = committer + " <" + committerEmail + ">";
 		}
 
 		loadPreviousCommit();
@@ -117,9 +118,13 @@ public void run(IAction act) {
 		commitDialog.setAmendAllowed(amendAllowed);
 		commitDialog.setFileList(files);
 		commitDialog.setAuthor(author);
+		commitDialog.setCommitter(committer);
 
-		if (previousCommit != null)
+		if (previousCommit != null) {
 			commitDialog.setPreviousCommitMessage(previousCommit.getMessage());
+			PersonIdent previousAuthor = previousCommit.getAuthor();
+			commitDialog.setPreviousAuthor(previousAuthor.getName() + " <" + previousAuthor.getEmailAddress() + ">");
+		}
 
 		if (commitDialog.open() != IDialogConstants.OK_ID)
 			return;
@@ -167,7 +172,7 @@ private void performCommit(CommitDialog commitDialog, String commitMessage)
 		}
 
 		try {
-			commitMessage = doCommits(commitDialog, commitMessage, treeMap);
+			doCommits(commitDialog, commitMessage, treeMap);
 		} catch (IOException e) {
 			throw new TeamException("Committing changes", e);
 		}
@@ -176,8 +181,17 @@ private void performCommit(CommitDialog commitDialog, String commitMessage)
 		}
 	}
 
-	private String doCommits(CommitDialog commitDialog, String commitMessage,
+	private void doCommits(CommitDialog commitDialog, String commitMessage,
 			HashMap<Repository, Tree> treeMap) throws IOException, TeamException {
+
+		String author = commitDialog.getAuthor();
+		String committer = commitDialog.getCommitter();
+		Date commitDate = new Date(Calendar.getInstance().getTimeInMillis());
+		TimeZone timeZone = TimeZone.getDefault();
+
+		PersonIdent authorIdent = new PersonIdent(author);
+		PersonIdent committerIdent = new PersonIdent(committer);
+
 		for (java.util.Map.Entry<Repository, Tree> entry : treeMap.entrySet()) {
 			Tree tree = entry.getValue();
 			Repository repo = tree.getRepository();
@@ -195,27 +209,9 @@ private String doCommits(CommitDialog commitDialog, String commitMessage,
 			}
 			Commit commit = new Commit(repo, parentIds);
 			commit.setTree(tree);
-			commitMessage = commitMessage.replaceAll("\r", "\n");
-
-			PersonIdent personIdent = new PersonIdent(repo);
-			String username = personIdent.getName();
-			String email = personIdent.getEmailAddress();
-
-			if (commitDialog.isSignedOff()) {
-				commitMessage += "\n\nSigned-off-by: " + username + " <"
-						+ email + ">";
-			}
 			commit.setMessage(commitMessage);
-
-			if (commitDialog.getAuthor() == null) {
-				commit.setAuthor(personIdent);
-			} else {
-				PersonIdent author = new PersonIdent(commitDialog.getAuthor());
-				commit.setAuthor(new PersonIdent(author, new Date(Calendar
-						.getInstance().getTimeInMillis()), TimeZone
-						.getDefault()));
-			}
-			commit.setCommitter(personIdent);
+			commit.setAuthor(new PersonIdent(authorIdent, commitDate, timeZone));
+			commit.setCommitter(new PersonIdent(committerIdent, commitDate, timeZone));
 
 			ObjectWriter writer = new ObjectWriter(repo);
 			commit.setCommitId(writer.writeCommit(commit));
@@ -228,7 +224,6 @@ private String doCommits(CommitDialog commitDialog, String commitMessage,
 						+ " to commit " + commit.getCommitId() + ".");
 			}
 		}
-		return commitMessage;
 	}
 
 	private void prepareTrees(IFile[] selectedItems,
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java
index fede948..6b6cb55 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java
@@ -17,6 +17,7 @@
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.Iterator;
+import java.util.regex.Pattern;
 
 import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IProject;
@@ -67,6 +68,8 @@
  */
 public class CommitDialog extends Dialog {
 
+	private static Pattern signedOffPattern = Pattern.compile("(.|\r|\n)*Signed-off-by: .*(\r|\n)*"); //$NON-NLS-1$
+
 	class CommitContentProvider implements IStructuredContentProvider {
 
 		public void inputChanged(Viewer viewer, Object oldInput, Object newInput) {
@@ -129,6 +132,7 @@ createButton(parent, IDialogConstants.CANCEL_ID,
 
 	Text commitText;
 	Text authorText;
+	Text committerText;
 	Button amendingButton;
 	Button signedOffButton;
 
@@ -170,11 +174,18 @@ public void keyPressed(KeyEvent arg0) {
 		if (author != null)
 			authorText.setText(author);
 
+		new Label(container, SWT.LEFT).setText(UIText.CommitDialog_Committer);
+		committerText = new Text(container, SWT.BORDER);
+		committerText.setLayoutData(GridDataFactory.fillDefaults().grab(true, false).create());
+		if (committer != null)
+			committerText.setText(committer);
+
 		amendingButton = new Button(container, SWT.CHECK);
 		if (amending) {
 			amendingButton.setSelection(amending);
 			amendingButton.setEnabled(false); // if already set, don't allow any changes
 			commitText.setText(previousCommitMessage);
+			authorText.setText(previousAuthor);
 		} else if (!amendAllowed) {
 			amendingButton.setEnabled(false);
 		}
@@ -187,8 +198,9 @@ public void widgetSelected(SelectionEvent arg0) {
 					alreadyAdded = true;
 					String curText = commitText.getText();
 					if (curText.length() > 0)
-						curText += "\n"; //$NON-NLS-1$
+						curText += Text.DELIMITER;
 					commitText.setText(curText + previousCommitMessage);
+					authorText.setText(previousAuthor);
 				}
 			}
 
@@ -205,6 +217,33 @@ public void widgetDefaultSelected(SelectionEvent arg0) {
 		signedOffButton.setText(UIText.CommitDialog_AddSOB);
 		signedOffButton.setLayoutData(GridDataFactory.fillDefaults().grab(true, false).span(2, 1).create());
 
+		signedOffButton.addSelectionListener(new SelectionListener() {
+			boolean alreadySigned = false;
+			public void widgetSelected(SelectionEvent arg0) {
+				if (alreadySigned)
+					return;
+				if (signedOffButton.getSelection()) {
+					alreadySigned = true;
+					String curText = commitText.getText();
+
+					// add new lines if necessary
+					if (!curText.endsWith(Text.DELIMITER)) {
+						curText += Text.DELIMITER;
+					}
+
+					if (!signedOffPattern.matcher(new StringBuilder(curText)).matches()) {
+						// if the last line is not a signed off (amend a commit), add a line break
+						curText += Text.DELIMITER;
+					}
+					commitText.setText(curText + "Signed-off-by: " + committerText.getText()); //$NON-NLS-1$
+				}
+			}
+
+			public void widgetDefaultSelected(SelectionEvent arg0) {
+				// Empty
+			}
+		});
+
 		Table resourcesTable = new Table(container, SWT.H_SCROLL | SWT.V_SCROLL
 				| SWT.FULL_SELECTION | SWT.MULTI | SWT.CHECK | SWT.BORDER);
 		resourcesTable.setLayoutData(GridDataFactory.fillDefaults().hint(600,
@@ -237,6 +276,7 @@ private Menu getContextMenu() {
 		MenuItem item = new MenuItem(menu, SWT.PUSH);
 		item.setText(UIText.CommitDialog_AddFileOnDiskToIndex);
 		item.addListener(SWT.Selection, new Listener() {
+			@SuppressWarnings("unchecked")
 			public void handleEvent(Event arg0) {
 				IStructuredSelection sel = (IStructuredSelection) filesViewer.getSelection();
 				if (sel.isEmpty()) {
@@ -312,6 +352,7 @@ private static String getFileStatus(IFile file) {
 			}
 
 		} catch (Exception e) {
+			e.printStackTrace();
 		}
 
 		return prefix;
@@ -321,6 +362,7 @@ private static String getFileStatus(IFile file) {
 	 * @return The message the user entered
 	 */
 	public String getCommitMessage() {
+		commitMessage.replaceAll(Text.DELIMITER, "\n"); //$NON-NLS-1$
 		return commitMessage;
 	}
 
@@ -334,6 +376,8 @@ public void setCommitMessage(String s) {
 
 	private String commitMessage = ""; //$NON-NLS-1$
 	private String author = null;
+	private String committer = null;
+	private String previousAuthor = null;
 	private boolean signedOff = false;
 	private boolean amending = false;
 	private boolean amendAllowed = true;
@@ -397,6 +441,7 @@ public void widgetSelected(SelectionEvent e) {
 	protected void okPressed() {
 		commitMessage = commitText.getText();
 		author = authorText.getText().trim();
+		committer = committerText.getText().trim();
 		signedOff = signedOffButton.getSelection();
 		amending = amendingButton.getSelection();
 
@@ -410,14 +455,33 @@ protected void okPressed() {
 			return;
 		}
 
+		boolean authorValid = false;
 		if (author.length() > 0) {
 			try {
 				new PersonIdent(author);
+				authorValid = true;
 			} catch (IllegalArgumentException e) {
-				MessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorInvalidAuthor, UIText.CommitDialog_ErrorInvalidAuthorSpecified);
-				return;
+				authorValid = false;
 			}
-		} else author = null;
+		}
+		if (!authorValid) {
+			MessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorInvalidAuthor, UIText.CommitDialog_ErrorInvalidAuthorSpecified);
+			return;
+		}
+
+		boolean committerValid = false;
+		if (committer.length() > 0) {
+			try {
+				new PersonIdent(committer);
+				committerValid = true;
+			} catch (IllegalArgumentException e) {
+				committerValid = false;
+			}
+		}
+		if (!committerValid) {
+			MessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorInvalidAuthor, UIText.CommitDialog_ErrorInvalidCommitterSpecified);
+			return;
+		}
 
 		if (selectedFiles.isEmpty() && !amending) {
 			MessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorNoItemsSelected, UIText.CommitDialog_ErrorNoItemsSelectedToBeCommitted);
@@ -470,6 +534,31 @@ public void setAuthor(String author) {
 	}
 
 	/**
+	 * @return The committer to set for the commit
+	 */
+	public String getCommitter() {
+		return committer;
+	}
+
+	/**
+	 * Pre-set committer for the commit
+	 *
+	 * @param committer
+	 */
+	public void setCommitter(String committer) {
+		this.committer = committer;
+	}
+
+	/**
+	 * Pre-set the previous author if amending the commit
+	 *
+	 * @param previousAuthor
+	 */
+	public void setPreviousAuthor(String previousAuthor) {
+		this.previousAuthor = previousAuthor;
+	}
+
+	/**
 	 * @return whether to auto-add a signed-off line to the message
 	 */
 	public boolean isSignedOff() {
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties b/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties
index 52fa4f8..142b426 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties
@@ -226,10 +226,12 @@ CommitDialog_AmendPreviousCommit=A&mend previous commit
 CommitDialog_Author=&Author:
 CommitDialog_Commit=&Commit
 CommitDialog_CommitChanges=Commit Changes
+CommitDialog_Committer=Committer:
 CommitDialog_CommitMessage=Commit &Message:
 CommitDialog_DeselectAll=&Deselect All
 CommitDialog_ErrorInvalidAuthor=Invalid author
 CommitDialog_ErrorInvalidAuthorSpecified=Invalid author specified. Please use the form:\nA U Thor <author@example.com>
+CommitDialog_ErrorInvalidCommitterSpecified=Invalid committer specified. Please use the form:\nCommi T Ter <committer@example.com>
 CommitDialog_ErrorMustEnterCommitMessage=You must enter a commit message
 CommitDialog_ErrorNoItemsSelected=No items selected
 CommitDialog_ErrorNoItemsSelectedToBeCommitted=No items are currently selected to be committed.
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PersonIdent.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PersonIdent.java
index bc5479a..8438683 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/PersonIdent.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PersonIdent.java
@@ -57,16 +57,6 @@
 
 	private final int tzOffset;
 
-	private static String getHostName() {
-		try {
-			java.net.InetAddress addr = java.net.InetAddress.getLocalHost();
-			String hostname = addr.getCanonicalHostName();
-			return hostname;
-		} catch (java.net.UnknownHostException e) {
-			return "localhost";
-		}
-	}
-
 	/**
 	 * Creates new PersonIdent from config info in repository, with current time
 	 * 
@@ -74,14 +64,8 @@ private static String getHostName() {
 	 */
 	public PersonIdent(final Repository repo) {
 		RepositoryConfig config = repo.getConfig();
-		String username = config.getString("user", null, "name");
-		if (username == null)
-			username = System.getProperty("user.name");
-
-		String email = config.getString("user", null, "email");
-		if (email == null)
-			email = System.getProperty("user.name") + "@" + getHostName();
-
+		String username = config.getAuthorName();
+		String email = config.getAuthorEmail();
 		name = username;
 		emailAddress = email;
 		when = System.currentTimeMillis();
-- 
1.6.0.4

Re: [PATCH JGIT] Propose author and committer in the commit dialog

[Shawn O. Pearce]

Yann Simon <yann.simon.fr@gmail.com> wrote:
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PersonIdent.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PersonIdent.java
> index bc5479a..8438683 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/PersonIdent.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PersonIdent.java
> @@ -57,16 +57,6 @@
>  
>  	private final int tzOffset;
>  
> -	private static String getHostName() {
> -		try {
> -			java.net.InetAddress addr = java.net.InetAddress.getLocalHost();
> -			String hostname = addr.getCanonicalHostName();
> -			return hostname;
> -		} catch (java.net.UnknownHostException e) {
> -			return "localhost";
> -		}
> -	}
> -
>  	/**
>  	 * Creates new PersonIdent from config info in repository, with current time
>  	 * 
> @@ -74,14 +64,8 @@ private static String getHostName() {
>  	 */
>  	public PersonIdent(final Repository repo) {
>  		RepositoryConfig config = repo.getConfig();
> -		String username = config.getString("user", null, "name");
> -		if (username == null)
> -			username = System.getProperty("user.name");
> -
> -		String email = config.getString("user", null, "email");
> -		if (email == null)
> -			email = System.getProperty("user.name") + "@" + getHostName();
> -
> +		String username = config.getAuthorName();
> +		String email = config.getAuthorEmail();
>  		name = username;
>  		emailAddress = email;
>  		when = System.currentTimeMillis();

Heh.  I meant to write this patch myself yesterday but got
sidetracked and forgot.  Thanks.

Its unrelated to the other changes you were making.  Please submit
it as its own patch.

Aren't the local "username" and "email" variables now redundant?
Can't we just assign directly to the fields in the object?

Also, I think you should be using getCommitter{Name,Email} here
by default.  Sometimes people override the GIT_AUTHOR_* variables
in order to apply patches from others, but they almost never set the
GIT_COMMITTER_* variables.  We're using this constructor to create an
identity for use in commit objects, tag objects, or for the reflog.
All three prefer the committer name over the author name.

-- 
Shawn.

[PATCH JGIT] Propose author and committer in the commit dialog

[Yann Simon]

Add a field 'committer'.
The fields 'author' and 'committer' are populated with the values
found in the configuration.

Validate the author and the committer.

Add the signed-off line in the comment text box when the user clicks
on the signed-off checkbox.

Use Text.DELIMITER as line break for plateform independance.

Signed-off-by: Yann Simon <yann.simon.fr@gmail.com>
---
I remove the modifications of PersonIdent and send them in another patch.

 .../src/org/spearce/egit/ui/UIText.java            |    6 +
 .../egit/ui/internal/actions/CommitAction.java     |   55 +++++------
 .../egit/ui/internal/dialogs/CommitDialog.java     |   97 +++++++++++++++++++-
 .../src/org/spearce/egit/ui/uitext.properties      |    2 +
 4 files changed, 126 insertions(+), 34 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java
index d74f53e..249f2a0 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java
@@ -584,6 +584,9 @@
 	public static String CommitDialog_CommitChanges;
 
 	/** */
+	public static String CommitDialog_Committer;
+
+	/** */
 	public static String CommitDialog_CommitMessage;
 
 	/** */
@@ -596,6 +599,9 @@
 	public static String CommitDialog_ErrorInvalidAuthorSpecified;
 
 	/** */
+	public static String CommitDialog_ErrorInvalidCommitterSpecified;
+
+	/** */
 	public static String CommitDialog_ErrorMustEnterCommitMessage;
 
 	/** */
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 ae26770..9a9d494 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
@@ -99,15 +99,16 @@ public void run(IAction act) {
 		}
 
 		String author = null;
+		String committer = null;
 		if (repository != null) {
 			final RepositoryConfig config = repository.getConfig();
 			author = config.getAuthorName();
-			if (author != null && author.length() != 0) {
-				final String email = config.getAuthorEmail();
-				if (email != null && email.length() != 0) {
-					author = author + " <" + email + ">";
-				}
-			}
+			final String authorEmail = config.getAuthorEmail();
+			author = author + " <" + authorEmail + ">";
+
+			committer = config.getCommitterName();
+			final String committerEmail = config.getAuthorEmail();
+			committer = committer + " <" + committerEmail + ">";
 		}
 
 		loadPreviousCommit();
@@ -117,9 +118,13 @@ public void run(IAction act) {
 		commitDialog.setAmendAllowed(amendAllowed);
 		commitDialog.setFileList(files);
 		commitDialog.setAuthor(author);
+		commitDialog.setCommitter(committer);
 
-		if (previousCommit != null)
+		if (previousCommit != null) {
 			commitDialog.setPreviousCommitMessage(previousCommit.getMessage());
+			PersonIdent previousAuthor = previousCommit.getAuthor();
+			commitDialog.setPreviousAuthor(previousAuthor.getName() + " <" + previousAuthor.getEmailAddress() + ">");
+		}
 
 		if (commitDialog.open() != IDialogConstants.OK_ID)
 			return;
@@ -167,7 +172,7 @@ private void performCommit(CommitDialog commitDialog, String commitMessage)
 		}
 
 		try {
-			commitMessage = doCommits(commitDialog, commitMessage, treeMap);
+			doCommits(commitDialog, commitMessage, treeMap);
 		} catch (IOException e) {
 			throw new TeamException("Committing changes", e);
 		}
@@ -176,8 +181,17 @@ private void performCommit(CommitDialog commitDialog, String commitMessage)
 		}
 	}
 
-	private String doCommits(CommitDialog commitDialog, String commitMessage,
+	private void doCommits(CommitDialog commitDialog, String commitMessage,
 			HashMap<Repository, Tree> treeMap) throws IOException, TeamException {
+
+		String author = commitDialog.getAuthor();
+		String committer = commitDialog.getCommitter();
+		Date commitDate = new Date(Calendar.getInstance().getTimeInMillis());
+		TimeZone timeZone = TimeZone.getDefault();
+
+		PersonIdent authorIdent = new PersonIdent(author);
+		PersonIdent committerIdent = new PersonIdent(committer);
+
 		for (java.util.Map.Entry<Repository, Tree> entry : treeMap.entrySet()) {
 			Tree tree = entry.getValue();
 			Repository repo = tree.getRepository();
@@ -195,27 +209,9 @@ private String doCommits(CommitDialog commitDialog, String commitMessage,
 			}
 			Commit commit = new Commit(repo, parentIds);
 			commit.setTree(tree);
-			commitMessage = commitMessage.replaceAll("\r", "\n");
-
-			PersonIdent personIdent = new PersonIdent(repo);
-			String username = personIdent.getName();
-			String email = personIdent.getEmailAddress();
-
-			if (commitDialog.isSignedOff()) {
-				commitMessage += "\n\nSigned-off-by: " + username + " <"
-						+ email + ">";
-			}
 			commit.setMessage(commitMessage);
-
-			if (commitDialog.getAuthor() == null) {
-				commit.setAuthor(personIdent);
-			} else {
-				PersonIdent author = new PersonIdent(commitDialog.getAuthor());
-				commit.setAuthor(new PersonIdent(author, new Date(Calendar
-						.getInstance().getTimeInMillis()), TimeZone
-						.getDefault()));
-			}
-			commit.setCommitter(personIdent);
+			commit.setAuthor(new PersonIdent(authorIdent, commitDate, timeZone));
+			commit.setCommitter(new PersonIdent(committerIdent, commitDate, timeZone));
 
 			ObjectWriter writer = new ObjectWriter(repo);
 			commit.setCommitId(writer.writeCommit(commit));
@@ -228,7 +224,6 @@ private String doCommits(CommitDialog commitDialog, String commitMessage,
 						+ " to commit " + commit.getCommitId() + ".");
 			}
 		}
-		return commitMessage;
 	}
 
 	private void prepareTrees(IFile[] selectedItems,
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java
index fede948..6b6cb55 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java
@@ -17,6 +17,7 @@
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.Iterator;
+import java.util.regex.Pattern;
 
 import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IProject;
@@ -67,6 +68,8 @@
  */
 public class CommitDialog extends Dialog {
 
+	private static Pattern signedOffPattern = Pattern.compile("(.|\r|\n)*Signed-off-by: .*(\r|\n)*"); //$NON-NLS-1$
+
 	class CommitContentProvider implements IStructuredContentProvider {
 
 		public void inputChanged(Viewer viewer, Object oldInput, Object newInput) {
@@ -129,6 +132,7 @@ createButton(parent, IDialogConstants.CANCEL_ID,
 
 	Text commitText;
 	Text authorText;
+	Text committerText;
 	Button amendingButton;
 	Button signedOffButton;
 
@@ -170,11 +174,18 @@ public void keyPressed(KeyEvent arg0) {
 		if (author != null)
 			authorText.setText(author);
 
+		new Label(container, SWT.LEFT).setText(UIText.CommitDialog_Committer);
+		committerText = new Text(container, SWT.BORDER);
+		committerText.setLayoutData(GridDataFactory.fillDefaults().grab(true, false).create());
+		if (committer != null)
+			committerText.setText(committer);
+
 		amendingButton = new Button(container, SWT.CHECK);
 		if (amending) {
 			amendingButton.setSelection(amending);
 			amendingButton.setEnabled(false); // if already set, don't allow any changes
 			commitText.setText(previousCommitMessage);
+			authorText.setText(previousAuthor);
 		} else if (!amendAllowed) {
 			amendingButton.setEnabled(false);
 		}
@@ -187,8 +198,9 @@ public void widgetSelected(SelectionEvent arg0) {
 					alreadyAdded = true;
 					String curText = commitText.getText();
 					if (curText.length() > 0)
-						curText += "\n"; //$NON-NLS-1$
+						curText += Text.DELIMITER;
 					commitText.setText(curText + previousCommitMessage);
+					authorText.setText(previousAuthor);
 				}
 			}
 
@@ -205,6 +217,33 @@ public void widgetDefaultSelected(SelectionEvent arg0) {
 		signedOffButton.setText(UIText.CommitDialog_AddSOB);
 		signedOffButton.setLayoutData(GridDataFactory.fillDefaults().grab(true, false).span(2, 1).create());
 
+		signedOffButton.addSelectionListener(new SelectionListener() {
+			boolean alreadySigned = false;
+			public void widgetSelected(SelectionEvent arg0) {
+				if (alreadySigned)
+					return;
+				if (signedOffButton.getSelection()) {
+					alreadySigned = true;
+					String curText = commitText.getText();
+
+					// add new lines if necessary
+					if (!curText.endsWith(Text.DELIMITER)) {
+						curText += Text.DELIMITER;
+					}
+
+					if (!signedOffPattern.matcher(new StringBuilder(curText)).matches()) {
+						// if the last line is not a signed off (amend a commit), add a line break
+						curText += Text.DELIMITER;
+					}
+					commitText.setText(curText + "Signed-off-by: " + committerText.getText()); //$NON-NLS-1$
+				}
+			}
+
+			public void widgetDefaultSelected(SelectionEvent arg0) {
+				// Empty
+			}
+		});
+
 		Table resourcesTable = new Table(container, SWT.H_SCROLL | SWT.V_SCROLL
 				| SWT.FULL_SELECTION | SWT.MULTI | SWT.CHECK | SWT.BORDER);
 		resourcesTable.setLayoutData(GridDataFactory.fillDefaults().hint(600,
@@ -237,6 +276,7 @@ private Menu getContextMenu() {
 		MenuItem item = new MenuItem(menu, SWT.PUSH);
 		item.setText(UIText.CommitDialog_AddFileOnDiskToIndex);
 		item.addListener(SWT.Selection, new Listener() {
+			@SuppressWarnings("unchecked")
 			public void handleEvent(Event arg0) {
 				IStructuredSelection sel = (IStructuredSelection) filesViewer.getSelection();
 				if (sel.isEmpty()) {
@@ -312,6 +352,7 @@ private static String getFileStatus(IFile file) {
 			}
 
 		} catch (Exception e) {
+			e.printStackTrace();
 		}
 
 		return prefix;
@@ -321,6 +362,7 @@ private static String getFileStatus(IFile file) {
 	 * @return The message the user entered
 	 */
 	public String getCommitMessage() {
+		commitMessage.replaceAll(Text.DELIMITER, "\n"); //$NON-NLS-1$
 		return commitMessage;
 	}
 
@@ -334,6 +376,8 @@ public void setCommitMessage(String s) {
 
 	private String commitMessage = ""; //$NON-NLS-1$
 	private String author = null;
+	private String committer = null;
+	private String previousAuthor = null;
 	private boolean signedOff = false;
 	private boolean amending = false;
 	private boolean amendAllowed = true;
@@ -397,6 +441,7 @@ public void widgetSelected(SelectionEvent e) {
 	protected void okPressed() {
 		commitMessage = commitText.getText();
 		author = authorText.getText().trim();
+		committer = committerText.getText().trim();
 		signedOff = signedOffButton.getSelection();
 		amending = amendingButton.getSelection();
 
@@ -410,14 +455,33 @@ protected void okPressed() {
 			return;
 		}
 
+		boolean authorValid = false;
 		if (author.length() > 0) {
 			try {
 				new PersonIdent(author);
+				authorValid = true;
 			} catch (IllegalArgumentException e) {
-				MessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorInvalidAuthor, UIText.CommitDialog_ErrorInvalidAuthorSpecified);
-				return;
+				authorValid = false;
 			}
-		} else author = null;
+		}
+		if (!authorValid) {
+			MessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorInvalidAuthor, UIText.CommitDialog_ErrorInvalidAuthorSpecified);
+			return;
+		}
+
+		boolean committerValid = false;
+		if (committer.length() > 0) {
+			try {
+				new PersonIdent(committer);
+				committerValid = true;
+			} catch (IllegalArgumentException e) {
+				committerValid = false;
+			}
+		}
+		if (!committerValid) {
+			MessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorInvalidAuthor, UIText.CommitDialog_ErrorInvalidCommitterSpecified);
+			return;
+		}
 
 		if (selectedFiles.isEmpty() && !amending) {
 			MessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorNoItemsSelected, UIText.CommitDialog_ErrorNoItemsSelectedToBeCommitted);
@@ -470,6 +534,31 @@ public void setAuthor(String author) {
 	}
 
 	/**
+	 * @return The committer to set for the commit
+	 */
+	public String getCommitter() {
+		return committer;
+	}
+
+	/**
+	 * Pre-set committer for the commit
+	 *
+	 * @param committer
+	 */
+	public void setCommitter(String committer) {
+		this.committer = committer;
+	}
+
+	/**
+	 * Pre-set the previous author if amending the commit
+	 *
+	 * @param previousAuthor
+	 */
+	public void setPreviousAuthor(String previousAuthor) {
+		this.previousAuthor = previousAuthor;
+	}
+
+	/**
 	 * @return whether to auto-add a signed-off line to the message
 	 */
 	public boolean isSignedOff() {
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties b/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties
index 52fa4f8..142b426 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties
@@ -226,10 +226,12 @@ CommitDialog_AmendPreviousCommit=A&mend previous commit
 CommitDialog_Author=&Author:
 CommitDialog_Commit=&Commit
 CommitDialog_CommitChanges=Commit Changes
+CommitDialog_Committer=Committer:
 CommitDialog_CommitMessage=Commit &Message:
 CommitDialog_DeselectAll=&Deselect All
 CommitDialog_ErrorInvalidAuthor=Invalid author
 CommitDialog_ErrorInvalidAuthorSpecified=Invalid author specified. Please use the form:\nA U Thor <author@example.com>
+CommitDialog_ErrorInvalidCommitterSpecified=Invalid committer specified. Please use the form:\nCommi T Ter <committer@example.com>
 CommitDialog_ErrorMustEnterCommitMessage=You must enter a commit message
 CommitDialog_ErrorNoItemsSelected=No items selected
 CommitDialog_ErrorNoItemsSelectedToBeCommitted=No items are currently selected to be committed.
-- 
1.6.0.4

Re: [PATCH JGIT] Propose author and committer in the commit dialog

[Shawn O. Pearce]

Yann Simon <yann.simon.fr@gmail.com> wrote:
> Add a field 'committer'.
> The fields 'author' and 'committer' are populated with the values
> found in the configuration.
> 
> Validate the author and the committer.
> 
> Add the signed-off line in the comment text box when the user clicks
> on the signed-off checkbox.
> 
> Use Text.DELIMITER as line break for plateform independance.

Far too many things in one change.  Please break these apart into
multiple commits so they are easier to review and test in isolation.
 
> I remove the modifications of PersonIdent and send them in another patch.

Thanks, that one is now applied.
 
> 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 ae26770..9a9d494 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
> @@ -99,15 +99,16 @@ public void run(IAction act) {
>  		}
>  
>  		String author = null;
> +		String committer = null;
>  		if (repository != null) {
>  			final RepositoryConfig config = repository.getConfig();
>  			author = config.getAuthorName();
> -			if (author != null && author.length() != 0) {
> -				final String email = config.getAuthorEmail();
> -				if (email != null && email.length() != 0) {
> -					author = author + " <" + email + ">";
> -				}
> -			}
> +			final String authorEmail = config.getAuthorEmail();
> +			author = author + " <" + authorEmail + ">";
> +
> +			committer = config.getCommitterName();
> +			final String committerEmail = config.getAuthorEmail();
> +			committer = committer + " <" + committerEmail + ">";

Don't you mean getCommitterEmail() here?

> @@ -117,9 +118,13 @@ public void run(IAction act) {
>  		commitDialog.setAmendAllowed(amendAllowed);
>  		commitDialog.setFileList(files);
>  		commitDialog.setAuthor(author);
> +		commitDialog.setCommitter(committer);
>  
> -		if (previousCommit != null)
> +		if (previousCommit != null) {
>  			commitDialog.setPreviousCommitMessage(previousCommit.getMessage());
> +			PersonIdent previousAuthor = previousCommit.getAuthor();
> +			commitDialog.setPreviousAuthor(previousAuthor.getName() + " <" + previousAuthor.getEmailAddress() + ">");

Isn't this a bug fix for "amend" so that the original author is
reused when amending the prior commit?  Please mention it in the
commit message; and ideally this should be its own change.

> @@ -312,6 +352,7 @@ private static String getFileStatus(IFile file) {
>  			}
>  
>  		} catch (Exception e) {
> +			e.printStackTrace();

Would it be better to log this through an Activator so it shows
up in the Error Log view?

> @@ -321,6 +362,7 @@ private static String getFileStatus(IFile file) {
>  	 * @return The message the user entered
>  	 */
>  	public String getCommitMessage() {
> +		commitMessage.replaceAll(Text.DELIMITER, "\n"); //$NON-NLS-1$
>  		return commitMessage;

String is immutable.  This replaceAll is a noop.  You probably
meant to return the return value of the replaceAll method.

There may be other comments for these changes; it would be easier
to review if they were more broken up into independent commits.

-- 
Shawn.

[PATCH JGIT] Propose author and committer in the commit dialog

[Yann Simon]

Add a field 'committer'.
The fields 'author' and 'committer' are populated with the values
found in the configuration.

The author and the committer are validated against empty values
before accepting the commit.

Signed-off-by: Yann Simon <yann.simon.fr@gmail.com>
---
I reduced the number of changes for better reviewing.

 .../src/org/spearce/egit/ui/UIText.java            |    6 ++
 .../egit/ui/internal/actions/CommitAction.java     |   46 +++++++++---------
 .../egit/ui/internal/dialogs/CommitDialog.java     |   50 ++++++++++++++++++-
 .../src/org/spearce/egit/ui/uitext.properties      |    2 +
 4 files changed, 77 insertions(+), 27 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java
index d74f53e..249f2a0 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java
@@ -584,6 +584,9 @@
 	public static String CommitDialog_CommitChanges;
 
 	/** */
+	public static String CommitDialog_Committer;
+
+	/** */
 	public static String CommitDialog_CommitMessage;
 
 	/** */
@@ -596,6 +599,9 @@
 	public static String CommitDialog_ErrorInvalidAuthorSpecified;
 
 	/** */
+	public static String CommitDialog_ErrorInvalidCommitterSpecified;
+
+	/** */
 	public static String CommitDialog_ErrorMustEnterCommitMessage;
 
 	/** */
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 d30172f..97aa60f 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
@@ -99,15 +99,16 @@ public void run(IAction act) {
 		}
 
 		String author = null;
+		String committer = null;
 		if (repository != null) {
 			final RepositoryConfig config = repository.getConfig();
 			author = config.getAuthorName();
-			if (author != null && author.length() != 0) {
-				final String email = config.getAuthorEmail();
-				if (email != null && email.length() != 0) {
-					author = author + " <" + email + ">";
-				}
-			}
+			final String authorEmail = config.getAuthorEmail();
+			author = author + " <" + authorEmail + ">";
+
+			committer = config.getCommitterName();
+			final String committerEmail = config.getCommitterEmail();
+			committer = committer + " <" + committerEmail + ">";
 		}
 
 		loadPreviousCommit();
@@ -117,6 +118,7 @@ public void run(IAction act) {
 		commitDialog.setAmendAllowed(amendAllowed);
 		commitDialog.setFileList(files);
 		commitDialog.setAuthor(author);
+		commitDialog.setCommitter(committer);
 
 		if (previousCommit != null) {
 			commitDialog.setPreviousCommitMessage(previousCommit.getMessage());
@@ -181,6 +183,15 @@ private void performCommit(CommitDialog commitDialog, String commitMessage)
 
 	private String doCommits(CommitDialog commitDialog, String commitMessage,
 			HashMap<Repository, Tree> treeMap) throws IOException, TeamException {
+
+		String author = commitDialog.getAuthor();
+		String committer = commitDialog.getCommitter();
+		Date commitDate = new Date(Calendar.getInstance().getTimeInMillis());
+		TimeZone timeZone = TimeZone.getDefault();
+
+		PersonIdent authorIdent = new PersonIdent(author);
+		PersonIdent committerIdent = new PersonIdent(committer);
+
 		for (java.util.Map.Entry<Repository, Tree> entry : treeMap.entrySet()) {
 			Tree tree = entry.getValue();
 			Repository repo = tree.getRepository();
@@ -199,26 +210,13 @@ private String doCommits(CommitDialog commitDialog, String commitMessage,
 			Commit commit = new Commit(repo, parentIds);
 			commit.setTree(tree);
 			commitMessage = commitMessage.replaceAll("\r", "\n");
+			if (commitDialog.isSignedOff())
+				commitMessage += "\n\nSigned-off-by: " + committerIdent.getName() + " <"
+								+ committerIdent.getEmailAddress() + ">";
 
-			PersonIdent personIdent = new PersonIdent(repo);
-			String username = personIdent.getName();
-			String email = personIdent.getEmailAddress();
-
-			if (commitDialog.isSignedOff()) {
-				commitMessage += "\n\nSigned-off-by: " + username + " <"
-						+ email + ">";
-			}
 			commit.setMessage(commitMessage);
-
-			if (commitDialog.getAuthor() == null) {
-				commit.setAuthor(personIdent);
-			} else {
-				PersonIdent author = new PersonIdent(commitDialog.getAuthor());
-				commit.setAuthor(new PersonIdent(author, new Date(Calendar
-						.getInstance().getTimeInMillis()), TimeZone
-						.getDefault()));
-			}
-			commit.setCommitter(personIdent);
+			commit.setAuthor(new PersonIdent(authorIdent, commitDate, timeZone));
+			commit.setCommitter(new PersonIdent(committerIdent, commitDate, timeZone));
 
 			ObjectWriter writer = new ObjectWriter(repo);
 			commit.setCommitId(writer.writeCommit(commit));
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java
index b122fb8..9d062cc 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java
@@ -129,6 +129,7 @@ createButton(parent, IDialogConstants.CANCEL_ID,
 
 	Text commitText;
 	Text authorText;
+	Text committerText;
 	Button amendingButton;
 	Button signedOffButton;
 
@@ -170,6 +171,12 @@ public void keyPressed(KeyEvent arg0) {
 		if (author != null)
 			authorText.setText(author);
 
+		new Label(container, SWT.LEFT).setText(UIText.CommitDialog_Committer);
+		committerText = new Text(container, SWT.BORDER);
+		committerText.setLayoutData(GridDataFactory.fillDefaults().grab(true, false).create());
+		if (committer != null)
+			committerText.setText(committer);
+
 		amendingButton = new Button(container, SWT.CHECK);
 		if (amending) {
 			amendingButton.setSelection(amending);
@@ -336,6 +343,7 @@ public void setCommitMessage(String s) {
 
 	private String commitMessage = ""; //$NON-NLS-1$
 	private String author = null;
+	private String committer = null;
 	private String previousAuthor = null;
 	private boolean signedOff = false;
 	private boolean amending = false;
@@ -400,6 +408,7 @@ public void widgetSelected(SelectionEvent e) {
 	protected void okPressed() {
 		commitMessage = commitText.getText();
 		author = authorText.getText().trim();
+		committer = committerText.getText().trim();
 		signedOff = signedOffButton.getSelection();
 		amending = amendingButton.getSelection();
 
@@ -413,14 +422,33 @@ protected void okPressed() {
 			return;
 		}
 
+		boolean authorValid = false;
 		if (author.length() > 0) {
 			try {
 				new PersonIdent(author);
+				authorValid = true;
+			} catch (IllegalArgumentException e) {
+				authorValid = false;
+			}
+		}
+		if (!authorValid) {
+			MessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorInvalidAuthor, UIText.CommitDialog_ErrorInvalidAuthorSpecified);
+			return;
+		}
+
+		boolean committerValid = false;
+		if (committer.length() > 0) {
+			try {
+				new PersonIdent(committer);
+				committerValid = true;
 			} catch (IllegalArgumentException e) {
-				MessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorInvalidAuthor, UIText.CommitDialog_ErrorInvalidAuthorSpecified);
-				return;
+				committerValid = false;
 			}
-		} else author = null;
+		}
+		if (!committerValid) {
+			MessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorInvalidAuthor, UIText.CommitDialog_ErrorInvalidCommitterSpecified);
+			return;
+		}
 
 		if (selectedFiles.isEmpty() && !amending) {
 			MessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorNoItemsSelected, UIText.CommitDialog_ErrorNoItemsSelectedToBeCommitted);
@@ -473,6 +501,22 @@ public void setAuthor(String author) {
 	}
 
 	/**
+	 * @return The committer to set for the commit
+	 */
+	public String getCommitter() {
+		return committer;
+	}
+
+	/**
+	 * Pre-set committer for the commit
+	 *
+	 * @param committer
+	 */
+	public void setCommitter(String committer) {
+		this.committer = committer;
+	}
+
+	/**
 	 * Pre-set the previous author if amending the commit
 	 *
 	 * @param previousAuthor
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties b/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties
index 52fa4f8..142b426 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties
@@ -226,10 +226,12 @@ CommitDialog_AmendPreviousCommit=A&mend previous commit
 CommitDialog_Author=&Author:
 CommitDialog_Commit=&Commit
 CommitDialog_CommitChanges=Commit Changes
+CommitDialog_Committer=Committer:
 CommitDialog_CommitMessage=Commit &Message:
 CommitDialog_DeselectAll=&Deselect All
 CommitDialog_ErrorInvalidAuthor=Invalid author
 CommitDialog_ErrorInvalidAuthorSpecified=Invalid author specified. Please use the form:\nA U Thor <author@example.com>
+CommitDialog_ErrorInvalidCommitterSpecified=Invalid committer specified. Please use the form:\nCommi T Ter <committer@example.com>
 CommitDialog_ErrorMustEnterCommitMessage=You must enter a commit message
 CommitDialog_ErrorNoItemsSelected=No items selected
 CommitDialog_ErrorNoItemsSelectedToBeCommitted=No items are currently selected to be committed.
-- 
1.6.0.4