git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Miriam Rubio <mirucam@gmail.com>
Cc: git@vger.kernel.org, Pranit Bauva <pranit.bauva@gmail.com>,
	Christian Couder <chriscool@tuxfamily.org>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Tanushree Tumane <tanushreetumane@gmail.com>
Subject: Re: [PATCH v2 2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C
Date: Wed, 07 Apr 2021 14:37:18 -0700	[thread overview]
Message-ID: <xmqqo8epydwx.fsf@gitster.g> (raw)
In-Reply-To: <20210407173334.68222-3-mirucam@gmail.com> (Miriam Rubio's message of "Wed, 7 Apr 2021 19:33:31 +0200")

Miriam Rubio <mirucam@gmail.com> 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.

  reply	other threads:[~2021-04-07 21:37 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-07 17:33 [PATCH v2 0/4] Finish converting git bisect to C part 4 Miriam Rubio
2021-04-07 17:33 ` [PATCH v2 1/4] run-command: make `exists_in_PATH()` non-static Miriam Rubio
2021-04-08 11:22   ` Đoàn Trần Công Danh
2021-04-08 17:38     ` Junio C Hamano
2021-04-07 17:33 ` [PATCH v2 2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C Miriam Rubio
2021-04-07 21:37   ` Junio C Hamano [this message]
2021-04-07 17:33 ` [PATCH v2 3/4] bisect--helper: reimplement `bisect_run` shell " Miriam Rubio
2021-04-07 22:09   ` Junio C Hamano
2021-04-11 10:04     ` Miriam R.
2021-04-09  6:15   ` Junio C Hamano
2021-04-07 17:33 ` [PATCH v2 4/4] bisect--helper: retire `--bisect-next-check` subcommand Miriam Rubio

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqo8epydwx.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=chriscool@tuxfamily.org \
    --cc=git@vger.kernel.org \
    --cc=mirucam@gmail.com \
    --cc=pranit.bauva@gmail.com \
    --cc=tanushreetumane@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).