add-interactive: reject malformed numerical input

From: Seonghyeon Cho <[email protected]>

The list-and-choose interface accepts malformed input such as "2m3" and interprets it as "2-", silently selecting a range to the end. This is misleading and makes it easy to select unintended items.

Reject such input by treating it as invalid.

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.

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.

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.

Quoted 6 lines · Patrick Steinhardt

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?

Quoted 4 lines

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?

I don't think that would need to be part of your patch series. But we should have proper error checking for strtoul() if we're already improving this code.