On Wed, Jul 13 2022, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>> I'm not claiming that we always use 129 when we're fed bad options etc.,
>> but rather that that's what parse_options() does, so at this point most
>> commands do that consistently.
>>
>> ./git --blah >/dev/null 2>&1; echo $?
>> 129
>> ./git status --blah >/dev/null 2>&1; echo $?
>> 129
>>
>> But yes, you can find exceptions still, e.g. try that with "git log" and
>> it'll return 128.
>
> Yup, that was my understanding as well. We may have existing
> breakage that we shouldn't be actively imitating when we do not have
> to.
>
>> Which, I'm not saying should hold this series up, but just that having
>> reviewed it for a few iterations I'm not comfortable saying we haven't
>> missed something else, and given the nature of the past whack-a-mole
>> fixes it looks like you haven't really looked it either.
>
> "We haven't missed" is not something we can comfortably say, ever,
> aobut a series with any meaningful size. I am not so worried about
> it, if it is your only worries. Would it make it less likely to
> have introduced unintended bugs if we find a way to keep using
> parse-options?
I started writing something here, but found myself rewriting the last 3
paragraphs of [1] seen in the context below, so I'll just refer to that.
FWIW ejecting 11-14/16, i.e. these patches:
- Turn `git bisect` into a full built-in
- bisect: move even the command-line parsing to `bisect--helper`
- bisect: teach the `bisect--helper` command to show the correct usage strings
- bisect--helper: return only correct exit codes in `cmd_*()`
Yields a result that I've got no concerns about whatsoever, and reduces
the diffstat from:
7 files changed, 110 insertions(+), 207 deletions(-)
To just:
4 files changed, 71 insertions(+), 67 deletions(-)
Which I think might be worth considering, similar to how the submodule
migration is happening in multi-step fashion. I.e. advancing the parts
that don't migrate it away from parse_options(), since I showed a way to
keep using it in [1] (while changing less code).
Or just merge it down, up to you :)
>> I'm referring to e.g. the thread ending at [3], where I pointed out that
>> "git <subcommand> -h" was broken, you apparently tested one of the
>> subcommands and concluded it wasn't broken, but clearly not all of them.
>>
>> Which, I'd be less concerned about if as pointed out in [1] we don't
>> have entirte bisect sub-commands that don't have any test coverage at
>> all, so whatever coverage we have probably has major gaps.
>>
>> 1. https://lore.kernel.org/git/220627.86mtdxhnwk.gmgdl@xxxxxxxxxxxxxxxxxxx/
>> 2. https://lore.kernel.org/git/220523.865ylwzgji.gmgdl@xxxxxxxxxxxxxxxxxxx/
>> 3. https://lore.kernel.org/git/220225.86ilt27uln.gmgdl@xxxxxxxxxxxxxxxxxxx/
