From: Patrick Steinhardt Date: Tue, 02 Sep 2025 09:07:59 GMT Subject: Re: [PATCH] add-interactive: reject malformed numerical input Message-ID: In-Reply-To: On Sat, Aug 30, 2025 at 11:31:35AM +0000, Seonghyeon Cho (조성현) via GitGitGadget wrote: > From: Seonghyeon Cho > > 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. > diff --git a/add-interactive.c b/add-interactive.c > index 3e692b47ec..86ff632288 100644 > --- a/add-interactive.c > +++ b/add-interactive.c > @@ -396,6 +396,8 @@ static ssize_t list_and_choose(struct add_i_state *s, > if (endp != p + sep) > from = -1; > } > + else > + from = -1; > } 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. Patrick