From 361858370ebd8da014f0ec4e3a713b1b4eb1a9a3 Mon Sep 17 00:00:00 2001
From: Craig de Stigter <craig@destigter.nz>
Date: Fri, 19 Nov 2021 08:55:24 +1300
Subject: [PATCH] log: Fix limit options not working

In https://github.com/koordinates/kart/pull/498 I added some code to
pre-parse arguments that we're going to pass to `git log`, to figure out
which of the arguments were paths.

This was necessary for future work
(https://github.com/koordinates/kart/issues/496). Unfortunately I failed
to account for git's options that take an argument, notably `-n 1`, and
the `1` got treated as a path.

This change fixes the bug for:
* `-n / --max-limit`
* `--after / --since / --until / --before`
* `--skip`

And incidentally adds those options to the `--help` output.
---
 kart/log.py       | 59 ++++++++++++++++++++++++++++++++++++++++++++---
 tests/test_log.py | 39 +++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 3 deletions(-)

diff --git a/kart/log.py b/kart/log.py
index 329a5d9ff..bec49910c 100644
--- a/kart/log.py
+++ b/kart/log.py
@@ -93,7 +93,7 @@ def parse_extra_args(repo, args):
                 break
             else:
                 other_args.append(arg)
-    return other_args, paths
+    return list(other_args), list(paths)
 
 
 @click.command(
@@ -132,13 +132,66 @@ def parse_extra_args(repo, args):
         "Otherwise, the feature count will be approximated with varying levels of accuracy."
     ),
 )
+# Some standard git options
+@click.option(
+    "-n",
+    "--max-count",
+    type=int,
+    nargs=1,
+    help="Limit the number of commits to output.",
+)
+@click.option(
+    "--skip",
+    type=int,
+    nargs=1,
+    metavar="INTEGER",
+    help="Skip INTEGER commits before starting to show the commit output.",
+)
+@click.option(
+    "--since",
+    "--after",
+    type=click.STRING,
+    nargs=1,
+    metavar="DATE",
+    help="Show commits more recent than a specific date.",
+)
+@click.option(
+    "--until",
+    "--before",
+    type=click.STRING,
+    nargs=1,
+    metavar="DATE",
+    help="Show commits older than a specific date.",
+)
 @click.argument("args", nargs=-1, type=click.UNPROCESSED)
-def log(ctx, output_format, json_style, do_dataset_changes, with_feature_count, args):
-    """ Show commit logs """
+def log(
+    ctx,
+    output_format,
+    json_style,
+    do_dataset_changes,
+    with_feature_count,
+    max_count,
+    skip,
+    since,
+    until,
+    args,
+):
+    """
+    Show commit logs
+    """
     repo = ctx.obj.get_repo(allowed_states=KartRepoState.ALL_STATES)
 
     other_args, paths = parse_extra_args(repo, args)
 
+    if max_count is not None:
+        other_args.append(f"--max-count={max_count}")
+    if skip is not None:
+        other_args.append(f"--skip={skip}")
+    if since is not None:
+        other_args.append(f"--since={since}")
+    if until is not None:
+        other_args.append(f"--until={until}")
+
     # TODO: should we check paths exist here? git doesn't!
     if output_format == "text":
         git_args = ["git", "-C", repo.path, "log", *other_args, "--", *paths]
diff --git a/tests/test_log.py b/tests/test_log.py
index fda35a2a4..41d0e1669 100644
--- a/tests/test_log.py
+++ b/tests/test_log.py
@@ -267,3 +267,42 @@ def num_commits(r):
             )
             assert r.exit_code == 0, r.stderr
             assert num_commits(r) == 2
+
+
+@pytest.mark.parametrize(
+    "args,expected_commits",
+    [
+        pytest.param(["-n", "0"], [], id="-n 0"),
+        pytest.param(["-n", "1"], [H.POINTS.HEAD_SHA], id="-n 1"),
+        pytest.param(["-n1"], [H.POINTS.HEAD_SHA], id="-n1"),
+        pytest.param(["-n", "99"], [H.POINTS.HEAD_SHA, H.POINTS.HEAD1_SHA], id="-n 99"),
+        pytest.param(["--skip", "1"], [H.POINTS.HEAD1_SHA], id="skip-1"),
+        pytest.param(["--", "--skip", "1"], [], id="skip-interpreted-as-path"),
+        pytest.param(
+            ["--skip", "1", "--"],
+            [H.POINTS.HEAD1_SHA],
+            id="skip-1-followed-by-doubledash",
+        ),
+        pytest.param(["--since", "a-broken-date"], [], id="since-broken"),
+        pytest.param(["--since", "9999-01-01"], [], id="since-future"),
+        pytest.param(
+            ["--since", "2000-01-01"],
+            [H.POINTS.HEAD_SHA, H.POINTS.HEAD1_SHA],
+            id="since-ancient-past",
+        ),
+        pytest.param(
+            ["--since", "2019-06-15"], [H.POINTS.HEAD_SHA], id="since-halfway"
+        ),
+        pytest.param(
+            ["--until", "2019-06-15"], [H.POINTS.HEAD1_SHA], id="until-halfway"
+        ),
+    ],
+)
+def test_extra_git_log_options(data_archive, cli_runner, args, expected_commits):
+    def get_log_hashes(r):
+        return re.findall(r"(?m)^commit ([0-9a-f]{40})$", r.stdout)
+
+    with data_archive("points"):
+        r = cli_runner.invoke(["log", *args])
+        assert r.exit_code == 0, r.stderr
+        assert get_log_hashes(r) == expected_commits