diff mbox series

[v2,10/11] bisect--helper: log: allow arbitrary number of arguments

Message ID de3075eff9ffffea27d19e749af897b72f83ef41.1668097966.git.congdanhqx@gmail.com (mailing list archive)
State Accepted
Commit 0da4b538e40f38f2ee7adc625cac18ae0d8df4d1
Headers show
Series Turn git-bisect to be builtin | expand

Commit Message

Đoàn Trần Công Danh Nov. 10, 2022, 4:36 p.m. UTC
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@gmail.com>
---
 builtin/bisect--helper.c | 4 +---
 git-bisect.sh            | 2 --
 2 files changed, 1 insertion(+), 5 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Nov. 11, 2022, 2:01 p.m. UTC | #1
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@gmail.com>
> ---
>  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.
diff mbox series

Patch

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