From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 14 Jan 2026 10:56:21 GMT
Subject: Re: [PATCH v2 3/3] last-modified: verify revision argument is a commit-ish
Message-ID: <aWd2VcX7gERTDK2t@pks.im>
In-Reply-To: <20260114-toon-last-modified-tree-v2-3-ba3b1860898f@iotcl.com>

On Wed, Jan 14, 2026 at 11:24:47AM +0100, Toon Claes wrote:
> Passing a tree OID to git-last-modified(1) would trigger BUG behavior.

I guess passing a blob OID would cause the same. So maybe:

    Passing a non-committish revision to git-lsat-modified(1) triggers
    the following BUG:

>     git last-modified HEAD^{tree}
>     BUG: builtin/last-modified.c:456: paths remaining beyond boundary in last-modified
> 
> Fix this error by verifying the parsed revision peels to a commit-ish.
> 
> While at it, also fix a memory leak in populate_paths_from_revs().

You fixed this memory leak in a preceding commit now, so this remark is
not accurate anymore.

> diff --git a/t/t8020-last-modified.sh b/t/t8020-last-modified.sh
> index 1183ae667b..22635de447 100755
> --- a/t/t8020-last-modified.sh
> +++ b/t/t8020-last-modified.sh
> @@ -55,6 +56,13 @@ test_expect_success 'last-modified recursive' '
>  	EOF
>  '
>  
> +test_expect_success 'last-modified on annotated tag' '
> +	check_last_modified t2 <<-\EOF
> +	2 a
> +	1 file
> +	EOF
> +'
> +
>  test_expect_success 'last-modified recursive with show-trees' '
>  	check_last_modified -r -t <<-\EOF
>  	3 a/b

Nice, thanks for adding this test.

> @@ -235,4 +243,9 @@ test_expect_success 'last-modified complains about unknown arguments' '
>  	grep "unknown last-modified argument: --foo" err
>  '
>  
> +test_expect_success 'last-modified expects commit-ish' '
> +	test_must_fail git last-modified HEAD^{tree} 2>err &&
> +	grep "revision argument is not a commit-ish" err
> +'

We typically use `test_grep ()` so that we know what the actual contents
are in case the assertion ever fails.

Patrick