[PATCH] last-modified: verify revision argument is a commit-ish

[PATCH] last-modified: verify revision argument is a commit-ish

[Toon Claes]

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

While at it, also fix a memory leak in populate_paths_from_revs().

Reported-by: Gusted <gusted@codeberg.org>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
Recently there was a bug reported[1] passing a tree OID triggers a BUG:

    $ git last-modified fb06ce04173d47aaaa498385621cba8b8dfd7584
    BUG: builtin/last-modified.c:456: paths remaining beyond boundary in last-modified
    [1]    690163 IOT instruction (core dumped)  git last-modified

    `fb06ce04173d47aaaa498385621cba8b8dfd7584` is the tree commit id of web_src. I
    suppose this should've returned a nice error message or blank output.

Fix this bug by checking the revision argument.

[1]: https://lore.kernel.org/git/03f96860-29fc-42a7-a220-c3ec65eb8516@codeberg.org/
---
 builtin/last-modified.c  | 15 +++++++++++----
 t/t8020-last-modified.sh |  5 +++++
 2 files changed, 16 insertions(+), 4 deletions(-)

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
@@ -123,7 +123,7 @@ static void add_path_from_diff(struct diff_queue_struct *q,
 
 static int populate_paths_from_revs(struct last_modified *lm)
 {
-	int num_interesting = 0;
+	int num_interesting = 0, ret = 0;
 	struct diff_options diffopt;
 
 	/*
@@ -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"));
+			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;
+		}
 
 		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
 			      &obj->item->oid, "", &diffopt);
@@ -154,7 +161,7 @@ static int populate_paths_from_revs(struct last_modified *lm)
 	}
 	clear_pathspec(&diffopt.pathspec);
 
-	return 0;
+	return ret;
 }
 
 static void last_modified_emit(struct last_modified *lm,
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
+'
+
 test_done

---
base-commit: d529f3a197364881746f558e5652f0236131eb86
change-id: 20260112-toon-last-modified-tree-fdd96b2feaf7

Re: [PATCH] last-modified: verify revision argument is a commit-ish

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> 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

Of course, the starting point last-modified needs to be something
that will yield series of commits when given to get_revision() ;-)

> Fix this error by verifying the parsed revision is peels to a
> commit-ish.

OK.

Re: [PATCH] last-modified: verify revision argument is a commit-ish

[Patrick Steinhardt]

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

[PATCH v2 0/3] Fix git-last-modified(1) bug triggered when passing a tree-ish

[Toon Claes]

Recently there was a bug reported[1] passing a tree OID triggers a BUG:

    $ git last-modified fb06ce04173d47aaaa498385621cba8b8dfd7584
    BUG: builtin/last-modified.c:456: paths remaining beyond boundary in last-modified
    [1]    690163 IOT instruction (core dumped)  git last-modified

    `fb06ce04173d47aaaa498385621cba8b8dfd7584` is the tree commit id of web_src. I
    suppose this should've returned a nice error message or blank output.

Fix this bug by checking the revision argument.

[1]: https://lore.kernel.org/git/03f96860-29fc-42a7-a220-c3ec65eb8516@codeberg.org/

---
Changes in v2:
- Prepend the change with a commit that modifies the error message
  when more than one revision is given.
- Prepend another commit that removes double error message.
- Add test to ensure the command works with annotated tags too.
- Link to v1: https://patch.msgid.link/20260112-toon-last-modified-tree-v1-1-ecbc78341f76@iotcl.com

---
Toon Claes (3):
      last-modified: rewrite error message when more than one revision given
      last-modified: remove double error message
      last-modified: verify revision argument is a commit-ish

 builtin/last-modified.c  | 19 ++++++++++++++-----
 t/t8020-last-modified.sh | 15 ++++++++++++++-
 2 files changed, 28 insertions(+), 6 deletions(-)

Range-diff versus v1:

1:  ebd05211ab ! 1:  70baa9b4eb last-modified: verify revision argument is a commit-ish
    @@ Metadata
     Author: Toon Claes <toon@iotcl.com>
     
      ## Commit message ##
    -    last-modified: verify revision argument is a commit-ish
    +    last-modified: rewrite error message when more than one revision given
     
    -    Passing a tree OID to git-last-modified(1) would trigger BUG behavior.
    +    When more than one revision is passed to the git-last-modified(1)
    +    command, this error message was printed:
     
    -        git last-modified HEAD^{tree}
    -        BUG: builtin/last-modified.c:456: paths remaining beyond boundary in last-modified
    +        error: last-modified can only operate on one tree at a time
     
    -    Fix this error by verifying the parsed revision is peels to a
    -    commit-ish.
    +    Calling these a "tree" is technically not correct. git-last-modified(1)
    +    expects revisions that peel to a commit.
     
    -    While at it, also fix a memory leak in populate_paths_from_revs().
    +    Rephrase the error message to:
    +
    +        error: last-modified can only operate on one revision at a time
    +
    +    While at it, also fix a memory leak that remained uncovered so far.
     
    -    Reported-by: Gusted <gusted@codeberg.org>
         Signed-off-by: Toon Claes <toon@iotcl.com>
     
      ## builtin/last-modified.c ##
    @@ builtin/last-modified.c: static int populate_paths_from_revs(struct last_modifie
     -		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"));
    -+			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;
    ++			ret = error(_("last-modified can only operate on one revision at a time"));
    ++			goto out;
     +		}
      
      		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
      			      &obj->item->oid, "", &diffopt);
    -@@ builtin/last-modified.c: static int populate_paths_from_revs(struct last_modified *lm)
    + 		diff_flush(&diffopt);
      	}
    ++
    ++out:
      	clear_pathspec(&diffopt.pathspec);
      
     -	return 0;
    @@ builtin/last-modified.c: static int populate_paths_from_revs(struct last_modifie
      static void last_modified_emit(struct last_modified *lm,
     
      ## t/t8020-last-modified.sh ##
    -@@ t/t8020-last-modified.sh: test_expect_success 'last-modified complains about unknown arguments' '
    - 	grep "unknown last-modified argument: --foo" err
    +@@ t/t8020-last-modified.sh: 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' '
    + 	test_must_fail git last-modified HEAD HEAD~1
      '
      
    -+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
    -+'
    -+
    - test_done
-:  ---------- > 2:  150c43580d last-modified: remove double error message
-:  ---------- > 3:  ab89cb1ef3 last-modified: verify revision argument is a commit-ish


---
base-commit: d529f3a197364881746f558e5652f0236131eb86
change-id: 20260112-toon-last-modified-tree-fdd96b2feaf7

[PATCH v2 1/3] last-modified: rewrite error message when more than one revision given

[Toon Claes]

When more than one revision is passed to the git-last-modified(1)
command, this error message was printed:

    error: last-modified can only operate on one tree at a time

Calling these a "tree" is technically not correct. git-last-modified(1)
expects revisions that peel to a commit.

Rephrase the error message to:

    error: last-modified can only operate on one revision at a time

While at it, also fix a memory leak that remained uncovered so far.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c  | 12 ++++++++----
 t/t8020-last-modified.sh |  2 +-
 2 files changed, 9 insertions(+), 5 deletions(-)

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
@@ -123,7 +123,7 @@ static void add_path_from_diff(struct diff_queue_struct *q,
 
 static int populate_paths_from_revs(struct last_modified *lm)
 {
-	int num_interesting = 0;
+	int num_interesting = 0, ret = 0;
 	struct diff_options diffopt;
 
 	/*
@@ -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"));
+			goto out;
+		}
 
 		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
 			      &obj->item->oid, "", &diffopt);
 		diff_flush(&diffopt);
 	}
+
+out:
 	clear_pathspec(&diffopt.pathspec);
 
-	return 0;
+	return ret;
 }
 
 static void last_modified_emit(struct last_modified *lm,
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' '
 	test_must_fail git last-modified HEAD HEAD~1
 '
 

-- 
2.52.0

Re: [PATCH v2 1/3] last-modified: rewrite error message when more than one revision given

[Patrick Steinhardt]

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

Re: [PATCH v2 1/3] last-modified: rewrite error message when more than one revision given

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

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

I was thinking about mentioning something like "commit-ish" instead, but
I felt "commit-ish" isn't a commonly used term toward end-users. Looking
at gitglossary(7), it says "revision" is a "synonym for commit". I'm
happy to change this message, but I'm not sure s/revision/commit-ish/ is
the best change for this.

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

Thanks.

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

Okay, I'll rework this one and the one adding in the other commit.

-- 
Cheers,
Toon

Re: [PATCH v2 1/3] last-modified: rewrite error message when more than one revision given

[Patrick Steinhardt]

On Thu, Jan 15, 2026 at 12:33:36PM +0100, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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?
> 
> I was thinking about mentioning something like "commit-ish" instead, but
> I felt "commit-ish" isn't a commonly used term toward end-users. Looking
> at gitglossary(7), it says "revision" is a "synonym for commit". I'm
> happy to change this message, but I'm not sure s/revision/commit-ish/ is
> the best change for this.

gitglossary(7) also defines commit-ish, but I guess you're right that
revision is the more common term. I'm a bit surprised that it's defined
to be a synonym for a commit, but oh, well.

Patrick

Re: [PATCH v2 1/3] last-modified: rewrite error message when more than one revision given

[Kristoffer Haugsbakk]

On Thu, Jan 15, 2026, at 12:54, Patrick Steinhardt wrote:
> On Thu, Jan 15, 2026 at 12:33:36PM +0100, Toon Claes wrote:
>>[snip]
>> I was thinking about mentioning something like "commit-ish" instead, but
>> I felt "commit-ish" isn't a commonly used term toward end-users. Looking
>> at gitglossary(7), it says "revision" is a "synonym for commit". I'm
>> happy to change this message, but I'm not sure s/revision/commit-ish/ is
>> the best change for this.
>
> gitglossary(7) also defines commit-ish, but I guess you're right that
> revision is the more common term. I'm a bit surprised that it's defined
> to be a synonym for a commit, but oh, well.

Surprised that “revision” is a synonym for commit? Why is that?

Re: [PATCH v2 1/3] last-modified: rewrite error message when more than one revision given

[Patrick Steinhardt]

On Thu, Jan 15, 2026 at 03:34:50PM +0100, Kristoffer Haugsbakk wrote:
> On Thu, Jan 15, 2026, at 12:54, Patrick Steinhardt wrote:
> > On Thu, Jan 15, 2026 at 12:33:36PM +0100, Toon Claes wrote:
> >>[snip]
> >> I was thinking about mentioning something like "commit-ish" instead, but
> >> I felt "commit-ish" isn't a commonly used term toward end-users. Looking
> >> at gitglossary(7), it says "revision" is a "synonym for commit". I'm
> >> happy to change this message, but I'm not sure s/revision/commit-ish/ is
> >> the best change for this.
> >
> > gitglossary(7) also defines commit-ish, but I guess you're right that
> > revision is the more common term. I'm a bit surprised that it's defined
> > to be a synonym for a commit, but oh, well.
> 
> Surprised that “revision” is a synonym for commit? Why is that?

Because in my mind a revision can resolve to any object type.
"<rev>^{<type>}" for example is a revision, but it can resolve for
example to a tree if you say "HEAD^{tree}". Still a revision, but
definitely does not resolve to a commit.

Also, it's confusing to conflate the way to name a commit with a commit
itself. "HEAD~10" is a revision, but taken by itself it's not a commit.
It's not even clear whether it resolves, so it feels sensible to me to
keep these two concepts separate from one another.

Patrick

Re: [PATCH v2 1/3] last-modified: rewrite error message when more than one revision given

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

>> Surprised that “revision” is a synonym for commit? Why is that?

I must admit, I was surprised too.

> Because in my mind a revision can resolve to any object type.
> "<rev>^{<type>}" for example is a revision, but it can resolve for
> example to a tree if you say "HEAD^{tree}". Still a revision, but
> definitely does not resolve to a commit.

Besides the discussion if "HEAD^{tree}" is a revision or not. Passing in
two revisions to git-last-modified(1) is not allowed. So I think the
error message I proposed makes sense.

> Also, it's confusing to conflate the way to name a commit with a commit
> itself. "HEAD~10" is a revision, but taken by itself it's not a commit.
> It's not even clear whether it resolves, so it feels sensible to me to
> keep these two concepts separate from one another.

-- 
Cheers,
Toon

Re: [PATCH v2 1/3] last-modified: rewrite error message when more than one revision given

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> Surprised that “revision” is a synonym for commit? Why is that?
>
> Because in my mind a revision can resolve to any object type.

Yup, in the early days of this mailing list (like in 2005 ;-), the
word "revision" was used more or less interchangeably with "object
name", but "a revision" was much more likely to refer to a commit
than "an object name".  The name of the file that implements one of
the more core-ish part of the system is "revision.c" and talks about
"revision traversal", which is mostly about following parent pointers
in commit DAG, but also follows into trees starting from commits.

> Also, it's confusing to conflate the way to name a commit with a commit
> itself. "HEAD~10" is a revision, but taken by itself it's not a commit.

I do not know about this.  If HEAD~10 does not resolve to anything,
it would not be a commit and it would not be a revision, either.

> It's not even clear whether it resolves, so it feels sensible to me to
> keep these two concepts separate from one another.
>
> Patrick

Re: [PATCH v2 1/3] last-modified: rewrite error message when more than one revision given

[Patrick Steinhardt]

On Fri, Jan 16, 2026 at 09:16:03AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> Surprised that “revision” is a synonym for commit? Why is that?
> >
> > Because in my mind a revision can resolve to any object type.
> 
> Yup, in the early days of this mailing list (like in 2005 ;-), the
> word "revision" was used more or less interchangeably with "object
> name", but "a revision" was much more likely to refer to a commit
> than "an object name". 

It's probably still much more likely that a revision refers to a commit
rather than anything else.

> The name of the file that implements one of the more core-ish part of
> the system is "revision.c" and talks about "revision traversal", which
> is mostly about following parent pointers in commit DAG, but also
> follows into trees starting from commits.

This discussion makes me wonder whether we should maybe update how we
define a "revision" in our glossary. One could take gitrevisions(1) as a
starting point:

    A revision typically, but not necessarily, names a commit object. It
    uses what is called an extended SHA-1 syntax.

We should probably get rid of "SHA-1" though. So maybe:

    A revision is used to refer to a specific object, typically a
    commit, using extended object name syntax. Refer to
    gitlink:gitrevisions[7] for more information.

> > Also, it's confusing to conflate the way to name a commit with a commit
> > itself. "HEAD~10" is a revision, but taken by itself it's not a commit.
> 
> I do not know about this.  If HEAD~10 does not resolve to anything,
> it would not be a commit and it would not be a revision, either.

I guess things are getting philosophical here :) I rather see it like a
pointer: a pointer is still a pointer even if it doesn't point to
anything.

Patrick

Re: [PATCH v2 1/3] last-modified: rewrite error message when more than one revision given

[Kristoffer Haugsbakk]

On Fri, Jan 16, 2026, at 08:09, Patrick Steinhardt wrote:
> On Thu, Jan 15, 2026 at 03:34:50PM +0100, Kristoffer Haugsbakk wrote:
>> On Thu, Jan 15, 2026, at 12:54, Patrick Steinhardt wrote:
>> > On Thu, Jan 15, 2026 at 12:33:36PM +0100, Toon Claes wrote:
>> >>[snip]
>> >> I was thinking about mentioning something like "commit-ish" instead, but
>> >> I felt "commit-ish" isn't a commonly used term toward end-users. Looking
>> >> at gitglossary(7), it says "revision" is a "synonym for commit". I'm
>> >> happy to change this message, but I'm not sure s/revision/commit-ish/ is
>> >> the best change for this.
>> >
>> > gitglossary(7) also defines commit-ish, but I guess you're right that
>> > revision is the more common term. I'm a bit surprised that it's defined
>> > to be a synonym for a commit, but oh, well.
>>
>> Surprised that “revision” is a synonym for commit? Why is that?
>
> Because in my mind a revision can resolve to any object type.
> "<rev>^{<type>}" for example is a revision, but it can resolve for
> example to a tree if you say "HEAD^{tree}". Still a revision, but
> definitely does not resolve to a commit.
>
> Also, it's confusing to conflate the way to name a commit with a commit
> itself. "HEAD~10" is a revision, but taken by itself it's not a commit.
> It's not even clear whether it resolves, so it feels sensible to me to
> keep these two concepts separate from one another.

Maybe I didn’t read the context well enough. I always read “revision” as
a generic word like “commit”. Not as the gitrevisions(7) expressions.

Re: [PATCH v2 1/3] last-modified: rewrite error message when more than one revision given

[Kristoffer Haugsbakk]

On Thu, Jan 15, 2026, at 12:33, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>> On Wed, Jan 14, 2026 at 11:24:45AM +0100, Toon Claes wrote:
>>>[snip]
>>> -		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?
>
> I was thinking about mentioning something like "commit-ish" instead, but
> I felt "commit-ish" isn't a commonly used term toward end-users. Looking
> at gitglossary(7), it says "revision" is a "synonym for commit". I'm
> happy to change this message, but I'm not sure s/revision/commit-ish/ is
> the best change for this.

I just stumbled upon the “IDENTIFIER TERMINOLOGY” section of
git(1). “commit-ish” is there.

>[snip]

[PATCH v2 2/3] last-modified: remove double error message

[Toon Claes]

When the user passes two revisions, they get the following output:

    $ git last-modified HEAD HEAD~
    error: last-modified can only operate on one revision at a time
    error: unable to setup last-modified

The error message about "unable to setup" is not very informative,
remove it.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 06e3f79aec..0df85be318 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -495,7 +495,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
 	lm->rev.bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
 
 	if (populate_paths_from_revs(lm) < 0)
-		return error(_("unable to setup last-modified"));
+		return -1;
 
 	CALLOC_ARRAY(lm->all_paths, hashmap_get_size(&lm->paths));
 	lm->all_paths_nr = 0;

-- 
2.52.0

Re: [PATCH v2 2/3] last-modified: remove double error message

[Patrick Steinhardt]

On Wed, Jan 14, 2026 at 11:24:46AM +0100, Toon Claes wrote:
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index 06e3f79aec..0df85be318 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -495,7 +495,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
>  	lm->rev.bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
>  
>  	if (populate_paths_from_revs(lm) < 0)
> -		return error(_("unable to setup last-modified"));
> +		return -1;

Makes sense. There's only one error condition in the function, and that
error condition already prints an error message.

Patrick

[PATCH v2 3/3] last-modified: verify revision argument is a commit-ish

[Toon Claes]

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 peels to a commit-ish.

While at it, also fix a memory leak in populate_paths_from_revs().

Reported-by: Gusted <gusted@codeberg.org>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c  |  5 +++++
 t/t8020-last-modified.sh | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 0df85be318..5366cedd0f 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -150,6 +150,11 @@ static int populate_paths_from_revs(struct last_modified *lm)
 			goto out;
 		}
 
+		if (!repo_peel_to_type(lm->rev.repo, obj->path, 0, obj->item, OBJ_COMMIT)) {
+			ret = error(_("revision argument is not a commit-ish"));
+			goto out;
+		}
+
 		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
 			      &obj->item->oid, "", &diffopt);
 		diff_flush(&diffopt);
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
@@ -8,6 +8,7 @@ test_expect_success 'setup' '
 	test_commit 1 file &&
 	mkdir a &&
 	test_commit 2 a/file &&
+	git tag -mA t2 2 &&
 	mkdir a/b &&
 	test_commit 3 a/b/file
 '
@@ -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
@@ -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
+'
+
 test_done

-- 
2.52.0

Re: [PATCH v2 3/3] last-modified: verify revision argument is a commit-ish

[Patrick Steinhardt]

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

Re: [PATCH v2 3/3] last-modified: verify revision argument is a commit-ish

[Kristoffer Haugsbakk]

On Wed, Jan 14, 2026, at 11:24, 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 peels to a commit-ish.

Nitpick: “peels to commit-ish” = commit-ish so this is a bit
redundant. Either just “commit-ish” or “peels to commit” would be
enough.

s/verifying the parsed revision peels to a commit-ish./verifying that the parsed revision is a commit-ish./

>
> While at it, also fix a memory leak in populate_paths_from_revs().

(Whether or not this is a stale sentence (see Patricks’)) Why not a
separate commit for fixing a memory leak?

>
> Reported-by: Gusted <gusted@codeberg.org>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
>[snip]

Re: [PATCH v2 3/3] last-modified: verify revision argument is a commit-ish

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

> On Wed, Jan 14, 2026, at 11:24, 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 peels to a commit-ish.
>
> Nitpick: “peels to commit-ish” = commit-ish so this is a bit
> redundant. Either just “commit-ish” or “peels to commit” would be
> enough.

Great eyes ;-)

>
> s/verifying the parsed revision peels to a commit-ish./verifying that the parsed revision is a commit-ish./
>
>>
>> While at it, also fix a memory leak in populate_paths_from_revs().
>
> (Whether or not this is a stale sentence (see Patricks’)) Why not a
> separate commit for fixing a memory leak?
>
>>
>> Reported-by: Gusted <gusted@codeberg.org>
>> Signed-off-by: Toon Claes <toon@iotcl.com>
>> ---
>>[snip]

Re: [PATCH v2 3/3] last-modified: verify revision argument is a commit-ish

[Toon Claes]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

> On Wed, Jan 14, 2026, at 11:24, 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 peels to a commit-ish.
>
> Nitpick: “peels to commit-ish” = commit-ish so this is a bit
> redundant. Either just “commit-ish” or “peels to commit” would be
> enough.
>
> s/verifying the parsed revision peels to a commit-ish./verifying that
> the parsed revision is a commit-ish./

Thanks! I've addressed this in v3.

>> While at it, also fix a memory leak in populate_paths_from_revs().
>
> (Whether or not this is a stale sentence (see Patricks’)) Why not a
> separate commit for fixing a memory leak?

Yeah, it's better to do that separately, so I've done so now.

-- 
Cheers,
Toon

[PATCH v3 0/4] Fix git-last-modified(1) bug triggered when passing a tree-ish

[Toon Claes]

Recently there was a bug reported[1] passing a tree OID triggers a BUG:

    $ git last-modified fb06ce04173d47aaaa498385621cba8b8dfd7584
    BUG: builtin/last-modified.c:456: paths remaining beyond boundary in last-modified
    [1]    690163 IOT instruction (core dumped)  git last-modified

    `fb06ce04173d47aaaa498385621cba8b8dfd7584` is the tree commit id of web_src. I
    suppose this should've returned a nice error message or blank output.

Fix this bug by checking the revision argument.

[1]: https://lore.kernel.org/git/03f96860-29fc-42a7-a220-c3ec65eb8516@codeberg.org/

---
Changes in v3:
- Split the change to plug the leak into a separate commit.
- Small changes to commit messages.
- Link to v2: https://patch.msgid.link/20260114-toon-last-modified-tree-v2-0-ba3b1860898f@iotcl.com

Changes in v2:
- Prepend the change with a commit that modifies the error message
  when more than one revision is given.
- Prepend another commit that removes double error message.
- Add test to ensure the command works with annotated tags too.
- Link to v1: https://patch.msgid.link/20260112-toon-last-modified-tree-v1-1-ecbc78341f76@iotcl.com

---
Toon Claes (4):
      last-modified: rewrite error message when more than one revision given
      last-modified: fix memory leak when more than one revision is given
      last-modified: remove double error message
      last-modified: verify revision argument is a commit-ish

 builtin/last-modified.c  | 19 ++++++++++++++-----
 t/t8020-last-modified.sh | 15 ++++++++++++++-
 2 files changed, 28 insertions(+), 6 deletions(-)

Range-diff versus v2:

-:  ---------- > 1:  053c119ab1 last-modified: rewrite error message when more than one revision given
1:  5c9936500b ! 2:  827b1500fe last-modified: rewrite error message when more than one revision given
    @@ Metadata
     Author: Toon Claes <toon@iotcl.com>
     
      ## Commit message ##
    -    last-modified: rewrite error message when more than one revision given
    +    last-modified: fix memory leak when more than one revision is given
     
    -    When more than one revision is passed to the git-last-modified(1)
    -    command, this error message was printed:
    -
    -        error: last-modified can only operate on one tree at a time
    -
    -    Calling these a "tree" is technically not correct. git-last-modified(1)
    -    expects revisions that peel to a commit.
    -
    -    Rephrase the error message to:
    -
    -        error: last-modified can only operate on one revision at a time
    -
    -    While at it, also fix a memory leak that remained uncovered so far.
    +    When more than one revision is given, the function
    +    populate_paths_from_revs() leaks a `struct pathspec`. Plug it.
     
         Signed-off-by: Toon Claes <toon@iotcl.com>
     
    @@ builtin/last-modified.c: static int populate_paths_from_revs(struct last_modifie
      			continue;
      
     -		if (num_interesting++)
    --			return error(_("last-modified can only operate on one tree at a time"));
    +-			return error(_("last-modified can only operate on one revision at a time"));
     +		if (num_interesting++) {
     +			ret = error(_("last-modified can only operate on one revision at a time"));
     +			goto out;
    @@ builtin/last-modified.c: static int populate_paths_from_revs(struct last_modifie
      }
      
      static void last_modified_emit(struct last_modified *lm,
    -
    - ## t/t8020-last-modified.sh ##
    -@@ t/t8020-last-modified.sh: 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' '
    - 	test_must_fail git last-modified HEAD HEAD~1
    - '
    - 
2:  5c964488fd = 3:  7748574724 last-modified: remove double error message
3:  cb6ff40853 ! 4:  6846722750 last-modified: verify revision argument is a commit-ish
    @@ Commit message
             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().
    +    Fix this error by verifying the parsed revision is a commit-ish.
     
         Reported-by: Gusted <gusted@codeberg.org>
         Signed-off-by: Toon Claes <toon@iotcl.com>


---
base-commit: 7264e61d87e58b9d0f5e6424c47c11e9657dfb75
change-id: 20260112-toon-last-modified-tree-fdd96b2feaf7

[PATCH v3 1/4] last-modified: rewrite error message when more than one revision given

[Toon Claes]

When more than one revision is passed to the git-last-modified(1)
command, this error message was printed:

    error: last-modified can only operate on one tree at a time

Calling these a "tree" is technically not correct. git-last-modified(1)
expects revisions that peel to a commit.

Rephrase the error message to:

    error: last-modified can only operate on one revision at a time

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c  | 2 +-
 t/t8020-last-modified.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index c80f0535f6..7d95244e3f 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -146,7 +146,7 @@ static int populate_paths_from_revs(struct last_modified *lm)
 			continue;
 
 		if (num_interesting++)
-			return error(_("last-modified can only operate on one tree at a time"));
+			return error(_("last-modified can only operate on one revision at a time"));
 
 		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..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' '
 	test_must_fail git last-modified HEAD HEAD~1
 '
 

-- 
2.52.0

Re: [PATCH v3 1/4] last-modified: rewrite error message when more than one revision given

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> -test_expect_success 'cannot run last-modified on two trees' '
> +test_expect_success 'cannot run last-modified on two revision' '

Not "revisions"?

>  	test_must_fail git last-modified HEAD HEAD~1
>  '

[PATCH v3 2/4] last-modified: fix memory leak when more than one revision is given

[Toon Claes]

When more than one revision is given, the function
populate_paths_from_revs() leaks a `struct pathspec`. Plug it.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 7d95244e3f..06e3f79aec 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -123,7 +123,7 @@ static void add_path_from_diff(struct diff_queue_struct *q,
 
 static int populate_paths_from_revs(struct last_modified *lm)
 {
-	int num_interesting = 0;
+	int num_interesting = 0, ret = 0;
 	struct diff_options diffopt;
 
 	/*
@@ -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 revision at a time"));
+		if (num_interesting++) {
+			ret = error(_("last-modified can only operate on one revision at a time"));
+			goto out;
+		}
 
 		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
 			      &obj->item->oid, "", &diffopt);
 		diff_flush(&diffopt);
 	}
+
+out:
 	clear_pathspec(&diffopt.pathspec);
 
-	return 0;
+	return ret;
 }
 
 static void last_modified_emit(struct last_modified *lm,

-- 
2.52.0

[PATCH v3 3/4] last-modified: remove double error message

[Toon Claes]

When the user passes two revisions, they get the following output:

    $ git last-modified HEAD HEAD~
    error: last-modified can only operate on one revision at a time
    error: unable to setup last-modified

The error message about "unable to setup" is not very informative,
remove it.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 06e3f79aec..0df85be318 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -495,7 +495,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
 	lm->rev.bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
 
 	if (populate_paths_from_revs(lm) < 0)
-		return error(_("unable to setup last-modified"));
+		return -1;
 
 	CALLOC_ARRAY(lm->all_paths, hashmap_get_size(&lm->paths));
 	lm->all_paths_nr = 0;

-- 
2.52.0

Re: [PATCH v3 3/4] last-modified: remove double error message

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> When the user passes two revisions, they get the following output:
>
>     $ git last-modified HEAD HEAD~
>     error: last-modified can only operate on one revision at a time
>     error: unable to setup last-modified
>
> The error message about "unable to setup" is not very informative,
> remove it.
>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
>  builtin/last-modified.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index 06e3f79aec..0df85be318 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -495,7 +495,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
>  	lm->rev.bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
>  
>  	if (populate_paths_from_revs(lm) < 0)
> -		return error(_("unable to setup last-modified"));
> +		return -1;

It makes perfect sense to keep the more detailed error that
immediately leads to a more correct action and take out the more
generic "we failed" one.  Nice.

[PATCH v3 4/4] last-modified: verify revision argument is a commit-ish

[Toon Claes]

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 a commit-ish.

Reported-by: Gusted <gusted@codeberg.org>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c  |  5 +++++
 t/t8020-last-modified.sh | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 0df85be318..5366cedd0f 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -150,6 +150,11 @@ static int populate_paths_from_revs(struct last_modified *lm)
 			goto out;
 		}
 
+		if (!repo_peel_to_type(lm->rev.repo, obj->path, 0, obj->item, OBJ_COMMIT)) {
+			ret = error(_("revision argument is not a commit-ish"));
+			goto out;
+		}
+
 		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
 			      &obj->item->oid, "", &diffopt);
 		diff_flush(&diffopt);
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
@@ -8,6 +8,7 @@ test_expect_success 'setup' '
 	test_commit 1 file &&
 	mkdir a &&
 	test_commit 2 a/file &&
+	git tag -mA t2 2 &&
 	mkdir a/b &&
 	test_commit 3 a/b/file
 '
@@ -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
@@ -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
+'
+
 test_done

-- 
2.52.0

Re: [PATCH v3 4/4] last-modified: verify revision argument is a commit-ish

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> 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 a commit-ish.
>
> Reported-by: Gusted <gusted@codeberg.org>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
>  builtin/last-modified.c  |  5 +++++
>  t/t8020-last-modified.sh | 13 +++++++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index 0df85be318..5366cedd0f 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -150,6 +150,11 @@ static int populate_paths_from_revs(struct last_modified *lm)
>  			goto out;
>  		}
>  
> +		if (!repo_peel_to_type(lm->rev.repo, obj->path, 0, obj->item, OBJ_COMMIT)) {
> +			ret = error(_("revision argument is not a commit-ish"));
> +			goto out;
> +		}

This will do, at least for now, but I tend to prefer to say what the
user gave us when we expected a commit, e.g., "commit expected, got
a tree".

[PATCH v4 0/4] Fix git-last-modified(1) bug triggered when passing a tree-ish

[Toon Claes]

Recently there was a bug reported[1] passing a tree OID triggers a BUG:

    $ git last-modified fb06ce04173d47aaaa498385621cba8b8dfd7584
    BUG: builtin/last-modified.c:456: paths remaining beyond boundary in last-modified
    [1]    690163 IOT instruction (core dumped)  git last-modified

    `fb06ce04173d47aaaa498385621cba8b8dfd7584` is the tree commit id of web_src. I
    suppose this should've returned a nice error message or blank output.

Fix this bug by checking the revision argument.

[1]: https://lore.kernel.org/git/03f96860-29fc-42a7-a220-c3ec65eb8516@codeberg.org/

---
Changes in v4:
- Extend the error message when revision is not a tree
- Extend the test to verify the error message when more than one
  revision is given.
- Link to v3: https://patch.msgid.link/20260116-toon-last-modified-tree-v3-0-e6ade4dc35ab@iotcl.com

Changes in v3:
- Split the change to plug the leak into a separate commit.
- Small changes to commit messages.
- Link to v2: https://patch.msgid.link/20260114-toon-last-modified-tree-v2-0-ba3b1860898f@iotcl.com

Changes in v2:
- Prepend the change with a commit that modifies the error message
  when more than one revision is given.
- Prepend another commit that removes double error message.
- Add test to ensure the command works with annotated tags too.
- Link to v1: https://patch.msgid.link/20260112-toon-last-modified-tree-v1-1-ecbc78341f76@iotcl.com

---
Toon Claes (4):
      last-modified: rewrite error message when more than one revision given
      last-modified: fix memory leak when more than one revision is given
      last-modified: remove double error message
      last-modified: verify revision argument is a commit-ish

 builtin/last-modified.c  | 19 ++++++++++++++-----
 t/t8020-last-modified.sh | 24 +++++++++++++++++++-----
 2 files changed, 33 insertions(+), 10 deletions(-)

Range-diff versus v3:

1:  8786f5d6a4 ! 1:  1bd4bb7cb8 last-modified: rewrite error message when more than one revision given
    @@ Commit message
     
             error: last-modified can only operate on one revision at a time
     
    +    While at it, ensure modify the test to ensure the correct error message
    +    is printed.
    +
         Signed-off-by: Toon Claes <toon@iotcl.com>
     
      ## builtin/last-modified.c ##
    @@ t/t8020-last-modified.sh: test_expect_success 'setup' '
      '
      
     -test_expect_success 'cannot run last-modified on two trees' '
    +-	test_must_fail git last-modified HEAD HEAD~1
    +-'
    +-
    + check_last_modified() {
    + 	local indir= &&
    + 	while test $# != 0
    +@@ t/t8020-last-modified.sh: test_expect_success 'last-modified merge undoes changes' '
    + 	EOF
    + '
    + 
     +test_expect_success 'cannot run last-modified on two revision' '
    - 	test_must_fail git last-modified HEAD HEAD~1
    ++	test_must_fail git last-modified HEAD HEAD~1 2>err &&
    ++	test_grep "last-modified can only operate on one revision at a time" err
    ++'
    ++
    + test_expect_success 'last-modified complains about unknown arguments' '
    + 	test_must_fail git last-modified --foo 2>err &&
    +-	grep "unknown last-modified argument: --foo" err
    ++	test_grep "unknown last-modified argument: --foo" err
      '
      
    + test_done
2:  78ec93a9e2 = 2:  ed1bd102a2 last-modified: fix memory leak when more than one revision is given
3:  05b4424289 = 3:  7a6e587da1 last-modified: remove double error message
4:  00e29cd6a1 ! 4:  fee3aa92a9 last-modified: verify revision argument is a commit-ish
    @@ builtin/last-modified.c: static int populate_paths_from_revs(struct last_modifie
      		}
      
     +		if (!repo_peel_to_type(lm->rev.repo, obj->path, 0, obj->item, OBJ_COMMIT)) {
    -+			ret = error(_("revision argument is not a commit-ish"));
    ++			ret = error(_("revision argument '%s' is a %s, not a commit-ish"), obj->name, type_name(obj->item->type));
     +			goto out;
     +		}
     +
    @@ t/t8020-last-modified.sh: test_expect_success 'last-modified recursive' '
      	check_last_modified -r -t <<-\EOF
      	3 a/b
     @@ t/t8020-last-modified.sh: test_expect_success 'last-modified complains about unknown arguments' '
    - 	grep "unknown last-modified argument: --foo" err
    + 	test_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
    ++	grep "revision argument '"'"'HEAD^{tree}'"'"' is a tree, not a commit-ish" err
     +'
     +
      test_done


---
base-commit: 7264e61d87e58b9d0f5e6424c47c11e9657dfb75
change-id: 20260112-toon-last-modified-tree-fdd96b2feaf7

[PATCH v4 1/4] last-modified: rewrite error message when more than one revision given

[Toon Claes]

When more than one revision is passed to the git-last-modified(1)
command, this error message was printed:

    error: last-modified can only operate on one tree at a time

Calling these a "tree" is technically not correct. git-last-modified(1)
expects revisions that peel to a commit.

Rephrase the error message to:

    error: last-modified can only operate on one revision at a time

While at it, ensure modify the test to ensure the correct error message
is printed.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c  |  2 +-
 t/t8020-last-modified.sh | 11 ++++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index c80f0535f6..7d95244e3f 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -146,7 +146,7 @@ static int populate_paths_from_revs(struct last_modified *lm)
 			continue;
 
 		if (num_interesting++)
-			return error(_("last-modified can only operate on one tree at a time"));
+			return error(_("last-modified can only operate on one revision at a time"));
 
 		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..91901eed58 100755
--- a/t/t8020-last-modified.sh
+++ b/t/t8020-last-modified.sh
@@ -12,10 +12,6 @@ test_expect_success 'setup' '
 	test_commit 3 a/b/file
 '
 
-test_expect_success 'cannot run last-modified on two trees' '
-	test_must_fail git last-modified HEAD HEAD~1
-'
-
 check_last_modified() {
 	local indir= &&
 	while test $# != 0
@@ -230,9 +226,14 @@ test_expect_success 'last-modified merge undoes changes' '
 	EOF
 '
 
+test_expect_success 'cannot run last-modified on two revision' '
+	test_must_fail git last-modified HEAD HEAD~1 2>err &&
+	test_grep "last-modified can only operate on one revision at a time" err
+'
+
 test_expect_success 'last-modified complains about unknown arguments' '
 	test_must_fail git last-modified --foo 2>err &&
-	grep "unknown last-modified argument: --foo" err
+	test_grep "unknown last-modified argument: --foo" err
 '
 
 test_done

-- 
2.52.0

Re: [PATCH v4 1/4] last-modified: rewrite error message when more than one revision given

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> When more than one revision is passed to the git-last-modified(1)
> command, this error message was printed:
>
>     error: last-modified can only operate on one tree at a time
>
> Calling these a "tree" is technically not correct. git-last-modified(1)
> expects revisions that peel to a commit.
>
> Rephrase the error message to:
>
>     error: last-modified can only operate on one revision at a time
>
> While at it, ensure modify the test to ensure the correct error message
> is printed.

"ensure modify" -> "modify".

>  		if (num_interesting++)
> -			return error(_("last-modified can only operate on one tree at a time"));
> +			return error(_("last-modified can only operate on one revision at a time"));

I recall we earlier had discussions on "commit" vs "revision", but
was the conclusion that 'revision' is the appropriate term here?  I
somehow feel that it is sufficient to insist on "commit" (not
"commit-ish").  The way you are allowed to give that commit might be
more lenient and you may be able to pass a tag that points at a
commit, but that does not change the fact that the "last-modified"
command can only operate on one commit at a time, does it?

> +test_expect_success 'cannot run last-modified on two revision' '

"two revision" -> "two revisions".

> +	test_must_fail git last-modified HEAD HEAD~1 2>err &&
> +	test_grep "last-modified can only operate on one revision at a time" err
> +'
> +
>  test_expect_success 'last-modified complains about unknown arguments' '
>  	test_must_fail git last-modified --foo 2>err &&
> -	grep "unknown last-modified argument: --foo" err
> +	test_grep "unknown last-modified argument: --foo" err
>  '
>  
>  test_done

[PATCH v4 2/4] last-modified: fix memory leak when more than one revision is given

[Toon Claes]

When more than one revision is given, the function
populate_paths_from_revs() leaks a `struct pathspec`. Plug it.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 7d95244e3f..06e3f79aec 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -123,7 +123,7 @@ static void add_path_from_diff(struct diff_queue_struct *q,
 
 static int populate_paths_from_revs(struct last_modified *lm)
 {
-	int num_interesting = 0;
+	int num_interesting = 0, ret = 0;
 	struct diff_options diffopt;
 
 	/*
@@ -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 revision at a time"));
+		if (num_interesting++) {
+			ret = error(_("last-modified can only operate on one revision at a time"));
+			goto out;
+		}
 
 		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
 			      &obj->item->oid, "", &diffopt);
 		diff_flush(&diffopt);
 	}
+
+out:
 	clear_pathspec(&diffopt.pathspec);
 
-	return 0;
+	return ret;
 }
 
 static void last_modified_emit(struct last_modified *lm,

-- 
2.52.0

Re: [PATCH v4 2/4] last-modified: fix memory leak when more than one revision is given

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> When more than one revision is given, the function
> populate_paths_from_revs() leaks a `struct pathspec`. Plug it.

Makes sense.

>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
>  builtin/last-modified.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index 7d95244e3f..06e3f79aec 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -123,7 +123,7 @@ static void add_path_from_diff(struct diff_queue_struct *q,
>  
>  static int populate_paths_from_revs(struct last_modified *lm)
>  {
> -	int num_interesting = 0;
> +	int num_interesting = 0, ret = 0;
>  	struct diff_options diffopt;
>  
>  	/*
> @@ -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 revision at a time"));
> +		if (num_interesting++) {
> +			ret = error(_("last-modified can only operate on one revision at a time"));
> +			goto out;
> +		}
>  
>  		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
>  			      &obj->item->oid, "", &diffopt);
>  		diff_flush(&diffopt);
>  	}
> +
> +out:
>  	clear_pathspec(&diffopt.pathspec);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static void last_modified_emit(struct last_modified *lm,

[PATCH v4 3/4] last-modified: remove double error message

[Toon Claes]

When the user passes two revisions, they get the following output:

    $ git last-modified HEAD HEAD~
    error: last-modified can only operate on one revision at a time
    error: unable to setup last-modified

The error message about "unable to setup" is not very informative,
remove it.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 06e3f79aec..0df85be318 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -495,7 +495,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
 	lm->rev.bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
 
 	if (populate_paths_from_revs(lm) < 0)
-		return error(_("unable to setup last-modified"));
+		return -1;
 
 	CALLOC_ARRAY(lm->all_paths, hashmap_get_size(&lm->paths));
 	lm->all_paths_nr = 0;

-- 
2.52.0

Re: [PATCH v4 3/4] last-modified: remove double error message

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> When the user passes two revisions, they get the following output:
>
>     $ git last-modified HEAD HEAD~
>     error: last-modified can only operate on one revision at a time
>     error: unable to setup last-modified
>
> The error message about "unable to setup" is not very informative,
> remove it.

We can see that an error message is always given when
populate_paths_from_revs() returns negative, so this change is safe
(i.e., if there is a code path in the function that returns negative
without giving any message, this change will lead to a silent
failure in such a code path).  Looking good.

Thanks.

> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
>  builtin/last-modified.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index 06e3f79aec..0df85be318 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -495,7 +495,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
>  	lm->rev.bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
>  
>  	if (populate_paths_from_revs(lm) < 0)
> -		return error(_("unable to setup last-modified"));
> +		return -1;
>  
>  	CALLOC_ARRAY(lm->all_paths, hashmap_get_size(&lm->paths));
>  	lm->all_paths_nr = 0;

[PATCH v4 4/4] last-modified: verify revision argument is a commit-ish

[Toon Claes]

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 a commit-ish.

Reported-by: Gusted <gusted@codeberg.org>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c  |  5 +++++
 t/t8020-last-modified.sh | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 0df85be318..686cb258bb 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -150,6 +150,11 @@ static int populate_paths_from_revs(struct last_modified *lm)
 			goto out;
 		}
 
+		if (!repo_peel_to_type(lm->rev.repo, obj->path, 0, obj->item, OBJ_COMMIT)) {
+			ret = error(_("revision argument '%s' is a %s, not a commit-ish"), obj->name, type_name(obj->item->type));
+			goto out;
+		}
+
 		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
 			      &obj->item->oid, "", &diffopt);
 		diff_flush(&diffopt);
diff --git a/t/t8020-last-modified.sh b/t/t8020-last-modified.sh
index 91901eed58..c88ec1854f 100755
--- a/t/t8020-last-modified.sh
+++ b/t/t8020-last-modified.sh
@@ -8,6 +8,7 @@ test_expect_success 'setup' '
 	test_commit 1 file &&
 	mkdir a &&
 	test_commit 2 a/file &&
+	git tag -mA t2 2 &&
 	mkdir a/b &&
 	test_commit 3 a/b/file
 '
@@ -51,6 +52,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
@@ -236,4 +244,9 @@ test_expect_success 'last-modified complains about unknown arguments' '
 	test_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 '"'"'HEAD^{tree}'"'"' is a tree, not a commit-ish" err
+'
+
 test_done

-- 
2.52.0

Re: [PATCH v4 4/4] last-modified: verify revision argument is a commit-ish

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> 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 a commit-ish.

Is it "the parsed revision", or what the command line argument given
to the program?  I am wondering if it is easier to understand to
rephrase this to something like "... by ensuring that the given
revision peels to a commit".

> +		if (!repo_peel_to_type(lm->rev.repo, obj->path, 0, obj->item, OBJ_COMMIT)) {
> +			ret = error(_("revision argument '%s' is a %s, not a commit-ish"), obj->name, type_name(obj->item->type));
> +			goto out;
> +		}
> +
>  		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
>  			      &obj->item->oid, "", &diffopt);
>  		diff_flush(&diffopt);

It is somewhat unsatisfying that the result of peeling is discarded
and diff_tree_oid() is forced to peel it down to tree again, but I
do not think of a better way offhand.

Re: [PATCH v4 4/4] last-modified: verify revision argument is a commit-ish

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> +		if (!repo_peel_to_type(lm->rev.repo, obj->path, 0, obj->item, OBJ_COMMIT)) {
> +			ret = error(_("revision argument '%s' is a %s, not a commit-ish"), obj->name, type_name(obj->item->type));
> +			goto out;
> +		}
> +

I do not use l10n version, but I am not sure how localization should
work with this message.

    _("revision argument '%s' is a %s, not a commit-ish")

There is no way type_name(obj->item->type) would be localized in
this code (after all, it is used in format_object_header() that is
the helper function to prepare the contents of the loose object
file).  Yet, it is tempting to translate "commit-ish" for those
preparing the .po files, which would lead to mixture of C-locale
'tree' and end-user-locale _("commit-ish").

I am wondering if we want to avoid this mixture by forcing the
C-locale for both, i.e.,

    error(_("revision argument '%s' is a %s, not a %s"),
	  obj->name, type_name(obj->item->type), 'commit-ish')

We can leave it as-is and polish the error messages later, of
course, but I am curious what the best practice is.

Thanks.

[PATCH v5 0/4] Fix git-last-modified(1) bug triggered when passing a tree-ish

[Toon Claes]

Recently there was a bug reported[1] passing a tree OID triggers a BUG:

    $ git last-modified fb06ce04173d47aaaa498385621cba8b8dfd7584
    BUG: builtin/last-modified.c:456: paths remaining beyond boundary in last-modified
    [1]    690163 IOT instruction (core dumped)  git last-modified

    `fb06ce04173d47aaaa498385621cba8b8dfd7584` is the tree commit id of web_src. I
    suppose this should've returned a nice error message or blank output.

Fix this bug by checking the revision argument.

[1]: https://lore.kernel.org/git/03f96860-29fc-42a7-a220-c3ec65eb8516@codeberg.org/

---
Changes in v5:
- Use 'commit' instead of 'revision'.
- Small typo fixes.
- Link to v4: https://patch.msgid.link/20260123-toon-last-modified-tree-v4-0-86bf97bad4e2@iotcl.com

Changes in v4:
- Extend the error message when revision is not a tree
- Extend the test to verify the error message when more than one
  revision is given.
- Link to v3: https://patch.msgid.link/20260116-toon-last-modified-tree-v3-0-e6ade4dc35ab@iotcl.com

Changes in v3:
- Split the change to plug the leak into a separate commit.
- Small changes to commit messages.
- Link to v2: https://patch.msgid.link/20260114-toon-last-modified-tree-v2-0-ba3b1860898f@iotcl.com

Changes in v2:
- Prepend the change with a commit that modifies the error message
  when more than one revision is given.
- Prepend another commit that removes double error message.
- Add test to ensure the command works with annotated tags too.
- Link to v1: https://patch.msgid.link/20260112-toon-last-modified-tree-v1-1-ecbc78341f76@iotcl.com

---
Toon Claes (4):
      last-modified: rewrite error message when more than one commit given
      last-modified: fix memory leak when more than one commit is given
      last-modified: remove double error message
      last-modified: verify revision argument is a commit-ish

 builtin/last-modified.c  | 19 ++++++++++++++-----
 t/t8020-last-modified.sh | 24 +++++++++++++++++++-----
 2 files changed, 33 insertions(+), 10 deletions(-)

Range-diff versus v4:

1:  497901f0ab ! 1:  2107d672e9 last-modified: rewrite error message when more than one revision given
    @@ Metadata
     Author: Toon Claes <toon@iotcl.com>
     
      ## Commit message ##
    -    last-modified: rewrite error message when more than one revision given
    +    last-modified: rewrite error message when more than one commit given
     
    -    When more than one revision is passed to the git-last-modified(1)
    -    command, this error message was printed:
    +    When more than one commit is passed to the git-last-modified(1) command,
    +    this error message was printed:
     
             error: last-modified can only operate on one tree at a time
     
    @@ Commit message
     
         Rephrase the error message to:
     
    -        error: last-modified can only operate on one revision at a time
    +        error: last-modified can only operate on one commit at a time
     
    -    While at it, ensure modify the test to ensure the correct error message
    -    is printed.
    +    While at it, modify the test to ensure the correct error message is
    +    printed.
     
         Signed-off-by: Toon Claes <toon@iotcl.com>
     
    @@ builtin/last-modified.c: static int populate_paths_from_revs(struct last_modifie
      
      		if (num_interesting++)
     -			return error(_("last-modified can only operate on one tree at a time"));
    -+			return error(_("last-modified can only operate on one revision at a time"));
    ++			return error(_("last-modified can only operate on one commit at a time"));
      
      		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
      			      &obj->item->oid, "", &diffopt);
    @@ t/t8020-last-modified.sh: test_expect_success 'last-modified merge undoes change
      	EOF
      '
      
    -+test_expect_success 'cannot run last-modified on two revision' '
    ++test_expect_success 'cannot run last-modified on two commits' '
     +	test_must_fail git last-modified HEAD HEAD~1 2>err &&
    -+	test_grep "last-modified can only operate on one revision at a time" err
    ++	test_grep "last-modified can only operate on one commit at a time" err
     +'
     +
      test_expect_success 'last-modified complains about unknown arguments' '
2:  1316ca90b4 ! 2:  dbabde6b54 last-modified: fix memory leak when more than one revision is given
    @@ Metadata
     Author: Toon Claes <toon@iotcl.com>
     
      ## Commit message ##
    -    last-modified: fix memory leak when more than one revision is given
    +    last-modified: fix memory leak when more than one commit is given
     
    -    When more than one revision is given, the function
    +    When more than one commit is given, the function
         populate_paths_from_revs() leaks a `struct pathspec`. Plug it.
     
         Signed-off-by: Toon Claes <toon@iotcl.com>
    @@ builtin/last-modified.c: static int populate_paths_from_revs(struct last_modifie
      			continue;
      
     -		if (num_interesting++)
    --			return error(_("last-modified can only operate on one revision at a time"));
    +-			return error(_("last-modified can only operate on one commit at a time"));
     +		if (num_interesting++) {
    -+			ret = error(_("last-modified can only operate on one revision at a time"));
    ++			ret = error(_("last-modified can only operate on one commit at a time"));
     +			goto out;
     +		}
      
3:  151f0ff0ae = 3:  8f37827504 last-modified: remove double error message
4:  f2cf0dd371 ! 4:  9a810e12be last-modified: verify revision argument is a commit-ish
    @@ Metadata
      ## Commit message ##
         last-modified: verify revision argument is a commit-ish
     
    -    Passing a tree OID to git-last-modified(1) would trigger BUG behavior.
    +    Passing a non-committish revision to git-last-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 is a commit-ish.
    +    Fix this error by ensuring that the given revision peels to a commit.
     
         Reported-by: Gusted <gusted@codeberg.org>
         Signed-off-by: Toon Claes <toon@iotcl.com>


---
base-commit: ab689ea7f91ab0858e85776f31102203d3ea7b83
change-id: 20260112-toon-last-modified-tree-fdd96b2feaf7

[PATCH v5 1/4] last-modified: rewrite error message when more than one commit given

[Toon Claes]

When more than one commit is passed to the git-last-modified(1) command,
this error message was printed:

    error: last-modified can only operate on one tree at a time

Calling these a "tree" is technically not correct. git-last-modified(1)
expects revisions that peel to a commit.

Rephrase the error message to:

    error: last-modified can only operate on one commit at a time

While at it, modify the test to ensure the correct error message is
printed.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c  |  2 +-
 t/t8020-last-modified.sh | 11 ++++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index c80f0535f6..1219f6802e 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -146,7 +146,7 @@ static int populate_paths_from_revs(struct last_modified *lm)
 			continue;
 
 		if (num_interesting++)
-			return error(_("last-modified can only operate on one tree at a time"));
+			return error(_("last-modified can only operate on one commit at a time"));
 
 		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..d1aad12319 100755
--- a/t/t8020-last-modified.sh
+++ b/t/t8020-last-modified.sh
@@ -12,10 +12,6 @@ test_expect_success 'setup' '
 	test_commit 3 a/b/file
 '
 
-test_expect_success 'cannot run last-modified on two trees' '
-	test_must_fail git last-modified HEAD HEAD~1
-'
-
 check_last_modified() {
 	local indir= &&
 	while test $# != 0
@@ -230,9 +226,14 @@ test_expect_success 'last-modified merge undoes changes' '
 	EOF
 '
 
+test_expect_success 'cannot run last-modified on two commits' '
+	test_must_fail git last-modified HEAD HEAD~1 2>err &&
+	test_grep "last-modified can only operate on one commit at a time" err
+'
+
 test_expect_success 'last-modified complains about unknown arguments' '
 	test_must_fail git last-modified --foo 2>err &&
-	grep "unknown last-modified argument: --foo" err
+	test_grep "unknown last-modified argument: --foo" err
 '
 
 test_done

-- 
2.53.0.rc1.267.g6e3a78c723

[PATCH v5 2/4] last-modified: fix memory leak when more than one commit is given

[Toon Claes]

When more than one commit is given, the function
populate_paths_from_revs() leaks a `struct pathspec`. Plug it.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 1219f6802e..31dea975a0 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -123,7 +123,7 @@ static void add_path_from_diff(struct diff_queue_struct *q,
 
 static int populate_paths_from_revs(struct last_modified *lm)
 {
-	int num_interesting = 0;
+	int num_interesting = 0, ret = 0;
 	struct diff_options diffopt;
 
 	/*
@@ -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 commit at a time"));
+		if (num_interesting++) {
+			ret = error(_("last-modified can only operate on one commit at a time"));
+			goto out;
+		}
 
 		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
 			      &obj->item->oid, "", &diffopt);
 		diff_flush(&diffopt);
 	}
+
+out:
 	clear_pathspec(&diffopt.pathspec);
 
-	return 0;
+	return ret;
 }
 
 static void last_modified_emit(struct last_modified *lm,

-- 
2.53.0.rc1.267.g6e3a78c723

[PATCH v5 3/4] last-modified: remove double error message

[Toon Claes]

When the user passes two revisions, they get the following output:

    $ git last-modified HEAD HEAD~
    error: last-modified can only operate on one revision at a time
    error: unable to setup last-modified

The error message about "unable to setup" is not very informative,
remove it.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 31dea975a0..e02ec8428b 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -495,7 +495,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
 	lm->rev.bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
 
 	if (populate_paths_from_revs(lm) < 0)
-		return error(_("unable to setup last-modified"));
+		return -1;
 
 	CALLOC_ARRAY(lm->all_paths, hashmap_get_size(&lm->paths));
 	lm->all_paths_nr = 0;

-- 
2.53.0.rc1.267.g6e3a78c723

[PATCH v5 4/4] last-modified: verify revision argument is a commit-ish

[Toon Claes]

Passing a non-committish revision to git-last-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 ensuring that the given revision peels to a commit.

Reported-by: Gusted <gusted@codeberg.org>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c  |  5 +++++
 t/t8020-last-modified.sh | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index e02ec8428b..d0944673f0 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -150,6 +150,11 @@ static int populate_paths_from_revs(struct last_modified *lm)
 			goto out;
 		}
 
+		if (!repo_peel_to_type(lm->rev.repo, obj->path, 0, obj->item, OBJ_COMMIT)) {
+			ret = error(_("revision argument '%s' is a %s, not a commit-ish"), obj->name, type_name(obj->item->type));
+			goto out;
+		}
+
 		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
 			      &obj->item->oid, "", &diffopt);
 		diff_flush(&diffopt);
diff --git a/t/t8020-last-modified.sh b/t/t8020-last-modified.sh
index d1aad12319..6024e8bd60 100755
--- a/t/t8020-last-modified.sh
+++ b/t/t8020-last-modified.sh
@@ -8,6 +8,7 @@ test_expect_success 'setup' '
 	test_commit 1 file &&
 	mkdir a &&
 	test_commit 2 a/file &&
+	git tag -mA t2 2 &&
 	mkdir a/b &&
 	test_commit 3 a/b/file
 '
@@ -51,6 +52,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
@@ -236,4 +244,9 @@ test_expect_success 'last-modified complains about unknown arguments' '
 	test_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 '"'"'HEAD^{tree}'"'"' is a tree, not a commit-ish" err
+'
+
 test_done

-- 
2.53.0.rc1.267.g6e3a78c723

Re: [PATCH v5 0/4] Fix git-last-modified(1) bug triggered when passing a tree-ish

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> Changes in v5:
> - Use 'commit' instead of 'revision'.
> - Small typo fixes.
> - Link to v4: https://patch.msgid.link/20260123-toon-last-modified-tree-v4-0-86bf97bad4e2@iotcl.com

Looking good.  Queued.  Thanks.

Re: [PATCH v5 0/4] Fix git-last-modified(1) bug triggered when passing a tree-ish

[Toon Claes]

Junio C Hamano <gitster@pobox.com> writes:

> Toon Claes <toon@iotcl.com> writes:
>
>> Changes in v5:
>> - Use 'commit' instead of 'revision'.
>> - Small typo fixes.
>> - Link to v4: https://patch.msgid.link/20260123-toon-last-modified-tree-v4-0-86bf97bad4e2@iotcl.com
>
> Looking good.  Queued.  Thanks.

I noticed this causes a failure[1] with Meson on Windows. Better leave
it out for now. One of the failures:

    expecting success of 8020.3 'last-modified recursive': 
    	check_last_modified -r <<-\EOF
    	3 a/b/file
    	2 a/file
    	1 file
    	EOF

    ++ check_last_modified -r
    ++ local indir=
    ++ test 1 '!=' 0
    ++ case "$1" in
    ++ break
    ++ cat
    ++ git last-modified -r
    ++ git name-rev --annotate-stdin --name-only --tags
    ++ tr '\t' ' '
    ++ test_cmp expect actual
    ++ test 2 -ne 2
    ++ eval 'GIT_DIR=/dev/null git diff --no-index --ignore-cr-at-eol --' '"$@"'
    +++ GIT_DIR=/dev/null
    +++ git diff --no-index --ignore-cr-at-eol -- expect actual
    diff --git a/expect b/actual
    index 65ac8be..d17615e 100644
    --- a/expect
    +++ b/actual
    @@ -1,3 +1,3 @@
     3 a/b/file
    -2 a/file
    +t2^0 a/file
     1 file
    error: last command exited with $?=1

[1]: https://gitlab.com/gitlab-org/git/-/jobs/12899403428


-- 
Cheers,
Toon

[PATCH v6 0/4] Fix git-last-modified(1) bug triggered when passing a tree-ish

[Toon Claes]

Recently there was a bug reported[1] passing a tree OID triggers a BUG:

    $ git last-modified fb06ce04173d47aaaa498385621cba8b8dfd7584
    BUG: builtin/last-modified.c:456: paths remaining beyond boundary in last-modified
    [1]    690163 IOT instruction (core dumped)  git last-modified

    `fb06ce04173d47aaaa498385621cba8b8dfd7584` is the tree commit id of web_src. I
    suppose this should've returned a nice error message or blank output.

Fix this bug by checking the revision argument.

[1]: https://lore.kernel.org/git/03f96860-29fc-42a7-a220-c3ec65eb8516@codeberg.org/

---
Changes in v6:
- Fix CI failure with Meson on Windows.
- Link to v5: https://patch.msgid.link/20260127-toon-last-modified-tree-v5-0-38d18a0956d4@iotcl.com

Changes in v5:
- Use 'commit' instead of 'revision'.
- Small typo fixes.
- Link to v4: https://patch.msgid.link/20260123-toon-last-modified-tree-v4-0-86bf97bad4e2@iotcl.com

Changes in v4:
- Extend the error message when revision is not a tree
- Extend the test to verify the error message when more than one
  revision is given.
- Link to v3: https://patch.msgid.link/20260116-toon-last-modified-tree-v3-0-e6ade4dc35ab@iotcl.com

Changes in v3:
- Split the change to plug the leak into a separate commit.
- Small changes to commit messages.
- Link to v2: https://patch.msgid.link/20260114-toon-last-modified-tree-v2-0-ba3b1860898f@iotcl.com

Changes in v2:
- Prepend the change with a commit that modifies the error message
  when more than one revision is given.
- Prepend another commit that removes double error message.
- Add test to ensure the command works with annotated tags too.
- Link to v1: https://patch.msgid.link/20260112-toon-last-modified-tree-v1-1-ecbc78341f76@iotcl.com

---
Toon Claes (4):
      last-modified: rewrite error message when more than one commit given
      last-modified: fix memory leak when more than one commit is given
      last-modified: remove double error message
      last-modified: verify revision argument is a commit-ish

 builtin/last-modified.c  | 19 ++++++++++++++-----
 t/t8020-last-modified.sh | 26 ++++++++++++++++++++------
 2 files changed, 34 insertions(+), 11 deletions(-)

Range-diff versus v5:

1:  d9f9531e67 = 1:  455e9bc0ca last-modified: rewrite error message when more than one commit given
2:  19ccaa2aae = 2:  2d51b5e381 last-modified: fix memory leak when more than one commit is given
3:  4bff975458 = 3:  68b3acefd7 last-modified: remove double error message
4:  a5d4c74ad7 ! 4:  d66ab6a033 last-modified: verify revision argument is a commit-ish
    @@ Commit message
     
         Fix this error by ensuring that the given revision peels to a commit.
     
    +    This change also adds a test to verify git-last-modified(1) can operate
    +    on an annotated tag. For this an annotated tag is added that points to
    +    the second commit. But this causes ambiguous results when calling
    +    git-name-rev(1) with `--tags`, because now two tags point to the same
    +    commit. To remove this ambiguity, pass `--exclude=<tag>` to
    +    git-name-rev(1) to exclude the new annotated tag.
    +
         Reported-by: Gusted <gusted@codeberg.org>
         Signed-off-by: Toon Claes <toon@iotcl.com>
     
    @@ t/t8020-last-modified.sh: test_expect_success 'setup' '
      	mkdir a/b &&
      	test_commit 3 a/b/file
      '
    +@@ t/t8020-last-modified.sh: check_last_modified() {
    + 
    + 	cat >expect &&
    + 	git ${indir:+-C "$indir"} last-modified "$@" >tmp.1 &&
    +-	git name-rev --annotate-stdin --name-only --tags \
    ++	git name-rev --annotate-stdin --name-only --tags --exclude=t2 \
    + 		<tmp.1 >tmp.2 &&
    + 	tr '\t' ' ' <tmp.2 >actual &&
    + 	test_cmp expect actual
     @@ t/t8020-last-modified.sh: test_expect_success 'last-modified recursive' '
      	EOF
      '
    @@ t/t8020-last-modified.sh: test_expect_success 'last-modified complains about unk
      
     +test_expect_success 'last-modified expects commit-ish' '
     +	test_must_fail git last-modified HEAD^{tree} 2>err &&
    -+	grep "revision argument '"'"'HEAD^{tree}'"'"' is a tree, not a commit-ish" err
    ++	test_grep "revision argument ${SQ}HEAD^{tree}${SQ} is a tree, not a commit-ish" err
     +'
     +
      test_done


---
base-commit: ea717645d199f6f1b66058886475db3e8c9330e9
change-id: 20260112-toon-last-modified-tree-fdd96b2feaf7

[PATCH v6 1/4] last-modified: rewrite error message when more than one commit given

[Toon Claes]

When more than one commit is passed to the git-last-modified(1) command,
this error message was printed:

    error: last-modified can only operate on one tree at a time

Calling these a "tree" is technically not correct. git-last-modified(1)
expects revisions that peel to a commit.

Rephrase the error message to:

    error: last-modified can only operate on one commit at a time

While at it, modify the test to ensure the correct error message is
printed.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c  |  2 +-
 t/t8020-last-modified.sh | 11 ++++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index c80f0535f6..1219f6802e 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -146,7 +146,7 @@ static int populate_paths_from_revs(struct last_modified *lm)
 			continue;
 
 		if (num_interesting++)
-			return error(_("last-modified can only operate on one tree at a time"));
+			return error(_("last-modified can only operate on one commit at a time"));
 
 		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..d1aad12319 100755
--- a/t/t8020-last-modified.sh
+++ b/t/t8020-last-modified.sh
@@ -12,10 +12,6 @@ test_expect_success 'setup' '
 	test_commit 3 a/b/file
 '
 
-test_expect_success 'cannot run last-modified on two trees' '
-	test_must_fail git last-modified HEAD HEAD~1
-'
-
 check_last_modified() {
 	local indir= &&
 	while test $# != 0
@@ -230,9 +226,14 @@ test_expect_success 'last-modified merge undoes changes' '
 	EOF
 '
 
+test_expect_success 'cannot run last-modified on two commits' '
+	test_must_fail git last-modified HEAD HEAD~1 2>err &&
+	test_grep "last-modified can only operate on one commit at a time" err
+'
+
 test_expect_success 'last-modified complains about unknown arguments' '
 	test_must_fail git last-modified --foo 2>err &&
-	grep "unknown last-modified argument: --foo" err
+	test_grep "unknown last-modified argument: --foo" err
 '
 
 test_done

-- 
2.53.0.rc1.267.g6e3a78c723

[PATCH v6 2/4] last-modified: fix memory leak when more than one commit is given

[Toon Claes]

When more than one commit is given, the function
populate_paths_from_revs() leaks a `struct pathspec`. Plug it.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 1219f6802e..31dea975a0 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -123,7 +123,7 @@ static void add_path_from_diff(struct diff_queue_struct *q,
 
 static int populate_paths_from_revs(struct last_modified *lm)
 {
-	int num_interesting = 0;
+	int num_interesting = 0, ret = 0;
 	struct diff_options diffopt;
 
 	/*
@@ -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 commit at a time"));
+		if (num_interesting++) {
+			ret = error(_("last-modified can only operate on one commit at a time"));
+			goto out;
+		}
 
 		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
 			      &obj->item->oid, "", &diffopt);
 		diff_flush(&diffopt);
 	}
+
+out:
 	clear_pathspec(&diffopt.pathspec);
 
-	return 0;
+	return ret;
 }
 
 static void last_modified_emit(struct last_modified *lm,

-- 
2.53.0.rc1.267.g6e3a78c723

[PATCH v6 3/4] last-modified: remove double error message

[Toon Claes]

When the user passes two revisions, they get the following output:

    $ git last-modified HEAD HEAD~
    error: last-modified can only operate on one revision at a time
    error: unable to setup last-modified

The error message about "unable to setup" is not very informative,
remove it.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 31dea975a0..e02ec8428b 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -495,7 +495,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
 	lm->rev.bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
 
 	if (populate_paths_from_revs(lm) < 0)
-		return error(_("unable to setup last-modified"));
+		return -1;
 
 	CALLOC_ARRAY(lm->all_paths, hashmap_get_size(&lm->paths));
 	lm->all_paths_nr = 0;

-- 
2.53.0.rc1.267.g6e3a78c723

[PATCH v6 4/4] last-modified: verify revision argument is a commit-ish

[Toon Claes]

Passing a non-committish revision to git-last-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 ensuring that the given revision peels to a commit.

This change also adds a test to verify git-last-modified(1) can operate
on an annotated tag. For this an annotated tag is added that points to
the second commit. But this causes ambiguous results when calling
git-name-rev(1) with `--tags`, because now two tags point to the same
commit. To remove this ambiguity, pass `--exclude=<tag>` to
git-name-rev(1) to exclude the new annotated tag.

Reported-by: Gusted <gusted@codeberg.org>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/last-modified.c  |  5 +++++
 t/t8020-last-modified.sh | 15 ++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index e02ec8428b..d0944673f0 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -150,6 +150,11 @@ static int populate_paths_from_revs(struct last_modified *lm)
 			goto out;
 		}
 
+		if (!repo_peel_to_type(lm->rev.repo, obj->path, 0, obj->item, OBJ_COMMIT)) {
+			ret = error(_("revision argument '%s' is a %s, not a commit-ish"), obj->name, type_name(obj->item->type));
+			goto out;
+		}
+
 		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
 			      &obj->item->oid, "", &diffopt);
 		diff_flush(&diffopt);
diff --git a/t/t8020-last-modified.sh b/t/t8020-last-modified.sh
index d1aad12319..ec5bdc6aa0 100755
--- a/t/t8020-last-modified.sh
+++ b/t/t8020-last-modified.sh
@@ -8,6 +8,7 @@ test_expect_success 'setup' '
 	test_commit 1 file &&
 	mkdir a &&
 	test_commit 2 a/file &&
+	git tag -mA t2 2 &&
 	mkdir a/b &&
 	test_commit 3 a/b/file
 '
@@ -30,7 +31,7 @@ check_last_modified() {
 
 	cat >expect &&
 	git ${indir:+-C "$indir"} last-modified "$@" >tmp.1 &&
-	git name-rev --annotate-stdin --name-only --tags \
+	git name-rev --annotate-stdin --name-only --tags --exclude=t2 \
 		<tmp.1 >tmp.2 &&
 	tr '\t' ' ' <tmp.2 >actual &&
 	test_cmp expect actual
@@ -51,6 +52,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
@@ -236,4 +244,9 @@ test_expect_success 'last-modified complains about unknown arguments' '
 	test_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 &&
+	test_grep "revision argument ${SQ}HEAD^{tree}${SQ} is a tree, not a commit-ish" err
+'
+
 test_done

-- 
2.53.0.rc1.267.g6e3a78c723

Re: [PATCH v6 4/4] last-modified: verify revision argument is a commit-ish

[Patrick Steinhardt]

On Fri, Jan 30, 2026 at 03:26:38PM +0100, Toon Claes wrote:
> diff --git a/t/t8020-last-modified.sh b/t/t8020-last-modified.sh
> index d1aad12319..ec5bdc6aa0 100755
> --- a/t/t8020-last-modified.sh
> +++ b/t/t8020-last-modified.sh
> @@ -30,7 +31,7 @@ check_last_modified() {
>  
>  	cat >expect &&
>  	git ${indir:+-C "$indir"} last-modified "$@" >tmp.1 &&
> -	git name-rev --annotate-stdin --name-only --tags \
> +	git name-rev --annotate-stdin --name-only --tags --exclude=t2 \
>  		<tmp.1 >tmp.2 &&
>  	tr '\t' ' ' <tmp.2 >actual &&
>  	test_cmp expect actual

It's quite curious that we need to explicitly exclude t2 here to get a
deterministic result. But both of the results are correct, and in fact
I've seen it once before that we got different results on different
platforms.

Sooo... weird, but I'd say we can live with this weirdness. Doubly so
because it's not the fault of this patch series.

Patrick

Re: [PATCH v6 0/4] Fix git-last-modified(1) bug triggered when passing a tree-ish

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> Changes in v6:
> - Fix CI failure with Meson on Windows.
> - Link to v5: https://patch.msgid.link/20260127-toon-last-modified-tree-v5-0-38d18a0956d4@iotcl.com

Replaced.  Thanks.

Re: [PATCH v6 0/4] Fix git-last-modified(1) bug triggered when passing a tree-ish

[Patrick Steinhardt]

On Fri, Jan 30, 2026 at 09:07:33AM -0800, Junio C Hamano wrote:
> Toon Claes <toon@iotcl.com> writes:
> 
> > Changes in v6:
> > - Fix CI failure with Meson on Windows.
> > - Link to v5: https://patch.msgid.link/20260127-toon-last-modified-tree-v5-0-38d18a0956d4@iotcl.com
> 
> Replaced.  Thanks.

From my point of view this series looks good to be merged down to 'next'
now. Thanks!

Patrick