Re: [PATCH v3 1/4] rebase: start implementing it as a builtin

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 6, 2018, 21:09 UTC
Message-ID: <xmqq36wwjcl2.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180706120815.17851-2-predatoramigo@gmail.com>

Pratik Karki <predatoramigo@gmail.com> writes:

Show 34 quoted lines

> +static int use_builtin_rebase(void)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	int ret;
> +
> +	argv_array_pushl(&cp.args,
> +			 "config", "--bool", "rebase.usebuiltin", NULL);
> +	cp.git_cmd = 1;
> +	if (capture_command(&cp, &out, 6))
> +		return 0;
> +
> +	strbuf_trim(&out);
> +	ret = !strcmp("true", out.buf);
> +	strbuf_release(&out);
> +	return ret;
> +}
> +
> +int cmd_rebase(int argc, const char **argv, const char *prefix)
> +{
> +	/*
> +	 * NEEDSWORK: Once the builtin rebase has been tested enough
> +	 * and git-legacy-rebase.sh is retired to contrib/, this preamble
> +	 * can be removed.
> +	 */
> +
> +	if (!use_builtin_rebase()) {
> +		const char *path = mkpath("%s/git-legacy-rebase",
> +					  git_exec_path());
> +
> +		if (sane_execvp(path, (char **)argv) < 0)
> +			die_errno("could not exec %s", path);
> +		else
> +			die("sane_execvp() returned???");

This part got changed from returning 0 since the previous round, which makes sense, but if we are making this change relative to the original in be8a90e, just because we mention explicitly that we imitate that old commit, we must mention how and why we "improved" on it, in the log message.

	In the old original used while converting difftool, if
	sane_execvp() that attempts to run the legacy scripted
	version returned with non-negative status, the command
	silently exited without doing anything with success, but
	sane_execvp() should not retun with non-negative status in
	the first place, so make sure we die() to notice such an
	abnormal case.

or something like that, perhaps (but you should be able to shorten it further).

Between die() and BUG(), I am not sure which is more appropriate. It certainly is not _our_ bug if platform execvp() returns success, so BUG() would not help our developers all that much. But end-users won't be helped by being told that sane_execvp() returned, so die() is not all that useful, either. I guess it does not matter that much bewteen the two, as this is something that is not supposed to happen anyway ;-)

In any case, this one looks good.  Will queue.

Thanks.