Merge pull request #507 from koordinates/fix-log-options-with-args

log: Fix limit options not working

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

@@ -16,6 +16,10 @@
 from . import diff_estimation
 
 
+class RemovalInKart012Warning(UserWarning):
+    pass
+
+
 class PreserveDoubleDash(click.Command):
     """
     Preserves the double-dash ("--") arg from user input.
@@ -35,7 +39,7 @@ def parse_args(self, ctx, args):
                     # which ideally we'd like them to stop doing.
                     warnings.warn(
                         "Using '--' twice is no longer needed, and will behave differently or fail in Kart 0.12",
-                        UserWarning,
+                        RemovalInKart012Warning,
                     )
                 else:
                     # Insert a second `--` arg.
@@ -47,7 +51,18 @@ def parse_args(self, ctx, args):
         return super(PreserveDoubleDash, self).parse_args(ctx, args)
 
 
-def parse_extra_args(repo, args):
+def parse_extra_args(
+    repo,
+    args,
+    *,
+    max_count,
+    skip,
+    since,
+    until,
+    author,
+    committer,
+    grep,
+):
     """
     Interprets positional `kart log` args, including "--", commits/refs, and paths.
     Returns a two-tuple: (other_args, paths)
@@ -60,7 +75,7 @@ def parse_extra_args(repo, args):
     if "--" in args:
         dash_index = args.index("--")
         paths = args[dash_index + 1 :]
-        other_args = args[:dash_index]
+        other_args = list(args[:dash_index])
     else:
         other_args = []
         paths = []
@@ -73,6 +88,12 @@ def parse_extra_args(repo, args):
                 # So we can assume it's a CLI flag, presumably for git rather than kart.
                 # It *could* be a path, but in that case the user should add a `--` before this option
                 # to disambiguate, and they haven't done so here.
+                issue_link = "https://github.com/koordinates/kart/issues/508"
+                warnings.warn(
+                    f"{arg!r} is unknown to Kart and will be passed directly to git. "
+                    f"This will be removed in Kart 0.12! Please comment on {issue_link} if you need to use this option.",
+                    RemovalInKart012Warning,
+                )
                 other_args.append(arg)
                 continue
 
@@ -93,7 +114,23 @@ def parse_extra_args(repo, args):
                 break
             else:
                 other_args.append(arg)
-    return other_args, paths
+
+    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)
+    return other_args, list(paths)
 
 
 @click.command(
@@ -132,12 +169,72 @@ 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,
+    args,
+    **kwargs,
+):
+    """
+    Show commit logs
+    """
     repo = ctx.obj.get_repo(allowed_states=KartRepoState.ALL_STATES)
 
-    other_args, paths = parse_extra_args(repo, args)
+    other_args, paths = parse_extra_args(repo, args, **kwargs)
 
     # 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