Re: [PATCH] add-interactive: reject malformed numerical input

From: Seonghyeon Cho <seonghyeoncho96@gmail.com>
Date: Sep 7, 2025, 12:24 UTC
Message-ID: <aL15aTmKQOsNrF0D@Seonghyeons-Mac-Studio.local>
In-Reply-To: <aLaz7yCXWGG2_oP_@pks.im>

On Tue, Sep 02, 2025 at 11:07:59AM +0200, Patrick Steinhardt wrote:

Show 5 quoted lines

> 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.

Show 10 quoted lines

> 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