log: Fix limit options not working

In #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
(#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`
* `--author` / `--committer`
* `--grep`

And incidentally adds those options to the `--help` output.

CHANGELOG.md

@@ -8,7 +8,9 @@ _When adding new entries to the changelog, please include issue/PR numbers where
 
 ### Bugs fixed
 
-* `log`: Fixed a regression in 0.10.6 involving range arguments (`x..y` or `x...y`) being handled incorrectly. [#504](https://github.com/koordinates/kart/pull/504)
+* `log`: Fixed two regressions in 0.10.6 involving argument parsing:
+  - range arguments (`x..y` or `x...y`) were handled incorrectly. [#504](https://github.com/koordinates/kart/pull/504)
+  - `-n INTEGER` caused an error [#507](https://github.com/koordinates/kart/pull/507)
 * Auto-incrementing PK sequences now work in PostGIS working copies for table names containing the `.` character. [#468](https://github.com/koordinates/kart/pull/468)
 
 ## 0.10.6

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,12 +132,93 @@ 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",
+    nargs=1,
+    metavar="DATE",
+    help="Show commits more recent than a specific date.",
+)
+@click.option(
+    "--until",
+    "--before",
+    nargs=1,
+    metavar="DATE",
+    help="Show commits older than a specific date.",
+)
+@click.option(
+    "--author",
+    nargs=1,
+    metavar="PATTERN",
+    multiple=True,
+    help="Limit the commits output to ones with author matching the specified pattern (regular expression)",
+)
+@click.option(
+    "--committer",
+    nargs=1,
+    metavar="PATTERN",
+    multiple=True,
+    help="Limit the commits output to ones with committer matching the specified pattern (regular expression)",
+)
+@click.option(
+    "--grep",
+    nargs=1,
+    metavar="PATTERN",
+    multiple=True,
+    help="Limit the commits output to ones with log message matching the specified pattern (regular expression)",
+)
 @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,
+    author,
+    committer,
+    grep,
+    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}")
+    # These ones can be specified more than once
+    if author:
+        other_args.extend(f"--author={a}" for a in author)
+    if committer:
+        other_args.extend(f"--committer={c}" for c in committer)
+    if grep:
+        other_args.extend(f"--grep={g}" for g in grep)
 
     # TODO: should we check paths exist here? git doesn't!
     if output_format == "text":

tests/test_log.py

@@ -267,3 +267,55 @@ 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"
+        ),
+        pytest.param(
+            ["--author", "Robert"],
+            [H.POINTS.HEAD_SHA, H.POINTS.HEAD1_SHA],
+            id="author-match",
+        ),
+        pytest.param(["--author", "Telemachus"], [], id="author-no-match"),
+        pytest.param(
+            ["--committer", "Robert"],
+            [H.POINTS.HEAD_SHA, H.POINTS.HEAD1_SHA],
+            id="committer-match",
+        ),
+        pytest.param(["--committer", "Telemachus"], [], id="committer-no-match"),
+        pytest.param(["--grep", "Coromandel.*coast"], [H.POINTS.HEAD_SHA], id="grep"),
+    ],
+)
+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