git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Apr 12, 2022, 08:47 UTC
Message-ID
<220412.86pmlmhe9a.gmgdl@evledraar.gmail.com>
In-Reply-To
<YlCiqgO6rL908Zsi@vmiklos.hu>
On Fri, Apr 08 2022, Miklos Vajna wrote:
Show 15 quoted lines
> 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.
Show 29 quoted lines
>> 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.

Show 62 quoted lines
>  --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?

...
Show 50 quoted lines
> 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.

Previous: Miklos VajnaNext: Miklos Vajna
Message 12 of 24 in “git log --since to not stop after first old commit?”
  1. Miklos VajnaApr 1, 2022
  2. Ævar Arnfjörð BjarmasonApr 1, 2022
  3. Miklos VajnaApr 1, 2022
  4. Ævar Arnfjörð BjarmasonApr 1, 2022
  5. Junio C HamanoApr 1, 2022
  6. git-log: add a --since-as-filter optionMiklos Vajna, Apr 1, 2022
  7. git-log: add a --since-as-filter optionMiklos Vajna, Apr 2, 2022
  8. Miklos VajnaApr 7, 2022
  9. Junio C HamanoApr 8, 2022
  10. Junio C HamanoApr 8, 2022
  11. git-log: add a --since=... --as-filter optionMiklos Vajna, Apr 8, 2022
  12. Ævar Arnfjörð BjarmasonApr 12, 2022
  13. git-log: add a --since=... --as-filter optionMiklos Vajna, Apr 15, 2022
  14. Junio C HamanoApr 15, 2022
  15. log: "--as-filter" option adjusts how "--since" cut-off worksMiklos Vajna, Apr 16, 2022
  16. Miklos VajnaApr 22, 2022
  17. Miklos VajnaApr 11, 2022
  18. demerphqApr 11, 2022
  19. Junio C HamanoApr 11, 2022
  20. Junio C HamanoApr 22, 2022
  21. log: "--since-as-filter" option is a non-terminating "--since" variantMiklos Vajna, Apr 22, 2022
  22. Junio C HamanoApr 22, 2022
  23. log: "--since-as-filter" option is a non-terminating "--since" variantMiklos Vajna, Apr 23, 2022
  24. Junio C HamanoApr 22, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.