From: Patrick Steinhardt <ps@pks•im>
To: "Seonghyeon Cho (조성현) via GitGitGadget" <gitgitgadget@gmail•com>
Cc: git@vger•kernel.org, "Seonghyeon Cho (조성현)" <seonghyeoncho96@gmail•com>
Subject: Re: [PATCH] add-interactive: reject malformed numerical input
Date: Tue, 2 Sep 2025 11:07:59 +0200 [thread overview]
Message-ID: <aLaz7yCXWGG2_oP_@pks.im> (raw)
In-Reply-To: <pull.2044.git.git.1756553495661.gitgitgadget@gmail.com>
On Sat, Aug 30, 2025 at 11:31:35AM +0000, Seonghyeon Cho (조성현) via GitGitGadget wrote:
> From: Seonghyeon Cho <seonghyeoncho96@gmail•com>
>
> 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
next prev parent reply other threads:[~2025-09-02 9:08 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-30 11:31 [PATCH] add-interactive: reject malformed numerical input Seonghyeon Cho (조성현) via GitGitGadget
2025-09-02 9:07 ` Patrick Steinhardt [this message]
2025-09-07 12:24 ` Seonghyeon Cho
2025-09-08 4:04 ` Patrick Steinhardt
2025-09-08 13:31 ` Seonghyeon Cho
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aLaz7yCXWGG2_oP_@pks.im \
--to=ps@pks$(echo .)im \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=seonghyeoncho96@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox