Re: [PATCH v6] log: "--since-as-filter" option is a non-terminating "--since" variant

[Junio C Hamano <gitster@pobox.com>]

Miklos Vajna <vmiklos@vmiklos.hu> writes:

> 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.
>
> 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.
>
> Add a new "--since-as-filter" option that is a variant of
> "--since=<time>".  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.
>
> 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.
>
> It is quite unlikely for us to add traversal stopper other than since,
> so have this as a --since-as-filter option, rather than a separate
> --as-filter, that would be probably more confusing.
>
> Signed-off-by: Miklos Vajna <vmiklos@vmiklos.hu>
> ---
>
> Hi Junio,
>
> On Fri, Apr 22, 2022 at 11:48:45AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
>> Now I had some time to think about it, I have a feeling that it is
>> quite unlikely for us to add traversal stopper other than since, so
>> having a separate "--as-filter" would probably be more confusing
>> than adding "--since-as-filter", stressing on "only the 'show
>> commits with timestamp after this one' has two variants".
>
> I'm fine with this approach, it goes back to the initial version. I 
> rather reworked v5 in practice, to keep the other improvements.
>
> Here is a patch that does this.
>
> Regards,
>
> Miklos

Thanks.  Now 2.36 release is behind us, let's queue this in 'seen'
and after getting reviewed by somebody else---I do not trust
anything looked at by me and nobody else---merge it down, aiming for
graduation during this cycle.

> +--since-as-filter=<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.

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

Much ealrier than this part in the same loop there is

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

which taints the commit we are looking at as UNINTERESTING, cutting
the traversal down to its parents, if its timestamp is older than
max_age, and it would need to be taught not to do that, no?

Ah, OK, max_age_as_filter is *NOT* a boolean (false is -1 and true
is something else) but is an independent timestamp and the
expectation is that max_age is left to be -1 when --since is not in
use.  --since-as-filter's timestamp is stored in max_age_as_filter
so the earlier "compare with max_age and use UNINTERESTING bit to
stop traversal" code does not have to be modified.

> +		if (revs->max_age_as_filter != -1 &&
> +			(commit->date < revs->max_age) && !revs->line_level_traverse)
> +			continue;
>  		date = commit->date;
>  		p = &commit_list_insert(commit, p)->next;

And if that is the assumption of this code, shouldn't this part be
using "if max_age is set (i.e. not -1) and the commit is older than
that and we are not doing -La,b traversal"?  "--since-as-filter"
being used does not mean max_age must be set to a value that can be
compared to timestamps in commits in the world order of this version
of the patch, right?

> @@ -1838,6 +1841,7 @@ void repo_init_revisions(struct repository *r,
>  	revs->dense = 1;
>  	revs->prefix = prefix;
>  	revs->max_age = -1;
> +	revs->max_age_as_filter = -1;
>  	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 ((argcount = parse_long_opt("since-as-filter", argv, &optarg))) {
> +		revs->max_age_as_filter = approxidate(optarg);
> +		return argcount;

OK, so max_age_as_filter is a timestamp to be compared with commits
when --since-as-filter option is given.

> @@ -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_as_filter != -1 &&
> +	    comparison_date(revs, commit) < revs->max_age_as_filter)
> +			return commit_ignore;

This one does make sense.

> diff --git a/revision.h b/revision.h
> index 5bc59c7bfe..e80c148b19 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -263,6 +263,7 @@ struct rev_info {
>  	int skip_count;
>  	int max_count;
>  	timestamp_t max_age;
> +	timestamp_t max_age_as_filter;
>  	timestamp_t min_age;

So does this.

Everything except that "why are we checking if --since-as-filter is
set and then comparing with the value came from --since?" part looks
great to me.  If that indeed is a bug (it is very possible that I am
misreading the logic and the comparison with continue is perfectly
correct), and if the tests added by this patch didn't catch it, then
the test script may need a bit more work to catch such a mistake.

Thanks.