Miklos Vajna <vmiklos@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>
> Hi Junio,
>
> On Fri, Apr 22, 2022 at 11:48:45AM -0700, Junio C Hamano <gitster@xxxxxxxxx> 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.
