Re: [PATCH v3] git-log: add a --since=... --as-filter option

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Apr 08 2022, Miklos Vajna wrote:

> On Thu, Apr 07, 2022 at 07:30:33PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
>> As a single-shot change, "--since-as-filter" is certainly an easy to
>> explain approach of least resistance.
>> 
>> But when viewed from a higher level as a general design problem, I
>> am unsure if it is a good direction to go in.
>> 
>> Giving "--since" the "as-filter" variant sets a precedent, and
>> closes the door for a better UI that we can extend more generally
>> without having to add "--X-as-filter" for each and every conceivable
>> "--X" that is a traversal stopper into a filtering kind.
>
> I like the idea of doing this mode as "--since=... --as-filter". I can
> still implement it just for --since=... initially, but it can be
> extended for other flags as well in the future if there is a need.

Yes, I think this is much better.

>> If we pursue the possibility further, perhaps we may realize that
>> there isn't much room for us to add too many "traversal stoppers" in
>> the future, in which case giving "as-filter" to a very limited few
>> traversal stoppers may not be too bad.  I just do not think we have
>> explored that enough to decide that "--since-as-filter" is a good UI
>
> My understanding is that get_revision_1() has a special-case for the max
> age case to be a "traversal stopper", and all other options are just 
> filtering in limit_range(). But perhaps I missed something.
> [...]
>  Documentation/rev-list-options.txt |  5 +++++
>  revision.c                         | 14 +++++++++++--
>  revision.h                         |  1 +
>  t/t4217-log-limit.sh               | 32 ++++++++++++++++++++++++++++++
>  4 files changed, 50 insertions(+), 2 deletions(-)
>  create mode 100755 t/t4217-log-limit.sh
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index fd4f4e26c9..8565299264 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -25,6 +25,11 @@ ordering and formatting options, such as `--reverse`.
>  --after=<date>::
>  	Show commits more recent than a specific date.
>  
> +--as-filter::
> +	When combined with `--since=<date>`, show all commits more recent than
> +	a specific date. This visits all commits in the range, rather than stopping at
> +	the first commit which is older than a specific date.

I wonder if we should be more future-proof here and say that we'll run
anything as a filter, and that --since is the one option currently
affected.

But maybe there's no reason to do so...

In any case these docs are inaccurate because they cover --since, but if
you check revision.c we'll set "max_age" on other options too
(synonyms?).

All in all I wonder if this wouldn't be much more understandable if we
advertised is as another option to do "HISTORY SIMPLIFICATION", which
looking at e.g. get_commit_action() and "prune" is kind of what we're
doing with the existing --since behavior.

>  --until=<date>::
>  --before=<date>::
>  	Show commits older than a specific date.
> diff --git a/revision.c b/revision.c
> index 7d435f8048..41ea72e516 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1440,6 +1440,9 @@ static int limit_list(struct rev_info *revs)
>  		if (revs->min_age != -1 && (commit->date > revs->min_age) &&
>  		    !revs->line_level_traverse)
>  			continue;
> +		if (revs->max_age != -1 && revs->as_filter && (commit->date < revs->max_age) &&
> +		    !revs->line_level_traverse)
> +			continue;
>  		date = commit->date;
>  		p = &commit_list_insert(commit, p)->next;
>  
> @@ -1838,6 +1841,7 @@ void repo_init_revisions(struct repository *r,
>  	revs->dense = 1;
>  	revs->prefix = prefix;
>  	revs->max_age = -1;
> +	revs->as_filter = 0;
>  	revs->min_age = -1;
>  	revs->skip_count = -1;
>  	revs->max_count = -1;
> @@ -2218,6 +2222,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  	} else if ((argcount = parse_long_opt("since", argv, &optarg))) {
>  		revs->max_age = approxidate(optarg);
>  		return argcount;
> +	} else if (!strcmp(arg, "--as-filter")) {
> +		revs->as_filter = 1;
> +		return argcount;
>  	} else if ((argcount = parse_long_opt("after", argv, &optarg))) {
>  		revs->max_age = approxidate(optarg);
>  		return argcount;
> @@ -3365,7 +3372,7 @@ static void explore_walk_step(struct rev_info *revs)
>  	if (revs->sort_order == REV_SORT_BY_AUTHOR_DATE)
>  		record_author_date(&info->author_date, c);
>  
> -	if (revs->max_age != -1 && (c->date < revs->max_age))
> +	if (revs->max_age != -1 && !revs->as_filter && (c->date < revs->max_age))
>  		c->object.flags |= UNINTERESTING;
>  
>  	if (process_parents(revs, c, NULL, NULL) < 0)
> @@ -3862,6 +3869,9 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi
>  	if (revs->min_age != -1 &&
>  	    comparison_date(revs, commit) > revs->min_age)
>  			return commit_ignore;
> +	if (revs->max_age != -1 && revs->as_filter &&
> +	    comparison_date(revs, commit) < revs->max_age)
> +			return commit_ignore;
>  	if (revs->min_parents || (revs->max_parents >= 0)) {
>  		int n = commit_list_count(commit->parents);
>  		if ((n < revs->min_parents) ||
> @@ -4019,7 +4029,7 @@ static struct commit *get_revision_1(struct rev_info *revs)
>  		 * that we'd otherwise have done in limit_list().
>  		 */
>  		if (!revs->limited) {
> -			if (revs->max_age != -1 &&
> +			if (revs->max_age != -1 && !revs->as_filter &&
>  			    comparison_date(revs, commit) < revs->max_age)
>  				continue;

I think it's good to do this as a general mechanism, but if you now
remove the "max_age" field from "struct rev_info" and:

	make -k

You'll see a bunch of callers who check "max_age" outside of revision.c,
since those will accept these revision options are they doing the right
thing now too?

...

> diff --git a/revision.h b/revision.h
> index 5bc59c7bfe..fe37ebd83d 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -263,6 +263,7 @@ struct rev_info {
>  	int skip_count;
>  	int max_count;
>  	timestamp_t max_age;
> +	int as_filter;
>  	timestamp_t min_age;
>  	int min_parents;
>  	int max_parents;
> diff --git a/t/t4217-log-limit.sh b/t/t4217-log-limit.sh
> new file mode 100755
> index 0000000000..a66830e3d7
> --- /dev/null
> +++ b/t/t4217-log-limit.sh
> @@ -0,0 +1,32 @@
> +#!/bin/sh
> +
> +test_description='git log with filter options limiting the output'
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +
> +GIT_TEST_COMMIT_GRAPH=0
> +GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
> +
> +test_expect_success 'setup test' '
> +	git init &&
> +	echo a > file &&
> +	git add file &&
> +	GIT_COMMITTER_DATE="2022-02-01 0:00" git commit -m init &&
> +	echo a >> file &&
> +	git add file &&
> +	GIT_COMMITTER_DATE="2021-01-01 0:00" git commit -m second &&
> +	echo a >> file &&
> +	git add file &&
> +	GIT_COMMITTER_DATE="2022-03-01 0:00" git commit -m third
> +'
> +
> +test_expect_success 'git log --since-as-filter' '
> +	git log --since="2022-01-01" --as-filter --pretty="format:%s" > actual &&
> +	test_i18ngrep init actual &&
> +	! test_i18ngrep second actual &&
> +	test_i18ngrep third actual
> +'
> +
> +test_done

In any case we should have tests for those callers, i.e. blame, bundle
etc.