From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 14 Jan 2026 10:56:08 GMT
Subject: Re: [PATCH v2 1/3] last-modified: rewrite error message when more than one revision given
Message-ID: <aWd2SLrBQRBC1e2S@pks.im>
In-Reply-To: <20260114-toon-last-modified-tree-v2-1-ba3b1860898f@iotcl.com>

On Wed, Jan 14, 2026 at 11:24:45AM +0100, Toon Claes wrote:
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index c80f0535f6..06e3f79aec 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -145,16 +145,20 @@ 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 revision at a time"));

Do we maybe want to be a bit more specific and say committish instead of
revision?

> diff --git a/t/t8020-last-modified.sh b/t/t8020-last-modified.sh
> index 50f4312f71..1183ae667b 100755
> --- a/t/t8020-last-modified.sh
> +++ b/t/t8020-last-modified.sh
> @@ -12,7 +12,7 @@ test_expect_success 'setup' '
>  	test_commit 3 a/b/file
>  '
>  
> -test_expect_success 'cannot run last-modified on two trees' '
> +test_expect_success 'cannot run last-modified on two revision' '

Nit: s/revision/revisions/

>  	test_must_fail git last-modified HEAD HEAD~1

Another tiny nit: I'm always a bit wary around tests that don't verify
the reason for failure. We might want to add:

    test_grep "last-modified can only operate on one revision at a time" err

Patrick