diff --git a/CHANGELOG.md b/CHANGELOG.md index 09ebc810a..ffc7dc85d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/kart/log.py b/kart/log.py index 329a5d9ff..21eb31ac3 100644 --- a/kart/log.py +++ b/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": diff --git a/tests/test_log.py b/tests/test_log.py index fda35a2a4..c411ade23 100644 --- a/tests/test_log.py +++ b/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