patchlast-modified: verify revision argument is a commit-ish

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

Toon Claes <toon@iotcl.com> writes:

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

OK.

On Mon, Jan 12, 2026 at 05:17:41PM +0100, Toon Claes wrote:

s/is peels/peels/

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.

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.

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

Thanks!

Patrick

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

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

On Wed, Jan 14, 2026 at 11:24:45AM +0100, Toon Claes wrote:

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

Nit: s/revision/revisions/

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

Patrick Steinhardt <ps@pks.im> writes:

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.

Thanks.

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

On Thu, Jan 15, 2026 at 12:33:36PM +0100, Toon Claes wrote:

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

On Thu, Jan 15, 2026, at 12:54, Patrick Steinhardt wrote:

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

On Thu, Jan 15, 2026 at 03:34:50PM +0100, Kristoffer Haugsbakk wrote:

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

Patrick Steinhardt <ps@pks.im> writes:

I must admit, I was surprised too.

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.

Patrick Steinhardt <ps@pks.im> writes:

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.

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.

On Fri, Jan 16, 2026 at 09:16:03AM -0800, Junio C Hamano wrote:

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

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.

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

On Fri, Jan 16, 2026, at 08:09, Patrick Steinhardt wrote:

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.

On Thu, Jan 15, 2026, at 12:33, Toon Claes wrote:

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

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

On Wed, Jan 14, 2026 at 11:24:46AM +0100, Toon Claes wrote:

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

Patrick

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(+)

On Wed, Jan 14, 2026 at 11:24:47AM +0100, Toon Claes wrote:

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:

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

Nice, thanks for adding this test.

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

Patrick

On Wed, Jan 14, 2026, at 11:24, Toon Claes wrote:

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

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

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

Great eyes ;-)

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

Thanks! I've addressed this in v3.

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

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

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

Toon Claes <toon@iotcl.com> writes:

Not "revisions"?

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

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

Toon Claes <toon@iotcl.com> writes:

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.

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(+)

Toon Claes <toon@iotcl.com> writes:

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

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

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

Toon Claes <toon@iotcl.com> writes:

"ensure modify" -> "modify".

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?

"two revision" -> "two revisions".

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

Toon Claes <toon@iotcl.com> writes:

Makes sense.

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

Toon Claes <toon@iotcl.com> writes:

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.

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(+)

Toon Claes <toon@iotcl.com> writes:

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

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.

Toon Claes <toon@iotcl.com> writes:

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.

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

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

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

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

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(+)

Toon Claes <toon@iotcl.com> writes:

Looking good.  Queued.  Thanks.

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

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

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

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

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

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

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

On Fri, Jan 30, 2026 at 03:26:38PM +0100, Toon Claes wrote:

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

Toon Claes <toon@iotcl.com> writes:

Replaced.  Thanks.

On Fri, Jan 30, 2026 at 09:07:33AM -0800, Junio C Hamano wrote:

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

Patrick