[v4] git-log: add a --since=... --as-filter option

This is similar to --since, but it will filter out not matching commits,
rather than stopping at the first not matching commit.

This is useful if you e.g. want to list the commits from the last year,
but one odd commit has a bad commit date and that would hide lots of
earlier commits in that range.

The behavior of --since is left unchanged, since it's valid to depend on
its current behavior.

Signed-off-by: Miklos Vajna <vmiklos@vmiklos.hu>
---

Hi Ævar,

On Tue, Apr 12, 2022 at 10:47:15AM +0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > +--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...

My understanding is that in practice --since is the only option that
terminates the revision walk on the first match, so I would argue there
is no need for this.

> 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?).

Good catch, I've added --max-age and --after as well.

> 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.

Makes sense, we kind of simplify history by default here & then this
option could be documented as one that modifies this terminating
behavior.

> 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?

I found the following callers:

- some builtins that want to make sure that no history limiting is used,
  an additional --as-filter doesn't change behavior there

- blame: this has its own commit walking loop, so --as-filter doesn't
  change any behavior here unintentionally.

- bundle: --since is not used for revision walking here, just to check
  what tags to include/exclude, so this is already not terminating

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

t/t5607-clone-bundle.sh already tests bundle --since. I've added a new
t/t4218-blame-limit.sh to test blame --since, it seems there were no
tests for this so far.

Thanks,

Miklos

 Documentation/rev-list-options.txt |  6 +++++
 revision.c                         | 13 +++++++++--
 revision.h                         |  1 +
 t/t4217-log-limit.sh               | 36 ++++++++++++++++++++++++++++++
 t/t4218-blame-limit.sh             | 36 ++++++++++++++++++++++++++++++
 5 files changed, 90 insertions(+), 2 deletions(-)
 create mode 100755 t/t4217-log-limit.sh
 create mode 100755 t/t4218-blame-limit.sh

Miklos Vajna <vmiklos@vmiklos.hu> writes:

> This is similar to --since, but it will filter out not matching commits,
> rather than stopping at the first not matching commit.
>
> This is useful if you e.g. want to list the commits from the last year,
> but one odd commit has a bad commit date and that would hide lots of
> earlier commits in that range.
>
> The behavior of --since is left unchanged, since it's valid to depend on
> its current behavior.

The above is good if it were a new --since-as-filter option.  Adding
"--as-filter" as a separate option to be used in conjunction with
"--since" would fundamentally mean "The behaviour of --since is left
unchanged" cannot possibly be true.

A suggested rewrite.  I label each section and highlight its point,
but in the end product, you should just separate them with a blank
line, making each of them into its own paragraph.

Title (single line, short and to the point)

    log: "--as-filter" option adjusts how "--since" cut-off works

Intro (observation of the current state)

    The "--since=<time>" option of "git log" limits the commits
    displayed by the command by stopping the traversal once it
    sees a commit whose timestamp is older than the given time and
    not digging further into its parents.

Problem description (pros and cons of the current state)

    This is OK in a history where a commit always has a newer
    timestamp than any of its parents'.  Once you see a commit older
    than the given <time>, all ancestor commits of it are even older
    than the time anyway.  It poses, however, a problem when there
    is a commit with a wrong timestamp that makes it appear older
    than its parents.  Stopping traversal at the "incorrectly old"
    commit will hide its ancestors that are newer than that wrong
    commit and are newer than the cut-off time given with the --since
    option.  --max-age and --after being the synonyms to --since,
    they share the same issue.

Solution (give orders to the codebase to "be like so")

    Add a new "--as-filter" option that modifies how "--since=<time>"
    is used.  Instead of stopping the traversal to hide an old
    enough commit and its all ancestors, exclude commits with an old
    timestamp from the output but still keep digging the history.

Other comments (caveats, etc.)

    Without other traversal stopping options, this will force the
    command in "git log" family to dig down the history to the root.
    It may be an acceptable cost for a small project with short
    history and many commits with screwy timestamps.

> diff --git a/revision.c b/revision.c
> index 7d435f8048..ff018c3976 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)

That's an overly long line.

> +			continue;

In any case, isn't this too late in limit_list() to adjust --since?
There is this logic earlier in the loop:

	while (original_list) {
		struct commit *commit = pop_commit(&original_list);
		struct object *obj = &commit->object;
		show_early_output_fn_t show;

		if (commit == interesting_cache)
			interesting_cache = NULL;

		if (revs->max_age != -1 && (commit->date < revs->max_age))
			obj->flags |= UNINTERESTING;
		if (process_parents(revs, commit, &original_list, NULL) < 0)
			return -1;

We look at max_age and if it is set and newer than the timestamp of
the commit we are looking at, we immediately mark that commit
uninteresting so that this and all its ancestors are excluded from
the output.  Don't you want to disable that logic so that the
traversal continues?

> @@ -3862,6 +3868,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) ||

I am not sure how --since should affect (or not affect) the history
simplification, but my gut feeling says this may cause unintended
fallout.  I do not have time to dig deeper today, but something to
keep in mind...

> diff --git a/t/t4217-log-limit.sh b/t/t4217-log-limit.sh
> new file mode 100755
> index 0000000000..2a3705c714
> --- /dev/null
> +++ b/t/t4217-log-limit.sh
> @@ -0,0 +1,36 @@
> +#!/bin/sh
> +
> +test_description='git log with filter options limiting the output'

OK.

> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

Does any of the tests in this file care?  The above is a mechanism
primarily for transitioning scripts that were written long time ago,
and newly written tests that do not have to care about the initial
branch should not need it (they can instead explicitly create a
branch and work on it, if they really need to refer the initial
branch by name, but as far as I can tell, none your test in this
file even needs to refer to any branch with any name).

> +
> +. ./test-lib.sh
> +
> +GIT_TEST_COMMIT_GRAPH=0
> +GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0

Do these matter, and should these matter?  Does anybody export them
to make any difference in this test?

> +test_expect_success 'setup test' '
> +	git init &&
> +	echo a > file &&

Style:

	echo a >file &&

(cf. Documentation/CodingGuidelines)

> +	git add file &&
> +	GIT_COMMITTER_DATE="2021-02-01 0:00" git commit -m init &&

It is somewhat annoying to see 00:00 spelled as 0:00 here (and below).

> +	echo a >> file &&
> +	git add file &&
> +	GIT_COMMITTER_DATE="2022-02-01 0:00" git commit -m first &&
> +	echo a >> file &&
> +	git add file &&
> +	GIT_COMMITTER_DATE="2021-03-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 &&

I think you meant --format=%s (which is --pretty="tformat:%s"), as
you do not want "actual" to end in an incomplete line.  The same
"no space between the redirection operator and its target" applies
here as everywhere else.

> +	! test_i18ngrep init actual &&
> +	test_i18ngrep first actual &&
> +	! test_i18ngrep second actual &&
> +	test_i18ngrep third actual

test_i18ngrep -> grep

But stepping back a bit, do we or do we not care the order in which
first and third appear in the "actual" file?  It's not like we have
a mergy history and two commits that cannot topologically be
compared.  We have a simple linear single strand of pearls here, and
if you start traversing from the tip, you'll reliably see third
first, then second, then first and then finally init, in that order
and in no other order.  So, wouldn't we be better off writing it
more like ...

	git log --since=2022-01-01 --as-filter --format=%s >actual &&
	cat >expect <<-\EOF &&
	third
	first
	EOF
	test_cmp expect actual

... i.e. explicitly spell out what we expect not to change?

Thanks.