On Tue, Sep 02, 2025 at 11:07:59AM +0200, Patrick Steinhardt wrote: > Okay, that does feel fishy indeed. It would be good though to have a > test case that demonstrates the new behaviour and at the same time > ensures that we don't regress in the future. You can have a look at > "t3701-add-interactive.sh", which has a bunch of other tests for this > command, as well. Okay, I'll add tests. > In general though we're not doing a good job here of error checking. We > don't at all verify whether `strtoul()` returned an error, for example > ERANGE. So if a user passes an integer that exceeds whatever we can > store in an `unsigned long` we'll silently proceed with a bogus result, > won't we? > > Ideally, we'd use a saner interface to parse these integers, like for > example our own `git_parse_ulong()`. But unfortunately, that interface > does not handle the case where we only want to parse a substring in a > longer string. Too bad. Good point. Would you prefer I introduce new parse method here, or should this be handled in separate patch? > Coding style: the `else` should sit on the same line as the closing > curly brace. And furthermore, if one of the branches of an if-else chain > requires curly braces, then all branches should have curly braces. Ok, I'll fix coding styles. Thanks, Seonghyeon
