git: Don't use rev-list's --no-commit-header (#363)

This flag was introduced in Git 2.33,
which is newer than the default on many systems.
We don't absolutely need this flag -- just manually skip the header.

Author: abhinav

.changes/unreleased/Fixed-20240827-221331.yaml

@@ -0,0 +1,3 @@
+kind: Fixed
+body: Support use with older Git versions by dropping use of a v2.33-only flag.
+time: 2024-08-27T22:13:31.124136-07:00

internal/git/commit.go

@@ -158,8 +158,8 @@ func (r *Repository) Commit(ctx context.Context, req CommitRequest) error {
 
 // CommitSubject returns the subject of a commit.
 func (r *Repository) CommitSubject(ctx context.Context, commitish string) (string, error) {
-	out, err := r.gitCmd(ctx, "rev-list",
-		"--no-commit-header", "-n1", "--format=%s", commitish, "--",
+	out, err := r.gitCmd(ctx,
+		"show", "--no-patch", "--format=%s", commitish,
 	).OutputString(r.exec)
 	if err != nil {
 		return "", fmt.Errorf("git log: %w", err)
@@ -189,7 +189,6 @@ func (m CommitMessage) String() string {
 // That is, all commits reachable from start but not from stop.
 func (r *Repository) CommitMessageRange(ctx context.Context, start, stop string) ([]CommitMessage, error) {
 	cmd := r.gitCmd(ctx, "rev-list",
-		"--no-commit-header",
 		"--format=%B%x00", // null-byte separated
 		start, "--not", stop, "--",
 	)
@@ -211,6 +210,14 @@ func (r *Repository) CommitMessageRange(ctx context.Context, start, stop string)
 		if len(raw) == 0 {
 			continue
 		}
+
+		// --format with rev-list writes in the form:
+		//
+		//	commit <hash>\n
+		//	<format string>
+		//
+		// We need to drop the first line.
+		_, raw, _ = strings.Cut(raw, "\n")
 		subject, body, _ := strings.Cut(raw, "\n")
 		bodies = append(bodies, CommitMessage{
 			Subject: strings.TrimSpace(subject),

internal/git/integration_test.go

@@ -0,0 +1,111 @@
+package git_test
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.abhg.dev/gs/internal/git"
+	"go.abhg.dev/gs/internal/git/gittest"
+	"go.abhg.dev/gs/internal/logtest"
+	"go.abhg.dev/gs/internal/text"
+)
+
+func TestIntegrationCommitListing(t *testing.T) {
+	t.Parallel()
+
+	fixture, err := gittest.LoadFixtureScript([]byte(text.Dedent(`
+		as 'Test <[email protected]>'
+
+		at '2024-08-27T21:48:32Z'
+		git init
+		git config core.editor 'mockedit'
+
+		git add init.txt
+		git commit -m 'Initial commit'
+
+		at '2024-08-27T21:52:12Z'
+		git add feature1.txt
+		env MOCKEDIT_GIVE=$WORK/input/feature1-commit.txt
+		git commit
+
+		at '2024-08-27T22:10:11Z'
+		git add feature2.txt
+		env MOCKEDIT_GIVE=$WORK/input/feature2-commit.txt
+		git commit
+
+		-- init.txt --
+		-- feature1.txt --
+		feature 1
+		-- feature2.txt --
+		feature 2
+		-- input/feature1-commit.txt --
+		Add feature1
+
+		This is the first feature.
+		-- input/feature2-commit.txt --
+		Add feature2
+
+		This is the second feature.
+	`)))
+	require.NoError(t, err)
+
+	ctx := context.Background()
+	repo, err := git.Open(ctx, fixture.Dir(), git.OpenOptions{
+		Log: logtest.New(t),
+	})
+	require.NoError(t, err)
+
+	t.Run("ListCommitsDetails", func(t *testing.T) {
+		commits, err := repo.ListCommitsDetails(ctx, git.CommitRangeFrom("HEAD").Limit(2))
+		require.NoError(t, err)
+
+		// Normalize to UTC
+		for i := range commits {
+			commits[i].AuthorDate = commits[i].AuthorDate.UTC()
+		}
+
+		assert.Equal(t, []git.CommitDetail{
+			{
+				Hash:       "9045e63b1d4d4e6e2db3fa03d4eb98a6ed653f3a",
+				ShortHash:  "9045e63",
+				Subject:    "Add feature2",
+				AuthorDate: time.Date(2024, 8, 27, 22, 10, 11, 0, time.UTC),
+			},
+			{
+				Hash:       "acca66548dc31594dc3bc669c804e98eda1edc3d",
+				ShortHash:  "acca665",
+				Subject:    "Add feature1",
+				AuthorDate: time.Date(2024, 8, 27, 21, 52, 12, 0, time.UTC),
+			},
+		}, commits)
+	})
+
+	t.Run("CommitSubject", func(t *testing.T) {
+		subject, err := repo.CommitSubject(ctx, "HEAD")
+		require.NoError(t, err)
+		assert.Equal(t, "Add feature2", subject)
+
+		subject, err = repo.CommitSubject(ctx, "HEAD^")
+		require.NoError(t, err)
+		assert.Equal(t, "Add feature1", subject)
+	})
+
+	t.Run("CommitMessageRange", func(t *testing.T) {
+		msgs, err := repo.CommitMessageRange(ctx, "HEAD", "HEAD~2")
+		require.NoError(t, err)
+
+		assert.Equal(t, []git.CommitMessage{
+			{
+				Subject: "Add feature2",
+				Body:    "This is the second feature.",
+			},
+			{
+				Subject: "Add feature1",
+				Body:    "This is the first feature.",
+			},
+		}, msgs)
+	})
+}

internal/git/rev_list.go

@@ -94,7 +94,7 @@ func (r *Repository) listCommitsFormat(ctx context.Context, commits CommitRange,
 	args := make([]string, 0, len(commits)+3)
 	args = append(args, "rev-list")
 	if format != "" {
-		args = append(args, "--format="+format, "--no-commit-header")
+		args = append(args, "--format="+format)
 	}
 	args = append(args, []string(commits)...)
 
@@ -112,6 +112,23 @@ func (r *Repository) listCommitsFormat(ctx context.Context, commits CommitRange,
 	var lines []string
 	scanner := bufio.NewScanner(out)
 	for scanner.Scan() {
+		line := scanner.Text()
+
+		// With --format, rev-list output is in the form:
+		//
+		//    commit <hash>
+		//    <formatted message>
+		//
+		// We'll need to ignore the first line.
+		//
+		// This is a bit of a hack, but the --no-commit-header flag
+		// that suppresses this line is only available in git 2.33+.
+		if format != "" && strings.HasPrefix(line, "commit ") {
+			if !scanner.Scan() {
+				break
+			}
+		}
+
 		lines = append(lines, scanner.Text())
 	}