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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Feb 8, 2009, 20:24 UTC
Message-ID: <20090208202405.GB30949@spearce.org>
In-Reply-To: <498C5F4E.1040200@gmail.com>

Yann Simon <yann.simon.fr@gmail.com> wrote:

Show 10 quoted lines

> 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.

Show 24 quoted lines

> 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?

Show 11 quoted lines

> @@ -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.

Show 5 quoted lines

> @@ -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?

Show 6 quoted lines

> @@ -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.