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

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

Pratik Karki <predatoramigo@gmail.com> writes:

Show 5 quoted lines

> This commit imitates the strategy that was used to convert the
> difftool to a builtin, see be8a90e (difftool: add a skeleton for the
> upcoming builtin, 2017-01-17) for details: This commit renames the
> shell script `git-rebase.sh` to `git-legacy-rebase.sh` and hands off to
> it by default.

I'd appreciate it if mentors can teach a bit more log message writing to students, thanks.

Show 8 quoted lines

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> new file mode 100644
> index 000000000..c0c9d6cd2
> --- /dev/null
> +++ b/builtin/rebase.c
> @@ -0,0 +1,55 @@
> +/*
> + * "git rebase" builtin command

Nice ;-)

Show 26 quoted lines

> + * Copyright (c) 2018 Pratik Karki
> + */
> +
> +#include "builtin.h"
> +#include "run-command.h"
> +#include "exec-cmd.h"
> +#include "argv-array.h"
> +#include "dir.h"
> +
> +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;
> +}

OK. As we give "--bool", it is perfectly OK to check with "true" and no other forms of positive setting.

Show 5 quoted lines

> +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

Align these asterisks, i.e.

	/*
	 * NEEDSWORK: ...
	 */

Show 8 quoted lines

> +	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);
> +
> +		return 0;

Hmph, I know this was inherited from the old commit you modelled this patch after, but can sane_execvp() ever give us control back with non-negative value returned? IOW I am wondering if this should be more like

	if (sane_execvp(...) < 0)
		die_errno(...);
	else
		die("sane_execvp() returned???");

Not worth a reroll, but something to think about.

Show 17 quoted lines

> diff --git a/git-rebase.sh b/git-legacy-rebase.sh
> similarity index 100%
> rename from git-rebase.sh
> rename to git-legacy-rebase.sh
> diff --git a/git.c b/git.c
> index 9dbe6ffaa..bacfefb2d 100644
> --- a/git.c
> +++ b/git.c
> @@ -518,6 +518,12 @@ static struct cmd_struct commands[] = {
>  	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
>  	{ "push", cmd_push, RUN_SETUP },
>  	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
> +	/*
> +	 *NEEDSWORK: Until the rebase is independent and needs no redirection
> +	 to rebase shell script this is kept as is, then should be changed to
> +	 RUN_SETUP | NEED_WORK_TREE
> +	*/

Likewise.

> +	{ "rebase", cmd_rebase },
>  	{ "rebase--helper", cmd_rebase__helper, RUN_SETUP | NEED_WORK_TREE },
>  	{ "receive-pack", cmd_receive_pack },
>  	{ "reflog", cmd_reflog, RUN_SETUP },