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

Miklos Vajna <vmiklos@vmiklos.hu> writes:

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.

That's an overly long line.

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?

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

OK.

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

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

Style:

	echo a >file &&

(cf. Documentation/CodingGuidelines)

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

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