Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(git-bulk): fix a bad integer expression #1198

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

pierreay
Copy link
Contributor

Fix a logic error inside the allowedargcount() function. This function may be called with one or two arguments. However, no default values are assigned to $1 and $2 that are used inside a numerical comparison. Therefore, when using a bad number of arguments for the following lines:

listall|purge) allowedargcount 1;;
addcurrent|removeworkspace) allowedargcount 2;;

Then, we would get the error [: : integer expression expected. To fix this, we assign the 0 default value to $1 and $2, such that we trigger the error message destined to the user without any integer error when there is a bad number of argument and that the function is called with only 1 argument instead of 2.

Fix a logic error inside the `allowedargcount()` function. This function
may be called with one or two arguments. However, no default values are
assigned to `$1` and `$2` that are used inside a numerical comparison.
Therefore, when using a bad number of arguments for the following lines:

```
listall|purge) allowedargcount 1;;
addcurrent|removeworkspace) allowedargcount 2;;
```

Then, we would get the error `[: : integer expression expected`. To fix
this, we assign the 0 default value to `$1` and `$2`, such that we
trigger the error message destined to the user without any integer error
when there is a bad number of argument and that the function is called
with only 1 argument instead of 2.
@pierreay pierreay marked this pull request as ready for review February 17, 2025 06:35
Copy link
Collaborator

@hyperupcall hyperupcall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! This probably should have been a loop so the function is valid for any arity. But I think it's a bit moot since I think this fix is good too.

@spacewander spacewander merged commit 82cc37d into tj:main Feb 20, 2025
5 checks passed
@spacewander
Copy link
Collaborator

@pierreay
Merged! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants