Fix recent_commits(limit=0) returning 1 commit instead of 0

[Towaiji]

Fixes #7333

This is a small bug fix.

This PR fixes an inconsistency in recent_commits where limit == 0 still returns 1 commit due to the use of limit.max(1) when constructing the git log -n argument.

Expected behavior: requesting 0 commits should return an empty list.

This PR:

• returns an empty Vec when limit == 0
• adds a test for recent_commits(limit == 0) that fails before the change and passes afterwards
• maintains existing behavior for limit > 0

This aligns behavior with API expectations and avoids downstream consumers misinterpreting the repository as having commit history when limit == 0 is used to explicitly request none.

Happy to adjust if the current behavior is intentional.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[Towaiji]

I have read the CLA Document and I hereby sign the CLA

[etraut-openai]

@codex review

[etraut-openai]

@Towaiji, thanks for the contribution. Your PR changes more than needed to fix this issue. We prefer targeted and surgical changes wherever possible. I don't think a new test case is needed for this change, since it's unlikely to regress.

Also, it looks like this is triggering a CI test failure. I haven't investigated, but this may indicate that code relies on the existing behavior.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 🎉

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Towaiji]

Thanks for the feedback @etraut-openai!

Happy to make this more targeted.

I can:

• remove the test case
• limit the change to returning early when limit == 0

Before I update the PR, do you know which part of the codebase depends on
recent_commits(0) returning 1 entry?

That would help ensure the fix doesn't break existing behavior.

[Towaiji]

Hey @etraut-openai!

Based on the CI failures and your note about callers relying on the current behavior, I explored a more minimal approach.

Instead of returning an empty Vec for limit == 0, treating 0 as “no -n argument” seems to preserve existing expectations while still fixing the limit.max(1) issue:

let limit_arg = (limit > 0).then(|| limit.to_string());
let mut args: Vec<String> = vec!["log".to_string()];
if let Some(n) = &limit_arg {
    args.push("-n".to_string());
    args.push(n.clone());
}
args.push(format!("--pretty=format:{fmt}"));

This keeps limit > 0 behavior identical, and avoids the downstream breakage I saw when returning an empty list. It also maintains the previous implicit behavior where limit == 0 results in full history (no -n), which callers appear to rely on. This avoids changing the semantic meaning of limit == 0, which previously implied "no limit" rather than "no commits."

Does this direction align with what you had in mind before I update the PR?

[etraut-openai]

@Towaiji, the direction you're suggesting sounds reasonable to me. I'm personally not that familiar with this part of the code. I'll need to ask a teammate to look at your change once it's ready for review.

[Towaiji]

Hey @etraut-openai ,

All CI checks have passed with the latest update.

I reverted the broader changes and applied the minimal fix we discussed. When limit == 0, the -n argument is omitted instead of forcing -n 1. This keeps the previous implicit "no limit" behavior while still removing the limit.max(1) issue.

You mentioned looping in a teammate who is more familiar with this area, so I am happy to wait for their review.

Happy to contribute!

[etraut-openai]

Thanks! It's a holiday week in the U.S. (Thanksgiving), and most of the codex team is on break, so it's unlikely that this will get reviewed until next week.

[Towaiji]

Thanks for the update! No worries at all. I appreciate the heads up and I will keep an eye on it next week.

Happy holidays!