{"thread":{"id":"17617","subject":"[PATCH JGIT] Propose author and committer in the commit dialog","startedAt":"2009-02-06T16:03:26Z","lastAt":"2009-02-08T20:24:05Z","messageCount":2,"participants":["Yann Simon","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"103518","messageId":"498C5F4E.1040200@gmail.com","threadId":"17617","inReplyTo":null,"subject":"[PATCH JGIT] Propose author and committer in the commit dialog","fromName":"Yann Simon","fromEmail":"yann.simon.fr@gmail.com","sentAt":"2009-02-06T16:03:26Z","receivedAt":"2009-02-06T16:03:26Z","isPatch":true,"sender":{"key":"yann.simon.fr@gmail.com","avatar":"https://gravatar.com/avatar/2d926895d27ac988c5c8e591887e5a6a4c7036390403c74fce92519119b887a0?d=mp&s=160"},"body":"Add a field 'committer'.\nThe fields 'author' and 'committer' are populated with the values\nfound in the configuration.\n\nValidate the author and the committer.\n\nAdd the signed-off line in the comment text box when the user clicks\non the signed-off checkbox.\n\nUse Text.DELIMITER as line break for plateform independance.\n\nSigned-off-by: Yann Simon <yann.simon.fr@gmail.com>\n---\nI remove the modifications of PersonIdent and send them in another patch.\n\n .../src/org/spearce/egit/ui/UIText.java            |    6 +\n .../egit/ui/internal/actions/CommitAction.java     |   55 +++++------\n .../egit/ui/internal/dialogs/CommitDialog.java     |   97 +++++++++++++++++++-\n .../src/org/spearce/egit/ui/uitext.properties      |    2 +\n 4 files changed, 126 insertions(+), 34 deletions(-)\n\ndiff --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\nindex d74f53e..249f2a0 100644\n--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java\n+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java\n@@ -584,6 +584,9 @@\n \tpublic static String CommitDialog_CommitChanges;\n \n \t/** */\n+\tpublic static String CommitDialog_Committer;\n+\n+\t/** */\n \tpublic static String CommitDialog_CommitMessage;\n \n \t/** */\n@@ -596,6 +599,9 @@\n \tpublic static String CommitDialog_ErrorInvalidAuthorSpecified;\n \n \t/** */\n+\tpublic static String CommitDialog_ErrorInvalidCommitterSpecified;\n+\n+\t/** */\n \tpublic static String CommitDialog_ErrorMustEnterCommitMessage;\n \n \t/** */\ndiff --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\nindex ae26770..9a9d494 100644\n--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java\n+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java\n@@ -99,15 +99,16 @@ public void run(IAction act) {\n \t\t}\n \n \t\tString author = null;\n+\t\tString committer = null;\n \t\tif (repository != null) {\n \t\t\tfinal RepositoryConfig config = repository.getConfig();\n \t\t\tauthor = config.getAuthorName();\n-\t\t\tif (author != null && author.length() != 0) {\n-\t\t\t\tfinal String email = config.getAuthorEmail();\n-\t\t\t\tif (email != null && email.length() != 0) {\n-\t\t\t\t\tauthor = author + \" <\" + email + \">\";\n-\t\t\t\t}\n-\t\t\t}\n+\t\t\tfinal String authorEmail = config.getAuthorEmail();\n+\t\t\tauthor = author + \" <\" + authorEmail + \">\";\n+\n+\t\t\tcommitter = config.getCommitterName();\n+\t\t\tfinal String committerEmail = config.getAuthorEmail();\n+\t\t\tcommitter = committer + \" <\" + committerEmail + \">\";\n \t\t}\n \n \t\tloadPreviousCommit();\n@@ -117,9 +118,13 @@ public void run(IAction act) {\n \t\tcommitDialog.setAmendAllowed(amendAllowed);\n \t\tcommitDialog.setFileList(files);\n \t\tcommitDialog.setAuthor(author);\n+\t\tcommitDialog.setCommitter(committer);\n \n-\t\tif (previousCommit != null)\n+\t\tif (previousCommit != null) {\n \t\t\tcommitDialog.setPreviousCommitMessage(previousCommit.getMessage());\n+\t\t\tPersonIdent previousAuthor = previousCommit.getAuthor();\n+\t\t\tcommitDialog.setPreviousAuthor(previousAuthor.getName() + \" <\" + previousAuthor.getEmailAddress() + \">\");\n+\t\t}\n \n \t\tif (commitDialog.open() != IDialogConstants.OK_ID)\n \t\t\treturn;\n@@ -167,7 +172,7 @@ private void performCommit(CommitDialog commitDialog, String commitMessage)\n \t\t}\n \n \t\ttry {\n-\t\t\tcommitMessage = doCommits(commitDialog, commitMessage, treeMap);\n+\t\t\tdoCommits(commitDialog, commitMessage, treeMap);\n \t\t} catch (IOException e) {\n \t\t\tthrow new TeamException(\"Committing changes\", e);\n \t\t}\n@@ -176,8 +181,17 @@ private void performCommit(CommitDialog commitDialog, String commitMessage)\n \t\t}\n \t}\n \n-\tprivate String doCommits(CommitDialog commitDialog, String commitMessage,\n+\tprivate void doCommits(CommitDialog commitDialog, String commitMessage,\n \t\t\tHashMap<Repository, Tree> treeMap) throws IOException, TeamException {\n+\n+\t\tString author = commitDialog.getAuthor();\n+\t\tString committer = commitDialog.getCommitter();\n+\t\tDate commitDate = new Date(Calendar.getInstance().getTimeInMillis());\n+\t\tTimeZone timeZone = TimeZone.getDefault();\n+\n+\t\tPersonIdent authorIdent = new PersonIdent(author);\n+\t\tPersonIdent committerIdent = new PersonIdent(committer);\n+\n \t\tfor (java.util.Map.Entry<Repository, Tree> entry : treeMap.entrySet()) {\n \t\t\tTree tree = entry.getValue();\n \t\t\tRepository repo = tree.getRepository();\n@@ -195,27 +209,9 @@ private String doCommits(CommitDialog commitDialog, String commitMessage,\n \t\t\t}\n \t\t\tCommit commit = new Commit(repo, parentIds);\n \t\t\tcommit.setTree(tree);\n-\t\t\tcommitMessage = commitMessage.replaceAll(\"\\r\", \"\\n\");\n-\n-\t\t\tPersonIdent personIdent = new PersonIdent(repo);\n-\t\t\tString username = personIdent.getName();\n-\t\t\tString email = personIdent.getEmailAddress();\n-\n-\t\t\tif (commitDialog.isSignedOff()) {\n-\t\t\t\tcommitMessage += \"\\n\\nSigned-off-by: \" + username + \" <\"\n-\t\t\t\t\t\t+ email + \">\";\n-\t\t\t}\n \t\t\tcommit.setMessage(commitMessage);\n-\n-\t\t\tif (commitDialog.getAuthor() == null) {\n-\t\t\t\tcommit.setAuthor(personIdent);\n-\t\t\t} else {\n-\t\t\t\tPersonIdent author = new PersonIdent(commitDialog.getAuthor());\n-\t\t\t\tcommit.setAuthor(new PersonIdent(author, new Date(Calendar\n-\t\t\t\t\t\t.getInstance().getTimeInMillis()), TimeZone\n-\t\t\t\t\t\t.getDefault()));\n-\t\t\t}\n-\t\t\tcommit.setCommitter(personIdent);\n+\t\t\tcommit.setAuthor(new PersonIdent(authorIdent, commitDate, timeZone));\n+\t\t\tcommit.setCommitter(new PersonIdent(committerIdent, commitDate, timeZone));\n \n \t\t\tObjectWriter writer = new ObjectWriter(repo);\n \t\t\tcommit.setCommitId(writer.writeCommit(commit));\n@@ -228,7 +224,6 @@ private String doCommits(CommitDialog commitDialog, String commitMessage,\n \t\t\t\t\t\t+ \" to commit \" + commit.getCommitId() + \".\");\n \t\t\t}\n \t\t}\n-\t\treturn commitMessage;\n \t}\n \n \tprivate void prepareTrees(IFile[] selectedItems,\ndiff --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\nindex fede948..6b6cb55 100644\n--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java\n+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/dialogs/CommitDialog.java\n@@ -17,6 +17,7 @@\n import java.util.Collections;\n import java.util.Comparator;\n import java.util.Iterator;\n+import java.util.regex.Pattern;\n \n import org.eclipse.core.resources.IFile;\n import org.eclipse.core.resources.IProject;\n@@ -67,6 +68,8 @@\n  */\n public class CommitDialog extends Dialog {\n \n+\tprivate static Pattern signedOffPattern = Pattern.compile(\"(.|\\r|\\n)*Signed-off-by: .*(\\r|\\n)*\"); //$NON-NLS-1$\n+\n \tclass CommitContentProvider implements IStructuredContentProvider {\n \n \t\tpublic void inputChanged(Viewer viewer, Object oldInput, Object newInput) {\n@@ -129,6 +132,7 @@ createButton(parent, IDialogConstants.CANCEL_ID,\n \n \tText commitText;\n \tText authorText;\n+\tText committerText;\n \tButton amendingButton;\n \tButton signedOffButton;\n \n@@ -170,11 +174,18 @@ public void keyPressed(KeyEvent arg0) {\n \t\tif (author != null)\n \t\t\tauthorText.setText(author);\n \n+\t\tnew Label(container, SWT.LEFT).setText(UIText.CommitDialog_Committer);\n+\t\tcommitterText = new Text(container, SWT.BORDER);\n+\t\tcommitterText.setLayoutData(GridDataFactory.fillDefaults().grab(true, false).create());\n+\t\tif (committer != null)\n+\t\t\tcommitterText.setText(committer);\n+\n \t\tamendingButton = new Button(container, SWT.CHECK);\n \t\tif (amending) {\n \t\t\tamendingButton.setSelection(amending);\n \t\t\tamendingButton.setEnabled(false); // if already set, don't allow any changes\n \t\t\tcommitText.setText(previousCommitMessage);\n+\t\t\tauthorText.setText(previousAuthor);\n \t\t} else if (!amendAllowed) {\n \t\t\tamendingButton.setEnabled(false);\n \t\t}\n@@ -187,8 +198,9 @@ public void widgetSelected(SelectionEvent arg0) {\n \t\t\t\t\talreadyAdded = true;\n \t\t\t\t\tString curText = commitText.getText();\n \t\t\t\t\tif (curText.length() > 0)\n-\t\t\t\t\t\tcurText += \"\\n\"; //$NON-NLS-1$\n+\t\t\t\t\t\tcurText += Text.DELIMITER;\n \t\t\t\t\tcommitText.setText(curText + previousCommitMessage);\n+\t\t\t\t\tauthorText.setText(previousAuthor);\n \t\t\t\t}\n \t\t\t}\n \n@@ -205,6 +217,33 @@ public void widgetDefaultSelected(SelectionEvent arg0) {\n \t\tsignedOffButton.setText(UIText.CommitDialog_AddSOB);\n \t\tsignedOffButton.setLayoutData(GridDataFactory.fillDefaults().grab(true, false).span(2, 1).create());\n \n+\t\tsignedOffButton.addSelectionListener(new SelectionListener() {\n+\t\t\tboolean alreadySigned = false;\n+\t\t\tpublic void widgetSelected(SelectionEvent arg0) {\n+\t\t\t\tif (alreadySigned)\n+\t\t\t\t\treturn;\n+\t\t\t\tif (signedOffButton.getSelection()) {\n+\t\t\t\t\talreadySigned = true;\n+\t\t\t\t\tString curText = commitText.getText();\n+\n+\t\t\t\t\t// add new lines if necessary\n+\t\t\t\t\tif (!curText.endsWith(Text.DELIMITER)) {\n+\t\t\t\t\t\tcurText += Text.DELIMITER;\n+\t\t\t\t\t}\n+\n+\t\t\t\t\tif (!signedOffPattern.matcher(new StringBuilder(curText)).matches()) {\n+\t\t\t\t\t\t// if the last line is not a signed off (amend a commit), add a line break\n+\t\t\t\t\t\tcurText += Text.DELIMITER;\n+\t\t\t\t\t}\n+\t\t\t\t\tcommitText.setText(curText + \"Signed-off-by: \" + committerText.getText()); //$NON-NLS-1$\n+\t\t\t\t}\n+\t\t\t}\n+\n+\t\t\tpublic void widgetDefaultSelected(SelectionEvent arg0) {\n+\t\t\t\t// Empty\n+\t\t\t}\n+\t\t});\n+\n \t\tTable resourcesTable = new Table(container, SWT.H_SCROLL | SWT.V_SCROLL\n \t\t\t\t| SWT.FULL_SELECTION | SWT.MULTI | SWT.CHECK | SWT.BORDER);\n \t\tresourcesTable.setLayoutData(GridDataFactory.fillDefaults().hint(600,\n@@ -237,6 +276,7 @@ private Menu getContextMenu() {\n \t\tMenuItem item = new MenuItem(menu, SWT.PUSH);\n \t\titem.setText(UIText.CommitDialog_AddFileOnDiskToIndex);\n \t\titem.addListener(SWT.Selection, new Listener() {\n+\t\t\t@SuppressWarnings(\"unchecked\")\n \t\t\tpublic void handleEvent(Event arg0) {\n \t\t\t\tIStructuredSelection sel = (IStructuredSelection) filesViewer.getSelection();\n \t\t\t\tif (sel.isEmpty()) {\n@@ -312,6 +352,7 @@ private static String getFileStatus(IFile file) {\n \t\t\t}\n \n \t\t} catch (Exception e) {\n+\t\t\te.printStackTrace();\n \t\t}\n \n \t\treturn prefix;\n@@ -321,6 +362,7 @@ private static String getFileStatus(IFile file) {\n \t * @return The message the user entered\n \t */\n \tpublic String getCommitMessage() {\n+\t\tcommitMessage.replaceAll(Text.DELIMITER, \"\\n\"); //$NON-NLS-1$\n \t\treturn commitMessage;\n \t}\n \n@@ -334,6 +376,8 @@ public void setCommitMessage(String s) {\n \n \tprivate String commitMessage = \"\"; //$NON-NLS-1$\n \tprivate String author = null;\n+\tprivate String committer = null;\n+\tprivate String previousAuthor = null;\n \tprivate boolean signedOff = false;\n \tprivate boolean amending = false;\n \tprivate boolean amendAllowed = true;\n@@ -397,6 +441,7 @@ public void widgetSelected(SelectionEvent e) {\n \tprotected void okPressed() {\n \t\tcommitMessage = commitText.getText();\n \t\tauthor = authorText.getText().trim();\n+\t\tcommitter = committerText.getText().trim();\n \t\tsignedOff = signedOffButton.getSelection();\n \t\tamending = amendingButton.getSelection();\n \n@@ -410,14 +455,33 @@ protected void okPressed() {\n \t\t\treturn;\n \t\t}\n \n+\t\tboolean authorValid = false;\n \t\tif (author.length() > 0) {\n \t\t\ttry {\n \t\t\t\tnew PersonIdent(author);\n+\t\t\t\tauthorValid = true;\n \t\t\t} catch (IllegalArgumentException e) {\n-\t\t\t\tMessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorInvalidAuthor, UIText.CommitDialog_ErrorInvalidAuthorSpecified);\n-\t\t\t\treturn;\n+\t\t\t\tauthorValid = false;\n \t\t\t}\n-\t\t} else author = null;\n+\t\t}\n+\t\tif (!authorValid) {\n+\t\t\tMessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorInvalidAuthor, UIText.CommitDialog_ErrorInvalidAuthorSpecified);\n+\t\t\treturn;\n+\t\t}\n+\n+\t\tboolean committerValid = false;\n+\t\tif (committer.length() > 0) {\n+\t\t\ttry {\n+\t\t\t\tnew PersonIdent(committer);\n+\t\t\t\tcommitterValid = true;\n+\t\t\t} catch (IllegalArgumentException e) {\n+\t\t\t\tcommitterValid = false;\n+\t\t\t}\n+\t\t}\n+\t\tif (!committerValid) {\n+\t\t\tMessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorInvalidAuthor, UIText.CommitDialog_ErrorInvalidCommitterSpecified);\n+\t\t\treturn;\n+\t\t}\n \n \t\tif (selectedFiles.isEmpty() && !amending) {\n \t\t\tMessageDialog.openWarning(getShell(), UIText.CommitDialog_ErrorNoItemsSelected, UIText.CommitDialog_ErrorNoItemsSelectedToBeCommitted);\n@@ -470,6 +534,31 @@ public void setAuthor(String author) {\n \t}\n \n \t/**\n+\t * @return The committer to set for the commit\n+\t */\n+\tpublic String getCommitter() {\n+\t\treturn committer;\n+\t}\n+\n+\t/**\n+\t * Pre-set committer for the commit\n+\t *\n+\t * @param committer\n+\t */\n+\tpublic void setCommitter(String committer) {\n+\t\tthis.committer = committer;\n+\t}\n+\n+\t/**\n+\t * Pre-set the previous author if amending the commit\n+\t *\n+\t * @param previousAuthor\n+\t */\n+\tpublic void setPreviousAuthor(String previousAuthor) {\n+\t\tthis.previousAuthor = previousAuthor;\n+\t}\n+\n+\t/**\n \t * @return whether to auto-add a signed-off line to the message\n \t */\n \tpublic boolean isSignedOff() {\ndiff --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\nindex 52fa4f8..142b426 100644\n--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties\n+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties\n@@ -226,10 +226,12 @@ CommitDialog_AmendPreviousCommit=A&mend previous commit\n CommitDialog_Author=&Author:\n CommitDialog_Commit=&Commit\n CommitDialog_CommitChanges=Commit Changes\n+CommitDialog_Committer=Committer:\n CommitDialog_CommitMessage=Commit &Message:\n CommitDialog_DeselectAll=&Deselect All\n CommitDialog_ErrorInvalidAuthor=Invalid author\n CommitDialog_ErrorInvalidAuthorSpecified=Invalid author specified. Please use the form:\\nA U Thor <author@example.com>\n+CommitDialog_ErrorInvalidCommitterSpecified=Invalid committer specified. Please use the form:\\nCommi T Ter <committer@example.com>\n CommitDialog_ErrorMustEnterCommitMessage=You must enter a commit message\n CommitDialog_ErrorNoItemsSelected=No items selected\n CommitDialog_ErrorNoItemsSelectedToBeCommitted=No items are currently selected to be committed.\n-- \n1.6.0.4\n"},{"id":"103779","messageId":"20090208202405.GB30949@spearce.org","threadId":"17617","inReplyTo":"498C5F4E.1040200@gmail.com","subject":"Re: [PATCH JGIT] Propose author and committer in the commit dialog","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-02-08T20:24:05Z","receivedAt":"2009-02-08T20:24:05Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Yann Simon <yann.simon.fr@gmail.com> wrote:\n> Add a field 'committer'.\n> The fields 'author' and 'committer' are populated with the values\n> found in the configuration.\n> \n> Validate the author and the committer.\n> \n> Add the signed-off line in the comment text box when the user clicks\n> on the signed-off checkbox.\n> \n> Use Text.DELIMITER as line break for plateform independance.\n\nFar too many things in one change.  Please break these apart into\nmultiple commits so they are easier to review and test in isolation.\n \n> I remove the modifications of PersonIdent and send them in another patch.\n\nThanks, that one is now applied.\n \n> 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\n> index ae26770..9a9d494 100644\n> --- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java\n> +++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/actions/CommitAction.java\n> @@ -99,15 +99,16 @@ public void run(IAction act) {\n>  \t\t}\n>  \n>  \t\tString author = null;\n> +\t\tString committer = null;\n>  \t\tif (repository != null) {\n>  \t\t\tfinal RepositoryConfig config = repository.getConfig();\n>  \t\t\tauthor = config.getAuthorName();\n> -\t\t\tif (author != null && author.length() != 0) {\n> -\t\t\t\tfinal String email = config.getAuthorEmail();\n> -\t\t\t\tif (email != null && email.length() != 0) {\n> -\t\t\t\t\tauthor = author + \" <\" + email + \">\";\n> -\t\t\t\t}\n> -\t\t\t}\n> +\t\t\tfinal String authorEmail = config.getAuthorEmail();\n> +\t\t\tauthor = author + \" <\" + authorEmail + \">\";\n> +\n> +\t\t\tcommitter = config.getCommitterName();\n> +\t\t\tfinal String committerEmail = config.getAuthorEmail();\n> +\t\t\tcommitter = committer + \" <\" + committerEmail + \">\";\n\nDon't you mean getCommitterEmail() here?\n\n> @@ -117,9 +118,13 @@ public void run(IAction act) {\n>  \t\tcommitDialog.setAmendAllowed(amendAllowed);\n>  \t\tcommitDialog.setFileList(files);\n>  \t\tcommitDialog.setAuthor(author);\n> +\t\tcommitDialog.setCommitter(committer);\n>  \n> -\t\tif (previousCommit != null)\n> +\t\tif (previousCommit != null) {\n>  \t\t\tcommitDialog.setPreviousCommitMessage(previousCommit.getMessage());\n> +\t\t\tPersonIdent previousAuthor = previousCommit.getAuthor();\n> +\t\t\tcommitDialog.setPreviousAuthor(previousAuthor.getName() + \" <\" + previousAuthor.getEmailAddress() + \">\");\n\nIsn't this a bug fix for \"amend\" so that the original author is\nreused when amending the prior commit?  Please mention it in the\ncommit message; and ideally this should be its own change.\n\n> @@ -312,6 +352,7 @@ private static String getFileStatus(IFile file) {\n>  \t\t\t}\n>  \n>  \t\t} catch (Exception e) {\n> +\t\t\te.printStackTrace();\n\nWould it be better to log this through an Activator so it shows\nup in the Error Log view?\n\n> @@ -321,6 +362,7 @@ private static String getFileStatus(IFile file) {\n>  \t * @return The message the user entered\n>  \t */\n>  \tpublic String getCommitMessage() {\n> +\t\tcommitMessage.replaceAll(Text.DELIMITER, \"\\n\"); //$NON-NLS-1$\n>  \t\treturn commitMessage;\n\nString is immutable.  This replaceAll is a noop.  You probably\nmeant to return the return value of the replaceAll method.\n\nThere may be other comments for these changes; it would be easier\nto review if they were more broken up into independent commits.\n\n-- \nShawn.\n"}]}