From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 13 Jan 2026 06:54:55 GMT
Subject: Re: [PATCH] last-modified: verify revision argument is a commit-ish
Message-ID: <aWXsP1GJ2YfrZyh0@pks.im>
In-Reply-To: <20260112-toon-last-modified-tree-v1-1-ecbc78341f76@iotcl.com>

On Mon, Jan 12, 2026 at 05:17:41PM +0100, Toon Claes wrote:
> Passing a tree OID to git-last-modified(1) would trigger BUG behavior.
> 
>     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 is peels to a

s/is peels/peels/

> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index c80f0535f6..cac94e384d 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -145,8 +145,15 @@ static int populate_paths_from_revs(struct last_modified *lm)
>  		if (obj->item->flags & UNINTERESTING)
>  			continue;
>  
> -		if (num_interesting++)
> -			return error(_("last-modified can only operate on one tree at a time"));
> +		if (num_interesting++) {
> +			ret = error(_("last-modified can only operate on one tree at a time"));

A preexisting issue, but isn't this error message a bit weird? Below we
assert that we've got a commit, but here we say that we expect to work
on a tree.

> +			break;
> +		}
> +
> +		if (!repo_peel_to_type(lm->rev.repo, obj->path, 0, obj->item, OBJ_COMMIT)) {
> +			ret = error(_("revision argument is not a commit-ish"));
> +			break;
> +		}

I'd prefer a `goto out` in both error cases, but that's a matter of
style and thus a subjective proposal. So no need to address this.

>  		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
>  			      &obj->item->oid, "", &diffopt);
> diff --git a/t/t8020-last-modified.sh b/t/t8020-last-modified.sh
> index 50f4312f71..d0d52add05 100755
> --- a/t/t8020-last-modified.sh
> +++ b/t/t8020-last-modified.sh
> @@ -235,4 +235,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
> +'

Do we have tests that verify that this works when passed for example an
annotated tag?

Thanks!

Patrick