Re: [PATCH] name-rev: test showing failure with non-monotonic commit dates

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jacob Keller <jacob.e.keller@intel.com>
Cc: git@vger.kernel.org, Jacob Keller <jacob.keller@gmail.com>
Subject: Re: [PATCH] name-rev: test showing failure with non-monotonic commit dates
Date: Tue, 15 Feb 2022 08:15:58 +0100	[thread overview]
Message-ID: <220215.868rucy4t6.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <20220214210136.1532574-1-jacob.e.keller@intel.com>

On Mon, Feb 14 2022, Jacob Keller wrote:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> If a commit in a sequence of linear history has a non-monotonically
> increasing commit timestamp, git name-rev will not properly name the
> commit.
>
> However, if you use --annotate-stdin then the commit does actually get
> picked up and named properly.
>
> Analyzing the source, it appears to be caused by the cutoff logic which
> is some sort of heuristic which relies on monotonically increasing
> commit dates.
>
> This seems like the cutoff using commit date is some sort of heuristic
> which reduces the cost of describing something.. but --annotate-stdin
> and --all don't use it.
>
> In the example setup I could do:
>
> echo "<commit id>" | git name-rev --annotate-stdin
>
> and get the expected result without the cutoff logic, and it seems at
> least on small repositories to be as fast as the normal attempt, except
> it produces accurate results.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>  t/t6120-describe.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index 9781b92aeddf..e9f897e42591 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -488,6 +488,68 @@ test_expect_success 'name-rev covers all conditions while looking at parents' '
>  	)
>  '
>  
> +# A-B-C-D-E-main
> +#
> +# Where C has a non-monotonically increasing commit timestamp w.r.t. other
> +# commits
> +test_expect_success 'non-monotonic commit dates setup' '
> +	git init non-monotonic &&
> +	(
> +		cd non-monotonic &&
> +
> +		echo A >file &&
> +		git add file &&
> +		GIT_COMMITTER_DATE="2020-01-01 18:00" git commit -m A &&
> +
> +		echo B >file &&
> +		git add file &&
> +		GIT_COMMITTER_DATE="2020-01-02 18:00" git commit -m B &&
> +
> +		echo C >file &&
> +		git add file &&
> +		GIT_COMMITTER_DATE="2005-01-01 18:00" git commit -m C &&
> +
> +		echo D >file &&
> +		git add file &&
> +		GIT_COMMITTER_DATE="2020-01-04 18:00" git commit -m D &&
> +
> +		echo E >file &&
> +		git add file &&
> +		GIT_COMMITTER_DATE="2020-01-05 18:00" git commit -m E
> +	)

Shorter & avoids the needless subshell as:

    git init repo &&
    test_commit -C repo --date="2020-01-01 18:00" A &&
    test_commit -C repo --date="2020-01-02 18:00" B &&
    [...]

> +test_expect_failure 'name-rev commit timestamp prevents naming commits' '
> +	(
> +		cd non-monotonic &&
> +
> +		B=$(git rev-parse main~3) &&
> +
> +		echo "$B main~3" >expect &&
> +		git name-rev $B >actual &&
> +
> +		test_cmp expect actual
> +	)
> +'

I haven't checked, but is the explicit peeling to $B really needed here,
are the results different with a main~3 or main~3^{commit}?

I.e. the first column of the output will of course be, but will the
result on the second column? I suspect not, but haven't run this. In any
case I tihnk teh test/commit message could do with an explanation.

> +test_expect_success 'name-rev --all works with non-monotonic' '
> +	(
> +		cd non-monotonic &&
> +
> +		cat >expect <<EOF &&

You can use "<<-\EOF" here so you can indent these:

> +main
> +main~1
> +main~2
> +main~3
> +main~4
> +EOF
> +
> +		git log --pretty=%H | git name-rev --annotate-stdin --name-only >actual &&

Don't use "git" on the LHS of a pipe, in case it segfaults, so:

    git log [...] >revs &&
    git name-rev [...] <revs >actual

> +
> +		test_cmp expect actual
> +	)
> +'
> +
>  #               B
>  #               o
>  #                \