feat: [CODE-2567]: improve git commit message handling (#2849)

* make the change backward compatible
* improve git commit message handling

app/api/controller/aiagent/analyse_execution.go

@@ -111,8 +111,7 @@ func (c *Controller) commit(ctx context.Context,
 	now := time.Now()
 	commit, err := c.git.CommitFiles(ctx, &git.CommitFilesParams{
 		WriteParams:   writeParams,
-		Title:         payload.Title,
-		Message:       payload.Message,
+		Message:       git.CommitMessage(payload.Title, payload.Message),
 		Branch:        payload.Branch,
 		NewBranch:     payload.NewBranch,
 		Actions:       actions,

app/api/controller/pullreq/comment_apply_suggestions.go

@@ -300,8 +300,7 @@ func (c *Controller) CommentApplySuggestions(
 	now := time.Now()
 	commitOut, err := c.git.CommitFiles(ctx, &git.CommitFilesParams{
 		WriteParams:   writeParams,
-		Title:         in.Title,
-		Message:       in.Message,
+		Message:       git.CommitMessage(in.Title, in.Message),
 		Branch:        pr.SourceBranch,
 		Committer:     controller.SystemServicePrincipalInfo(),
 		CommitterDate: &now,

app/api/controller/pullreq/merge.go

@@ -416,8 +416,7 @@ func (c *Controller) Merge(
 		BaseBranch:      pr.TargetBranch,
 		HeadRepoUID:     sourceRepo.GitUID,
 		HeadBranch:      pr.SourceBranch,
-		Title:           in.Title,
-		Message:         in.Message,
+		Message:         git.CommitMessage(in.Title, in.Message),
 		Committer:       committer,
 		CommitterDate:   &now,
 		Author:          author,

app/api/controller/repo/commit.go

@@ -18,6 +18,7 @@ import (
 	"context"
 	"encoding/base64"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/harness/gitness/app/api/controller"
@@ -61,6 +62,15 @@ type CommitFilesOptions struct {
 	BypassRules bool `json:"bypass_rules"`
 }
 
+func (in *CommitFilesOptions) Sanitize() error {
+	in.Title = strings.TrimSpace(in.Title)
+	in.Message = strings.TrimSpace(in.Message)
+
+	// TODO: Validate title and message length.
+
+	return nil
+}
+
 func (c *Controller) CommitFiles(ctx context.Context,
 	session *auth.Session,
 	repoRef string,
@@ -71,6 +81,10 @@ func (c *Controller) CommitFiles(ctx context.Context,
 		return types.CommitFilesResponse{}, nil, err
 	}
 
+	if err := in.Sanitize(); err != nil {
+		return types.CommitFilesResponse{}, nil, err
+	}
+
 	rules, isRepoOwner, err := c.fetchRules(ctx, session, repo)
 	if err != nil {
 		return types.CommitFilesResponse{}, nil, err
@@ -146,8 +160,7 @@ func (c *Controller) CommitFiles(ctx context.Context,
 	now := time.Now()
 	commit, err := c.git.CommitFiles(ctx, &git.CommitFilesParams{
 		WriteParams:   writeParams,
-		Title:         in.Title,
-		Message:       in.Message,
+		Message:       git.CommitMessage(in.Title, in.Message),
 		Branch:        in.Branch,
 		NewBranch:     in.NewBranch,
 		Actions:       actions,

app/api/controller/repo/squash.go

@@ -26,6 +26,7 @@ import (
 	"github.com/harness/gitness/app/services/protection"
 	"github.com/harness/gitness/git"
 	gitenum "github.com/harness/gitness/git/enum"
+	"github.com/harness/gitness/git/parser"
 	"github.com/harness/gitness/git/sha"
 	"github.com/harness/gitness/types"
 	"github.com/harness/gitness/types/enum"
@@ -186,17 +187,17 @@ func (c *Controller) Squash(
 		return nil, nil, usererror.BadRequest("Failed to find commits between head and base")
 	}
 
-	commit0Title, commit0Message := splitTitleAndMessage(commits.Commits[0].Message)
+	commit0Subject, commit0Body := parser.SplitMessage(commits.Commits[0].Message)
 
 	if in.Title == "" {
-		in.Title = commit0Title
+		in.Title = commit0Subject
 		if commitCount > 1 {
 			in.Title = fmt.Sprintf("Squashed %d commits", commits.TotalCommits)
 		}
 	}
 
 	if in.Message == "" {
-		in.Message = commit0Message
+		in.Message = commit0Body
 		if commitCount > 1 {
 			sb := strings.Builder{}
 			for i := range min(commitBulletLimit, len(commits.Commits)) {
@@ -221,8 +222,7 @@ func (c *Controller) Squash(
 		BaseSHA:         mergeBase.MergeBaseSHA,
 		HeadRepoUID:     repo.GitUID,
 		HeadBranch:      in.HeadBranch,
-		Title:           in.Title,
-		Message:         in.Message,
+		Message:         git.CommitMessage(in.Title, in.Message),
 		RefType:         refType,
 		RefName:         refName,
 		Committer:       committer,
@@ -259,14 +259,3 @@ func (c *Controller) Squash(
 		RuleViolations:   violations,
 	}, nil, nil
 }
-
-func splitTitleAndMessage(message string) (string, string) {
-	message = strings.TrimSpace(message)
-
-	idx := strings.Index(message, "\n")
-	if idx < 0 {
-		return message, ""
-	}
-
-	return message[:idx], strings.TrimLeft(message[idx+1:], "\n")
-}

app/services/importer/pipelines.go

@@ -75,8 +75,7 @@ func (r *Repository) processPipelines(ctx context.Context,
 
 	_, err = r.git.CommitFiles(ctx, &git.CommitFilesParams{
 		WriteParams:   writeParams,
-		Title:         commitMessage,
-		Message:       "",
+		Message:       commitMessage,
 		Branch:        repo.DefaultBranch,
 		NewBranch:     repo.DefaultBranch,
 		Actions:       actions,

git/api/commit.go

@@ -679,7 +679,6 @@ func parseLinesToSlice(output []byte) []string {
 }
 
 // getCommit returns info about a commit.
-// TODO: This function is used only for last used cache
 func getCommit(
 	ctx context.Context,
 	repoPath string,
@@ -696,7 +695,7 @@ func getCommit(
 		fmtCommitterEmail + fmtZero + // 6
 		fmtCommitterTime + fmtZero + // 7
 		fmtSubject + fmtZero + // 8
-		fmtBody // 9
+		fmtMessage // 9
 
 	cmd := command.New("log",
 		command.WithFlag("--max-count", "1"),
@@ -743,7 +742,7 @@ func getCommit(
 	committerEmail := commitData[6]
 	committerTimestamp := commitData[7]
 	subject := commitData[8]
-	body := commitData[9]
+	message := commitData[9]
 
 	authorTime, _ := time.Parse(time.RFC3339Nano, authorTimestamp)
 	committerTime, _ := time.Parse(time.RFC3339Nano, committerTimestamp)
@@ -752,7 +751,7 @@ func getCommit(
 		SHA:        commitSHA,
 		ParentSHAs: parentSHAs,
 		Title:      subject,
-		Message:    body,
+		Message:    message,
 		Author: Signature{
 			Identity: Identity{
 				Name:  authorName,

git/api/format.go

@@ -34,7 +34,7 @@ const (
 	fmtCommitterUnix  = "%ct" // Unix timestamp
 
 	fmtSubject = "%s"
-	fmtBody    = "%B"
+	fmtMessage = "%B"
 
 	fmtFieldObjectType = "%(objecttype)"
 	fmtFieldPath       = "%(path)"

git/commit.go

@@ -29,6 +29,13 @@ import (
 	"github.com/harness/gitness/git/sha"
 )
 
+func CommitMessage(subject, body string) string {
+	if body == "" {
+		return subject
+	}
+	return subject + "\n\n" + body
+}
+
 type GetCommitParams struct {
 	ReadParams
 	Revision string

git/merge.go

@@ -17,7 +17,6 @@ package git
 import (
 	"context"
 	"fmt"
-	"strings"
 	"time"
 
 	"github.com/harness/gitness/errors"
@@ -25,6 +24,7 @@ import (
 	"github.com/harness/gitness/git/enum"
 	"github.com/harness/gitness/git/hook"
 	"github.com/harness/gitness/git/merge"
+	"github.com/harness/gitness/git/parser"
 	"github.com/harness/gitness/git/sha"
 )
 
@@ -39,8 +39,9 @@ type MergeParams struct {
 	// WARNING: This field is currently not supported yet!
 	HeadRepoUID string
 	HeadBranch  string
-	Title       string
-	Message     string
+
+	Title   string // Deprecated
+	Message string
 
 	// Committer overwrites the git committer used for committing the files
 	// (optional, default: actor)
@@ -243,9 +244,12 @@ func (s *Service) Merge(ctx context.Context, params *MergeParams) (MergeOutput,
 
 	// merge message
 
-	mergeMsg := strings.TrimSpace(params.Title)
-	if len(params.Message) > 0 {
-		mergeMsg += "\n\n" + strings.TrimSpace(params.Message)
+	var message string
+	if params.Title != "" {
+		// Title is deprecated and should not be sent, but if it's sent assume we need to generate the full message.
+		message = parser.CleanUpWhitespace(CommitMessage(params.Title, params.Message))
+	} else {
+		message = parser.CleanUpWhitespace(params.Message)
 	}
 
 	// merge
@@ -268,7 +272,7 @@ func (s *Service) Merge(ctx context.Context, params *MergeParams) (MergeOutput,
 		refUpdater,
 		repoPath, s.tmpDir,
 		&author, &committer,
-		mergeMsg,
+		message,
 		mergeBaseCommitSHA, baseCommitSHA, headCommitSHA)
 	if errors.IsConflict(err) {
 		return MergeOutput{}, fmt.Errorf("failed to merge %q to %q in %q using the %q merge method: %w",

git/operations.go

@@ -23,6 +23,7 @@ import (
 	"github.com/harness/gitness/errors"
 	"github.com/harness/gitness/git/api"
 	"github.com/harness/gitness/git/hook"
+	"github.com/harness/gitness/git/parser"
 	"github.com/harness/gitness/git/sha"
 	"github.com/harness/gitness/git/sharedrepo"
 )
@@ -56,7 +57,7 @@ type CommitFileAction struct {
 // CommitFilesParams holds the data for file operations.
 type CommitFilesParams struct {
 	WriteParams
-	Title     string
+	Title     string // Deprecated
 	Message   string
 	Branch    string
 	NewBranch string
@@ -190,11 +191,6 @@ func (s *Service) CommitFiles(ctx context.Context, params *CommitFilesParams) (C
 			return errors.InvalidArgument("No effective changes.")
 		}
 
-		message := strings.TrimSpace(params.Title)
-		if len(params.Message) > 0 {
-			message += "\n\n" + strings.TrimSpace(params.Message)
-		}
-
 		authorSig := &api.Signature{
 			Identity: api.Identity{
 				Name:  author.Name,
@@ -211,6 +207,14 @@ func (s *Service) CommitFiles(ctx context.Context, params *CommitFilesParams) (C
 			When: committerDate,
 		}
 
+		var message string
+		if params.Title != "" {
+			// Title is deprecated and should not be sent, but if it's sent assume we need to generate the full message.
+			message = parser.CleanUpWhitespace(CommitMessage(params.Title, params.Message))
+		} else {
+			message = parser.CleanUpWhitespace(params.Message)
+		}
+
 		commitSHA, err := r.CommitTree(ctx, authorSig, committerSig, treeSHA, message, false, parentCommits...)
 		if err != nil {
 			return fmt.Errorf("failed to commit the tree: %w", err)

git/parser/commit_message.go

@@ -0,0 +1,145 @@
+// Copyright 2023 Harness, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package parser
+
+import (
+	"bufio"
+	"strings"
+	"unicode"
+)
+
+// CleanUpWhitespace removes extra whitespace for the multiline string passed as parameter.
+// The intended usage is to clean up commit messages.
+func CleanUpWhitespace(message string) string {
+	var messageStarted bool
+	var isLastLineEmpty bool
+
+	const eol = '\n'
+
+	builder := strings.Builder{}
+
+	scan := bufio.NewScanner(strings.NewReader(message))
+	for scan.Scan() {
+		line := strings.TrimRightFunc(scan.Text(), unicode.IsSpace)
+
+		if len(line) == 0 {
+			if messageStarted {
+				isLastLineEmpty = true
+			}
+			continue
+		}
+
+		if isLastLineEmpty {
+			builder.WriteByte(eol)
+		}
+
+		builder.WriteString(line)
+		builder.WriteByte(eol)
+		isLastLineEmpty = false
+		messageStarted = true
+	}
+
+	return builder.String()
+}
+
+// ExtractSubject extracts subject from a commit message. The result should be like output of
+// the one line commit summary, like "git log --oneline" or "git log --format=%s".
+func ExtractSubject(message string) string {
+	var messageStarted bool
+
+	builder := strings.Builder{}
+
+	scan := bufio.NewScanner(strings.NewReader(message))
+	for scan.Scan() {
+		line := strings.TrimSpace(scan.Text())
+
+		// process empty lines
+		if len(line) == 0 {
+			if messageStarted {
+				return builder.String()
+			}
+			continue
+		}
+
+		if messageStarted {
+			builder.WriteByte(' ')
+		}
+
+		builder.WriteString(line)
+		messageStarted = true
+	}
+
+	return builder.String()
+}
+
+// SplitMessage splits a commit message. Returns two strings:
+// * subject (the one line commit summary, like "git log --oneline" or "git log --format=%s),
+// * body only (like "git log --format=%b").
+func SplitMessage(message string) (string, string) {
+	var state int
+	var lastLineEmpty bool
+	const (
+		stateInit = iota
+		stateSubject
+		stateSeparator
+		stateBody
+	)
+
+	const eol = '\n'
+
+	subjectBuilder := strings.Builder{}
+	bodyBuilder := strings.Builder{}
+
+	scan := bufio.NewScanner(strings.NewReader(message))
+	for scan.Scan() {
+		line := strings.TrimRightFunc(scan.Text(), unicode.IsSpace)
+
+		// process empty lines
+		if len(line) == 0 {
+			switch state {
+			case stateInit, stateSeparator:
+				// ignore all empty lines before the first line of the subject
+			case stateSubject:
+				state = stateSeparator
+			case stateBody:
+				lastLineEmpty = true
+			}
+			continue
+		}
+
+		switch state {
+		case stateInit:
+			state = stateSubject
+			subjectBuilder.WriteString(strings.TrimLeftFunc(line, unicode.IsSpace))
+		case stateSubject:
+			subjectBuilder.WriteByte(' ')
+			subjectBuilder.WriteString(strings.TrimLeftFunc(line, unicode.IsSpace))
+		case stateSeparator:
+			state = stateBody
+			bodyBuilder.WriteString(line)
+			bodyBuilder.WriteByte(eol)
+			lastLineEmpty = false
+		case stateBody:
+			if lastLineEmpty {
+				bodyBuilder.WriteByte(eol)
+			}
+			bodyBuilder.WriteString(line)
+			bodyBuilder.WriteByte(eol)
+			lastLineEmpty = false
+		}
+	}
+
+	return subjectBuilder.String(), bodyBuilder.String()
+}

git/parser/commit_message_test.go

@@ -0,0 +1,296 @@
+// Copyright 2023 Harness, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package parser
+
+import "testing"
+
+func TestCleanUpWhitespace(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		exp   string
+	}{
+		{
+			name: "remove_trailing_spaces_in_lines",
+			input: "" +
+				"ABC   \n" +
+				"\t\t\n" +
+				"DEF\t\n",
+			exp: "" +
+				"ABC\n" +
+				"\n" +
+				"DEF\n",
+		},
+		{
+			name: "add_eof_to_the_last_line",
+			input: "" +
+				"ABC\n" +
+				"DEF",
+			exp: "" +
+				"ABC\n" +
+				"DEF\n",
+		},
+		{
+			name: "remove_consecutive_empty_lines",
+			input: "" +
+				"ABC\n" +
+				"\n" +
+				"\t\t\n" +
+				"\n" +
+				"DEF\n",
+			exp: "" +
+				"ABC\n" +
+				"\n" +
+				"DEF\n",
+		},
+		{
+			name: "remove_empty_lines_from_the_message_bottom",
+			input: "" +
+				"ABC\n" +
+				"\n" +
+				"DEF\n" +
+				"\n" +
+				"\n" +
+				"\n",
+			exp: "" +
+				"ABC\n" +
+				"\n" +
+				"DEF\n",
+		},
+		{
+			name: "remove_empty_lines_from_the_message_top",
+			input: "" +
+				"\n" +
+				"\n" +
+				"ABC\n" +
+				"\n" +
+				"DEF\n" +
+				"\n",
+			exp: "" +
+				"ABC\n" +
+				"\n" +
+				"DEF\n",
+		},
+		{
+			name: "multi_line_body",
+			input: "" +
+				"ABC\n" +
+				"DEF\n" +
+				"\n" +
+				"GHI\n" +
+				"JKL\n" +
+				"\n" +
+				"NMO\n",
+			exp: "" +
+				"ABC\n" +
+				"DEF\n" +
+				"\n" +
+				"GHI\n" +
+				"JKL\n" +
+				"\n" +
+				"NMO\n",
+		},
+		{
+			name: "complex",
+			input: "" +
+				"\n" +
+				"subj one\n" +
+				"   subj two\n" +
+				"\t\t\n" +
+				"  \n" +
+				"  body one\n" +
+				"body two\n" +
+				" \t \n" +
+				"   body three\n" +
+				" \n" +
+				"    ",
+			exp: "" +
+				"subj one\n" +
+				"   subj two\n" +
+				"\n" +
+				"  body one\n" +
+				"body two\n" +
+				"\n" +
+				"   body three\n",
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			cleaned := CleanUpWhitespace(test.input)
+			if want, got := test.exp, cleaned; want != got {
+				t.Errorf("want=%q, got=%q", want, got)
+			}
+		})
+	}
+}
+
+func TestExtractSubject(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		exp   string
+	}{
+		{
+			name: "join_lines",
+			input: "" +
+				"ABC\n" +
+				"DEF\n",
+			exp: "ABC DEF",
+		},
+		{
+			name: "stop_after_empty",
+			input: "" +
+				"ABC\n" +
+				"DEF\n" +
+				"\n" +
+				"GHI\n",
+			exp: "ABC DEF",
+		},
+		{
+			name: "ignore_extra_whitespace",
+			input: "" +
+				"\t\n" +
+				" ABC  \n" +
+				"\tDEF   \n" +
+				"\t\t\n" +
+				"GHI",
+			exp: "ABC DEF",
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			subject := ExtractSubject(test.input)
+			if want, got := test.exp, subject; want != got {
+				t.Errorf("want=%q, got=%q", want, got)
+			}
+		})
+	}
+}
+
+func TestSplitMessage(t *testing.T) {
+	tests := []struct {
+		name       string
+		input      string
+		expSubject string
+		expBody    string
+	}{
+		{
+			name: "remove_trailing_spaces_in_lines",
+			input: "" +
+				"ABC   \n" +
+				"\t\t\n" +
+				"DEF\n",
+			expSubject: "ABC",
+			expBody:    "DEF\n",
+		},
+		{
+			name: "add_eof_to_the_last_line",
+			input: "" +
+				"ABC\n" +
+				"DEF",
+			expSubject: "ABC DEF",
+			expBody:    "",
+		},
+		{
+			name: "add_eof_to_the_last_line_of_body",
+			input: "" +
+				"ABC\n" +
+				"DEF\n" +
+				"\n" +
+				"GHI",
+			expSubject: "ABC DEF",
+			expBody:    "GHI\n",
+		},
+		{
+			name: "remove_consecutive_empty_lines",
+			input: "" +
+				"ABC\n" +
+				"\n" +
+				"\t\t\n" +
+				"\n" +
+				"DEF\n",
+			expSubject: "ABC",
+			expBody:    "DEF\n",
+		},
+		{
+			name: "multi_line_body",
+			input: "" +
+				"ABC\n" +
+				"\n" +
+				"DEF\n" +
+				"GHI\n",
+			expSubject: "ABC",
+			expBody:    "DEF\nGHI\n",
+		},
+		{
+			name: "remove_empty_lines_from_the_message_bottom",
+			input: "" +
+				"ABC\n" +
+				"\n" +
+				"DEF\n" +
+				"\n" +
+				"\n" +
+				"\n",
+			expSubject: "ABC",
+			expBody:    "DEF\n",
+		},
+		{
+			name: "remove_empty_lines_from_the_message_top",
+			input: "" +
+				"\n" +
+				"\n" +
+				"ABC\n" +
+				"\n" +
+				"DEF\n" +
+				"\n",
+			expSubject: "ABC",
+			expBody:    "DEF\n",
+		},
+		{
+			name: "complex",
+			input: "" +
+				"\n" +
+				"subj one\n" +
+				"   subj two\n" +
+				"\t\t\n" +
+				"  \n" +
+				"  body one\n" +
+				"body two\n" +
+				" \t \n" +
+				"   body three\n" +
+				" \n" +
+				"    ",
+			expSubject: "subj one subj two",
+			expBody: "" +
+				"  body one\n" +
+				"body two\n" +
+				"\n" +
+				"   body three\n",
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			subject, body := SplitMessage(test.input)
+
+			if want, got := test.expSubject, subject; want != got {
+				t.Errorf("subject: want=%q, got=%q", want, got)
+			}
+
+			if want, got := test.expBody, body; want != got {
+				t.Errorf("body: want=%q, got=%q", want, got)
+			}
+		})
+	}
+}