Re: [PATCH v6 4/6] bisect--helper: reimplement `bisect_visualize()`shell function in C

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 2, 2021, 22:28 UTC
Message-ID: <xmqqfsumipd4.fsf@gitster.g>
In-Reply-To: <20210902090421.93113-5-mirucam@gmail.com>

Miriam Rubio <mirucam@gmail.com> writes:

> From: Pranit Bauva <pranit.bauva@gmail.com>

Need a SP before "shell" on the title line.

> Reimplement the `bisect_visualize()` shell function
> in C and also add `--bisect-visualize` subcommand to
> `git bisect--helper` to call it from git-bisect.sh.

Nice.

Show 17 quoted lines

> +static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)
> +{
> +	struct strvec args = STRVEC_INIT;
> +	int flags = RUN_COMMAND_NO_STDIN, res = 0;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	if (bisect_next_check(terms, NULL) != 0)
> +		return BISECT_FAILED;
> +
> +	if (!argc) {
> +		if ((getenv("DISPLAY") || getenv("SESSIONNAME") || getenv("MSYSTEM") ||
> +		     getenv("SECURITYSESSIONID")) && exists_in_PATH("gitk"))
> +			strvec_push(&args, "gitk");
> +		else {
> +			strvec_push(&args, "log");
> +			flags |= RUN_GIT_CMD;
> +		}

Let's have {} on the if() side, even though it only has one statement and does not require one, because the else side needs one.

> +	} else {
> +		if (argv[0][0] == '-') {
> +			strvec_push(&args, "log");
> +			flags |= RUN_GIT_CMD;

OK, any -option makes it "git log -option ..." invocation.

> +		} else if (strcmp(argv[0], "tig") && !starts_with(argv[0], "git"))
> +			flags |= RUN_GIT_CMD;

OK, when the first token is "tig", or it begins with "git", the scripted version just leaves the command line intact. Everything else is taken as a subcommand to git. And this conditional is a faithful translation of that logic.

Show 12 quoted lines

> +		strvec_pushv(&args, argv);
> +	}
> +
> +	strvec_pushl(&args, "--bisect", "--", NULL);
> +
> +	strbuf_read_file(&sb, git_path_bisect_names(), 0);
> +	sq_dequote_to_strvec(sb.buf, &args);
> +	strbuf_release(&sb);
> +
> +	res = run_command_v_opt(args.v, flags);
> +	strvec_clear(&args);
> +	return res;

OK.

The code is quite easy to follow, thanks to many helpers that have been invented for this exact purpose, like sq_dequote_to_strvec().

Show 21 quoted lines

> diff --git a/git-bisect.sh b/git-bisect.sh
> index 6a7afaea8d..95f7f3fb8c 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -39,29 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
>  TERM_BAD=bad
>  TERM_GOOD=good
>  
> -bisect_visualize() {
> ...
> -}
> -
>  bisect_run () {
>  	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
>  
> @@ -152,7 +129,7 @@ case "$#" in
>  		# Not sure we want "next" at the UI level anymore.
>  		git bisect--helper --bisect-next "$@" || exit ;;
>  	visualize|view)
> -		bisect_visualize "$@" ;;
> +		git bisect--helper --bisect-visualize "$@" || exit;;

Nice.

Thanks.