# [PATCH JGIT] Propose author and committer in the commit dialog 2 messages from 2009-02-06 to 2009-02-08. Participants: Yann Simon, Shawn O. Pearce. Thread: https://gitlist.dev/t/17617 ## Yann Simon, 2009-02-06 16:03 Subject: [PATCH JGIT] Propose author and committer in the commit dialog Message-ID: <498C5F4E.1040200@gmail.com> URL: https://gitlist.dev/e/498C5F4E.1040200%40gmail.com ``` 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 --- 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 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 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 +CommitDialog_ErrorInvalidCommitterSpecified=Invalid committer specified. Please use the form:\nCommi T Ter 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 ``` ## Shawn O. Pearce, 2009-02-08 20:24 Subject: Re: [PATCH JGIT] Propose author and committer in the commit dialog Message-ID: <20090208202405.GB30949@spearce.org> URL: https://gitlist.dev/e/20090208202405.GB30949%40spearce.org In-Reply-To: <498C5F4E.1040200@gmail.com> ``` Yann Simon 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. ```