On Thu, Nov 10 2022, Đoàn Trần Công Danh wrote:
> In a later change, we would like to turn bisect into a builtin by
> renaming bisect--helper.
>
> However, there's an oddity that "git bisect log" accepts any number of
> arguments and it will just ignore them all.
>
> Let's prepare for the next step by ignoring any arguments passed to
> "git bisect--helper log"
>
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@xxxxxxxxx>
> ---
> builtin/bisect--helper.c | 4 +---
> git-bisect.sh | 2 --
> 2 files changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 29d5a26c64..6066f553fd 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -1347,10 +1347,8 @@ static int cmd_bisect__next(int argc, const char **argv UNUSED, const char *pref
> return res;
> }
>
> -static int cmd_bisect__log(int argc, const char **argv UNUSED, const char *prefix UNUSED)
> +static int cmd_bisect__log(int argc UNUSED, const char **argv UNUSED, const char *prefix UNUSED)
> {
> - if (argc)
> - return error(_("'%s' requires 0 arguments"), "git bisect log");
> return bisect_log();
> }
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 9f6c8cc093..f95b8103a9 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -57,8 +57,6 @@ case "$#" in
> case "$cmd" in
> help)
> git bisect -h ;;
> - log)
> - git bisect--helper log || exit ;;
> *)
> git bisect--helper "$cmd" "$@" ;;
> esac
I'm agnostic on whether we keep this oddity, or just say we're fixing it
as we're converting things. We could also go for some in-between and
issue a warning(), if we suspect users in the wild are relying on this
for whatever reason.
I'd be fine with just making it error. E.g. in my upcoming (and already
aired on the list as an RFC) series to migrate git-submodule.sh to a
built-in I *do* change some behavior, because some of it's too insane to
carry forward in a bug-for-bug compatible way.
But I when doing so I add tests for it, explain why etc.
So, I think whatever we do here, we should be adding tests for this. If
it's worth preserving it's worth testing.
