Re: [PATCH v2 4/4] builtin/rebase: support running "git rebase <upstream>"

[Junio C Hamano <gitster@pobox.com>]

Pratik Karki <predatoramigo@gmail.com> writes:

> +static struct commit *peel_committish(const char *name)
> +{
> +	struct object *obj;
> +	struct object_id oid;

It by itself is not a big enough deal to warrant a reroll, but
please make it a habit to leave a blank line between the
declarations and the first statement (i.e. here), to help readers.

> +	if (get_oid(name, &oid))
> +		return NULL;
> +	obj = parse_object(&oid);
> +	return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
> +}
> +

> +{
> +	const char *argv[] = { NULL, NULL };
> +	struct strbuf script_snippet = STRBUF_INIT;
> +	struct argv_array env = ARGV_ARRAY_INIT;
> +	int status;
> +	const char *backend, *backend_func;
> +
> +	argv_array_pushf(&env, "upstream_name=%s", opts->upstream_name);
> +	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
> +	argv_array_pushf(&env, "upstream=%s",
> +				oid_to_hex(&opts->upstream->object.oid));
> +	argv_array_pushf(&env, "orig_head=%s", oid_to_hex(&opts->orig_head));
> +	argv_array_pushf(&env, "onto=%s",
> +				oid_to_hex(&opts->onto->object.oid));
> +	argv_array_pushf(&env, "onto_name=%s", opts->onto_name);
> +	argv_array_pushf(&env, "revisions=%s", opts->revisions);
> +
> +	switch (opts->type) {
> +		case REBASE_AM:
> +			backend = "git-rebase--am";
> +			backend_func = "git_rebase__am";
> +			break;
> +		case REBASE_INTERACTIVE:
> +			backend = "git-rebase--interactive";
> +			backend_func = "git_rebase__interactive";
> +			break;
> +		case REBASE_MERGE:
> +			backend = "git-rebase--merge";
> +			backend_func = "git_rebase__merge";
> +			break;
> +		case REBASE_PRESERVE_MERGES:
> +			backend = "git-rebase--preserve-merges";
> +			backend_func = "git_rebase__preserve_merges";
> +			break;
> +		default:
> +			BUG("Unhandled rebase type %d", opts->type);
> +			break;

De-dent these lines so that case/default and switch all sit on the
same column, and the body of each case arm is indented one level
deeper than the case labels.

> +	}
> +
> +	strbuf_addf(&script_snippet,
> +		    ". git-rebase--common && . %s && %s",
> +		    backend, backend_func);
> +	argv[0] = script_snippet.buf;
> +
> +	status = run_command_v_opt_cd_env(argv, RUN_USING_SHELL, NULL,
> +					  env.argv);

Hmph.  These shell variables that tell rebase backend scriptlets
what rebase frontend figured out about the request and current state
used to be just plain shell variables that are not exported.  Now
they are exported and visible even to subprocesses these scriptlets
spawn.  That does not sound safe at all, especially GIT_DIR being
among these variables (git-sh-setup sets GIT_DIR but does not export
it and that has been very much deliberate).

You should actually be able to avoid exporthing them by building
these variable assignment into script_snippet (you need to quote the
values properly, using quote.c::sq_quote_buf() etc.), I think, and
that would be a more faithful-to-the-original safe covnersion.

Thanks for a pleasant read.  This step smells more like WIP, but I
can see it is already moving in the right direction.  The previous
ones (except for the issues I noticed and sent responses separately)
looked more-or-less good, too.