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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 7, 2021, 21:37 UTC
Message-ID: <xmqqo8epydwx.fsf@gitster.g>
In-Reply-To: <20210407173334.68222-3-mirucam@gmail.com>

Miriam Rubio <mirucam@gmail.com> writes:

This bit

Show 5 quoted lines

> -		case "$1" in
> -		git*|tig) ;;
> -		-*)	set git log "$@" ;;
> -		*)	set git "$@" ;;
> -		esac

in the original corresponds to the following.

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

If it begins with "-", we assume that is an option to "git log".  OK.

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

The condition (if it is not "tig" and does not start with "git") is the opposite for the first case arm where we just use the rest of the command line as is. We take it as a git subcommand name and its arguments. OK.

> +		strvec_pushv(&args, argv);

And the remainder of the command line is pushed into the arguments.

OK.

> +	}

And this part from the original

> -	eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")

corresponds to the rest. What the original calls "$@" is already captured in args.

> +	strvec_pushl(&args, "--bisect",  NULL);

We lost the "--" ("end of options and revs, now the pathspec follows") here. Not good.

> +	strbuf_read_file(&sb, git_path_bisect_names(), 0);
> +	strvec_split(&args, sb.buf);

This is probably quite wrong. What we will place in the BISECT_NAMES file, as it is a list of pathspec elements (i.e. a pathspec), can contain spaces. For that reason, when we write to the file, we use sq_quote_argv() on each pathspec elements.

See builtin/bisect--helper.c::bisect_start()

If I understand correctly, strvec_split() is to split a buffer at runs of whitespaces, which is a helper for a very different purpose.

Perhaps sq_dequote_to_strvec() is what we want to use here?

Show 16 quoted lines

> +	strbuf_release(&sb);
> +	res = run_command_v_opt(args.v, flags);
> +	strvec_clear(&args);
> +	return res;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> @@ -1046,7 +1085,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		BISECT_STATE,
>  		BISECT_LOG,
>  		BISECT_REPLAY,
> -		BISECT_SKIP
> +		BISECT_SKIP,
> +		BISECT_VISUALIZE

Perhaps it is a good time to add trailing comma after this new token. cf. Documentation/CodingGuidelines (look for "since early 2012"). The next patch to add yet another enum won't have to touch the line added here that way.

Everything below looked trivially correct.

Thanks.