Miriam Rubio <mirucam@xxxxxxxxx> writes:
This bit
> - 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?
> + 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.
