From: Junio C Hamano Date: Fri, 22 Apr 2022 22:11:11 GMT Subject: Re: [PATCH v6] log: "--since-as-filter" option is a non-terminating "--since" variant Message-ID: In-Reply-To: Miklos Vajna writes: > The "--since=" 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 , > 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=". 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 > --- > > Hi Junio, > > On Fri, Apr 22, 2022 at 11:48:45AM -0700, Junio C Hamano 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=:: > + 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.