git: Fix ordering of `--end-of-options`

Git commands support two kinds of separators to disambiguate command
line options: `--end-of-options` tells it that from here on, option
parsing has concluded and that all remaining options are to be
considered normal positional command line parameters. And `--` typically
separates revisions from paths for positional arguments, like for
example in git-log(1). A command using both would thus e.g. like the
following:

    $ git log --follow --end-of-options HEAD -- Makefile

As it turns out, our Git DSL puts both types of separators in the same
place though, ending up with the following equivalent instead:

    $ git log --follow HEAD --end-of-options -- Makefile

This command does work the same in this case, but it is not how it's
intended to work given that we may now potentially interpret "HEAD" as
an option. While we do validate that no positional arguments start with
leading dashes, this is rather about safety in depth.

Fix this bug and move `--end-of-options` immediately after all flags and
before the positional arguments.

Note that this change requires us to label git-rev-list(1) as a command
which doesn't support `--end-of-options`: we use `--all` and `--not`,
whose order interacts with other positional arguments. But if the
separator is specified, then the command will not even accept pseudo
revisions as parameters anymore. We're still safe against injection
though given that we have custom logic in place to verify the command's
positional arguments.

2 files changed, 8 insertions, 6 deletions

diff --git a/internal/git/command_description.go b/internal/git/command_description.go
index 79e500aa8..7f9a23b0b 100644
--- a/internal/git/command_description.go
+++ b/internal/git/command_description.go
@@ -187,7 +187,9 @@ var commandDescriptions = map[string]commandDescription{
 		},
 	},
 	"rev-list": {
-		flags: scNoRefUpdates,
+		// We cannot use --end-of-options here because pseudo revisions like `--all`
+		// and `--not` count as options.
+		flags: scNoRefUpdates | scNoEndOfOptions,
 		validatePositionalArgs: func(args []string) error {
 			for _, arg := range args {
 				// git-rev-list(1) supports pseudo-revision arguments which can be
@@ -306,6 +308,10 @@ func (c commandDescription) args(flags []Option, args []string, postSepArgs []st
 		commandArgs = append(commandArgs, args...)
 	}
 
+	if c.supportsEndOfOptions() {
+		commandArgs = append(commandArgs, "--end-of-options")
+	}
+
 	if c.validatePositionalArgs != nil {
 		if err := c.validatePositionalArgs(args); err != nil {
 			return nil, err
@@ -319,10 +325,6 @@ func (c commandDescription) args(flags []Option, args []string, postSepArgs []st
 	}
 	commandArgs = append(commandArgs, args...)
 
-	if c.supportsEndOfOptions() {
-		commandArgs = append(commandArgs, "--end-of-options")
-	}
-
 	if len(postSepArgs) > 0 {
 		commandArgs = append(commandArgs, "--")
 	}
diff --git a/internal/git/localrepo/remote_test.go b/internal/git/localrepo/remote_test.go
index 158f3ea49..aa1d30e35 100644
--- a/internal/git/localrepo/remote_test.go
+++ b/internal/git/localrepo/remote_test.go
@@ -367,7 +367,7 @@ func TestRepo_FetchRemote(t *testing.T) {
 
 		var stderr bytes.Buffer
 		require.NoError(t, repo.FetchRemote(ctx, "source", FetchOpts{Stderr: &stderr, Env: []string{"GIT_TRACE=1"}}))
-		require.Contains(t, stderr.String(), "trace: built-in: git fetch --quiet source --end-of-options")
+		require.Contains(t, stderr.String(), "trace: built-in: git fetch --quiet --end-of-options source")
 	})
 
 	t.Run("with globals", func(t *testing.T) {